On Mon, Apr 14, 2014 at 06:59PM, Jose A. Lopes wrote: > Hi Ilias, > > Can you please explain me why this change is necessary? I don't > really see why moving the disks to top-level requires changing the > hooks. Your comment says that the instance is not available in the > config anymore, but I guess that was also true before this change. > > Thanks, > Jose
Hello Jose,
As I say in the commit message, the function 'BuildHooksEnv' calls the
function 'BuildInstanceHookEnvByObject' which takes an instance object
and builds the env variables from its primary nodes, secondary nodes,
disks etc. Up until now that worked because the instance object had all
that information. Since the disks objects are not attached to the
instance object any more, the 'BuildInstanceHookEnvByObject' function
needs to access the config file in order to get the disks and the
secondary nodes for an instance object (this change has been made in the
next patch).
The function 'BuildHooksEnv' is called before and after the 'Exec'
function. In case of 'LUInstanceRemove', the second time it will be
called (after the 'Exec' function has finished) the instance object will
not be in the config file any more and so the
'BuildInstanceHookEnvByObject' function will not be able to use the
config file to retrieve the disks and the secondary nodes for the given
instance.
This patch fixes the above problem by computing/storing the above values
at the beginning of the LU (while they are still available) and then
passes them to 'BuildInstanceHookEnvByObject'.
Thanks,
Ilias
> On Apr 14 14:29, Ilias Tsitsimpis wrote:
> > The function BuildHooksEnv in LUInstanceRemove, calls the function
> > BuildInstanceHookEnvByObject which needs to query the config to get
> > the secondary_nodes/disks of an instance. The function BuildHooksEnv
> > will be called before and after the Exec function. The second time
> > it will fail to query for the secondary_nodes/disks of the instance as
> > the instance will not be available at the config any more.
> >
> > This patch changes BuildInstanceHookEnvByObject to allow it
> > to override the secondary_nodes/disks of an instance.
> > ---
> > lib/cmdlib/backup.py | 7 ++++++-
> > lib/cmdlib/instance.py | 6 +++++-
> > lib/cmdlib/instance_utils.py | 16 +++++++++++++---
> > 3 files changed, 24 insertions(+), 5 deletions(-)
> >
> > diff --git a/lib/cmdlib/backup.py b/lib/cmdlib/backup.py
> > index 67582c4..4787e3d 100644
> > --- a/lib/cmdlib/backup.py
> > +++ b/lib/cmdlib/backup.py
> > @@ -173,7 +173,9 @@ class LUBackupExport(LogicalUnit):
> > "REMOVE_INSTANCE": str(bool(self.op.remove_instance)),
> > }
> >
> > - env.update(BuildInstanceHookEnvByObject(self, self.instance))
> > + env.update(BuildInstanceHookEnvByObject(
> > + self, self.instance,
> > + secondary_nodes=self.secondary_nodes, disks=self.inst_disks))
> >
> > return env
> >
> > @@ -312,6 +314,9 @@ class LUBackupExport(LogicalUnit):
> > raise errors.OpPrereqError("Zeroing timeout options can only be
> > used"
> > " only with the --zero-free-space
> > option")
> >
> > + self.secondary_nodes = self.instance.secondary_nodes
> > + self.inst_disks = self.instance.disks
> > +
> > def _CleanupExports(self, feedback_fn):
> > """Removes exports of current instance from all other nodes.
> >
> > diff --git a/lib/cmdlib/instance.py b/lib/cmdlib/instance.py
> > index 3a7a5ac..e5e29fd 100644
> > --- a/lib/cmdlib/instance.py
> > +++ b/lib/cmdlib/instance.py
> > @@ -1794,7 +1794,9 @@ class LUInstanceRemove(LogicalUnit):
> > This runs on master, primary and secondary nodes of the instance.
> >
> > """
> > - env = BuildInstanceHookEnvByObject(self, self.instance)
> > + env = BuildInstanceHookEnvByObject(self, self.instance,
> > +
> > secondary_nodes=self.secondary_nodes,
> > + disks=self.inst_disks)
> > env["SHUTDOWN_TIMEOUT"] = self.op.shutdown_timeout
> > return env
> >
> > @@ -1815,6 +1817,8 @@ class LUInstanceRemove(LogicalUnit):
> > self.instance = self.cfg.GetInstanceInfo(self.op.instance_uuid)
> > assert self.instance is not None, \
> > "Cannot retrieve locked instance %s" % self.op.instance_name
> > + self.secondary_nodes = self.instance.secondary_nodes
> > + self.inst_disks = self.instance.disks
> >
> > def Exec(self, feedback_fn):
> > """Remove the instance.
> > diff --git a/lib/cmdlib/instance_utils.py b/lib/cmdlib/instance_utils.py
> > index 4670397..628c1bb 100644
> > --- a/lib/cmdlib/instance_utils.py
> > +++ b/lib/cmdlib/instance_utils.py
> > @@ -147,7 +147,8 @@ def BuildInstanceHookEnv(name, primary_node_name,
> > secondary_node_names, os_type,
> > return env
> >
> >
> > -def BuildInstanceHookEnvByObject(lu, instance, override=None):
> > +def BuildInstanceHookEnvByObject(lu, instance, secondary_nodes=None,
> > + disks=None, override=None):
> > """Builds instance related env variables for hooks from an object.
> >
> > @type lu: L{LogicalUnit}
> > @@ -165,10 +166,19 @@ def BuildInstanceHookEnvByObject(lu, instance,
> > override=None):
> > cluster = lu.cfg.GetClusterInfo()
> > bep = cluster.FillBE(instance)
> > hvp = cluster.FillHV(instance)
> > +
> > + # Override secondary_nodes
> > + if secondary_nodes is None:
> > + secondary_nodes = instance.secondary_nodes
> > +
> > + # Override disks
> > + if disks is None:
> > + disks = instance.disks
> > +
> > args = {
> > "name": instance.name,
> > "primary_node_name": lu.cfg.GetNodeName(instance.primary_node),
> > - "secondary_node_names": lu.cfg.GetNodeNames(instance.secondary_nodes),
> > + "secondary_node_names": lu.cfg.GetNodeNames(secondary_nodes),
> > "os_type": instance.os,
> > "status": instance.admin_state,
> > "maxmem": bep[constants.BE_MAXMEM],
> > @@ -177,7 +187,7 @@ def BuildInstanceHookEnvByObject(lu, instance,
> > override=None):
> > "nics": NICListToTuple(lu, instance.nics),
> > "disk_template": instance.disk_template,
> > "disks": [(disk.name, disk.uuid, disk.size, disk.mode)
> > - for disk in instance.disks],
> > + for disk in disks],
> > "bep": bep,
> > "hvp": hvp,
> > "hypervisor_name": instance.hypervisor,
> > --
> > 1.9.1
> >
signature.asc
Description: Digital signature
