From d15258a487dc94ab2503221acb4a13f10c373b3f Mon Sep 17 00:00:00 2001 From: Shoaib Lari Date: Mon, 8 Apr 2019 12:14:19 -0700 Subject: [PATCH] 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: Jamie McAtamney Co-authored-by: Shoaib Lari Co-authored-by: David Krieger --- gpMgmt/bin/gpmovemirrors | 21 ++++++++++++----- .../behave/mgmt_utils/gpmovemirrors.feature | 23 +++++++++++++++++++ .../behave/mgmt_utils/steps/mgmt_utils.py | 2 +- .../mgmt_utils/steps/mirrors_mgmt_utils.py | 16 ++++++++++++- 4 files changed, 54 insertions(+), 8 deletions(-) diff --git a/gpMgmt/bin/gpmovemirrors b/gpMgmt/bin/gpmovemirrors index b452b3d775..ac56b4da9b 100755 --- a/gpMgmt/bin/gpmovemirrors +++ b/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) diff --git a/gpMgmt/test/behave/mgmt_utils/gpmovemirrors.feature b/gpMgmt/test/behave/mgmt_utils/gpmovemirrors.feature index 8eda626b6f..003a84eee3 100644 --- a/gpMgmt/test/behave/mgmt_utils/gpmovemirrors.feature +++ b/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 diff --git a/gpMgmt/test/behave/mgmt_utils/steps/mgmt_utils.py b/gpMgmt/test/behave/mgmt_utils/steps/mgmt_utils.py index e2b9d87860..d991a2e37d 100644 --- a/gpMgmt/test/behave/mgmt_utils/steps/mgmt_utils.py +++ b/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) diff --git a/gpMgmt/test/behave/mgmt_utils/steps/mirrors_mgmt_utils.py b/gpMgmt/test/behave/mgmt_utils/steps/mirrors_mgmt_utils.py index da0681ea77..60ed97cd32 100644 --- a/gpMgmt/test/behave/mgmt_utils/steps/mirrors_mgmt_utils.py +++ b/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)}. -- GitLab