Re: [PATCH 08/12] NetTestParse: Adding support for libvirt-integration

[Jiri Pirko]

Fri, Aug 10, 2012 at 12:49:11PM CEST, rpazdera(a)redhat.com wrote:
> From: Radek Pazdera <rpazdera(a)redhat.com&gt;
>
> This commit adds necessary changes to the parser in LNST in order
> to support the libvirt device creation.
>
> Netdevice tags are now grouped under a <netdevices> parent tag
> within <netmachineconfig>.
>
> On top of that a special mode for defining netdevices (called create)
> was added. Devices defined in this mode are not present in the machine
> and will be created on test execution.
>
> This mode is available only on virtual test machines created using
> libvirt. Here's an exmaple config of such a machine:
>
> <machine id="1">
>     <netmachineconfig>
>         <info hostname="192.168.122.11" libvirt_domain="RHEL6"
>               rootpass="redhat"/>
>         <netdevices>
>             <create>
>                 <netdevice network="tnet" phys_id="1"
> type="eth"/>
>                 <netdevice network="tnet" phys_id="2"
> type="eth"/>
>             </create> 

This is ok and I love your work!
But still I have some concerns about naming. I think that
"create" and "tagret_bridge" might be misleading for users.

How about to change it this way:
"create" -> "libvirt_create"
"target_bridge" -> "libvirt_bridge"

This naming would tell everyone on a first look that this xml element and
attribute are only valid for libvirt.

Also, I think that there should be check for <info libvirt_domain="">
attribute in case "libvirt_create" is detected.

Also, I think that dev["dynamic"] = True/False should be uniform to
"libvirt_create" somehow. How about:

dev["create"] = None
dev["create"] = "libvirt"
dev["create"] = "whateverisneededinfuture"

?


>         </netdevices>
>     </netmachineconfig>
>     <netconfig>
>         <interface id="1" phys_id="1" type="eth">
>             <addresses>
>                 <address value="192.168.100.226/24"/>
>             </addresses>
>         </interface>
>         <interface id="2" phys_id="2" type="eth">
>             <addresses>
>                 <address value="192.168.100.240/24"/>
>             </addresses>
>         </interface>
>     </netconfig>
> </machine>
>
> The dynamic devices are enclosed within the <create> tag. Their hwaddr
> doesn't need to be specified (it will be generated if missing).
>
> Additionally, 'target_bridge' attribute can be added to these devices
> to override the default LNST internal bridge, that these devices are
> connected to.
>
> Signed-off-by: Radek Pazdera <rpazdera(a)redhat.com&gt;
> ---
>  NetTest/NetTestParse.py |   34 +++++++++++++++++++++++++++++++---
>  1 files changed, 31 insertions(+), 3 deletions(-)
>
> diff --git a/NetTest/NetTestParse.py b/NetTest/NetTestParse.py
> index 746342c..d1edf6b 100644
> --- a/NetTest/NetTestParse.py
> +++ b/NetTest/NetTestParse.py
> @@ -118,7 +118,7 @@ class NetMachineConfigParse(RecipeParser):
>  
>      def parse(self, node):
>          scheme = {"info": self._info,
> -                  "netdevice": self._netdevice}
> +                  "netdevices": self._netdevices}
>          self._process_child_nodes(node, scheme)
>  
>      def _info(self, node, params):
> @@ -127,6 +127,10 @@ class NetMachineConfigParse(RecipeParser):
>  
>          info["hostname"] = self._get_attribute(node, "hostname")
>  
> +        if self._has_attribute(node, "libvirt_domain"):
> +            info["libvirt_domain"] = self._get_attribute(node,
> +                                                "libvirt_domain")
> +
>          if self._has_attribute(node, "rootpass"):
>              info["rootpass"] = self._get_attribute(node, "rootpass")
>  
> @@ -138,18 +142,42 @@ class NetMachineConfigParse(RecipeParser):
>          self._trigger_event("machine_info_ready",
>                              {"machine_id": self._machine_id})
>  
> +    def _netdevices(self, node, params):
> +        scheme = {"netdevice": self._netdevice,
> +                  "create": self._create}
> +
> +        new_params = {"dynamic": False}
> +        self._process_child_nodes(node, scheme, new_params)
> +
> +    def _create(self, node, params):
> +        scheme = {"netdevice": self._netdevice}
> +
> +        new_params = {"dynamic": True}
> +        self._process_child_nodes(node, scheme, new_params)
> +
>      def _netdevice(self, node, params):
>          machine = self._machine
>          phys_id = self._get_attribute(node, "phys_id", int)
>  
>          dev = machine["netdevices"][phys_id] = {}
> +        dev["dynamic"] = params["dynamic"]
>          dev["type"] = self._get_attribute(node, "type")
>          dev["network"] = self._get_attribute(node, "network")
> -        dev["hwaddr"] = normalize_hwaddr(self._get_attribute(node,
> "hwaddr"))
>  
> -        if self._has_attribute(node, "name"):
> +        # hwaddr attribute is optional for dynamic devices,
> +        # but it is required by non-dynamic devices
> +        if not dev["dynamic"] or self._has_attribute(node,
> "hwaddr"):
> +            hwaddr = self._get_attribute(node, "hwaddr")
> +            dev["hwaddr"] = normalize_hwaddr(hwaddr)
> +
> +        # name attribute is only valid when the device is not dynamic
> +        if not dev["dynamic"] and self._has_attribute(node, "name"):
>              dev["name"] = self._get_attribute(node, "name")
>  
> +        # bridge attribute is valid only when the device is dynamic
> +        if dev["dynamic"] and self._has_attribute(node,
> "target_bridge"):
> +            dev["target_bridge"] = self._get_attribute(node,
> "target_bridge")
> +
>          self._trigger_event("netdevice_ready", {"machine_id":
> self._machine_id,
>                                                  "dev_id": phys_id})
>  
> -- 
> 1.7.7.6
>
> _______________________________________________
> LNST-developers mailing list
> LNST-developers(a)lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/lnst-developers