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.
This is a bug! As the docstring says, it should be a dict of string
to bool.
>
> > + 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?
If you mean a cast as in call 'bool' that's not going to work:
$ bool("True")
True
$ bool("False")
True
I have fixed the bug above and added some unit tests to prevent this
bug in the future.
diff --git a/lib/ssconf.py b/lib/ssconf.py
index 55f9df1..d18d741 100644
--- a/lib/ssconf.py
+++ b/lib/ssconf.py
@@ -334,7 +334,7 @@ class SimpleStore(object):
vm_capable = {}
for line in data.splitlines(False):
(node_uuid, node_vm_capable) = line.split("=")
- vm_capable[node_uuid] = node_vm_capable
+ vm_capable[node_uuid] = node_vm_capable == "True"
return vm_capable
def GetNodegroupList(self):
@@ -434,6 +434,9 @@ class SimpleStore(object):
def GetEnabledUserShutdown(self):
"""Return whether user shutdown is enabled.
+ @rtype: bool
+ @return: 'True' if user shutdown is enabled, 'False' otherwise
+
"""
return self._ReadFile(constants.SS_ENABLED_USER_SHUTDOWN) == "True"
diff --git a/src/Ganeti/Ssconf.hs b/src/Ganeti/Ssconf.hs
index 3704c36..60848ad 100644
--- a/src/Ganeti/Ssconf.hs
+++ b/src/Ganeti/Ssconf.hs
@@ -30,17 +30,20 @@ module Ganeti.Ssconf
, sSKeyToRaw
, sSKeyFromRaw
, getPrimaryIPFamily
+ , parseNodesVmCapable
, getNodesVmCapable
, getMasterCandidatesIps
, getMasterNode
, getHypervisorList
+ , parseEnabledUserShutdown
, getEnabledUserShutdown
, keyToFilename
, sSFilePrefix
) where
+import Control.Applicative ((<$>))
import Control.Exception
-import Control.Monad (liftM)
+import Control.Monad (forM, liftM)
import Data.Maybe (fromMaybe)
import qualified Network.Socket as Socket
import System.FilePath ((</>))
@@ -148,16 +151,18 @@ getPrimaryIPFamily optpath = do
return (liftM rStripSpace result >>=
tryRead "Parsing af_family" >>= parseIPFamily)
--- | Read the nodes vm capable value.
+-- | Parse the nodes vm capable value from a 'String'.
+parseNodesVmCapable :: String -> Result [(String, Bool)]
+parseNodesVmCapable str = do
+ forM (lines str) $ \line -> do
+ (key, val) <- parseKeyValue "Parsing node_vm_capable" line
+ val' <- tryRead "Parsing value of node_vm_capable" val
+ return (key, val')
+
+-- | Read and parse the nodes vm capable.
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))
+getNodesVmCapable optPath =
+ (parseNodesVmCapable =<<) <$> readSSConfFile optPath Nothing SSNodeVmCapable
-- | Read the list of IP addresses of the master candidates of the cluster.
getMasterCandidatesIps :: Maybe FilePath -> IO (Result [String])
@@ -171,14 +176,19 @@ getMasterNode optPath = do
result <- readSSConfFile optPath Nothing SSMasterNode
return $ liftM rStripSpace result
--- | Read the list of enabled hypervisors
+-- | 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
+-- | Parse whether user shutdown is enabled from a 'String'.
+parseEnabledUserShutdown :: String -> Result Bool
+parseEnabledUserShutdown str =
+ tryRead "Parsing enabled_user_shutdown" (rStripSpace str)
+
+-- | Read and parse 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
+getEnabledUserShutdown optPath =
+ (parseEnabledUserShutdown =<<) <$>
+ readSSConfFile optPath Nothing SSEnabledUserShutdown
diff --git a/test/hs/Test/Ganeti/Ssconf.hs b/test/hs/Test/Ganeti/Ssconf.hs
index 3546018..a305d37 100644
--- a/test/hs/Test/Ganeti/Ssconf.hs
+++ b/test/hs/Test/Ganeti/Ssconf.hs
@@ -29,6 +29,7 @@ Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
module Test.Ganeti.Ssconf (testSsconf) where
import Test.QuickCheck
+import qualified Test.HUnit as HUnit
import Data.List
@@ -45,6 +46,27 @@ prop_filename key =
printTestCase "Key doesn't start with correct prefix" $
Ssconf.sSFilePrefix `isPrefixOf` Ssconf.keyToFilename "" key
+caseParseNodesVmCapable :: HUnit.Assertion
+caseParseNodesVmCapable = do
+ let str = "node1.example.com=True\nnode2.example.com=False"
+ result = Ssconf.parseNodesVmCapable str
+ expected = return
+ [ ("node1.example.com", True)
+ , ("node2.example.com", False)
+ ]
+ HUnit.assertEqual "Mismatch in parsed and expected result" expected result
+
+caseParseEnabledUserShutdown :: HUnit.Assertion
+caseParseEnabledUserShutdown = do
+ let result1 = Ssconf.parseEnabledUserShutdown "True"
+ result2 = Ssconf.parseEnabledUserShutdown "False"
+ HUnit.assertEqual "Mismatch in parsed and expected result"
+ (return True) result1
+ HUnit.assertEqual "Mismatch in parsed and expected result"
+ (return False) result2
+
testSuite "Ssconf"
[ 'prop_filename
+ , 'caseParseNodesVmCapable
+ , 'caseParseEnabledUserShutdown
]
diff --git a/test/py/ganeti.ssconf_unittest.py
b/test/py/ganeti.ssconf_unittest.py
index c0d4154..de70203 100755
--- a/test/py/ganeti.ssconf_unittest.py
+++ b/test/py/ganeti.ssconf_unittest.py
@@ -221,6 +221,32 @@ class TestSimpleStore(unittest.TestCase):
self.assertEqual(os.listdir(self.ssdir), [])
+ def testEnabledUserShutdown(self):
+ self.sstore._ReadFile = mock.Mock(return_value="True")
+ result = self.sstore.GetEnabledUserShutdown()
+ self.assertTrue(isinstance(result, bool))
+ self.assertEqual(result, True)
+
+ self.sstore._ReadFile = mock.Mock(return_value="False")
+ result = self.sstore.GetEnabledUserShutdown()
+ self.assertTrue(isinstance(result, bool))
+ self.assertEqual(result, False)
+
+ def testGetNodesVmCapable(self):
+ def _ToBool(x):
+ return x == "True"
+
+ vm_capable = [("node1.example.com", "True"), ("node2.example.com",
"False")]
+ ssconf_file_content = '\n'.join("%s=%s" % (key, value) for (key, value)
+ in vm_capable)
+ self.sstore._ReadFile = mock.Mock(return_value=ssconf_file_content)
+ result = self.sstore.GetNodesVmCapable()
+ for (key, value) in vm_capable:
+ self.assertTrue(isinstance(key, str))
+ self.assertTrue(key in result)
+ self.assertTrue(isinstance(result[key], bool))
+ self.assertEqual(_ToBool(value), result[key])
+
def testGetHvparamsForHypervisor(self):
hvparams = [("a", "A"), ("b", "B"), ("c", "C")]
ssconf_file_content = '\n'.join("%s=%s" % (key, value) for (key, value)
Thanks,
Jose
>
> > +
> >
> > 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
