On Fri, May 24, 2013 at 10:52 AM, Thomas Thrainer <[email protected]>wrote:
> > > > On Thu, May 23, 2013 at 1:03 PM, Guido Trotter <[email protected]>wrote: > >> >> >> >> On Wed, May 22, 2013 at 5:28 PM, Thomas Thrainer <[email protected]>wrote: >> >>> Add a NIC type and extend the Instance type by a list of NIC's. Parse >>> the NIC's in allocation requests and store them for now. Later patches >>> will make use of this field in order to ensure that the requested >>> instance is only placed in node groups wich are connected to those >>> networks. >>> >>> Signed-off-by: Thomas Thrainer <[email protected]> >>> --- >>> Makefile.am | 1 + >>> src/Ganeti/HTools/Backend/IAlloc.hs | 23 ++++++- >>> src/Ganeti/HTools/Backend/Luxi.hs | 2 +- >>> src/Ganeti/HTools/Backend/Rapi.hs | 4 +- >>> src/Ganeti/HTools/Backend/Text.hs | 2 +- >>> src/Ganeti/HTools/Instance.hs | 7 ++- >>> src/Ganeti/HTools/Nic.hs | 77 >>> +++++++++++++++++++++++ >>> src/Ganeti/HTools/Program/Hspace.hs | 4 +- >>> test/data/htools/hail-alloc-invalid-twodisks.json | 3 +- >>> test/data/htools/hail-alloc-twodisks.json | 3 +- >>> test/hs/Test/Ganeti/HTools/Instance.hs | 2 +- >>> test/hs/Test/Ganeti/TestHTools.hs | 2 +- >>> 12 files changed, 117 insertions(+), 13 deletions(-) >>> create mode 100644 src/Ganeti/HTools/Nic.hs >>> >>> diff --git a/Makefile.am b/Makefile.am >>> index ffdee66..507ffbe 100644 >>> --- a/Makefile.am >>> +++ b/Makefile.am >>> @@ -548,6 +548,7 @@ HS_LIB_SRCS = \ >>> src/Ganeti/HTools/Group.hs \ >>> src/Ganeti/HTools/Instance.hs \ >>> src/Ganeti/HTools/Loader.hs \ >>> + src/Ganeti/HTools/Nic.hs \ >>> src/Ganeti/HTools/Node.hs \ >>> src/Ganeti/HTools/PeerMap.hs \ >>> src/Ganeti/HTools/Program/Hail.hs \ >>> diff --git a/src/Ganeti/HTools/Backend/IAlloc.hs >>> b/src/Ganeti/HTools/Backend/IAlloc.hs >>> index 823e0c9..22dad86 100644 >>> --- a/src/Ganeti/HTools/Backend/IAlloc.hs >>> +++ b/src/Ganeti/HTools/Backend/IAlloc.hs >>> @@ -44,6 +44,7 @@ import qualified Ganeti.HTools.Container as Container >>> import qualified Ganeti.HTools.Group as Group >>> import qualified Ganeti.HTools.Node as Node >>> import qualified Ganeti.HTools.Instance as Instance >>> +import qualified Ganeti.HTools.Nic as Nic >>> import qualified Ganeti.Constants as C >>> import Ganeti.HTools.CLI >>> import Ganeti.HTools.Loader >>> @@ -56,6 +57,22 @@ import Ganeti.Utils >>> -- | Type alias for the result of an IAllocator call. >>> type IAllocResult = (String, JSValue, Node.List, Instance.List) >>> >>> +-- | Parse a NIC within an instance (in a creation request) >>> +parseNic :: String -> JSRecord -> Result Nic.Nic >>> +parseNic n a = do >>> + mac <- maybeFromObj a "mac" >>> + ip <- maybeFromObj a "ip" >>> + mode <- maybeFromObj a "mode" >>= \m -> case m of >>> + Just "bridged" -> Ok $ Just Nic.Bridged >>> + Just "routed" -> Ok $ Just Nic.Routed >>> + Just "openvswitch" -> Ok $ Just Nic.OpenVSwitch >>> + Nothing -> Ok Nothing >>> + _ -> Bad $ "invalid NIC mode in instance " ++ n >>> + link <- maybeFromObj a "link" >>> + bridge <- maybeFromObj a "bridge" >>> + network <- maybeFromObj a "network" >>> + return (Nic.create mac ip mode link bridge network) >>> + >>> -- | Parse the basic specifications of an instance. >>> -- >>> -- Instances in the cluster instance list and the instance in an >>> @@ -75,7 +92,11 @@ parseBaseInstance n a = do >>> tags <- extract "tags" >>> dt <- extract "disk_template" >>> su <- extract "spindle_use" >>> - return (n, Instance.create n mem disk disks vcpus Running tags True 0 >>> 0 dt su) >>> + nics <- extract "nics" >>= toArray >>= asObjectList >>= >>> + mapM (parseNic n . fromJSObject) >>> + return >>> + (n, >>> + Instance.create n mem disk disks vcpus Running tags True 0 0 dt su >>> nics) >>> >>> -- | Parses an instance as found in the cluster instance list. >>> parseInstance :: NameAssoc -- ^ The node name-to-index association list >>> diff --git a/src/Ganeti/HTools/Backend/Luxi.hs >>> b/src/Ganeti/HTools/Backend/Luxi.hs >>> index b36376f..7227844 100644 >>> --- a/src/Ganeti/HTools/Backend/Luxi.hs >>> +++ b/src/Ganeti/HTools/Backend/Luxi.hs >>> @@ -173,7 +173,7 @@ parseInstance ktn [ name, disk, mem, vcpus >>> xdt <- convert "disk_template" disk_template >>> xsu <- convert "be/spindle_use" su >>> let inst = Instance.create xname xmem xdisk [xdisk] xvcpus >>> - xrunning xtags xauto_balance xpnode snode xdt xsu >>> + xrunning xtags xauto_balance xpnode snode xdt xsu [] >>> return (xname, inst) >>> >>> parseInstance _ v = fail ("Invalid instance query result: " ++ show v) >>> diff --git a/src/Ganeti/HTools/Backend/Rapi.hs >>> b/src/Ganeti/HTools/Backend/Rapi.hs >>> index aa8ab1b..5049198 100644 >>> --- a/src/Ganeti/HTools/Backend/Rapi.hs >>> +++ b/src/Ganeti/HTools/Backend/Rapi.hs >>> @@ -140,7 +140,7 @@ parseInstance ktn a = do >>> dt <- extract "disk_template" a >>> su <- extract "spindle_use" beparams >>> let inst = Instance.create name mem disk disks vcpus running tags >>> - auto_balance pnode snode dt su >>> + auto_balance pnode snode dt su [] >>> return (name, inst) >>> >>> -- | Construct a node from a JSON object. >>> @@ -230,7 +230,7 @@ parseData (group_body, node_body, inst_body, >>> info_body) = do >>> let (node_names, node_idx) = assignIndices node_data >>> inst_data <- inst_body >>= getInstances node_names >>> let (_, inst_idx) = assignIndices inst_data >>> - (tags, ipolicy, master) <- >>> + (tags, ipolicy, master) <- >>> info_body >>= >>> (fromJResult "Parsing cluster info" . decodeStrict) >>= >>> parseCluster >>> diff --git a/src/Ganeti/HTools/Backend/Text.hs >>> b/src/Ganeti/HTools/Backend/Text.hs >>> index 17674a7..06e1a74 100644 >>> --- a/src/Ganeti/HTools/Backend/Text.hs >>> +++ b/src/Ganeti/HTools/Backend/Text.hs >>> @@ -245,7 +245,7 @@ loadInst ktn [ name, mem, dsk, vcpus, status, >>> auto_bal, pnode, snode >>> " has same primary and secondary node - " ++ pnode >>> let vtags = commaSplit tags >>> newinst = Instance.create name vmem vdsk [vdsk] vvcpus vstatus >>> vtags >>> - auto_balance pidx sidx disk_template spindle_use >>> + auto_balance pidx sidx disk_template spindle_use [] >>> return (name, newinst) >>> >>> loadInst ktn [ name, mem, dsk, vcpus, status, auto_bal, pnode, snode >>> diff --git a/src/Ganeti/HTools/Instance.hs >>> b/src/Ganeti/HTools/Instance.hs >>> index 4ba3e91..00c3d9a 100644 >>> --- a/src/Ganeti/HTools/Instance.hs >>> +++ b/src/Ganeti/HTools/Instance.hs >>> @@ -60,6 +60,7 @@ module Ganeti.HTools.Instance >>> import Ganeti.BasicTypes >>> import qualified Ganeti.HTools.Types as T >>> import qualified Ganeti.HTools.Container as Container >>> +import Ganeti.HTools.Nic (Nic) >>> >>> import Ganeti.Utils >>> >>> @@ -85,6 +86,7 @@ data Instance = Instance >>> , allTags :: [String] -- ^ List of all instance tags >>> , exclTags :: [String] -- ^ List of instance exclusion tags >>> , arPolicy :: T.AutoRepairPolicy -- ^ Instance's auto-repair >>> policy >>> + , nics :: [Nic] -- ^ NICs of the instance >>> } deriving (Show, Eq) >>> >>> instance T.Element Instance where >>> @@ -165,9 +167,9 @@ type List = Container.Container Instance >>> -- later (via 'setIdx' for example). >>> create :: String -> Int -> Int -> [Int] -> Int -> T.InstanceStatus >>> -> [String] -> Bool -> T.Ndx -> T.Ndx -> T.DiskTemplate -> Int >>> - -> Instance >>> + -> [Nic] -> Instance >>> create name_init mem_init dsk_init disks_init vcpus_init run_init >>> tags_init >>> - auto_balance_init pn sn dt su = >>> + auto_balance_init pn sn dt su nics_init = >>> Instance { name = name_init >>> , alias = name_init >>> , mem = mem_init >>> @@ -186,6 +188,7 @@ create name_init mem_init dsk_init disks_init >>> vcpus_init run_init tags_init >>> , allTags = tags_init >>> , exclTags = [] >>> , arPolicy = T.ArNotEnabled >>> + , nics = nics_init >>> } >>> >>> -- | Changes the index. >>> diff --git a/src/Ganeti/HTools/Nic.hs b/src/Ganeti/HTools/Nic.hs >>> new file mode 100644 >>> index 0000000..5919239 >>> --- /dev/null >>> +++ b/src/Ganeti/HTools/Nic.hs >>> @@ -0,0 +1,77 @@ >>> +{-| Module describing an NIC. >>> + >>> +The NIC data type only holds data about a NIC, but does not provide any >>> +logic. >>> + >>> +-} >>> + >>> +{- >>> + >>> +Copyright (C) 2009, 2010, 2011, 2012, 2013 Google Inc. >>> + >>> +This program is free software; you can redistribute it and/or modify >>> +it under the terms of the GNU General Public License as published by >>> +the Free Software Foundation; either version 2 of the License, or >>> +(at your option) any later version. >>> + >>> +This program is distributed in the hope that it will be useful, but >>> +WITHOUT ANY WARRANTY; without even the implied warranty of >>> +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU >>> +General Public License for more details. >>> + >>> +You should have received a copy of the GNU General Public License >>> +along with this program; if not, write to the Free Software >>> +Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA >>> +02110-1301, USA. >>> + >>> +-} >>> + >>> +module Ganeti.HTools.Nic >>> + ( Nic(..) >>> + , Mode(..) >>> + , List >>> + , create >>> + ) where >>> + >>> +import qualified Ganeti.HTools.Container as Container >>> + >>> +import qualified Ganeti.HTools.Types as T >>> + >>> +-- * Type declarations >>> + >>> +data Mode = Bridged | Routed | OpenVSwitch deriving (Show, Eq) >>> + >>> +-- | The NIC type. >>> +data Nic = Nic >>> + { mac :: Maybe String -- ^ MAC address of the NIC >>> + , ip :: Maybe String -- ^ IP address of the NIC >>> + , mode :: Maybe Mode -- ^ the mode the NIC operates in >>> + , link :: Maybe String -- ^ the link of the NIC >>> + , bridge :: Maybe String -- ^ the bridge this NIC is connected >>> to if >>> + -- the mode is Bridged >>> + , network :: Maybe T.NetworkID -- ^ network UUID if this NIC is >>> connected >>> + -- to a network >>> + } deriving (Show, Eq) >>> + >>> >> >> I wonder if it would make sense to use Objects.PartialNic for this. >> True, we don't in other parts of htools, but this was (in part) because >> we didn't have Objects implemented then. >> Maybe using those we can remove some duplicate code? (later, not in this >> patch series) >> >> > > I tried to explain the rational for the new type in the comment, and added > a TODO for reworking it when Objects.* are touched anyways. > Interdiff: > > diff --git a/src/Ganeti/HTools/Nic.hs b/src/Ganeti/HTools/Nic.hs > index 5919239..02f634d 100644 > --- a/src/Ganeti/HTools/Nic.hs > +++ b/src/Ganeti/HTools/Nic.hs > @@ -42,6 +42,17 @@ import qualified Ganeti.HTools.Types as T > data Mode = Bridged | Routed | OpenVSwitch deriving (Show, Eq) > > -- | The NIC type. > +-- > +-- It holds the data for a NIC as it is provided via the IAllocator > protocol > +-- for an instance creation request. All data in those request is > optional, > +-- that's why all fields are Maybe's. > +-- > +-- TODO: Another name might be more appropriate for this type, as for > example > +-- RequestedNic. But this type is used as a field in the Instance type, > which > +-- is not named RequestedInstance, so such a name would be weird. > PartialNic > +-- already exists in Objects, but doesn't fit the bill here, as it > contains > +-- a required field (mac). Objects and the types therein are subject to > being > +-- reworked, so until then this type is left as is. > data Nic = Nic > { mac :: Maybe String -- ^ MAC address of the NIC > , ip :: Maybe String -- ^ IP address of the NIC > > > > >> +-- | A simple name for an instance map. >>> >>> +type List = Container.Container Nic >>> + >>> +-- * Initialization >>> + >>> +-- | Create a NIC. >>> +-- >>> +create :: Maybe String >>> + -> Maybe String >>> + -> Maybe Mode >>> + -> Maybe String >>> + -> Maybe String >>> + -> Maybe T.NetworkID >>> + -> Nic >>> +create mac_init ip_init mode_init link_init bridge_init network_init = >>> + Nic { mac = mac_init >>> + , ip = ip_init >>> + , mode = mode_init >>> + , link = link_init >>> + , bridge = bridge_init >>> + , network = network_init >>> + } >>> diff --git a/src/Ganeti/HTools/Program/Hspace.hs >>> b/src/Ganeti/HTools/Program/Hspace.hs >>> index 78f0687..42c0aab 100644 >>> --- a/src/Ganeti/HTools/Program/Hspace.hs >>> +++ b/src/Ganeti/HTools/Program/Hspace.hs >>> @@ -394,9 +394,9 @@ runAllocation cdata stop_allocation actual_result >>> spec dt mode opts = do >>> -- of the disk space to individual disks), sensible defaults are >>> guessed (e.g., >>> -- having a single disk). >>> instFromSpec :: RSpec -> DiskTemplate -> Int -> Instance.Instance >>> -instFromSpec spx = >>> +instFromSpec spx dt su = >>> Instance.create "new" (rspecMem spx) (rspecDsk spx) [rspecDsk spx] >>> - (rspecCpu spx) Running [] True (-1) (-1) >>> + (rspecCpu spx) Running [] True (-1) (-1) dt su [] >>> >>> combineTiered :: Maybe Int -> Cluster.AllocNodes -> Cluster.AllocResult >>> -> >>> Instance.Instance -> Result Cluster.AllocResult >>> diff --git a/test/data/htools/hail-alloc-invalid-twodisks.json >>> b/test/data/htools/hail-alloc-invalid-twodisks.json >>> index 3914a5d..4a15e64 100644 >>> --- a/test/data/htools/hail-alloc-invalid-twodisks.json >>> +++ b/test/data/htools/hail-alloc-invalid-twodisks.json >>> @@ -85,7 +85,8 @@ >>> "spindle_use": 2, >>> "tags": [], >>> "type": "allocate", >>> - "vcpus": 1 >>> + "vcpus": 1, >>> + "nics": [] >>> }, >>> "version": 2 >>> } >>> diff --git a/test/data/htools/hail-alloc-twodisks.json >>> b/test/data/htools/hail-alloc-twodisks.json >>> index e4614a7..5e9e196 100644 >>> --- a/test/data/htools/hail-alloc-twodisks.json >>> +++ b/test/data/htools/hail-alloc-twodisks.json >>> @@ -85,7 +85,8 @@ >>> "spindle_use": 2, >>> "tags": [], >>> "type": "allocate", >>> - "vcpus": 1 >>> + "vcpus": 1, >>> + "nics": [] >>> }, >>> "version": 2 >>> } >>> diff --git a/test/hs/Test/Ganeti/HTools/Instance.hs >>> b/test/hs/Test/Ganeti/HTools/Instance.hs >>> index ca8f682..5e6e36a 100644 >>> --- a/test/hs/Test/Ganeti/HTools/Instance.hs >>> +++ b/test/hs/Test/Ganeti/HTools/Instance.hs >>> @@ -62,7 +62,7 @@ genInstanceSmallerThan lim_mem lim_dsk lim_cpu = do >>> sn <- arbitrary >>> vcpus <- choose (0, lim_cpu) >>> dt <- arbitrary >>> - return $ Instance.create name mem dsk [dsk] vcpus run_st [] True pn >>> sn dt 1 >>> + return $ Instance.create name mem dsk [dsk] vcpus run_st [] True pn >>> sn dt 1 [] >>> >>> -- | Generates an instance smaller than a node. >>> genInstanceSmallerThanNode :: Node.Node -> Gen Instance.Instance >>> diff --git a/test/hs/Test/Ganeti/TestHTools.hs >>> b/test/hs/Test/Ganeti/TestHTools.hs >>> index b51df04..eca4f57 100644 >>> --- a/test/hs/Test/Ganeti/TestHTools.hs >>> +++ b/test/hs/Test/Ganeti/TestHTools.hs >>> @@ -100,7 +100,7 @@ defGroupAssoc = Map.singleton (Group.uuid defGroup) >>> (Group.idx defGroup) >>> createInstance :: Int -> Int -> Int -> Instance.Instance >>> createInstance mem dsk vcpus = >>> Instance.create "inst-unnamed" mem dsk [dsk] vcpus Types.Running [] >>> True (-1) >>> - (-1) Types.DTDrbd8 1 >>> + (-1) Types.DTDrbd8 1 [] >>> >>> -- | Create a small cluster by repeating a node spec. >>> makeSmallCluster :: Node.Node -> Int -> Node.List >>> -- >>> 1.8.2.1 >>> >>> >> Thanks, >> >> Guido >> >> > LGTM Thanks, Guido
