Partial revert "Fix cleanup calls in plugin_test.sh"

This is a partial revert of commit fc807b0. fc807b0 is adding an undocumented argument to the plugin_test.sh. This test is provided to third parties to aid in their plugin development. The test is written to exercise and test a plugin's functionality. fc807b0 introduces changes in an effort to get the test to clean up after itself, but also doesn't seem to fully work as mention in a new comment. This commit also introduces utility/debugging functions we will keep to maintain parity with ddboost and s3 plugin. The API version will also be kept at 0.5.0.
silvertl · Feb 28, 2024 · 84c2a9f · 84c2a9f
1 parent 2782cd0
commit 84c2a9f
Show file tree

Hide file tree

Showing 5 changed files with 24 additions and 32 deletions.
diff --git a/ci/scripts/ddboost-plugin-tests.bash b/ci/scripts/ddboost-plugin-tests.bash
@@ -84,9 +84,9 @@ CONFIG
 
 pushd \${GOPATH}/src/github.com/greenplum-db/gpbackup/plugins
 
-./plugin_test.sh \${GPHOME}/bin/gpbackup_ddboost_plugin \${HOME}/ddboost_config_replication.yaml gpbackup_tests${GPDB_VERSION} \${HOME}/ddboost_config_replication_restore.yaml
+./plugin_test.sh \${GPHOME}/bin/gpbackup_ddboost_plugin \${HOME}/ddboost_config_replication.yaml \${HOME}/ddboost_config_replication_restore.yaml
 
-./plugin_test.sh \${GPHOME}/bin/gpbackup_ddboost_plugin \${HOME}/ddboost_config.yaml gpbackup_tests${GPDB_VERSION} \${HOME}/ddboost_config_replication_restore.yaml
+./plugin_test.sh \${GPHOME}/bin/gpbackup_ddboost_plugin \${HOME}/ddboost_config.yaml \${HOME}/ddboost_config_replication_restore.yaml
 
 # exercise boostfs, which is mounted at /data/gpdata/dd_dir
 pushd \${GOPATH}/src/github.com/greenplum-db/gpbackup

diff --git a/ci/scripts/s3-plugin-tests.bash b/ci/scripts/s3-plugin-tests.bash
@@ -30,7 +30,7 @@ options:
 CONFIG
 
   pushd ~/go/src/github.com/greenplum-db/gpbackup/plugins
-    ./plugin_test.sh \${GPHOME}/bin/gpbackup_s3_plugin \${HOME}/s3_config.yaml test/backup 
+    ./plugin_test.sh \${GPHOME}/bin/gpbackup_s3_plugin \${HOME}/s3_config.yaml
   popd
 SCRIPT
 

diff --git a/end_to_end/plugin_test.go b/end_to_end/plugin_test.go
@@ -453,7 +453,7 @@ var _ = Describe("End to End plugin tests", func() {
 				Skip("This test is only needed for the latest backup version")
 			}
 			copyPluginToAllHosts(backupConn, examplePluginExec)
-			command := exec.Command("bash", "-c", fmt.Sprintf("%s/plugin_test.sh %s %s %s", examplePluginDir, examplePluginExec, examplePluginTestConfig, examplePluginTestDir))
+			command := exec.Command("bash", "-c", fmt.Sprintf("%s/plugin_test.sh %s %s", examplePluginDir, examplePluginExec, examplePluginTestConfig))
 			mustRunCommand(command)
 		})
 	})

diff --git a/plugins/example_plugin.bash b/plugins/example_plugin.bash
@@ -90,11 +90,13 @@ delete_backup() {
 
 }
 
+# utility/debugging function to maintain parity with ddboost and s3 plugin
 list_directory() {
   echo "list_directory $1 /tmp/plugin_dest" >> /tmp/plugin_out.txt
   ls /tmp/plugin_dest
 }
 
