Re: [PATCH][KVM-AUTOTEST] TAP network support in kvm-autotest

[jason wang <jasowang@redhat.com>]

Michael Goldish 写道:

Hi Micheal, thanks for your comments.
> Hi Jason,
>
> We already have patches that implement similar functionality here in
> TLV, as mentioned in the to-do list (item #4 under 'Framework').
> They're not yet committed upstream because they're still quite fresh.
>   
OK, I would pay more attention to to-do list.
> Still, your patch looks good and is quite similar to mine. The main
> difference is that I use MAC/IP address pools specified by the user,
> instead of random MACs with arp/nmap to detect the matching IP
> addresses.
>   
We've considers the use of MAC/IP address pools, but this method need to 
handle the cases of multiple kvm-autotest running on multiple guests. 
The MAC pools should not overlapped when using public bridges.
> I will post my patch to the mailing list soon, but it will come
> together with quite a few other patches that I haven't posted yet, so
> please be patient.
>
> Comments/questions:
>
> Why do you use nmap in addition to arp? In what cases will arp not
> suffice? I'm a little put off by the fact that nmap imposes an
> additional requirement on the host. Three hosts I've tried don't come
> with nmap installed by default.
>   
We use nmap to make sure the guest IP could be finally found somehow. 
During our tests, the scripts may fail to get the IP address of guest 
when host iptables is turned on.
> Please see additional comments below.
>
> ----- "Jason Wang" <jasowang@redhat.com> wrote:
>
>   
>> Hi All:
>> This patch tries to add tap network support in kvm-autotest. Multiple
>> nics connected to different bridges could be achieved through this
>> script. Public bridge is important for testing real network traffic
>> and migration. The patch gives each nic with randomly generated mac
>> address. The ip address required in the test could be dynamically
>> probed through nmap/arp. Only the ip address of first NIC is used
>> through the test.
>>
>> Example:
>> nics = nic1 nic2
>> network = bridge
>> bridge = switch
>> ifup =/etc/qemu-ifup-switch
>> ifdown =/etc/qemu-ifdown-switch
>>
>> This would make the virtual machine have two nics both of which are
>> connected to a bridge with the name of 'switch'. Ifup/ifdown scripts
>> are also specified.
>>
>> Another Example:
>> nics = nic1 nic2
>> network = bridge
>> bridge = switch
>> bridge_nic2 = virbr0
>> ifup =/etc/qemu-ifup-switch
>> ifup_nic2 = /etc/qemu-ifup-virbr0
>>
>> This would makes the virtual machine have two nics: nic1 are connected
>> to bridge 'switch' and nci2 are connected to bridge 'virbr0'.
>>
>> Public mode and user mode nic could also be mixed:
>> nics = nic1 nic2
>> network = bridge
>> network_nic2 = user
>>
>> Looking forward for comments and suggestions.
>>
>> From: jason <jasowang@redhat.com>
>> Date: Wed, 13 May 2009 16:15:28 +0800
>> Subject: [PATCH] Add tap networking support.
>>
>> ---
>>  client/tests/kvm_runtest_2/kvm_utils.py |    7 +++
>>  client/tests/kvm_runtest_2/kvm_vm.py    |   74
>> ++++++++++++++++++++++++++-----
>>  2 files changed, 69 insertions(+), 12 deletions(-)
>>
>> diff --git a/client/tests/kvm_runtest_2/kvm_utils.py
>> b/client/tests/kvm_runtest_2/kvm_utils.py
>> index be8ad95..0d1f7f8 100644
>> --- a/client/tests/kvm_runtest_2/kvm_utils.py
>> +++ b/client/tests/kvm_runtest_2/kvm_utils.py
>> @@ -773,3 +773,10 @@ def md5sum_file(filename, size=None):
>>          size -= len(data)
>>      f.close()
>>      return o.hexdigest()
>> +
>> +def random_mac():
>> +    mac=[0x00,0x16,0x30,
>> +         random.randint(0x00,0x09),
>> +         random.randint(0x00,0x09),
>> +         random.randint(0x00,0x09)]
>> +    return ':'.join(map(lambda x: "%02x" %x,mac))
>>     
>
> Random MAC addresses will not necessarily work everywhere, as far as
> I know. That's why I prefer user specified MAC/IP address ranges.
>   
Yes, maybe we could use user specified mac address prefix or more useful 
algorithm to generate mac address.
>> diff --git a/client/tests/kvm_runtest_2/kvm_vm.py
>> b/client/tests/kvm_runtest_2/kvm_vm.py
>> index fab839f..ea7dab6 100644
>> --- a/client/tests/kvm_runtest_2/kvm_vm.py
>> +++ b/client/tests/kvm_runtest_2/kvm_vm.py
>> @@ -105,6 +105,10 @@ class VM:
>>          self.qemu_path = qemu_path
>>          self.image_dir = image_dir
>>          self.iso_dir = iso_dir
>> +        self.macaddr = []
>> +        for nic_name in kvm_utils.get_sub_dict_names(params,"nics"):
>> +            macaddr = kvm_utils.random_mac()
>> +            self.macaddr.append(macaddr)
>>
>>      def verify_process_identity(self):
>>          """Make sure .pid really points to the original qemu
>> process.
>> @@ -189,9 +193,25 @@ class VM:
>>          for nic_name in kvm_utils.get_sub_dict_names(params,
>> "nics"):
>>              nic_params = kvm_utils.get_sub_dict(params, nic_name)
>>              qemu_cmd += " -net nic,vlan=%d" % vlan
>> +            net = nic_params.get("network")
>> +            if net == "bridge":
>> +                qemu_cmd += ",macaddr=%s" % self.macaddr[vlan]
>>              if nic_params.get("nic_model"):
>>                  qemu_cmd += ",model=%s" % nic_params.get("nic_model")
>> -            qemu_cmd += " -net user,vlan=%d" % vlan
>> +            if net == "bridge":
>> +                qemu_cmd += " -net tap,vlan=%d" % vlan
>> +                ifup = nic_params.get("ifup")
>> +                if ifup:
>> +                    qemu_cmd += ",script=%s" % ifup
>> +                else:
>> +                    qemu_cmd += ",script=/etc/qemu-ifup"
>>     
>
> Why not just leave 'script' out if the user doesn't specify 'ifup'?
> There's no good reason to prefer /etc/qemu-ifup to /etc/kvm-ifup or
> anything else, so I think it's best to leave it up to qemu if the
> user has no preference. It's also slightly shorter.
>   
Agree
>   
>> +                ifdown = nic_params.get("ifdown")
>> +                if ifdown:
>> +                    qemu_cmd += ",downscript=%s" % ifdown
>> +                else:
>> +                    qemu_cmd += ",downscript=no"
>>     
>
> The same applies here.
>
> This is just my opinion; I'd like to hear your thoughts on this too.
>   
If no downscript is specified, qemu would give a warning.
>> +            else:
>> +                qemu_cmd += " -net user,vlan=%d" % vlan
>>              vlan += 1
>>  
>>          mem = params.get("mem")
>> @@ -206,11 +226,11 @@ class VM:
>>          extra_params = params.get("extra_params")
>>          if extra_params:
>>              qemu_cmd += " %s" % extra_params
>> -
>> +            
>>          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.get_port(guest_port)
>> +            host_port = self.get_port(guest_port,True)
>>              qemu_cmd += " -redir tcp:%s::%s" % (host_port,
>> guest_port)
>>  
>>          if params.get("display") == "vnc":
>> @@ -467,27 +487,57 @@ class VM:
>>          If port redirection is used, return 'localhost' (the guest
>> has no IP
>>          address of its own).  Otherwise return the guest's IP
>> address.
>>          """
>> -        # Currently redirection is always used, so return
>> 'localhost'
>> -        return "localhost"
>> +        if self.params.get("network") == "bridge":
>> +            # probing ip address through arp
>> +            bridge_name = self.params['bridge']
>> +            macaddr = self.macaddr[0]
>>     
>
> I think VM.get_address() should take an index parameter, instead of
> just return the first address. The index parameter can default to 0.
>
>   
Agree.
>> +            lines = os.popen("arp -a").readlines()
>> +            for line in lines:
>> +                if macaddr in line:
>> +                    return line.split()[1].strip('()')
>> +                
>> +            # probing ip address through nmap
>> +            lines = os.popen("ip route").readlines()
>> +            birdge_network = None
>> +            for line in lines:
>> +                if bridge_name in line:
>> +                    bridge_network = line.split()[0]
>> +                    break
>> +                
>> +            if bridge_network != None:
>> +                lines = os.popen("nmap -sP -n %s" %
>> bridge_network).readlines()
>> +                lastline = None
>> +                for line in lines:
>> +                    if macaddr in line:
>> +                        return lastline.split()[1]
>> +                    lastline = line
>> +
>> +            # could not found ip address
>> +            return None
>> +        else:
>> +            return "localhost"
>>  
>> -    def get_port(self, port):
>> +    def get_port(self, port, query = False):
>>          """Return the port in host space corresponding to port in
>> guest space.
>>  
>>          If port redirection is used, return the host port redirected
>> to guest port port.
>>          Otherwise return port.
>>          """
>> -        # Currently redirection is always used, so use the redirs
>> dict
>> -        if self.redirs.has_key(port):
>> -            return self.redirs[port]
>> +
>> +        if query == True or self.params.get("network") != "bridge":
>>     
>
> Why do we need a 'query' parameter here? It looks to me like
> self.params.get("network") != "bridge"
> should suffice.
>   
The query parameter is to make sure the guests nics could work well when 
mixing redirections and public bridges. I don't know if this kind of 
composition is useful.
>> +            if self.redirs.has_key(port):
>> +                return self.redirs[port]
>> +            else:
>> +                kvm_log.debug("Warning: guest port %s requested but
>> not redirected" % port)
>> +                return None
>>          else:
>> -            kvm_log.debug("Warning: guest port %s requested but not
>> redirected" % port)
>> -            return None
>> +            return port
>>  
>>      def is_sshd_running(self, timeout=10):
>>          """Return True iff the guest's SSH port is responsive."""
>>          address = self.get_address()
>>          port = self.get_port(int(self.params.get("ssh_port")))
>> -        if not port:
>> +        if not port or not address:
>>              return False
>>          return kvm_utils.is_sshd_running(address, port,
>> timeout=timeout)
>>     
>
> Again, I will do my best to post my patches soon.
>
>   
OK, wait for your patch.
> Thanks,
> Michael
> --
> 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
>   
Thanks.
Jason