LGTM, thanks (including the tests for hypervisor list)
On Tue, Jun 10, 2014 at 10:25 AM, 'Jose A. Lopes' via ganeti-devel < [email protected]> wrote: > While we're at it, also add a test for hypervisor list. > > diff --git a/src/Ganeti/Ssconf.hs b/src/Ganeti/Ssconf.hs > index 60848ad..b4483f9 100644 > --- a/src/Ganeti/Ssconf.hs > +++ b/src/Ganeti/Ssconf.hs > @@ -34,6 +34,7 @@ module Ganeti.Ssconf > , getNodesVmCapable > , getMasterCandidatesIps > , getMasterNode > + , parseHypervisorList > , getHypervisorList > , parseEnabledUserShutdown > , getEnabledUserShutdown > @@ -176,11 +177,16 @@ getMasterNode optPath = do > result <- readSSConfFile optPath Nothing SSMasterNode > return $ liftM rStripSpace result > > --- | Read the list of enabled hypervisors. > +-- | Parse the list of enabled hypervisors from a 'String'. > +parseHypervisorList :: String -> Result [Hypervisor] > +parseHypervisorList str = > + mapM Types.hypervisorFromRaw $ lines str > + > +-- | Read and parse the list of enabled hypervisors. > getHypervisorList :: Maybe FilePath -> IO (Result [Hypervisor]) > getHypervisorList optPath = do > - result <- readSSConfFile optPath Nothing SSHypervisorList > - return $ mapM Types.hypervisorFromRaw =<< liftM lines result > + (parseHypervisorList =<<) <$> > + readSSConfFile optPath Nothing SSHypervisorList > > -- | Parse whether user shutdown is enabled from a 'String'. > parseEnabledUserShutdown :: String -> Result Bool > diff --git a/test/hs/Test/Ganeti/Ssconf.hs b/test/hs/Test/Ganeti/Ssconf.hs > index a305d37..325cfbc 100644 > --- a/test/hs/Test/Ganeti/Ssconf.hs > +++ b/test/hs/Test/Ganeti/Ssconf.hs > @@ -36,6 +36,7 @@ import Data.List > import Test.Ganeti.TestHelper > > import qualified Ganeti.Ssconf as Ssconf > +import qualified Ganeti.Types as Types > > -- * Ssconf tests > > @@ -56,6 +57,12 @@ caseParseNodesVmCapable = do > ] > HUnit.assertEqual "Mismatch in parsed and expected result" expected > result > > +caseParseHypervisorList :: HUnit.Assertion > +caseParseHypervisorList = do > + let result = Ssconf.parseHypervisorList "kvm\nxen-pvm\nxen-hvm" > + expected = return [Types.Kvm, Types.XenPvm, Types.XenHvm] > + HUnit.assertEqual "Mismatch in parsed and expected result" expected > result > + > caseParseEnabledUserShutdown :: HUnit.Assertion > caseParseEnabledUserShutdown = do > let result1 = Ssconf.parseEnabledUserShutdown "True" > @@ -68,5 +75,6 @@ caseParseEnabledUserShutdown = do > testSuite "Ssconf" > [ 'prop_filename > , 'caseParseNodesVmCapable > + , 'caseParseHypervisorList > , 'caseParseEnabledUserShutdown > > On Jun 06 18:37, Hrvoje Ribicic wrote: > > On Fri, Jun 6, 2014 at 10:51 AM, 'Jose A. Lopes' via ganeti-devel < > > [email protected]> wrote: > > > > > Add list of enabled hypervisors, cluster wide parameter for user > > > shutdown, and a list of nodes and their respective vm capable > > > attributes to Ssconf. This will be needed by the KVM daemon to > > > determine whether it should start on a particular node. > > > > > > Signed-off-by: Jose A. Lopes <[email protected]> > > > --- > > > lib/config.py | 5 +++++ > > > lib/ssconf.py | 22 ++++++++++++++++++++++ > > > src/Ganeti/Constants.hs | 6 ++++++ > > > src/Ganeti/Ssconf.hs | 40 +++++++++++++++++++++++++++++++++++++++- > > > src/Ganeti/Types.hs | 1 + > > > 5 files changed, 73 insertions(+), 1 deletion(-) > > > > > > diff --git a/lib/config.py b/lib/config.py > > > index dd38a22..6ea2083 100644 > > > --- a/lib/config.py > > > +++ b/lib/config.py > > > @@ -2614,6 +2614,8 @@ class ConfigWriter(object): > > > for ninfo in node_infos] > > > node_snd_ips = ["%s %s" % (ninfo.name, ninfo.secondary_ip) > > > for ninfo in node_infos] > > > + node_vm_capable = ["%s=%s" % (ninfo.name, str(ninfo.vm_capable)) > > > + for ninfo in node_infos] > > > > > > instance_data = fn(instance_names) > > > off_data = fn(node.name for node in node_infos if node.offline) > > > @@ -2624,6 +2626,7 @@ class ConfigWriter(object): > > > node_data = fn(node_names) > > > node_pri_ips_data = fn(node_pri_ips) > > > node_snd_ips_data = fn(node_snd_ips) > > > + node_vm_capable_data = fn(node_vm_capable) > > > > > > cluster = self._config_data.cluster > > > cluster_tags = fn(cluster.GetTags()) > > > @@ -2660,6 +2663,7 @@ class ConfigWriter(object): > > > constants.SS_NODE_LIST: node_data, > > > constants.SS_NODE_PRIMARY_IPS: node_pri_ips_data, > > > constants.SS_NODE_SECONDARY_IPS: node_snd_ips_data, > > > + constants.SS_NODE_VM_CAPABLE: node_vm_capable_data, > > > constants.SS_OFFLINE_NODES: off_data, > > > constants.SS_ONLINE_NODES: on_data, > > > constants.SS_PRIMARY_IP_FAMILY: str(cluster.primary_ip_family), > > > @@ -2670,6 +2674,7 @@ class ConfigWriter(object): > > > constants.SS_UID_POOL: uid_pool, > > > constants.SS_NODEGROUPS: nodegroups_data, > > > constants.SS_NETWORKS: networks_data, > > > + constants.SS_ENABLED_USER_SHUTDOWN: > > > str(cluster.enabled_user_shutdown), > > > } > > > ssconf_values = self._ExtendByAllHvparamsStrings(ssconf_values, > > > all_hvparams) > > > diff --git a/lib/ssconf.py b/lib/ssconf.py > > > index a2a660f..55f9df1 100644 > > > --- a/lib/ssconf.py > > > +++ b/lib/ssconf.py > > > @@ -57,6 +57,7 @@ _VALID_KEYS = compat.UniqueFrozenset([ > > > constants.SS_NODE_LIST, > > > constants.SS_NODE_PRIMARY_IPS, > > > constants.SS_NODE_SECONDARY_IPS, > > > + constants.SS_NODE_VM_CAPABLE, > > > constants.SS_OFFLINE_NODES, > > > constants.SS_ONLINE_NODES, > > > constants.SS_PRIMARY_IP_FAMILY, > > > @@ -73,6 +74,7 @@ _VALID_KEYS = compat.UniqueFrozenset([ > > > constants.SS_HVPARAMS_XEN_KVM, > > > constants.SS_HVPARAMS_XEN_CHROOT, > > > constants.SS_HVPARAMS_XEN_LXC, > > > + constants.SS_ENABLED_USER_SHUTDOWN, > > > ]) > > > > > > #: Maximum size for ssconf files > > > @@ -321,6 +323,20 @@ class SimpleStore(object): > > > nl = data.splitlines(False) > > > return nl > > > > > > + def GetNodesVmCapable(self): > > > + """Return the cluster nodes' vm capable value. > > > + > > > + @rtype: dict of string to bool > > > + @return: mapping of node names to vm capable values > > > + > > > + """ > > > + data = self._ReadFile(constants.SS_NODE_VM_CAPABLE) > > > + vm_capable = {} > > > + for line in data.splitlines(False): > > > + (node_uuid, node_vm_capable) = line.split("=") > > > + vm_capable[node_uuid] = node_vm_capable > > > > > > > This will get you a str to str map that might be interpreted as all True > > later. > > > > > > > + return vm_capable > > > + > > > def GetNodegroupList(self): > > > """Return the list of nodegroups. > > > > > > @@ -415,6 +431,12 @@ class SimpleStore(object): > > > raise errors.ConfigurationError("Error while trying to parse > > > primary IP" > > > " family: %s" % err) > > > > > > + def GetEnabledUserShutdown(self): > > > + """Return whether user shutdown is enabled. > > > + > > > + """ > > > + return self._ReadFile(constants.SS_ENABLED_USER_SHUTDOWN) == > "True" > > > > > > > Why not a cast to bool here? > > > > > > > + > > > > > > def WriteSsconfFiles(values, dry_run=False): > > > """Update all ssconf files. > > > diff --git a/src/Ganeti/Constants.hs b/src/Ganeti/Constants.hs > > > index c13d6d6..c87510b 100644 > > > --- a/src/Ganeti/Constants.hs > > > +++ b/src/Ganeti/Constants.hs > > > @@ -3570,6 +3570,9 @@ ssNodePrimaryIps = "node_primary_ips" > > > ssNodeSecondaryIps :: String > > > ssNodeSecondaryIps = "node_secondary_ips" > > > > > > +ssNodeVmCapable :: String > > > +ssNodeVmCapable = "node_vm_capable" > > > + > > > ssOfflineNodes :: String > > > ssOfflineNodes = "offline_nodes" > > > > > > @@ -3637,6 +3640,9 @@ validSsHvparamsKeys = > > > ssFilePerms :: Int > > > ssFilePerms = 0o444 > > > > > > +ssEnabledUserShutdown :: String > > > +ssEnabledUserShutdown = "enabled_user_shutdown" > > > + > > > -- | Cluster wide default parameters > > > defaultEnabledHypervisor :: String > > > defaultEnabledHypervisor = htXenPvm > > > diff --git a/src/Ganeti/Ssconf.hs b/src/Ganeti/Ssconf.hs > > > index 3f7a64b..3704c36 100644 > > > --- a/src/Ganeti/Ssconf.hs > > > +++ b/src/Ganeti/Ssconf.hs > > > @@ -30,8 +30,11 @@ module Ganeti.Ssconf > > > , sSKeyToRaw > > > , sSKeyFromRaw > > > , getPrimaryIPFamily > > > + , getNodesVmCapable > > > , getMasterCandidatesIps > > > , getMasterNode > > > + , getHypervisorList > > > + , getEnabledUserShutdown > > > , keyToFilename > > > , sSFilePrefix > > > ) where > > > @@ -48,6 +51,8 @@ import Ganeti.BasicTypes > > > import qualified Ganeti.Constants as C > > > import qualified Ganeti.Path as Path > > > import Ganeti.THH > > > +import Ganeti.Types (Hypervisor) > > > +import qualified Ganeti.Types as Types > > > import Ganeti.Utils > > > > > > -- | Maximum ssconf file size we support. > > > @@ -73,6 +78,7 @@ $(declareSADT "SSKey" > > > , ("SSNodeList", 'C.ssNodeList) > > > , ("SSNodePrimaryIps", 'C.ssNodePrimaryIps) > > > , ("SSNodeSecondaryIps", 'C.ssNodeSecondaryIps) > > > + , ("SSNodeVmCapable", 'C.ssNodeVmCapable) > > > , ("SSOfflineNodes", 'C.ssOfflineNodes) > > > , ("SSOnlineNodes", 'C.ssOnlineNodes) > > > , ("SSPrimaryIpFamily", 'C.ssPrimaryIpFamily) > > > @@ -82,6 +88,7 @@ $(declareSADT "SSKey" > > > , ("SSMaintainNodeHealth", 'C.ssMaintainNodeHealth) > > > , ("SSUidPool", 'C.ssUidPool) > > > , ("SSNodegroups", 'C.ssNodegroups) > > > + , ("SSEnabledUserShutdown", 'C.ssEnabledUserShutdown) > > > ]) > > > > > > -- | Convert a ssconf key into a (full) file path. > > > @@ -118,6 +125,14 @@ readSSConfFile optpath def key = do > > > keyToFilename (fromMaybe dpath optpath) $ key > > > return (liftM (take maxFileSize) result) > > > > > > +-- | Parses a key-value pair of the form "key=value" from 'str', fails > > > +-- with 'desc' otherwise. > > > +parseKeyValue :: Monad m => String -> String -> m (String, String) > > > +parseKeyValue desc str = > > > + case sepSplit '=' str of > > > + [key, value] -> return (key, value) > > > + _ -> fail $ "Failed to parse key-value pair for " ++ desc > > > + > > > -- | Parses a string containing an IP family > > > parseIPFamily :: Int -> Result Socket.Family > > > parseIPFamily fam | fam == AutoConf.pyAfInet4 = Ok Socket.AF_INET > > > @@ -133,6 +148,17 @@ getPrimaryIPFamily optpath = do > > > return (liftM rStripSpace result >>= > > > tryRead "Parsing af_family" >>= parseIPFamily) > > > > > > +-- | Read the nodes vm capable value. > > > +getNodesVmCapable :: Maybe FilePath -> IO (Result [(String, Bool)]) > > > +getNodesVmCapable optPath = do > > > + result <- readSSConfFile optPath Nothing SSNodeVmCapable > > > + return $ > > > + liftM lines result >>= > > > + mapM (parseKeyValue "Parsing node_vm_capable") >>= > > > + mapM (\(x, y) -> > > > + do val <- tryRead "Parsing value of node_vm_capable" y > > > + return (x, val)) > > > + > > > -- | Read the list of IP addresses of the master candidates of the > > > cluster. > > > getMasterCandidatesIps :: Maybe FilePath -> IO (Result [String]) > > > getMasterCandidatesIps optPath = do > > > @@ -143,4 +169,16 @@ getMasterCandidatesIps optPath = do > > > getMasterNode :: Maybe FilePath -> IO (Result String) > > > getMasterNode optPath = do > > > result <- readSSConfFile optPath Nothing SSMasterNode > > > - return (liftM rStripSpace result) > > > + return $ liftM rStripSpace result > > > + > > > +-- | Read the list of enabled hypervisors > > > +getHypervisorList :: Maybe FilePath -> IO (Result [Hypervisor]) > > > +getHypervisorList optPath = do > > > + result <- readSSConfFile optPath Nothing SSHypervisorList > > > + return $ mapM Types.hypervisorFromRaw =<< liftM lines result > > > + > > > +-- | Read whether user shutdown is enabled > > > +getEnabledUserShutdown :: Maybe FilePath -> IO (Result Bool) > > > +getEnabledUserShutdown optPath = do > > > + result <- readSSConfFile optPath Nothing SSEnabledUserShutdown > > > + return $ tryRead "Parsing enabled_user_shutdown" =<< liftM > rStripSpace > > > result > > > diff --git a/src/Ganeti/Types.hs b/src/Ganeti/Types.hs > > > index 38d71b3..e61b983 100644 > > > --- a/src/Ganeti/Types.hs > > > +++ b/src/Ganeti/Types.hs > > > @@ -75,6 +75,7 @@ module Ganeti.Types > > > , CVErrorCode(..) > > > , cVErrorCodeToRaw > > > , Hypervisor(..) > > > + , hypervisorFromRaw > > > , hypervisorToRaw > > > , OobCommand(..) > > > , oobCommandToRaw > > > -- > > > 2.0.0.526.g5318336 > > > > > > > > -- > Jose Antonio Lopes > Ganeti Engineering > 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 > Steuernummer: 48/725/00206 > Umsatzsteueridentifikationsnummer: DE813741370 > -- -- Helga Velroyen | Software Engineer | [email protected] | 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
