On Thu, Feb 14, 2013 at 3:20 PM, Michael Hanselmann <[email protected]>wrote:
> 2013/2/14 Michele Tartara <[email protected]>: > > --- a/lib/client/gnt_instance.py > > +++ b/lib/client/gnt_instance.py > > - > shutdown_timeout=opts.shutdown_timeout) > > + > shutdown_timeout=opts.shutdown_timeout, > > + reason=(opts.reason, > > + > constants.INSTANCE_REASON_SOURCE_CLI)) > > This is already much better, but please swap the two elements. In > other places we use similar constructs and have the “static” element > first. The second then depends on the first. > > Ok. > > --- a/lib/constants.py > > +++ b/lib/constants.py > > +# The default reasons for the change of state of an instance > > +INSTANCE_REASON_REBOOT = "Reboot" > > See below for RAPI, but if you want to use the same value, you must > use a constants in rapi/client.py too and add a unittest to make sure > the defaults are the same. See “ganeti.rapi.client_unittest.py”. > I'm answering below, after the rapi/client.py comment. > > > # Do not re-export imported modules > > del re, _vcsversion, _autoconf, socket, pathutils, compat > > diff --git a/lib/opcodes.py b/lib/opcodes.py > > index 2ebf29c..977fb34 100644 > > --- a/lib/opcodes.py > > +++ b/lib/opcodes.py > > @@ -1451,6 +1451,12 @@ class OpInstanceReboot(OpCode): > > "Whether to start the instance even if secondary disks are > failing"), > > ("reboot_type", ht.NoDefault, ht.TElemOf(constants.REBOOT_TYPES), > > "How to reboot instance"), > > + ("reason", (None, constants.INSTANCE_REASON_SOURCE_UNKNOWN), > > + ht.TAnd(ht.TOr(ht.TList, ht.TTuple), > > You don't have to check for tuples. JSON always de-serializes into > lists. If it ever were to de-serialize into tuples, quite a few other > places will need updating as well. Let's keep the code simpler for > now. > Ok. > > + ht.TIsLength(2), > > + ht.TItems([ht.TMaybeString, > > + > ht.TElemOf(constants.INSTANCE_REASON_SOURCES)])), > > + "The reason why the reboot is happening"), > > > --- a/lib/rapi/client.py > > +++ b/lib/rapi/client.py > > @@ -1001,11 +1011,11 @@ class GanetiRapiClient(object): # pylint: > disable=R0904 > > (GANETI_RAPI_VERSION, instance)), query, > None) > > > > def RebootInstance(self, instance, reboot_type=None, > ignore_secondaries=None, > > - dry_run=False): > > + dry_run=False, reason_text="Reboot"): > > Is it actually good to specify a reason by default? I think it would > be better if it ended up as “unknown”. Think about users of the RAPI > client who don't update their code to include a reason. > > According to the latest version of the monitoring design document (still to receive the final LGTM, but already gone through several revision) the reason should be something like cli:reboot, or rapi:shutdown, unless a more explicit reason is specified by the user. That is, the default should be the command name, so that (for example) if an instance is rebooted willingly by the user, the change of state is recorded as a reboot command received via either RAPI or CLI. So, yes, a default reason is needed. But actually, it could be completely removed from the RAPI client, because when the RAPI server receives a reboot command with no reason, it will already add "Reboot" as the reason (taken from lib/constants.py). So I guess the best approach, with no risk of leading to inconsistencies and no need to add more unit tests, would be to update the RebootInstance command of the RAPI client to have a reason_text but no default, and to actually send it only if a reason_text is specified. In case it is not, it will be the server to automatically add "Reboot" as the reason. (and this will also work transparently for non-updated clients). > --- a/lib/rapi/rlib2.py > > +++ b/lib/rapi/rlib2.py > > @@ -1040,6 +1040,10 @@ class > R_2_instances_name_reboot(baserlib.OpcodeResource): > > self.queryargs.get("type", [constants.INSTANCE_REBOOT_HARD])[0], > > "ignore_secondaries": > bool(self._checkIntVariable("ignore_secondaries")), > > "dry_run": self.dryRun(), > > + "reason": ( > > Don't wrap after an opening parenthesis (style consistency). Just move > it to the beginning of the next line. > "It" what? The opening parenthesis or the whole self._checkIntVariable? > > > + self._checkStringVariable("reason_text", > > + > default=constants.INSTANCE_REASON_REBOOT), > > + constants.INSTANCE_REASON_SOURCE_RAPI) > > You're missing the final comma which is mandatory in Python lists (style). > > > }) > Ok. Thanks, Michele
