Re: [KVM-AUTOTEST PATCH 12/28] KVM test: use VM.clone() in make_qemu_command()

[Michael Goldish <mgoldish@redhat.com>]

On 12/28/2010 03:13 PM, Jason Wang wrote:
> Michael Goldish writes:
>  > When make_qemu_command() is called in order to see if a VM should be restarted
>  > using a different qemu command line, construction of the qemu command should be
>  > based only on the parameters provided to make_qemu_command() (name, params,
>  > root_dir).  However, some VM methods are called which use the internal state of
>  > the VM (self.params).  For example, when calling make_qemu_command() with
>  > params that define 3 NICs, for a VM that currently has only 1 NIC,
>  > make_qemu_command() will call VM.get_ifname() for 2 NICs that don't exist in
>  > self.params.
>  > To solve this, allow VM.clone() to copy the state of the VM to the clone, and
>  > use such a clone with altered params in make_qemu_command(), instead of using
>  > 'self'.  That way, all methods that use self.params will use the correct
>  > (altered) params in make_qemu_command().
>  > 
> 
> If we use VM.clone() we also need to destroy it as it may left entries in mac
> address pool. Other looks good.

I don't think so because the clone doesn't do anything except allow
proper access to VM data.  The clone shares the same instance string
with the original VM, so they share the same address pool entries.  If
we destroy() the clone, the original VM will be destroyed too.

The above is only true because we pass copy_state=True to clone().  If
we didn't do that, the clone would be independent with its own instance
string and would have to be create()d separately.

