Hi!

* Michele Tartara <[email protected]> [2014-01-09 15:28:48 +0100]:

> Hi Dimitris,
> 
> On Wed, Jan 8, 2014 at 8:55 PM, Dimitris Aragiorgis <[email protected]> wrote:
> > In case of DRBD, hooks run on both primary (source) and secondary
> > (target) nodes. To get the same behavior for DTS_EXT_MIRROR, where we
> > do not have secondary node, we should explicitly add target node to
> > hooks nodes during instance migration/failover.
> >
> > CheckPrereq() of TLMigrateInstance runs before BuildHooksManager(),
> > thus target_node calculated by Iallocator is available under
> > self._migrater.target_node. Use this value instead of
> > self.op.target_node which can be None.
> >
> > Update related doc entries.
> >
> > Signed-off-by: Dimitris Aragiorgis <[email protected]>
> > ---
> >  doc/hooks.rst                    |    8 ++++----
> >  lib/cmdlib/instance_migration.py |    8 +++++---
> >  2 files changed, 9 insertions(+), 7 deletions(-)
> >
> > diff --git a/doc/hooks.rst b/doc/hooks.rst
> > index c7fa9fb..5b317d2 100644
> > --- a/doc/hooks.rst
> > +++ b/doc/hooks.rst
> > @@ -379,8 +379,8 @@ and secondary before failover.
> >
> >  :directory: instance-failover
> >  :env. vars: IGNORE_CONSISTENCY, SHUTDOWN_TIMEOUT, OLD_PRIMARY, 
> > OLD_SECONDARY, NEW_PRIMARY, NEW_SECONDARY
> > -:pre-execution: master node, secondary node
> > -:post-execution: master node, primary and secondary nodes
> > +:pre-execution: master node, secondary (target) node
> > +:post-execution: master node, primary (source) and secondary (target) nodes
> >
> >  OP_INSTANCE_MIGRATE
> >  ++++++++++++++++++++
> > @@ -391,8 +391,8 @@ and secondary before migration.
> >
> >  :directory: instance-migrate
> >  :env. vars: MIGRATE_LIVE, MIGRATE_CLEANUP, OLD_PRIMARY, OLD_SECONDARY, 
> > NEW_PRIMARY, NEW_SECONDARY
> > -:pre-execution: master node, primary and secondary nodes
> > -:post-execution: master node, primary and secondary nodes
> > +:pre-execution: master node, primary (source) and secondary (target) nodes
> > +:post-execution: master node, primary (source) and secondary (target) nodes
> >
> >
> >  OP_INSTANCE_REMOVE
> > diff --git a/lib/cmdlib/instance_migration.py 
> > b/lib/cmdlib/instance_migration.py
> > index a5cbf4d..b680443 100644
> > --- a/lib/cmdlib/instance_migration.py
> > +++ b/lib/cmdlib/instance_migration.py
> > @@ -134,7 +134,7 @@ class LUInstanceFailover(LogicalUnit):
> >      """
> >      instance = self._migrater.instance
> >      source_node = instance.primary_node
> > -    target_node = self.op.target_node
> > +    target_node = self._migrater.target_node
> >      env = {
> >        "IGNORE_CONSISTENCY": self.op.ignore_consistency,
> >        "SHUTDOWN_TIMEOUT": self.op.shutdown_timeout,
> > @@ -159,6 +159,7 @@ class LUInstanceFailover(LogicalUnit):
> >      """
> >      instance = self._migrater.instance
> >      nl = [self.cfg.GetMasterNode()] + list(instance.secondary_nodes)
> > +    nl.append(self._migrater.target_node)
> >      return (nl, nl + [instance.primary_node])
> >
> >
> > @@ -197,7 +198,7 @@ class LUInstanceMigrate(LogicalUnit):
> >      """
> >      instance = self._migrater.instance
> >      source_node = instance.primary_node
> > -    target_node = self.op.target_node
> > +    target_node = self._migrater.target_node
> >      env = BuildInstanceHookEnvByObject(self, instance)
> >      env.update({
> >        "MIGRATE_LIVE": self._migrater.live,
> > @@ -211,7 +212,7 @@ class LUInstanceMigrate(LogicalUnit):
> >        env["OLD_SECONDARY"] = target_node
> >        env["NEW_SECONDARY"] = source_node
> >      else:
> > -      env["OLD_SECONDARY"] = env["NEW_SECONDARY"] = None
> > +      env["OLD_SECONDARY"] = env["NEW_SECONDARY"] = ""
> 
> Why changing the value from None to an empty string?
> 

I think having "" and not "None" for a enviroment variable in
bash scipts is the right way to go.

> >
> >      return env
> >
> > @@ -222,6 +223,7 @@ class LUInstanceMigrate(LogicalUnit):
> >      instance = self._migrater.instance
> >      snodes = list(instance.secondary_nodes)
> >      nl = [self.cfg.GetMasterNode(), instance.primary_node] + snodes
> > +    nl.append(self._migrater.target_node)
> >      return (nl, nl)
> >
> >
> > --
> > 1.7.10.4
> >
> 
> The patch in general looks good, but this seems to be an extension of
> the functionality of the hook, not a bugfix.
> As of now, 2.8 is not only stable, but old stable (2.9 is out as
> well). Therefore we limit new code in the 2.8 branch to fixing actual
> bugs.
> 
> Therefore, I'd suggest you to resubmit this for 2.10, which is not
> stable yet, or to 2.9 if you think there is a good reason to have it
> out as part of the source code as soon as possible.
> 

Ok then. I 'll resubmit this it for 2.10.

Thanks,
dimara

> Thanks,
> Michele
> 
> -- 
> 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

Attachment: signature.asc
Description: Digital signature

Reply via email to