Based on the discussion I think this patch series is definitely not the right place for such a change. I guess first we should change the way we handle version numbers during development, and then we should also gather realistic requirements for cfgupgrade, which could then lead to a re-write. In the meantime, I'll rewrite this patch to only add/remove the additional flag in cfgupgrade. IIRC the problem with upgrades in masterd were, that confd has to be able to read the config as well...
Thanks, Thomas On Wed, May 15, 2013 at 6:30 PM, Bernardo Dal Seno <[email protected]>wrote: > On 15 May 2013 16:04, Thomas Thrainer <[email protected]> wrote: > > cfgupgrade can now upgrade the configuration to thecfg newest format, > which > > requires the disks_active flag for all instances. > > This should be a separate patch from introducing target versions. > > > Additionally, cfgupgrade now supports the --target-version parameter, > > which is used during tests and has to be specified when downgrading a > > configuration. > > > > Signed-off-by: Thomas Thrainer <[email protected]> > > --- > > > > As discussed in the ganeti-core meeting, this patch is only a RFC for > cfgupgrade. > > > > cfgupgrade > > * now can freely upgrade/downgrade from/to 2.{6..9} (which might be > interesting > > for users who use distribution packages, which might be upgraded from > 2.6 to > > 2.9 directly for instance). > > Not really. The way we handle versions right now is very confusing > (and I think we should fix it soon). We update Ganeti version together > with the configuration file version only when making the first > release. So right now we have stable-2.7, stable-2.8, and master all > set at version 2.7.0, though they could potentially have different > configurations. Indeed, stable-2.8 has a different configuration than > stable-2.7. So, let's ignore the version numbers for now, and just > look at the data format. > > So the stable-2.8 cfgupgrade, which is still the same as the master > cfgupgrade, can do two things: > 1. upgrade to 2.8 from whatever version you throw at it, including 2.8 > (this is useful during development, as configuration changes can be > introduced more than once) > 2. downgrade from 2.8 to 2.7. > So there is no way to downgrade to 2.6, nor there is a way to upgrade > only to 2.7, unless you want to extract the changes applied between > 2.7 and 2.8 and perform separate upgrade steps. Hence, you have to > adjust most of the version numbers that appear in your patch. > > Besides, upgrading to an older version is something that it's not very > useful, so I don't see any reason to have it. > > > * does still not support downgrades to 2.{0..5}, and does not check for > that. > > Off by one, see above :-) > > > * supports a --target-version parameter to specify the version to > up-/downgrade > > to. Omitting it uses the current Ganeti version. > > * structures the code in such a way that future up-/downgrade steps > should be > > rather easy to integrate. > > > > On the downside, cfgupgrade got more complex through this, and it's > virtually > > impossible to test all up-/downgrade paths. > > That's why I'd stick with one upgrade target, even if we allow > multiple downgrade targets. > > > An alternative to this patch might be to add disks_active in objects.py, > > NodeGroup.UpgradeConfig(). The downgrade path could either be ignored or > > integrated in cfgupgrade. > > Ignoring is not good. The only downgrade path is through cfgupgrade, > and it should be complete, otherwise it's useless. > > > Do you see any need to support such up-/downgrade paths? For development > it > > comes in handy sometimes, when you are switching between branches a lot. > But > > would there be a need for others as well? And would structuring the code > in this > > way (having functions which upgrade just from one to the next version, > which > > then are applied one after the other) help maintainability or should we > just > > ignore which version changes came in, and try to upgrade to the current > version? > > Downgrading to an arbitrary (at least, if supported) version can be > useful to revert deployment and to switch branches during development. > But for that we need to change the versions in the configuration files > as soon as we change the configuration format. Otherwise there is no > way to specify a target. Also, during development you want to be able > to upgrade from the latest version to the latest version, as you want > to be able to upgrade your configuration each time a new change to the > configuration gets committed. > > In brief, for this to work I think first we should address the problem > of config versions on different branches, then we can support multiple > downgrade targets (and maybe upgrade targets, if they are really > useful), and then you can handle the new instance property. > > I've written a couple of notes on the code itself, which could be > useful to rework this patch. > > > diff --git a/test/py/cfgupgrade_unittest.py > b/test/py/cfgupgrade_unittest.py > > index 52a9299..09eb843 100755 > > --- a/test/py/cfgupgrade_unittest.py > > +++ b/test/py/cfgupgrade_unittest.py > > @@ -396,11 +408,19 @@ class TestCfgupgrade(unittest.TestCase): > > self._TestUpgradeFromFile("cluster_config_2.7.json", False) > > self._RunDowngradeUpgrade() > > > > - def _RunDowngradeTwice(self): > > + def testDowngradeFullConfigBackwardFrom_2_9(self): > > + """Test for upgrade + downgrade + upgrade combination.""" > > + self._TestUpgradeFromFile("cluster_config_2.7.json", False, > > + target_version="2.9") > > Here the starting point is 2.7, not 2.9 as implied by the function name. > > > + self._RunDowngradeUpgrade(old_version="2.8", target_version="2.9") > > + > > At least an (important) test is missing. You can check the result of > an upgrade by loading it (and that's what cfgupgrade can do), but you > cannot do the same for a downgrade. So the only possible tests is to > start from a known good old configuration, upgrade it (+ test it), > downgrade it, and compare it with the starting point. Or start from a > known good new configuration, downgrade it, upgrade it, and compare it > with the starting point. The test you're written checks the second > path, but you're still missing the first. I had a patch ready fort > this, I can send it to you. > > > @@ -433,6 +453,12 @@ class TestCfgupgrade(unittest.TestCase): > > def testUpgradeDryRunFrom_2_6(self): > > self._TestSimpleUpgrade(constants.BuildVersion(2, 6, 0), True) > > > > + def testUpgradeDryRunFrom_2_7(self): > > + self._TestSimpleUpgrade(constants.BuildVersion(2, 7, 0), True) > > + > > + def testUpgradeDryRunFrom_2_8(self): > > + self._TestSimpleUpgrade(constants.BuildVersion(2, 8, 0), True) > > + > > I'm not sure if these are useful until we update the version numbers. > > > > diff --git a/tools/cfgupgrade b/tools/cfgupgrade > > index 0526b1a..5ce57f2 100755 > > --- a/tools/cfgupgrade > > +++ b/tools/cfgupgrade > > @@ -365,6 +386,8 @@ def main(): > > options.SSCONF_MASTER_NODE = options.data_dir + "/ssconf_master_node" > > options.WATCHER_STATEFILE = options.data_dir + "/watcher.data" > > options.FILE_STORAGE_PATHS_FILE = options.conf_dir + > "/file-storage-paths" > > + options.TARGET_MAJOR, options.TARGET_MINOR = \ > > + SplitVersion(options.target_version) > > Please use lower-case letters for non-constants. > > > @@ -423,27 +446,38 @@ def main(): > > raise Error("Inconsistent configuration: found config_version in" > > " configuration file") > > > > + if config_major != 2 or options.TARGET_MAJOR != 2: > > + raise Error("Upgrade/downgrade supported only for 2.x series!") > > + > > + if config_revision != 0: > > + logging.warning("Config revision is %s, not 0", config_revision) > > + > > # Downgrade to the previous stable version > > if options.downgrade: > > - if config_major != TARGET_MAJOR or config_minor != TARGET_MINOR: > > - raise Error("Downgrade supported only from the latest version > (%s.%s)," > > - " found %s (%s.%s.%s) instead" % > > - (TARGET_MAJOR, TARGET_MINOR, config_version, > config_major, > > - config_minor, config_revision)) > > - DowngradeAll(config_data) > > - > > - # Upgrade from 2.{0..7} to 2.7 > > - elif config_major == 2 and config_minor in range(0, 8): > > - if config_revision != 0: > > - logging.warning("Config revision is %s, not 0", config_revision) > > - UpgradeAll(config_data) > > - > > - elif config_major == TARGET_MAJOR and config_minor == TARGET_MINOR: > > - logging.info("No changes necessary") > > + if config_minor >= 9 and options.TARGET_MINOR <= 8: > > + DowngradeTo28(config_data) > > + if config_minor >= 8 and options.TARGET_MINOR <= 7: > > + DowngradeTo26(config_data) > > Here you should check if the version is too old for a downgrade. > > > > > else: > > - raise Error("Configuration version %d.%d.%d not supported by this > tool" % > > - (config_major, config_minor, config_revision)) > > + if config_minor in range(0, 7) and options.TARGET_MINOR >= 7: > > + # Upgrade from 2.{0..6} to 2.7 > > + UpgradeTo27(config_data) > > + > > + if config_minor in range(0, 9) and options.TARGET_MINOR >= 9: > > + # Upgrade from 2.{0..8} to 2.9 > > + UpgradeTo29(config_data) > > + > > + if config_major == options.TARGET_MAJOR and \ > > + config_minor == options.TARGET_MINOR: > > + logging.info("No changes necessary") > > Notice that from the way it was used, this condition used to be true > only after a release. Unless we change the way we use releases, this > message can be misleading as it can appear also after performing an > upgrade. Remember, we want to be able to upgrade also from the latest > version during development. > > Thanks, > Bernardo > -- Thomas Thrainer | Software Engineer | [email protected] | Google Germany GmbH Dienerstr. 12 80331 München Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg Geschäftsführer: Graham Law, Katherine Stephens
