Great, thanks for the review!
On Thu, Mar 27, 2014 at 4:18 PM, Petr Pudlák <[email protected]> wrote: > Sorry, I missed that the misaligned code was actually being deleted. LGTM > > > On Thu, Mar 27, 2014 at 4:15 PM, Petr Pudlák <[email protected]> wrote: > >> >> >> >> On Thu, Mar 27, 2014 at 2:48 PM, Hrvoje Ribicic <[email protected]> wrote: >> >>> The UUID/name switch avoided this particular bit of code, and as a >>> result the list-drbd command failed as it tried to compare UUIDs and >>> names. This patch fixes the retrieval, converts the newly returned >>> UUIDs to names, and modifies the QA to the results and not only the >>> invocation are checked. >>> >>> Signed-off-by: Gerard Oskamp <[email protected]> >>> Signed-off-by: Hrvoje Ribicic <[email protected]> >>> --- >>> qa/ganeti-qa.py | 3 ++- >>> qa/qa_node.py | 18 +++++++++++++++--- >>> src/Ganeti/Confd/Server.hs | 21 +++++++++++++++++---- >>> src/Ganeti/Config.hs | 3 ++- >>> 4 files changed, 36 insertions(+), 9 deletions(-) >>> >>> diff --git a/qa/ganeti-qa.py b/qa/ganeti-qa.py >>> index 11d129a..38659b8 100755 >>> --- a/qa/ganeti-qa.py >>> +++ b/qa/ganeti-qa.py >>> @@ -728,7 +728,8 @@ def RunInstanceTests(): >>> RunTestIf("cluster-epo", qa_cluster.TestClusterEpo) >>> RunDaemonTests(instance) >>> for node in inodes: >>> - RunTestIf("haskell-confd", qa_node.TestNodeListDrbd, node) >>> + RunTestIf("haskell-confd", qa_node.TestNodeListDrbd, node, >>> + templ == constants.DT_DRBD8) >>> if len(inodes) > 1: >>> RunTestIf("group-rwops", >>> qa_group.TestAssignNodesIncludingSplit, >>> constants.INITIAL_NODE_GROUP_NAME, >>> diff --git a/qa/qa_node.py b/qa/qa_node.py >>> index 16fdb15..4d820b8 100644 >>> --- a/qa/qa_node.py >>> +++ b/qa/qa_node.py >>> @@ -32,7 +32,7 @@ import qa_config >>> import qa_error >>> import qa_utils >>> >>> -from qa_utils import AssertCommand, AssertEqual >>> +from qa_utils import AssertCommand, AssertEqual, AssertIn, >>> GetCommandOutput >>> >>> >>> def _NodeAdd(node, readd=False): >>> @@ -451,9 +451,21 @@ def TestNodeListFields(): >>> qa_utils.GenericQueryFieldsTest("gnt-node", query.NODE_FIELDS.keys()) >>> >>> >>> -def TestNodeListDrbd(node): >>> +def TestNodeListDrbd(node, is_drbd): >>> """gnt-node list-drbd""" >>> - AssertCommand(["gnt-node", "list-drbd", node.primary]) >>> + master = qa_config.GetMasterNode() >>> + result_output = GetCommandOutput(master.primary, >>> + "gnt-node list-drbd --no-header %s" % >>> + node.primary) >>> + # Meaningful to note: there is but one instance, and the node is >>> either the >>> + # primary or one of the secondaries >>> + if is_drbd: >>> + # Invoked for both primary and secondary >>> + drbd_node, _, _, _, _, drbd_peer = result_output.split() >>> + AssertIn(node.primary, [drbd_node, drbd_peer]) >>> + else: >>> + # Output should be empty, barring newlines >>> + AssertEqual(result_output.strip(), "") >>> >>> >>> def _BuildSetESCmd(action, value, node_name): >>> diff --git a/src/Ganeti/Confd/Server.hs b/src/Ganeti/Confd/Server.hs >>> index 25cbb17..05275c4 100644 >>> --- a/src/Ganeti/Confd/Server.hs >>> +++ b/src/Ganeti/Confd/Server.hs >>> @@ -29,6 +29,7 @@ module Ganeti.Confd.Server >>> , prepMain >>> ) where >>> >>> +import Control.Applicative((<$>)) >>> import Control.Concurrent >>> import Control.Monad (forever, liftM) >>> import Data.IORef >>> @@ -112,6 +113,20 @@ getNodePipByInstanceIp cfg linkipmap link instip = >>> Bad _ -> queryUnknownEntry -- either instance or node not found >>> Ok node -> (ReplyStatusOk, J.showJSON (nodePrimaryIp node)) >>> >>> +-- | Returns a node name for a given UUID >>> +uuidToNodeName :: ConfigData -> String -> Result String >>> +uuidToNodeName cfg uuid = gntErrorToResult $ nodeName <$> getNode cfg >>> uuid >>> + >>> +-- | Encodes a list of minors into a JSON representation, converting >>> UUIDs to >>> +-- names in the process >>> +encodeMinors :: ConfigData -> (String, Int, String, String, String, >>> String) >>> + -> Result J.JSValue >>> +encodeMinors cfg (node_uuid, a, b, c, d, peer_uuid) = do >>> + node_name <- uuidToNodeName cfg node_uuid >>> + peer_name <- uuidToNodeName cfg peer_uuid >>> + return . J.JSArray $ [J.showJSON node_name, J.showJSON a, J.showJSON >>> b, >>> + J.showJSON c, J.showJSON d, J.showJSON >>> peer_name] >>> + >>> -- | Builds the response to a given query. >>> buildResponse :: (ConfigData, LinkIpMap) -> ConfdRequest -> Result >>> StatusAnswer >>> buildResponse (cfg, _) (ConfdRequest { confdRqType = ReqPing }) = >>> @@ -186,11 +201,9 @@ buildResponse cdata req@(ConfdRequest { >>> confdRqType = ReqNodeDrbd }) = do >>> PlainQuery str -> return str >>> _ -> fail $ "Invalid query type " ++ show >>> (confdRqQuery req) >>> node <- gntErrorToResult $ getNode cfg node_name >>> - let minors = concatMap (getInstMinorsForNode (nodeName node)) . >>> + let minors = concatMap (getInstMinorsForNode (nodeUuid node)) . >>> M.elems . fromContainer . configInstances $ cfg >>> - encoded = [J.JSArray [J.showJSON a, J.showJSON b, J.showJSON c, >>> - J.showJSON d, J.showJSON e, J.showJSON f] | >>> >> >> Just nitpicking - the columns don't align. I'd suggest something like >> [ x, ... >> , y, ... z ] >> >> >>> - (a, b, c, d, e, f) <- minors] >>> + encoded <- mapM (encodeMinors cfg) minors >>> return (ReplyStatusOk, J.showJSON encoded) >>> >>> -- | Return the list of instances for a node (as ([primary], >>> [secondary])) given >>> diff --git a/src/Ganeti/Config.hs b/src/Ganeti/Config.hs >>> index 1f70836..fb0e2c6 100644 >>> --- a/src/Ganeti/Config.hs >>> +++ b/src/Ganeti/Config.hs >>> @@ -263,7 +263,8 @@ roleSecondary = "secondary" >>> >>> -- | Gets the list of DRBD minors for an instance that are related to >>> -- a given node. >>> -getInstMinorsForNode :: String -> Instance >>> +getInstMinorsForNode :: String -- ^ The UUID of a node. >>> + -> Instance >>> -> [(String, Int, String, String, String, String)] >>> getInstMinorsForNode node inst = >>> let role = if node == instPrimaryNode inst >>> -- >>> 1.9.1.423.g4596e3a >>> >>> >> Otherwise LGTM, thanks. (No need to resend.) >> > >
