Hey Klaus, thanks for the review. Responses inline.
On Monday, September 12, 2016 at 7:16:06 AM UTC-4, Klaus Aehlig wrote:
>
> > +extractOpCode :: JobWithStat -> Maybe OpCode
> > +extractOpCode job =
> > + let qop = listToMaybe . qjOps . jJob $ job
> > + metaOps = maybe [] (JQ.toMetaOpCode . qoInput) qop
> > + in (fmap metaOpCode) . listToMaybe $ metaOps
>
> As it is only used in a heuristics, it probably is fine; however it
> should be at least mentioned in a comment that only the first opcode
> of a job is returned (note that a job may be composed of several opcodes).
> A better name might be extractFirstOpCode.
>
Good point, I'll change it to reflect this.
> > +-- | Check if the opcode type requires the Big Ganeti Lock or not.
> > +hasBGL :: OpCode -> Bool
> > +hasBGL OpNodeAdd{} = True
> > +hasBGL OpNodeRemove{} = True
> > +hasBGL OpClusterActivateMasterIp{} = True
> > +hasBGL OpClusterDeactivateMasterIp{} = True
> > +hasBGL OpClusterDestroy{} = True
> > +hasBGL OpClusterPostInit{} = True
> > +hasBGL OpClusterRename{} = True
> > +hasBGL _ = False
>
> What about OpTestAllocator? Also, it might be worth to add a test to keep
> this predicate consistent with REQ_BGL_WHITELIST from
> test/py/ganeti.mcpu_unittest.p;
> this can happen in a follow up patch.
>
Argh I knew I had missed something. It's not a very used opcode but it's
good to have it as well, I'll add it.
> > @@ -712,6 +740,7 @@ genNodeGroup = do
> > serial tags
> > return group
> >
> > +
>
> unrelated whitespace change?
>
> > instance Arbitrary NodeGroup where
> > arbitrary = genNodeGroup
> >
> > --- a/test/hs/Test/Ganeti/TestCommon.hs
> > +++ b/test/hs/Test/Ganeti/TestCommon.hs
> > @@ -264,16 +264,17 @@ genPrintableAsciiStringNE = do
> > -- | Generates a single name component.
> > genName :: Gen String
> > genName = do
> > - n <- choose (1, 16)
> > - dn <- vector n
> > - return (map dnsGetChar dn)
> > + n <- choose (1, 40) :: Gen Int
> > + return $ "name" ++ show n
>
> This change is dangerous: genName is also used in tests different from the
> ones
> you added, in particular in ones used to verify encoding and decoding; so
> using
> only those specific names makes those exisiting tests less useful. What is
> the
> purpose of that change?
I've been running the test suite multiple times as I was changing the tests
and I didn't see any failures in the rest of the test cases. Is this really
that dangerous? I understand the logic but the problem is that it
definitely was not caught by our test suite so we might want to reconsider
how critical this is for our tests.
Regardless of this, I have changed this function because for our opcode
creation we need more realistic and reasonable naming. The static lock
system looks up instance/group/node names on the configdata and expands
them accordingly (so for example instance 'foo' gets expanded to
'foo.bar.com'), if we generate random name data that cannot be looked up in
the configdata then we're going to get approximate and non-realistic lock
declarations and predictions. What kind of troubles for decoding/encoding
can this change introduce?
>
>
> -- | Generates an entire FQDN.
> > genFQDN :: Gen String
> > genFQDN = do
> > - ncomps <- choose (1, 4)
> > - names <- vectorOf ncomps genName
> > - return $ intercalate "." names
> > + domain <- ("domain" ++) . show <$> (choose (1, 5) :: Gen Int)
> > + cluster <- ("cluster" ++) . show <$> (choose (1, 10) :: Gen Int)
> > + tld <- elements [ "com", "net", "org", "gov" ]
> > + name <- genName
> > + return $ name ++ "." ++ cluster ++ "." ++ domain ++ "." ++ tld
>
> dito
>
> --
> Klaus Aehlig
> Google Germany GmbH, Erika-Mann-Str. 33, 80636 Muenchen
> Registergericht und -nummer: Hamburg, HRB 86891
> Sitz der Gesellschaft: Hamburg
> Geschaeftsfuehrer: Matthew Scott Sucherman, Paul Terence Manicle
>
