On Wed, Feb 13, 2013 at 11:31 AM, Michael Hanselmann <[email protected]>wrote:
> 2013/2/12 Michele Tartara <[email protected]>: > > --- a/lib/client/gnt_instance.py > > +++ b/lib/client/gnt_instance.py > > @@ -631,7 +631,9 @@ def _RebootInstance(name, opts): > > return opcodes.OpInstanceReboot(instance_name=name, > > reboot_type=opts.reboot_type, > > > ignore_secondaries=opts.ignore_secondaries, > > - > shutdown_timeout=opts.shutdown_timeout) > > + > shutdown_timeout=opts.shutdown_timeout, > > + reason_text=opts.reason, > > + > reason_source=constants.INSTANCE_REASON_CLI) > > Why are you using two different options instead of a tuple with (source, > text)? > > No particular reason. I'll turn it into a tuple. > > --- a/lib/rapi/rlib2.py > > +++ b/lib/rapi/rlib2.py > > @@ -1040,6 +1040,8 @@ 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_text": self._checkStringVariable("reason_text", > default="Reboot"), > > + "reason_source": constants.INSTANCE_REASON_RAPI > > And another case of hardcoded defaults (see below). > Ok. > > --- a/lib/server/noded.py > > +++ b/lib/server/noded.py > > +def _DefaultAlternative(value, default): > > + return value if value else default > > This wouldn't work with Python 2.4. Please don't use inline-if yet. > Ok. > > class MlockallRequestExecutor(http.server.HttpServerRequestExecutor): > > """Subclass ensuring request handlers are locked in RAM. > > > > @@ -633,7 +645,11 @@ class > NodeRequestHandler(http.server.HttpServerHandler): > > instance = objects.Instance.FromDict(params[0]) > > reboot_type = params[1] > > shutdown_timeout = params[2] > > - return backend.InstanceReboot(instance, reboot_type, > shutdown_timeout) > > + reason_text = _DefaultAlternative(params[3], "Reboot") > > Please don't hardcode this inline, but make it a module-level constant. > Ok. > > > + reason_source = params[4] > > + reason = backend.InstReason(reason_text, reason_source) > > + return backend.InstanceReboot(instance, reboot_type, > shutdown_timeout, > > + reason) > > > > --- a/test/py/ganeti.rapi.client_unittest.py > > +++ b/test/py/ganeti.rapi.client_unittest.py > > @@ -594,13 +594,15 @@ class > GanetiRapiClientTests(testutils.GanetiTestCase): > > def testRebootInstance(self): > > self.rapi.AddResponse("6146") > > job_id = self.client.RebootInstance("i-bar", reboot_type="hard", > > - ignore_secondaries=True, > dry_run=True) > > + ignore_secondaries=True, > dry_run=True, > > + reason_text="Updates") > > self.assertEqual(6146, job_id) > > self.assertHandler(rlib2.R_2_instances_name_reboot) > > self.assertItems(["i-bar"]) > > self.assertDryRun() > > self.assertQuery("type", ["hard"]) > > self.assertQuery("ignore_secondaries", ["1"]) > > + self.assertQuery("reason_text", ["Updates"]) > > You should also test for the case where you don't specify “reason_text” at > all. > I'll add it. Thanks, Michele
