2013/2/14 Michele Tartara <[email protected]>:
> On Thu, Feb 14, 2013 at 3:20 PM, Michael Hanselmann <[email protected]>
> wrote:
>> > --- 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).
Ack
>> > --- 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?
The former. Like this:
"reason":
(self._checkStringVariable("reason_text",
default=constants.INSTANCE_REASON_REBOOT),
…),
Michael
