gpmovemirrors: Don't delete data directories if they are same

In the case where the user wants to change the port of a mirror, but
leave it in the same directory on the same host, gpmovemirrors was
previously deleting the directory as part of the cleanup because it
did not recognize that the old and new directories were the same.
This commit adds a check to prevent that.

This commit also logs a warning if the move_config_file contains
identical attributes(host,port,data directory) for the old and new
mirror.
Co-authored-by: NJamie McAtamney <jmcatamney@pivotal.io>
Co-authored-by: NShoaib Lari <slari@pivotal.io>
Co-authored-by: NDavid Krieger <dkrieger@pivotal.io>

gpMgmt/bin/gpmovemirrors

@@ -180,10 +180,11 @@ def lookupGpdb(address, port, dataDirectory):
 class Mirror:
     """ This class represents information about a mirror. """
 
-    def __init__(self, address, port, dataDirectory):
+    def __init__(self, address, port, dataDirectory, inPlace=False):
         self.address = address
         self.port = port
         self.dataDirectory = dataDirectory
+        self.inPlace = inPlace
 
     def __str__(self):
         tempStr = "address = " + str(self.address) + '\n'
@@ -224,14 +225,21 @@ class Configuration:
                         raise Exception(
                             "Old mirror segment is not currently in a mirror role: address = %s, port = %s, segment data directory = %s"
                             % (oldAddress, str(oldPort), oldDataDirectory))
+                newAddress = rowMap['newAddress']
+                newPort = rowMap['newPort']
+                newDataDirectory = rowMap['newDataDirectory']
+
+                inPlace = (oldAddress == newAddress and oldDataDirectory == newDataDirectory)
+                if inPlace and oldPort == newPort:
+                    logger.warn("input file %s contained request to move a mirror with identical attributes (%s, %s, %s)"
+                                % (inputFile, newAddress, newPort, newDataDirectory))
+
                 oldMirror = Mirror(address=oldAddress
                                    , port=oldPort
                                    , dataDirectory=oldDataDirectory
+                                   , inPlace=inPlace
                                    )
                 self.oldMirrorList.append(oldMirror)
-                newAddress = rowMap['newAddress']
-                newPort = rowMap['newPort']
-                newDataDirectory = rowMap['newDataDirectory']
                 newMirror = Mirror(address=newAddress
                                    , port=newPort
                                    , dataDirectory=newDataDirectory
@@ -344,11 +352,12 @@ try:
     cmd.run(validateAfter=True)
 
     """ Delete old mirror directories. """
-    totalDirsToDelete = len(newConfig.oldMirrorList)
+    mirrorsToDelete = [mirror for mirror in newConfig.oldMirrorList if not mirror.inPlace]
+    totalDirsToDelete = len(mirrorsToDelete)
     numberOfWorkers = min(totalDirsToDelete, options.batch_size)
     if numberOfWorkers > 1:
         pool = WorkerPool(numWorkers=numberOfWorkers)
-        for mirror in newConfig.oldMirrorList:
+        for mirror in mirrorsToDelete:
             logger.info("About to remove old mirror segment directory: " + mirror.dataDirectory)
             cmd = RemoveDirectory("remove old mirror segment directories", mirror.dataDirectory, ctxt=REMOTE,
                                   remoteHost=mirror.address)

gpMgmt/test/behave/mgmt_utils/gpmovemirrors.feature

@@ -33,6 +33,29 @@ Feature: Tests for gpmovemirrors
         And the segments are synchronized
         And verify that mirrors are recognized after a restart
 
+    Scenario: gpmovemirrors can change the port of mirrors within a single host
+        Given a standard local demo cluster is created
+        And a gpmovemirrors directory under '/tmp/gpmovemirrors' with mode '0700' is created
+        And a 'samedir' gpmovemirrors file is created
+        When the user runs gpmovemirrors
+        Then gpmovemirrors should return a return code of 0
+        And verify the database has mirrors
+        And all the segments are running
+        And the segments are synchronized
+        And verify that mirrors are recognized after a restart
+
+    Scenario: gpmovemirrors gives a warning when passed identical attributes for new and old mirrors
+        Given a standard local demo cluster is created
+        And a gpmovemirrors directory under '/tmp/gpmovemirrors' with mode '0700' is created
+        And a 'identicalAttributes' gpmovemirrors file is created
+        When the user runs gpmovemirrors
+        Then gpmovemirrors should return a return code of 0
+	And gpmovemirrors should print a "request to move a mirror with identical attributes" warning
+	And verify the database has mirrors
+        And all the segments are running
+        And the segments are synchronized
+        And verify that mirrors are recognized after a restart
+
 ########################### @concourse_cluster tests ###########################
 # The @concourse_cluster tag denotes the scenario that requires a remote cluster
 

gpMgmt/test/behave/mgmt_utils/steps/mgmt_utils.py

@@ -417,7 +417,6 @@ def impl(context, HOST, port, dir, ctxt):
                       gp_source_file)
     gpfdist.cleanupGpfdist()
 
-
 @then('{command} should print "{err_msg}" error message')
 def impl(context, command, err_msg):
     check_err_msg(context, err_msg)
@@ -425,6 +424,7 @@ def impl(context, command, err_msg):
 
 @when('{command} should print "{out_msg}" to stdout')
 @then('{command} should print "{out_msg}" to stdout')
+@then('{command} should print a "{out_msg}" warning')
 def impl(context, command, out_msg):
     check_stdout_msg(context, out_msg)
 

gpMgmt/test/behave/mgmt_utils/steps/mirrors_mgmt_utils.py

@@ -233,6 +233,20 @@ def impl(context, file_type):
                                        mirror.getSegmentPort(),
                                        context.mirror_context.working_directory)
         contents = '%s %s' % (valid_config, badhost_config)
+    elif file_type == 'samedir':
+        valid_config_with_same_dir = '%s:%s:%s' % (
+            mirror.getSegmentHostName(),
+            mirror.getSegmentPort() + 1000,
+            mirror.getSegmentDataDirectory()
+        )
+        contents = '%s %s' % (valid_config, valid_config_with_same_dir)
+    elif file_type == 'identicalAttributes':
+        valid_config_with_identical_attributes = '%s:%s:%s' % (
+            mirror.getSegmentHostName(),
+            mirror.getSegmentPort(),
+            mirror.getSegmentDataDirectory()
+        )
+        contents = '%s %s' % (valid_config, valid_config_with_identical_attributes)
     elif file_type == 'good':
         valid_config_with_different_dir = '%s:%s:%s' % (
             mirror.getSegmentHostName(),
@@ -281,7 +295,7 @@ def impl(context, mirror_config):
     input_filename = "/tmp/gpmovemirrors_input_%s" % mirror_config
     line_template = "%s:%d:%s %s:%d:%s\n"
 
-    # The mirrors for contents 0 and 3 are excluded from the two maps below because they are the same in either configuration    
+    # The mirrors for contents 0 and 3 are excluded from the two maps below because they are the same in either configuration
     # NOTE: this configuration of the GPDB cluster assumes that configuration set up in concourse's
     #   gpinitsystem task.  The maps below are from {contentID : (port|hostname)}.