On Wed, Apr 06, 2011 at 06:32:42PM +0200, René Nussbaumer wrote: > Before hbal decided on the fly if an instance is migratable or not. As > we implemented failover fallback in commit d5cafd31456 we can start to > use that.
Thanks! Two comments below: > --- > htools/Ganeti/HTools/Cluster.hs | 4 +--- > htools/Ganeti/HTools/QC.hs | 6 ++---- > htools/Ganeti/OpCodes.hs | 24 +++++++----------------- > 3 files changed, 10 insertions(+), 24 deletions(-) > > diff --git a/htools/Ganeti/HTools/Cluster.hs b/htools/Ganeti/HTools/Cluster.hs > index 4588e77..17f9085 100644 > --- a/htools/Ganeti/HTools/Cluster.hs > +++ b/htools/Ganeti/HTools/Cluster.hs > @@ -1042,9 +1042,7 @@ iMoveToJob nl il idx move = > let inst = Container.find idx il > iname = Instance.name inst > lookNode = Just . Container.nameOf nl > - opF = if Instance.running inst > - then OpCodes.OpMigrateInstance iname True False > - else OpCodes.OpFailoverInstance iname False > + opF = OpCodes.OpMigrateInstance iname True False True > opR n = OpCodes.OpReplaceDisks iname (lookNode n) > OpCodes.ReplaceNewSecondary [] Nothing > in case move of > instance Arbitrary Jobs.OpStatus where > diff --git a/htools/Ganeti/OpCodes.hs b/htools/Ganeti/OpCodes.hs > index 972a2dc..375ecfb 100644 > --- a/htools/Ganeti/OpCodes.hs > +++ b/htools/Ganeti/OpCodes.hs > @@ -58,16 +58,14 @@ instance JSON ReplaceDisksMode where > data OpCode = OpTestDelay Double Bool [String] > | OpReplaceDisks String (Maybe String) ReplaceDisksMode > [Int] (Maybe String) > - | OpFailoverInstance String Bool > - | OpMigrateInstance String Bool Bool > + | OpMigrateInstance String Bool Bool Bool > deriving (Show, Read, Eq) So you actually remove the Failover opcode. Even though it's not used (at the moment), I'd rather keep it (at least until it's removed from Ganeti too). > opID :: OpCode -> String > opID (OpTestDelay _ _ _) = "OP_TEST_DELAY" > opID (OpReplaceDisks _ _ _ _ _) = "OP_INSTANCE_REPLACE_DISKS" > -opID (OpFailoverInstance _ _) = "OP_INSTANCE_FAILOVER" > -opID (OpMigrateInstance _ _ _) = "OP_INSTANCE_MIGRATE" > +opID (OpMigrateInstance _ _ _ _) = "OP_INSTANCE_MIGRATE" > > loadOpCode :: JSValue -> J.Result OpCode > loadOpCode v = do > @@ -87,15 +85,12 @@ loadOpCode v = do > disks <- extract "disks" > ialloc <- maybeFromObj o "iallocator" > return $ OpReplaceDisks inst node mode disks ialloc > - "OP_INSTANCE_FAILOVER" -> do > - inst <- extract "instance_name" > - consist <- extract "ignore_consistency" > - return $ OpFailoverInstance inst consist > "OP_INSTANCE_MIGRATE" -> do > inst <- extract "instance_name" > live <- extract "live" > cleanup <- extract "cleanup" > - return $ OpMigrateInstance inst live cleanup > + allow_fallback <- extract "allow_fallback" Does this mean that the allow_fallback is a required parameter? In the Python code, to me it seems like this can be missing (and then it defaults to False). In that case, what you want is to use maybeFromObj (which returns a Maybe type), and convert that from Nothing to False, like this: allow_fallback <- liftM (fromMaybe False) $ maybeFromObj o "allow_fallback" (not sure it compiles, didn't try it). Basically, you use maybeFromObj, which can return (in that monad) a Maybe Bool (so it can be Nothing, Just False, Just True), and you pass that through (fromMaybe False) so that the Nothing is removed (converted to False) and the Just is dropped. liftM is still needed due to the monad. You can also leave it like this and I fix it in a subsequent patch. PS: Maybe I should add a new utility which specifies a default. thanks! iustin
