LGTM, thanks
On Fri, May 30, 2014 at 1:26 PM, Jose A. Lopes <[email protected]> wrote: > On May 30 13:06, Hrvoje Ribicic wrote: > > On Wed, May 28, 2014 at 12:37 PM, 'Jose A. Lopes' via ganeti-devel < > > [email protected]> wrote: > > > > > Fix instance queries to report USER_down when the instance is not > > > running but the admin state is ADMINST_down and the admin state source > > > is the user. > > > > > > * In instance info, fix 'hvparams' to get the dict filled with the > > > cluster defaults and fix the reported user down state. > > > > > > * In instance list (in Python and Haskell), > > > > > > Fix instance list (in Python and Haskell) to report USER_down in the > same > > > > > > > Stray or incomplete sentence? > > Fix instance queries to correctly report USER_down > > * Fix instance queries (gnt-instance list and info) in Python and > Haskell to report 'USER_down' when the instance is not running but > the admin state is 'ADMINST_down' and the admin state source is the > user. > > * Fix 'hvparams' to get the dict filled with the cluster defaults. > > > > > > > > > > > Signed-off-by: Jose A. Lopes <[email protected]> > > > --- > > > lib/cmdlib/instance_query.py | 22 ++++++++++----- > > > lib/query.py | 64 > > > +++++++++++++++++++++++++++++--------------- > > > src/Ganeti/Query/Instance.hs | 33 +++++++++++++++-------- > > > 3 files changed, 79 insertions(+), 40 deletions(-) > > > > > > diff --git a/lib/cmdlib/instance_query.py > b/lib/cmdlib/instance_query.py > > > index 36ae190..a5fab51 100644 > > > --- a/lib/cmdlib/instance_query.py > > > +++ b/lib/cmdlib/instance_query.py > > > @@ -28,7 +28,7 @@ from ganeti import constants > > > from ganeti import locking > > > from ganeti.cmdlib.base import NoHooksLU > > > from ganeti.cmdlib.common import ShareAll, GetWantedInstances, \ > > > - CheckInstancesNodeGroups, AnnotateDiskParams, GetUpdatedParams > > > + CheckInstancesNodeGroups, AnnotateDiskParams > > > from ganeti.cmdlib.instance_utils import NICListToTuple > > > from ganeti.hypervisor import hv_base > > > > > > @@ -215,6 +215,7 @@ class LUInstanceQueryData(NoHooksLU): > > > > > > for instance in self.wanted_instances: > > > pnode = nodes[instance.primary_node] > > > + hvparams = cluster.FillHV(instance, skip_globals=True) > > > > > > if self.op.static or pnode.offline: > > > remote_state = None > > > @@ -228,12 +229,14 @@ class LUInstanceQueryData(NoHooksLU): > > > cluster.hvparams[instance.hypervisor]) > > > remote_info.Raise("Error checking node %s" % pnode.name) > > > remote_info = remote_info.payload > > > + > > > + allow_userdown = \ > > > + instance.hypervisor != constants.HT_KVM or \ > > > + hvparams[constants.HV_KVM_USER_SHUTDOWN] > > > > > > > Only Xen and KVM support this at the moment - could the if reflect that? > > As discussed offline, whether the hypevisor supports userdown does not > change this code. > > > > > > + > > > if remote_info and "state" in remote_info: > > > - hvparams = GetUpdatedParams(instance.hvparams, > > > - > > > cluster.hvparams[instance.hypervisor]) > > > if hv_base.HvInstanceState.IsShutdown(remote_info["state"]): > > > - if (instance.hypervisor != constants.HT_KVM > > > - or hvparams[constants.HV_KVM_USER_SHUTDOWN]): > > > + if allow_userdown: > > > remote_state = "user down" > > > else: > > > remote_state = "down" > > > @@ -242,8 +245,13 @@ class LUInstanceQueryData(NoHooksLU): > > > else: > > > if instance.admin_state == constants.ADMINST_UP: > > > remote_state = "down" > > > + elif instance.admin_state == constants.ADMINST_DOWN: > > > + if instance.admin_state_source == constants.USER_SOURCE: > > > + remote_state = "user down" > > > + else: > > > + remote_state = "down" > > > else: > > > - remote_state = instance.admin_state > > > + remote_state = "offline" > > > > > > group2name_fn = lambda uuid: groups[uuid].name > > > node_uuid2name_fn = lambda uuid: nodes[uuid].name > > > @@ -273,7 +281,7 @@ class LUInstanceQueryData(NoHooksLU): > > > "hypervisor": instance.hypervisor, > > > "network_port": instance.network_port, > > > "hv_instance": instance.hvparams, > > > - "hv_actual": cluster.FillHV(instance, skip_globals=True), > > > + "hv_actual": hvparams, > > > "be_instance": instance.beparams, > > > "be_actual": cluster.FillBE(instance), > > > "os_instance": instance.osparams, > > > diff --git a/lib/query.py b/lib/query.py > > > index f3fc1f0..e42a862 100644 > > > --- a/lib/query.py > > > +++ b/lib/query.py > > > @@ -1524,6 +1524,42 @@ def _GetInstLiveData(name): > > > return fn > > > > > > > > > +def _GetLiveInstStatus(ctx, instance, instance_state): > > > + hvparams = ctx.cluster.FillHV(instance, skip_globals=True) > > > + > > > + allow_userdown = \ > > > + instance.hypervisor != constants.HT_KVM or \ > > > + hvparams[constants.HV_KVM_USER_SHUTDOWN] > > > + > > > + if instance.uuid in ctx.wrongnode_inst: > > > + return constants.INSTST_WRONGNODE > > > + else: > > > + if hv_base.HvInstanceState.IsShutdown(instance_state): > > > + if instance.admin_state == constants.ADMINST_UP and > allow_userdown: > > > + return constants.INSTST_USERDOWN > > > + elif instance.admin_state == constants.ADMINST_UP: > > > + return constants.INSTST_ERRORDOWN > > > + else: > > > + return constants.INSTST_ADMINDOWN > > > + else: > > > + if instance.admin_state == constants.ADMINST_UP: > > > + return constants.INSTST_RUNNING > > > + else: > > > + return constants.INSTST_ERRORUP > > > + > > > + > > > +def _GetDeadInstStatus(inst): > > > + if inst.admin_state == constants.ADMINST_UP: > > > + return constants.INSTST_ERRORDOWN > > > + elif inst.admin_state == constants.ADMINST_DOWN: > > > + if inst.admin_state_source == constants.USER_SOURCE: > > > + return constants.INSTST_USERDOWN > > > + else: > > > + return constants.INSTST_ADMINDOWN > > > + else: > > > + return constants.INSTST_ADMINOFFLINE > > > + > > > + > > > def _GetInstStatus(ctx, inst): > > > """Get instance status. > > > > > > @@ -1541,28 +1577,9 @@ def _GetInstStatus(ctx, inst): > > > instance_live_data = ctx.live_data.get(inst.uuid) > > > > > > if bool(instance_live_data): > > > - instance_state = instance_live_data["state"] > > > - > > > - if inst.uuid in ctx.wrongnode_inst: > > > - return constants.INSTST_WRONGNODE > > > - else: > > > - if hv_base.HvInstanceState.IsShutdown(instance_state): > > > - if inst.admin_state == constants.ADMINST_UP: > > > - return constants.INSTST_USERDOWN > > > - else: > > > - return constants.INSTST_ADMINDOWN > > > - else: > > > - if inst.admin_state == constants.ADMINST_UP: > > > - return constants.INSTST_RUNNING > > > - else: > > > - return constants.INSTST_ERRORUP > > > - > > > - if inst.admin_state == constants.ADMINST_UP: > > > - return constants.INSTST_ERRORDOWN > > > - elif inst.admin_state == constants.ADMINST_DOWN: > > > - return constants.INSTST_ADMINDOWN > > > - > > > - return constants.INSTST_ADMINOFFLINE > > > + return _GetLiveInstStatus(ctx, inst, instance_live_data["state"]) > > > + else: > > > + return _GetDeadInstStatus(inst) > > > > > > > > > def _GetInstDisk(index, cb): > > > @@ -2183,6 +2200,9 @@ def _BuildInstanceFields(): > > > (_MakeField("admin_state", "InstanceState", QFT_TEXT, > > > "Desired state of instance"), > > > IQ_CONFIG, 0, _GetItemAttr("admin_state")), > > > + (_MakeField("admin_state_source", "InstanceStateSource", QFT_TEXT, > > > + "Who last changed the desired state of instance"), > > > > > > > Nitpick: "of the instance" below, should be the same here > > diff --git a/lib/query.py b/lib/query.py > index e42a862..515e979 100644 > --- a/lib/query.py > +++ b/lib/query.py > @@ -2198,16 +2198,16 @@ def _BuildInstanceFields(): > lambda ctx, inst: map(compat.partial(_GetInstNodeGroup, ctx, None), > inst.secondary_nodes)), > (_MakeField("admin_state", "InstanceState", QFT_TEXT, > - "Desired state of instance"), > + "Desired state of the instance"), > IQ_CONFIG, 0, _GetItemAttr("admin_state")), > - (_MakeField("admin_state_source", "InstanceStateSource", QFT_TEXT, > - "Who last changed the desired state of instance"), > - IQ_CONFIG, 0, _GetItemAttr("admin_state_source")), > (_MakeField("admin_up", "Autostart", QFT_BOOL, > - "Desired state of instance"), > + "Desired state of the instance"), > IQ_CONFIG, 0, lambda ctx, inst: inst.admin_state == > constants.ADMINST_UP), > + (_MakeField("admin_state_source", "InstanceStateSource", QFT_TEXT, > + "Who last changed the desired state of the instance"), > + IQ_CONFIG, 0, _GetItemAttr("admin_state_source")), > (_MakeField("disks_active", "DisksActive", QFT_BOOL, > - "Desired state of instance disks"), > + "Desired state of the instance disks"), > IQ_CONFIG, 0, _GetItemAttr("disks_active")), > (_MakeField("tags", "Tags", QFT_OTHER, "Tags"), IQ_CONFIG, 0, > lambda ctx, inst: list(inst.GetTags())), > > > > > > + IQ_CONFIG, 0, _GetItemAttr("admin_state_source")), > > > (_MakeField("admin_up", "Autostart", QFT_BOOL, > > > "Desired state of instance"), > > > IQ_CONFIG, 0, lambda ctx, inst: inst.admin_state == > > > constants.ADMINST_UP), > > > diff --git a/src/Ganeti/Query/Instance.hs > b/src/Ganeti/Query/Instance.hs > > > index 856bbca..e40b81f 100644 > > > --- a/src/Ganeti/Query/Instance.hs > > > +++ b/src/Ganeti/Query/Instance.hs > > > @@ -100,6 +100,10 @@ instanceFields = > > > [ (FieldDefinition "admin_state" "InstanceState" QFTText > > > "Desired state of instance", > > > FieldSimple (rsNormal . adminStateToRaw . instAdminState), > QffNormal) > > > + , (FieldDefinition "admin_state_source" "InstanceStateSource" > QFTText > > > + "Who last changed the desired state of the instance", > > > + FieldSimple (rsNormal . adminStateSourceToRaw . > > > instAdminStateSource), > > > + QffNormal) > > > , (FieldDefinition "admin_up" "Autostart" QFTBool > > > "Desired state of instance", > > > FieldSimple (rsNormal . (== AdminUp) . instAdminState), > QffNormal) > > > @@ -621,20 +625,26 @@ isPrimaryOffline cfg inst = > > > Bad _ -> error "Programmer error - result assumed to be OK is > > > Bad!" > > > > > > -- | Determines the status of a live instance > > > -liveInstanceStatus :: (InstanceInfo, Bool) -> Instance -> > InstanceStatus > > > -liveInstanceStatus (instInfo, foundOnPrimary) inst > > > +liveInstanceStatus :: ConfigData > > > + -> (InstanceInfo, Bool) > > > + -> Instance > > > + -> InstanceStatus > > > +liveInstanceStatus cfg (instInfo, foundOnPrimary) inst > > > | not foundOnPrimary = WrongNode > > > | otherwise = > > > case instanceState of > > > - InstanceStateRunning | adminState == AdminUp -> Running > > > - | otherwise -> ErrorUp > > > - InstanceStateShutdown | adminState == AdminUp && allowDown -> > > > UserDown > > > - | adminState == AdminUp -> ErrorDown > > > - | otherwise -> StatusDown > > > + InstanceStateRunning > > > + | adminState == AdminUp -> Running > > > + | otherwise -> ErrorUp > > > + InstanceStateShutdown > > > + | adminState == AdminUp && allowDown -> UserDown > > > + | adminState == AdminUp -> ErrorDown > > > + | otherwise -> StatusDown > > > where adminState = instAdminState inst > > > instanceState = instInfoState instInfo > > > > > > - hvparams = fromContainer $ instHvparams inst > > > + hvparams = > > > + fromContainer $ getFilledInstHvParams (C.toList > C.hvcGlobals) > > > cfg inst > > > > > > allowDown = > > > instHypervisor inst /= Kvm || > > > @@ -645,8 +655,9 @@ liveInstanceStatus (instInfo, foundOnPrimary) inst > > > deadInstanceStatus :: Instance -> InstanceStatus > > > deadInstanceStatus inst = > > > case instAdminState inst of > > > - AdminUp -> ErrorDown > > > - AdminDown -> StatusDown > > > + AdminUp -> ErrorDown > > > + AdminDown | instAdminStateSource inst == UserSource -> UserDown > > > + | otherwise -> StatusDown > > > AdminOffline -> StatusOffline > > > > > > -- | Determines the status of the instance, depending on whether it is > > > possible > > > @@ -660,7 +671,7 @@ determineInstanceStatus cfg res inst > > > | isPrimaryOffline cfg inst = NodeOffline > > > | otherwise = case res of > > > Left _ -> NodeDown > > > - Right (Just liveData, _) -> liveInstanceStatus liveData inst > > > + Right (Just liveData, _) -> liveInstanceStatus cfg liveData inst > > > Right (Nothing, _) -> deadInstanceStatus inst > > > > > > -- | Extracts the instance status, retrieving it using the functions > > > above and > > > -- > > > 1.9.1.423.g4596e3a > > > > > > > > Barring the nitpicks, LGTM, thanks > > -- > 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 >
