On Thu, Jan 9, 2014 at 6:06 PM, Dimitris Aragiorgis <[email protected]> wrote: > 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.
Makes sense. But given that it's a change to something existing, it will have to be listed in the NEWS entry related to the change, as an incompatible/important change. Can you add that to the patch while you resend it for 2.10? Thanks, Michele > >> > >> > 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 > > -----BEGIN PGP SIGNATURE----- > Version: GnuPG v1.4.10 (GNU/Linux) > > iQEcBAEBAgAGBQJSztcNAAoJEHFDHex6CBG9gD8IALPeaGK1QS6s0p1Nz4ySgDnI > F4Nca2MdHcDvjEDJRYg8dI2C958xKr2LFt0WdEJrH3qZq1e0hFTLLi42bw7tCJMn > jD/7xfn0RfbtljXY/mOO7iCSK5IDxc48jPLtAfQUg7qDyys3p4rqqgPlgYrC/FSA > Vs+lxM/u7avZH1ONa5MjlmVbEvvzJV5CI56RF5WBscBpBRGY5/DrXYPUAzSui/9/ > nuzp6W0TiNw8owuJqSPJI54Pgzcc5TVI+J4WuwjDsvglj9VXUZS9QNHZBEyZe4+1 > tpJyWQwbAbrri1M5WlIwg1sdZgrla3zscCRs246qBxJs7r10N4KGF/bXb4DMuNY= > =WwB2 > -----END PGP SIGNATURE----- > -- 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
