LGTM, thanks
On Fri, May 30, 2014 at 1:31 PM, Jose A. Lopes <[email protected]> wrote: > On May 30 13:23, Hrvoje Ribicic wrote: > > On Wed, May 28, 2014 at 12:37 PM, 'Jose A. Lopes' via ganeti-devel < > > [email protected]> wrote: > > > > > Modify opcode and LU for instance shutdown to mark an instance as user > > > down. This is used by the watcher to cleanup instances that were > > > shutdown by the user while retaining the instance status 'USER_down'. > > > > > > Signed-off-by: Jose A. Lopes <[email protected]> > > > --- > > > lib/cmdlib/instance_operation.py | 52 > > > ++++++++++++++++++++++++++++++++++------ > > > src/Ganeti/OpCodes.hs | 1 + > > > src/Ganeti/OpParams.hs | 7 ++++++ > > > 3 files changed, 53 insertions(+), 7 deletions(-) > > > > > > diff --git a/lib/cmdlib/instance_operation.py > > > b/lib/cmdlib/instance_operation.py > > > index d210b72..22e982d 100644 > > > --- a/lib/cmdlib/instance_operation.py > > > +++ b/lib/cmdlib/instance_operation.py > > > @@ -45,6 +45,15 @@ from ganeti.cmdlib.instance_utils import > > > BuildInstanceHookEnvByObject, \ > > > from ganeti.hypervisor import hv_base > > > > > > > > > +def _IsInstanceUserDown(cluster, instance, instance_info): > > > + hvparams = cluster.FillHV(instance, skip_globals=True) > > > + return instance_info and \ > > > + "state" in instance_info and \ > > > + hv_base.HvInstanceState.IsShutdown(instance_info["state"]) and \ > > > + (instance.hypervisor != constants.HT_KVM or > > > + hvparams[constants.HV_KVM_USER_SHUTDOWN]) > > > + > > > > > > > Patch 6 has code that is very similar - please take a look if this can be > > refactored. > > In patch 6, this condition is broken into multiple 'if-else' branches, > and here it is a single expression, so I didn't merge them. > > > > > > + > > > class LUInstanceStartup(LogicalUnit): > > > """Starts an instance. > > > > > > @@ -183,6 +192,17 @@ class LUInstanceShutdown(LogicalUnit): > > > def ExpandNames(self): > > > self._ExpandAndLockInstance() > > > > > > + def CheckArguments(self): > > > + """Check arguments. > > > + > > > + """ > > > + if self.op.no_remember and self.op.admin_state_source: > > > + self.LogWarning("Parameter 'admin_state_source' has no effect if > > > used" > > > + " with parameter 'no_remember'") > > > + > > > + if not self.op.admin_state_source: > > > + self.op.admin_state_source = constants.ADMIN_SOURCE > > > + > > > def BuildHooksEnv(self): > > > """Build hooks env. > > > > > > @@ -210,10 +230,10 @@ class LUInstanceShutdown(LogicalUnit): > > > assert self.instance is not None, \ > > > "Cannot retrieve locked instance %s" % self.op.instance_name > > > > > > - if not self.op.force: > > > - CheckInstanceState(self, self.instance, INSTANCE_ONLINE) > > > - else: > > > + if self.op.force: > > > self.LogWarning("Ignoring offline instance check") > > > + else: > > > + CheckInstanceState(self, self.instance, INSTANCE_ONLINE) > > > > > > self.primary_offline = \ > > > self.cfg.GetNodeInfo(self.instance.primary_node).offline > > > @@ -223,6 +243,23 @@ class LUInstanceShutdown(LogicalUnit): > > > else: > > > CheckNodeOnline(self, self.instance.primary_node) > > > > > > + if self.op.admin_state_source == constants.USER_SOURCE: > > > + cluster = self.cfg.GetClusterInfo() > > > + > > > + result = self.rpc.call_instance_info( > > > + self.instance.primary_node, > > > + self.instance.name, > > > + self.instance.hypervisor, > > > + cluster.hvparams[self.instance.hypervisor]) > > > + result.Raise("Error checking instance '%s'" % > self.instance.name, > > > + prereq=True) > > > + > > > + if not _IsInstanceUserDown(cluster, > > > + self.instance, > > > + result.payload): > > > + raise errors.OpPrereqError("Instance '%s' was not shutdown by > the > > > user" > > > + % self.instance.name) > > > > + > > > def Exec(self, feedback_fn): > > > """Shutdown the instance. > > > > > > @@ -230,7 +267,10 @@ class LUInstanceShutdown(LogicalUnit): > > > # If the instance is offline we shouldn't mark it as down, as that > > > # resets the offline flag. > > > if not self.op.no_remember and self.instance.admin_state in > > > INSTANCE_ONLINE: > > > - self.cfg.MarkInstanceDown(self.instance.uuid) > > > + if self.op.admin_state_source == constants.ADMIN_SOURCE: > > > + self.cfg.MarkInstanceDown(self.instance.uuid) > > > + elif self.op.admin_state_source == constants.USER_SOURCE: > > > + self.cfg.MarkInstanceUserDown(self.instance.uuid) > > > > > > if self.primary_offline: > > > assert self.op.ignore_offline_nodes > > > @@ -240,9 +280,7 @@ class LUInstanceShutdown(LogicalUnit): > > > self.instance.primary_node, > > > self.instance, > > > self.op.timeout, self.op.reason) > > > - msg = result.fail_msg > > > - if msg: > > > - self.LogWarning("Could not shutdown instance: %s", msg) > > > + result.Raise("Could not shutdown instance '%s'", > self.instance.name > > > ) > > > > > > ShutdownInstanceDisks(self, self.instance) > > > > > > diff --git a/src/Ganeti/OpCodes.hs b/src/Ganeti/OpCodes.hs > > > index eb24b0a..3975afe 100644 > > > --- a/src/Ganeti/OpCodes.hs > > > +++ b/src/Ganeti/OpCodes.hs > > > @@ -511,6 +511,7 @@ $(genOpCode "OpCode" > > > , pIgnoreOfflineNodes > > > , pShutdownTimeout' > > > , pNoRemember > > > + , pAdminStateSource > > > ], > > > "instance_name") > > > , ("OpInstanceReboot", > > > diff --git a/src/Ganeti/OpParams.hs b/src/Ganeti/OpParams.hs > > > index c330c83..1d077d1 100644 > > > --- a/src/Ganeti/OpParams.hs > > > +++ b/src/Ganeti/OpParams.hs > > > @@ -260,6 +260,7 @@ module Ganeti.OpParams > > > , pReason > > > , pSequential > > > , pEnabledDiskTemplates > > > + , pAdminStateSource > > > ) where > > > > > > import Control.Monad (liftM, mplus) > > > @@ -1671,3 +1672,9 @@ pNetworkLink :: Field > > > pNetworkLink = > > > withDoc "Network link when connecting to a group" $ > > > simpleField "network_link" [t| NonEmptyString |] > > > + > > > +pAdminStateSource :: Field > > > +pAdminStateSource = > > > + withDoc "Who last changed the instance admin state" . > > > + optionalField $ > > > + simpleField "admin_state_source" [t| AdminStateSource |] > > > -- > > > 1.9.1.423.g4596e3a > > > > > > > > -- > Jose Antonio Lopes > Ganeti Engineering > 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 > Steuernummer: 48/725/00206 > Umsatzsteueridentifikationsnummer: DE813741370 >
