Hi! thanks for the review.
On Mon, Oct 7, 2013 at 2:58 PM, Thomas Thrainer <[email protected]> wrote: > > > > On Mon, Oct 7, 2013 at 2:30 PM, Helga Velroyen <[email protected]> wrote: > >> The up/downgrade procedure so far did not consider the >> changes in the 'dev_type' attribute of disks. This patch >> adds relevant checks and updates. A related problem >> was that the logical IDs of disks were adjusted depending >> on the dev_type, which made the order of updates not >> arbitrary anymore. To make this more robust, the upgrade >> procedure also considers the old versions of dev_types >> (at least for 2.9). >> >> Signed-off-by: Helga Velroyen <[email protected]> >> --- >> test/py/cfgupgrade_unittest.py | 1 + >> tools/cfgupgrade | 22 +++++++++++++++++++++- >> 2 files changed, 22 insertions(+), 1 deletion(-) >> >> diff --git a/test/py/cfgupgrade_unittest.py >> b/test/py/cfgupgrade_unittest.py >> index c94920b..7d84ef3 100755 >> --- a/test/py/cfgupgrade_unittest.py >> +++ b/test/py/cfgupgrade_unittest.py >> @@ -386,6 +386,7 @@ class TestCfgupgrade(unittest.TestCase): >> """Test for upgrade + downgrade combination.""" >> # This test can work only with the previous version of a >> configuration! >> oldconfname = "cluster_config_2.8.json" >> + self.maxDiff = None >> > > I'm not sure if everybody always want's to see the full diff... > Ah, right. left-over from debugging. Thanks for pointing out. > > >> self._TestUpgradeFromFile(oldconfname, False) >> _RunUpgrade(self.tmpdir, False, True, downgrade=True) >> oldconf = self._LoadTestDataConfig(oldconfname) >> diff --git a/tools/cfgupgrade b/tools/cfgupgrade >> index 554877f..404aaef 100755 >> --- a/tools/cfgupgrade >> +++ b/tools/cfgupgrade >> @@ -174,6 +174,12 @@ def GetExclusiveStorageValue(config_data): >> >> >> def UpgradeInstances(config_data): >> + """Upgrades the instances' configuration.""" >> + >> + # map of legacy device types (mapping differing old LD_* constants to >> new DT_* >> + # constants) >> + LEG_DEV_TYPE_MAP = {"lvm": constants.DT_PLAIN, "drbd8": >> constants.DT_DRBD8} >> + >> network2uuid = dict((n["name"], n["uuid"]) >> for n in config_data["networks"].values()) >> if "instances" not in config_data: >> @@ -201,6 +207,10 @@ def UpgradeInstances(config_data): >> " from '%s' to '%s'", >> instance, idx, current, expected) >> dobj["iv_name"] = expected >> + >> + if dobj["dev_type"] in LEG_DEV_TYPE_MAP: >> + dobj["dev_type"] = LEG_DEV_TYPE_MAP[dobj["dev_type"]] >> + >> > > I think this should be done recursively for child disks as well. > Especially because the child disks of a DRBD disk are PLAIN AFAIK. > Sure, thanks for the hint. I gave it some more thought to make it more elegant and merged the up/downgrade code for device types to share more common code. See the interdiff below. > > >> if not "spindles" in dobj: >> missing_spindles = True >> >> @@ -288,7 +298,10 @@ def GetNewNodeIndex(nodes_by_old_key, old_key, >> new_key_field): >> >> def ChangeNodeIndices(config_data, old_key_field, new_key_field): >> def ChangeDiskNodeIndices(disk): >> - if disk["dev_type"] in constants.LDS_DRBD: >> + # FIXME(2.10): 'drbd8' can be removed as soon as no updates from 2.8 >> + # to 2.9 are supported anymore >> > > Correct me if I'm wrong, but I thought that upgrades should work from any > version > 2.0 to the current version, whereas downgrades should only work > one version back. So the TODO would be misleading in this case. > Right, I forgot about the upgrade being able to jump more than one version. I sort of only had the downgrade in mind. I changed the comment accordingly. > > >> + drbd_disk_types = set(["drbd8"]) | constants.DTS_DRBD >> + if disk["dev_type"] in drbd_disk_types: >> for i in range(0, 2): >> disk["logical_id"][i] = GetNewNodeIndex(nodes_by_old_key, >> disk["logical_id"][i], >> @@ -351,6 +364,10 @@ def UpgradeAll(config_data): >> >> >> def DowngradeDisks(disks, owner): >> + # map of legacy device types (mapping differing old LD_* constants to >> new DT_* >> + # constants) >> + LEG_DEV_TYPE_MAP = {constants.DT_PLAIN: "lvm", constants.DT_DRBD8: >> "drbd8"} >> + >> for disk in disks: >> # Remove spindles to downgrade to 2.8 >> if "spindles" in disk: >> @@ -358,6 +375,9 @@ def DowngradeDisks(disks, owner): >> " instance %s", >> disk["spindles"], disk["iv_name"], disk["uuid"], >> owner) >> del disk["spindles"] >> + if disk["dev_type"] in LEG_DEV_TYPE_MAP: >> + old_val = disk["dev_type"] >> + disk["dev_type"] = LEG_DEV_TYPE_MAP[old_val] >> > > Same as above, this should updated child disks recursively. > Done, see the interdiff below. diff --git a/test/py/cfgupgrade_unittest.py b/test/py/cfgupgrade_unittest.py index 7d84ef3..c94920b 100755 --- a/test/py/cfgupgrade_unittest.py +++ b/test/py/cfgupgrade_unittest.py @@ -386,7 +386,6 @@ class TestCfgupgrade(unittest.TestCase): """Test for upgrade + downgrade combination.""" # This test can work only with the previous version of a configuration! oldconfname = "cluster_config_2.8.json" - self.maxDiff = None self._TestUpgradeFromFile(oldconfname, False) _RunUpgrade(self.tmpdir, False, True, downgrade=True) oldconf = self._LoadTestDataConfig(oldconfname) diff --git a/tools/cfgupgrade b/tools/cfgupgrade index 404aaef..7141e9b 100755 --- a/tools/cfgupgrade +++ b/tools/cfgupgrade @@ -58,6 +58,12 @@ DOWNGRADE_MAJOR = 2 #: Target minor version for downgrade DOWNGRADE_MINOR = 8 +# map of legacy device types +# (mapping differing old LD_* constants to new DT_* constants) +DEV_TYPE_OLD_NEW = {"lvm": constants.DT_PLAIN, "drbd8": constants.DT_DRBD8} +# (mapping differing new DT_* constants to old LD_* constants) +DEV_TYPE_NEW_OLD = {v:k for k,v in DEV_TYPE_OLD_NEW.items()} + class Error(Exception): """Generic exception""" @@ -173,13 +179,26 @@ def GetExclusiveStorageValue(config_data): return ret +def ChangeDiskDevType(disk, dev_type_map): + """Replaces disk's dev_type attributes according to the given map. + """ + if disk["dev_type"] in dev_type_map: + disk["dev_type"] = dev_type_map[disk["dev_type"]] + if "children" in disk: + for child in disk["children"]: + ChangeDiskDevType(child, dev_type_map) + + +def UpgradeDiskDevType(disk): + """Upgrades the disks' device type.""" + ChangeDiskDevType(disk, DEV_TYPE_OLD_NEW) + + def UpgradeInstances(config_data): """Upgrades the instances' configuration.""" - # map of legacy device types (mapping differing old LD_* constants to new DT_* - # constants) - LEG_DEV_TYPE_MAP = {"lvm": constants.DT_PLAIN, "drbd8": constants.DT_DRBD8} - network2uuid = dict((n["name"], n["uuid"]) for n in config_data["networks"].values()) if "instances" not in config_data: @@ -208,8 +227,8 @@ def UpgradeInstances(config_data): instance, idx, current, expected) dobj["iv_name"] = expected - if dobj["dev_type"] in LEG_DEV_TYPE_MAP: - dobj["dev_type"] = LEG_DEV_TYPE_MAP[dobj["dev_type"]] + if "dev_type" in dobj: + UpgradeDiskDevType(dobj) if not "spindles" in dobj: missing_spindles = True @@ -298,8 +317,9 @@ def GetNewNodeIndex(nodes_by_old_key, old_key, new_key_field): def ChangeNodeIndices(config_data, old_key_field, new_key_field): def ChangeDiskNodeIndices(disk): - # FIXME(2.10): 'drbd8' can be removed as soon as no updates from 2.8 - # to 2.9 are supported anymore + # Note: 'drbd8' is a legacy device type from pre 2.9 and needs to be + # considered when up/downgrading from/to any versions touching 2.9 on the + # way. drbd_disk_types = set(["drbd8"]) | constants.DTS_DRBD if disk["dev_type"] in drbd_disk_types: for i in range(0, 2): @@ -363,11 +383,11 @@ def UpgradeAll(config_data): UpgradeInstanceIndices(config_data) -def DowngradeDisks(disks, owner): - # map of legacy device types (mapping differing old LD_* constants to new DT_* - # constants) - LEG_DEV_TYPE_MAP = {constants.DT_PLAIN: "lvm", constants.DT_DRBD8: "drbd8"} +def DowngradeDiskDevType(disk): + """Downgrades the disks' device type.""" + ChangeDiskDevType(disk, DEV_TYPE_NEW_OLD) +def DowngradeDisks(disks, owner): for disk in disks: # Remove spindles to downgrade to 2.8 if "spindles" in disk: @@ -375,9 +395,8 @@ def DowngradeDisks(disks, owner): " instance %s", disk["spindles"], disk["iv_name"], disk["uuid"], owner) del disk["spindles"] - if disk["dev_type"] in LEG_DEV_TYPE_MAP: - old_val = disk["dev_type"] - disk["dev_type"] = LEG_DEV_TYPE_MAP[old_val] + if "dev_type" in disk: + DowngradeDiskDevType(disk) def DowngradeInstances(config_data):
