On Tue, Oct 8, 2013 at 9:01 AM, Klaus Aehlig <[email protected]> wrote:
> > > + @type versionstring: string > > > + @param versionstring: the version to upgrade to > > > + @rtype: (bool, list) > > > + @return: pair of a bit indicating sucess and a list of rollback > tasks > > > > > > > s/sucess/success/ > > s/pair of a bit/tuple of a bool/ > > changed as requested > > > > + @param downgrade: True, if the configuration should be downgraded > > > + @rtype: (bool, list) > > > + @return: pair of a boolean indicating success, and a list of > > > > > > > s/pair/tuple/ > > s/boolean/bool/ > > changed as requested > > > > +def _UpgradeAfterConfigurationChange(): > > > + """ > > > + Carry out the upgrade actions necessary after switching to the new > > > + Ganeti version and updating the configuration. > > > + > > > + As this part is run at a time where the new version of Ganeti is > already > > > + running, no communication should happen via luxi, as this is not a > > > stable > > > + interface. Also, as the configuration change is the point of no > return, > > > + all actions are pushed trough, even if some of them fail. > > > + > > > + @rtype: int > > > + @return: the intended return value > > > + > > > + """ > > > + returnvalue = 0 > > > > > > > Wouldn't it be more consistent to use a bool as in the other functions > > instead of an int? Given that its only possible values are 0 and 1 > anyway, > > this sounds much like a bool... > > The idea is that this is always the last task of any command, so you can > tail-recursively call it as > > return _UpgradeAfterConfigurationChange() > > instead of the more wordy > > if not _UpgradeAfterConfigurationChanage(): > return 1 > else: > return 0 > > > > + if version is None: > > > + return 1 > > > + configversion = _GetConfigVersion() > > > + if configversion is None: > > > + return 1 > > > + # If the upgrade we resume was an upgrade between compatible > > > + # verions (like 2.10.0 to 2.10.1), the correct configversion > > > > > > > s/verions/versions/ > > changed > > > > + # does not guarantee that the config has been updated. > > > + # However, in the case of a compatible update with the > configuration > > > + # not touched, we are running a different dirversion with the same > > > > > > > What is a dirversion? > > See below. > > > > + # config version. > > > + config_already_modified = \ > > > + (utils.IsCorrectConfigVersion(version, configversion) and > > > + not (versionstring != constants.DIR_VERSION and > > > > > > > Why do we have to check the DIR_VERSION here? I thought versionstring > holds > > something like "2.10.0" while constants.DIR_VERSION holds something like > > "2.10" (at least for me), so I guess they won't ever be equal. Or am I > > wrong here? > > DIR_VERSION is the version-dependent directory name in which the > executables > are stored. Depending on configure-time options, this is either somehting > like > "2.10" or something like "2.10.0~alpha1". As a prerequisite of automatic > updates > is that both versions are installed simultaneously, automatic upgrades are > only > possible between versions with different dir versions. Therefore, also the > version > you have to provide has to be the dirversion, i.e., a version string at > the granularity > at which you choose to have updates at configure time. > > (The reason why one might choose only-major-minor dir versions is, that > within the > same minor version you can just replace the executables on a node-per-node > bases, > and this can be handled by the package manager.) > > > Interdiff > > diff --git a/lib/client/gnt_cluster.py b/lib/client/gnt_cluster.py > index 19ea862..1fef84b 100644 > --- a/lib/client/gnt_cluster.py > +++ b/lib/client/gnt_cluster.py > @@ -1801,7 +1801,7 @@ def _UpgradeBeforeConfigurationChange(versionstring): > @type versionstring: string > @param versionstring: the version to upgrade to > @rtype: (bool, list) > - @return: pair of a bit indicating sucess and a list of rollback tasks > + @return: tuple of a bool indicating success and a list of rollback tasks > > """ > rollback = [] > @@ -1860,7 +1860,7 @@ def _SwitchVersionAndConfig(versionstring, > downgrade): > @type downgrade: bool > @param downgrade: True, if the configuration should be downgraded > @rtype: (bool, list) > - @return: pair of a boolean indicating success, and a list of > + @return: tupe of a bool indicating success, and a list of > additional rollback tasks > > """ > @@ -1977,7 +1977,7 @@ def UpgradeGanetiCommand(opts, args): > if configversion is None: > return 1 > # If the upgrade we resume was an upgrade between compatible > - # verions (like 2.10.0 to 2.10.1), the correct configversion > + # versions (like 2.10.0 to 2.10.1), the correct configversion > # does not guarantee that the config has been updated. > # However, in the case of a compatible update with the configuration > # not touched, we are running a different dirversion with the same > > > LGTM, thanks. > > > -- > Klaus Aehlig > Google Germany GmbH, Dienerstr. 12, 80331 Muenchen > Registergericht und -nummer: Hamburg, HRB 86891 > Sitz der Gesellschaft: Hamburg > Geschaeftsfuehrer: Graham Law, Christine Elizabeth Flores > -- 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, Christine Elizabeth Flores