>  > Signed-off-by: Michael Goldish <mgoldish@redhat.com>
>  > ---
>  >  client/tests/kvm/kvm_vm.py |   72 ++++++++++++++++++++++++++-----------------
>  >  1 files changed, 43 insertions(+), 29 deletions(-)
>  > 
>  > diff --git a/client/tests/kvm/kvm_vm.py b/client/tests/kvm/kvm_vm.py
>  > index aeb7448..b80d2c2 100755
>  > --- a/client/tests/kvm/kvm_vm.py
>  > +++ b/client/tests/kvm/kvm_vm.py
>  > @@ -97,7 +97,7 @@ class VM:
>  >      This class handles all basic VM operations.
>  >      """
>  >  
>  > -    def __init__(self, name, params, root_dir, address_cache):
>  > +    def __init__(self, name, params, root_dir, address_cache, state=None):
>  >          """
>  >          Initialize the object and set a few attributes.
>  >  
>  > @@ -106,30 +106,35 @@ class VM:
>  >                  (see method make_qemu_command for a full description)
>  >          @param root_dir: Base directory for relative filenames
>  >          @param address_cache: A dict that maps MAC addresses to IP addresses
>  > +        @param state: If provided, use this as self.__dict__
>  >          """
>  > -        self.process = None
>  > -        self.serial_console = None
>  > -        self.redirs = {}
>  > -        self.vnc_port = 5900
>  > -        self.monitors = []
>  > -        self.pci_assignable = None
>  > -        self.netdev_id = []
>  > -        self.uuid = None
>  > +        if state:
>  > +            self.__dict__ = state
>  > +        else:
>  > +            self.process = None
>  > +            self.serial_console = None
>  > +            self.redirs = {}
>  > +            self.vnc_port = 5900
>  > +            self.monitors = []
>  > +            self.pci_assignable = None
>  > +            self.netdev_id = []
>  > +            self.uuid = None
>  > +
>  > +            # Find a unique identifier for this VM
>  > +            while True:
>  > +                self.instance = (time.strftime("%Y%m%d-%H%M%S-") +
>  > +                                 kvm_utils.generate_random_string(4))
>  > +                if not glob.glob("/tmp/*%s" % self.instance):
>  > +                    break
>  >  
>  >          self.name = name
>  >          self.params = params
>  >          self.root_dir = root_dir
>  >          self.address_cache = address_cache
>  >  
>  > -        # Find a unique identifier for this VM
>  > -        while True:
>  > -            self.instance = (time.strftime("%Y%m%d-%H%M%S-") +
>  > -                             kvm_utils.generate_random_string(4))
>  > -            if not glob.glob("/tmp/*%s" % self.instance):
>  > -                break
>  > -
>  >  
>  > -    def clone(self, name=None, params=None, root_dir=None, address_cache=None):
>  > +    def clone(self, name=None, params=None, root_dir=None, address_cache=None,
>  > +              copy_state=False):
>  >          """
>  >          Return a clone of the VM object with optionally modified parameters.
>  >          The clone is initially not alive and needs to be started using create().
>  > @@ -140,6 +145,8 @@ class VM:
>  >          @param params: Optional new VM creation parameters
>  >          @param root_dir: Optional new base directory for relative filenames
>  >          @param address_cache: A dict that maps MAC addresses to IP addresses
>  > +        @param copy_state: If True, copy the original VM's state to the clone.
>  > +                Mainly useful for make_qemu_command().
>  >          """
>  >          if name is None:
>  >              name = self.name
>  > @@ -149,7 +156,11 @@ class VM:
>  >              root_dir = self.root_dir
>  >          if address_cache is None:
>  >              address_cache = self.address_cache
>  > -        return VM(name, params, root_dir, address_cache)
>  > +        if copy_state:
>  > +            state = self.__dict__.copy()
>  > +        else:
>  > +            state = None
>  > +        return VM(name, params, root_dir, address_cache, state)
>  >  
>  >  
>  >      def make_qemu_command(self, name=None, params=None, root_dir=None):
>  > @@ -350,6 +361,9 @@ class VM:
>  >          if params is None: params = self.params
>  >          if root_dir is None: root_dir = self.root_dir
>  >  
>  > +        # Clone this VM using the new params
>  > +        vm = self.clone(name, params, root_dir, copy_state=True)
>  > +
>  >          qemu_binary = kvm_utils.get_path(root_dir, params.get("qemu_binary",
>  >                                                                "qemu"))
>  >          # Get the output of 'qemu -help' (log a message in case this call never
>  > @@ -369,14 +383,14 @@ class VM:
>  >          # Add monitors
>  >          for monitor_name in kvm_utils.get_sub_dict_names(params, "monitors"):
>  >              monitor_params = kvm_utils.get_sub_dict(params, monitor_name)
>  > -            monitor_filename = self.get_monitor_filename(monitor_name)
>  > +            monitor_filename = vm.get_monitor_filename(monitor_name)
>  >              if monitor_params.get("monitor_type") == "qmp":
>  >                  qemu_cmd += add_qmp_monitor(help, monitor_filename)
>  >              else:
>  >                  qemu_cmd += add_human_monitor(help, monitor_filename)
>  >  
>  >          # Add serial console redirection
>  > -        qemu_cmd += add_serial(help, self.get_serial_console_filename())
>  > +        qemu_cmd += add_serial(help, vm.get_serial_console_filename())
>  >  
>  >          for image_name in kvm_utils.get_sub_dict_names(params, "images"):
>  >              image_params = kvm_utils.get_sub_dict(params, image_name)
>  > @@ -396,18 +410,18 @@ class VM:
>  >          for redir_name in kvm_utils.get_sub_dict_names(params, "redirs"):
>  >              redir_params = kvm_utils.get_sub_dict(params, redir_name)
>  >              guest_port = int(redir_params.get("guest_port"))
>  > -            host_port = self.redirs.get(guest_port)
>  > +            host_port = vm.redirs.get(guest_port)
>  >              redirs += [(host_port, guest_port)]
>  >  
>  >          vlan = 0
>  >          for nic_name in kvm_utils.get_sub_dict_names(params, "nics"):
>  >              nic_params = kvm_utils.get_sub_dict(params, nic_name)
>  >              try:
>  > -                netdev_id = self.netdev_id[vlan]
>  > +                netdev_id = vm.netdev_id[vlan]
>  >              except IndexError:
>  >                  netdev_id = None
>  >              # Handle the '-net nic' part
>  > -            mac = self.get_mac_address(vlan)
>  > +            mac = vm.get_mac_address(vlan)
>  >              qemu_cmd += add_nic(help, vlan, nic_params.get("nic_model"), mac,
>  >                                  netdev_id, nic_params.get("nic_extra_params"))
>  >              # Handle the '-net tap' or '-net user' part
>  > @@ -421,7 +435,7 @@ class VM:
>  >              if tftp:
>  >                  tftp = kvm_utils.get_path(root_dir, tftp)
>  >              qemu_cmd += add_net(help, vlan, nic_params.get("nic_mode", "user"),
>  > -                                self.get_ifname(vlan),
>  > +                                vm.get_ifname(vlan),
>  >                                  script, downscript, tftp,
>  >                                  nic_params.get("bootp"), redirs, netdev_id,
>  >                                  nic_params.get("vhost") == "yes")
>  > @@ -478,27 +492,27 @@ class VM:
>  >              qemu_cmd += add_tcp_redir(help, host_port, guest_port)
>  >  
>  >          if params.get("display") == "vnc":
>  > -            qemu_cmd += add_vnc(help, self.vnc_port)
>  > +            qemu_cmd += add_vnc(help, vm.vnc_port)
>  >          elif params.get("display") == "sdl":
>  >              qemu_cmd += add_sdl(help)
>  >          elif params.get("display") == "nographic":
>  >              qemu_cmd += add_nographic(help)
>  >  
>  >          if params.get("uuid") == "random":
>  > -            qemu_cmd += add_uuid(help, self.uuid)
>  > +            qemu_cmd += add_uuid(help, vm.uuid)
>  >          elif params.get("uuid"):
>  >              qemu_cmd += add_uuid(help, params.get("uuid"))
>  >  
>  >          if params.get("testdev") == "yes":
>  > -            qemu_cmd += add_testdev(help, self.get_testlog_filename())
>  > +            qemu_cmd += add_testdev(help, vm.get_testlog_filename())
>  >  
>  >          if params.get("disable_hpet") == "yes":
>  >              qemu_cmd += add_no_hpet(help)
>  >  
>  >          # If the PCI assignment step went OK, add each one of the PCI assigned
>  >          # devices to the qemu command line.
>  > -        if self.pci_assignable:
>  > -            for pci_id in self.pa_pci_ids:
>  > +        if vm.pci_assignable:
>  > +            for pci_id in vm.pa_pci_ids:
>  >                  qemu_cmd += add_pcidevice(help, pci_id)
>  >  
>  >          extra_params = params.get("extra_params")
>  > -- 
>  > 1.7.3.3
>  > 
>  > _______________________________________________
>  > Autotest mailing list
>  > Autotest@test.kernel.org
>  > http://test.kernel.org/cgi-bin/mailman/listinfo/autotest
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html