+# utility/debugging function to maintain parity with ddboost and s3 plugin
 delete_directory() {
   echo "delete_directory $1 /tmp/plugin_dest" >> /tmp/plugin_out.txt
   rm -rf /tmp/plugin_dest

diff --git a/plugins/plugin_test.sh b/plugins/plugin_test.sh
@@ -4,18 +4,17 @@ set -o pipefail
 
 plugin=$1
 plugin_config=$2
-plugin_dir=$3
-secondary_plugin_config=$4
+secondary_plugin_config=$3
 MINIMUM_API_VERSION="0.4.0"
 
 # ----------------------------------------------
 # Test suite setup
 # This will put small amounts of data in the
 # plugin destination location
 # ----------------------------------------------
-if [ $# -lt 3 ] || [ $# -gt 4 ]
+if [ $# -lt 2 ] || [ $# -gt 3 ]
   then
-    echo "Usage: plugin_test.sh [path_to_executable] [plugin_config] [plugin_testdir] [optional_config_for_secondary_destination]"
+    echo "Usage: plugin_test.sh [path_to_executable] [plugin_config] [optional_config_for_secondary_destination]"
     exit 1
 fi
 
@@ -37,9 +36,8 @@ testdata="$testdir/testdata_$time_second.txt"
 test_no_data="$testdir/test_no_data_$time_second.txt"
 testdatasmall="$testdir/testdatasmall_$time_second.txt"
 testdatalarge="$testdir/testdatalarge_$time_second.txt"
-logdir="/tmp/test_bench_logs"
 
-plugin_testdir="$plugin_dir/${current_date}/${time_second}"
+logdir="/tmp/test_bench_logs"
 
 text="this is some text"
 data=`LC_ALL=C tr -dc 'A-Za-z0-9' </dev/urandom | head -c 1000 ; echo`
@@ -59,18 +57,16 @@ cleanup_test_dir() {
   fi
 
   testdir_to_clean=$1
-  set +e
 
-  echo "[RUNNING] delete_directory on plugin: $testdir_to_clean"
-  $plugin delete_directory $plugin_config $testdir_to_clean 
-  echo "[PASSED] delete_directory on plugin: $testdir_to_clean"
+  $plugin cleanup_plugin_for_backup $plugin_config $testdir_to_clean coordinator \"-1\"
+  $plugin cleanup_plugin_for_backup $plugin_config $testdir_to_clean segment_host
+  $plugin cleanup_plugin_for_backup $plugin_config $testdir_to_clean segment \"0\"
+  echo "[PASSED - CLEANUP] cleanup_plugin_for_backup"
 
-if [[ "$plugin" == *gpbackup_ddboost_plugin ]] && [[ "$plugin_config" == *ddboost_config_replication.yaml ]] && [[ -n "$secondary_plugin_config" ]]; then
-  echo "[RUNNING] delete_directory on plugin remote: $testdir_to_clean"
-  $plugin delete_directory $secondary_plugin_config $testdir_to_clean 
-  echo "[PASSED] delete_directory on plugin remote: $testdir_to_clean"
-fi
-  set -e
+  $plugin cleanup_plugin_for_restore $plugin_config $testdir_to_clean coordinator \"-1\"
+  $plugin cleanup_plugin_for_restore $plugin_config $testdir_to_clean segment_host
+  $plugin cleanup_plugin_for_restore $plugin_config $testdir_to_clean segment \"0\"
+  echo "[PASSED - CLEANUP] cleanup_plugin_for_restore"
 }
 
 echo "# ----------------------------------------------"
@@ -156,8 +152,7 @@ echo "[PASSED] setup_plugin_for_backup"
 echo "[PASSED] backup_file"
 echo "[PASSED] setup_plugin_for_restore"
 echo "[PASSED] restore_file"
-# TODO -- This test is still not getting cleaned up.  The "backup_file" path construction logic places files 
-# very strangely, making delete_directory inappropraite for this case.
+cleanup_test_dir $testdir
 
 # ----------------------------------------------
 # Backup/Restore data functions
@@ -184,6 +179,7 @@ if [[ "$plugin_config" == *ddboost_config_replication.yaml ]] && [[ -n "$seconda
 fi
 echo "[PASSED] backup_data"
 echo "[PASSED] restore_data"
+cleanup_test_dir $testdir
 
 echo "[RUNNING] backup_data with no data"
 echo -n "" | $plugin backup_data $plugin_config $test_no_data
@@ -206,6 +202,7 @@ if [[ "$plugin_config" == *ddboost_config_replication.yaml ]] && [[ -n "$seconda
 fi
 echo "[PASSED] backup_data with no data"
 echo "[PASSED] restore_data with no data"
+cleanup_test_dir $testdir
 
 # ----------------------------------------------
 # Restore subset data functions
@@ -243,11 +240,10 @@ if [[ "$plugin" == *gpbackup_ddboost_plugin ]]; then
   data_part2=$(echo $data_large | cut -c900001-900001)
   if [ "$output" != "$data_part1$data_part2" ]; then
     echo "Failure restore_data_subset of two partitions from large data"
-    cleanup_test_dir $plugin_testdir
     exit 1
   fi
   echo "[PASSED] restore_data_subset of two partitions from large data"
-  cleanup_test_dir $plugin_testdir
+  cleanup_test_dir $testdir
 fi
 
 # ----------------------------------------------
@@ -259,12 +255,10 @@ current_date_for_del=$(echo $time_second_for_del | cut -c 1-8)
 time_second_for_del2=$(expr $time_second_for_del + 1)
 current_date_for_del2=$(echo $time_second_for_del2 | cut -c 1-8)
 
-plugin_testdir_for_del="$plugin_dir/${current_date_for_del}/${time_second_for_del}"
 testdir_for_del="/tmp/testseg/backups/${current_date_for_del}/${time_second_for_del}"
 testdata_for_del="$testdir_for_del/testdata_$time_second_for_del.txt"
 testfile_for_del="$testdir_for_del/testfile_$time_second_for_del.txt"
 
-plugin_testdir_for_del2="$plugin_dir/${current_date_for_del2}/${time_second_for_del2}"
 testdir_for_del2="/tmp/testseg/backups/${current_date_for_del2}/${time_second_for_del2}"
 testdata_for_del2="$testdir_for_del2/testdata_$time_second_for_del2.txt"
 testfile_for_del2="$testdir_for_del2/testfile_$time_second_for_del2.txt"
@@ -341,8 +335,7 @@ fi
 
 set -e
 echo "[PASSED] delete_backup"
-# cleanup_test_dir $plugin_testdir_for_del
-# cleanup_test_dir $plugin_testdir_for_del2
+cleanup_test_dir $testdir_for_del
 
 # ----------------------------------------------
 # Replicate backup function
@@ -351,7 +344,6 @@ if [[ "$plugin" == *gpbackup_ddboost_plugin ]] && [[ "$plugin_config" == *ddboos
     time_second_for_repl=$(expr 99999999999999 - $(od -vAn -N5 -tu < /dev/urandom | tr -d ' \n'))
     current_date_for_repl=$(echo $time_second_for_repl | cut -c 1-8)
 
-    plugin_testdir_for_repl="$plugin_dir/${current_date_for_repl}/${time_second_for_repl}"
     testdir_for_repl="/tmp/testseg/backups/${current_date_for_repl}/${time_second_for_repl}"
     testdata_for_repl="$testdir_for_repl/testdata_$time_second_for_repl.txt"
 
@@ -389,7 +381,7 @@ if [[ "$plugin" == *gpbackup_ddboost_plugin ]] && [[ "$plugin_config" == *ddboos
         fi
     fi
     echo "[PASSED] replicate backup"
-    cleanup_test_dir $plugin_testdir_for_repl
+    cleanup_test_dir $testdir_for_repl
 fi
 
 set +e
@@ -409,7 +401,6 @@ if [[ "$plugin" == *gpbackup_ddboost_plugin ]] && [[ "$plugin_config" == *ddboos
     time_second_for_repl=$(expr 99999999999999 - $(od -vAn -N5 -tu < /dev/urandom | tr -d ' \n'))
     current_date_for_repl=$(echo $time_second_for_repl | cut -c 1-8)
 
-    plugin_testdir_for_repl="$plugin_dir/${current_date_for_repl}/${time_second_for_repl}"
     testdir_for_repl="/tmp/testseg/backups/${current_date_for_repl}/${time_second_for_repl}"
     testdata_for_repl="$testdir_for_repl/testdata_$time_second_for_repl.txt"
 
@@ -439,7 +430,6 @@ if [[ "$plugin" == *gpbackup_ddboost_plugin ]] && [[ "$plugin_config" == *ddboos
     echo "[PASSED] delete_replica again to verify warning"
     set -e
 
-    cleanup_test_dir $plugin_testdir_for_repl
 fi
 
 # ----------------------------------------------