From mboxrd@z Thu Jan 1 00:00:00 1970 From: jason wang Subject: Re: [PATCH][KVM-AUTOTEST] TAP network support in kvm-autotest Date: Fri, 15 May 2009 16:29:18 +0800 Message-ID: <4A0D27DE.5040100@redhat.com> References: <1756214842.211491242297658011.JavaMail.root@zmail05.collab.prod.int.phx2.redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: ulublin@redhat.com, kvm@vger.kernel.org To: Michael Goldish Return-path: Received: from mx2.redhat.com ([66.187.237.31]:44825 "EHLO mx2.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1761303AbZEOI3d (ORCPT ); Fri, 15 May 2009 04:29:33 -0400 Received: from int-mx2.corp.redhat.com (int-mx2.corp.redhat.com [172.16.27.26]) by mx2.redhat.com (8.13.8/8.13.8) with ESMTP id n4F8TYNl001462 for ; Fri, 15 May 2009 04:29:34 -0400 In-Reply-To: <1756214842.211491242297658011.JavaMail.root@zmail05.collab.prod.int.phx2.redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: Michael Goldish =E5=86=99=E9=81=93: 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. > =20 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. > =20 We've considers the use of MAC/IP address pools, but this method need t= o=20 handle the cases of multiple kvm-autotest running on multiple guests.=20 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. > =20 We use nmap to make sure the guest IP could be finally found somehow.=20 During our tests, the scripts may fail to get the IP address of guest=20 when host iptables is turned on. > Please see additional comments below. > > ----- "Jason Wang" wrote: > > =20 >> Hi All: >> This patch tries to add tap network support in kvm-autotest. Multipl= e >> 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 =3D nic1 nic2 >> network =3D bridge >> bridge =3D switch >> ifup =3D/etc/qemu-ifup-switch >> ifdown =3D/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 =3D nic1 nic2 >> network =3D bridge >> bridge =3D switch >> bridge_nic2 =3D virbr0 >> ifup =3D/etc/qemu-ifup-switch >> ifup_nic2 =3D /etc/qemu-ifup-virbr0 >> >> This would makes the virtual machine have two nics: nic1 are connect= ed >> to bridge 'switch' and nci2 are connected to bridge 'virbr0'. >> >> Public mode and user mode nic could also be mixed: >> nics =3D nic1 nic2 >> network =3D bridge >> network_nic2 =3D user >> >> Looking forward for comments and suggestions. >> >> From: jason >> 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=3DNone): >> size -=3D len(data) >> f.close() >> return o.hexdigest() >> + >> +def random_mac(): >> + mac=3D[0x00,0x16,0x30, >> + random.randint(0x00,0x09), >> + random.randint(0x00,0x09), >> + random.randint(0x00,0x09)] >> + return ':'.join(map(lambda x: "%02x" %x,mac)) >> =20 > > Random MAC addresses will not necessarily work everywhere, as far as > I know. That's why I prefer user specified MAC/IP address ranges. > =20 Yes, maybe we could use user specified mac address prefix or more usefu= l=20 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 =3D qemu_path >> self.image_dir =3D image_dir >> self.iso_dir =3D iso_dir >> + self.macaddr =3D [] >> + for nic_name in kvm_utils.get_sub_dict_names(params,"nics")= : >> + macaddr =3D 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 =3D kvm_utils.get_sub_dict(params, nic_name) >> qemu_cmd +=3D " -net nic,vlan=3D%d" % vlan >> + net =3D nic_params.get("network") >> + if net =3D=3D "bridge": >> + qemu_cmd +=3D ",macaddr=3D%s" % self.macaddr[vlan] >> if nic_params.get("nic_model"): >> qemu_cmd +=3D ",model=3D%s" % nic_params.get("nic_m= odel") >> - qemu_cmd +=3D " -net user,vlan=3D%d" % vlan >> + if net =3D=3D "bridge": >> + qemu_cmd +=3D " -net tap,vlan=3D%d" % vlan >> + ifup =3D nic_params.get("ifup") >> + if ifup: >> + qemu_cmd +=3D ",script=3D%s" % ifup >> + else: >> + qemu_cmd +=3D ",script=3D/etc/qemu-ifup" >> =20 > > 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. > =20 Agree > =20 >> + ifdown =3D nic_params.get("ifdown") >> + if ifdown: >> + qemu_cmd +=3D ",downscript=3D%s" % ifdown >> + else: >> + qemu_cmd +=3D ",downscript=3Dno" >> =20 > > The same applies here. > > This is just my opinion; I'd like to hear your thoughts on this too. > =20 If no downscript is specified, qemu would give a warning. >> + else: >> + qemu_cmd +=3D " -net user,vlan=3D%d" % vlan >> vlan +=3D 1 >> =20 >> mem =3D params.get("mem") >> @@ -206,11 +226,11 @@ class VM: >> extra_params =3D params.get("extra_params") >> if extra_params: >> qemu_cmd +=3D " %s" % extra_params >> - >> + =20 >> for redir_name in kvm_utils.get_sub_dict_names(params, >> "redirs"): >> redir_params =3D kvm_utils.get_sub_dict(params, >> redir_name) >> guest_port =3D int(redir_params.get("guest_port")) >> - host_port =3D self.get_port(guest_port) >> + host_port =3D self.get_port(guest_port,True) >> qemu_cmd +=3D " -redir tcp:%s::%s" % (host_port, >> guest_port) >> =20 >> if params.get("display") =3D=3D "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") =3D=3D "bridge": >> + # probing ip address through arp >> + bridge_name =3D self.params['bridge'] >> + macaddr =3D self.macaddr[0] >> =20 > > I think VM.get_address() should take an index parameter, instead of > just return the first address. The index parameter can default to 0. > > =20 Agree. >> + lines =3D os.popen("arp -a").readlines() >> + for line in lines: >> + if macaddr in line: >> + return line.split()[1].strip('()') >> + =20 >> + # probing ip address through nmap >> + lines =3D os.popen("ip route").readlines() >> + birdge_network =3D None >> + for line in lines: >> + if bridge_name in line: >> + bridge_network =3D line.split()[0] >> + break >> + =20 >> + if bridge_network !=3D None: >> + lines =3D os.popen("nmap -sP -n %s" % >> bridge_network).readlines() >> + lastline =3D None >> + for line in lines: >> + if macaddr in line: >> + return lastline.split()[1] >> + lastline =3D line >> + >> + # could not found ip address >> + return None >> + else: >> + return "localhost" >> =20 >> - def get_port(self, port): >> + def get_port(self, port, query =3D False): >> """Return the port in host space corresponding to port in >> guest space. >> =20 >> If port redirection is used, return the host port redirecte= d >> 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 =3D=3D True or self.params.get("network") !=3D "br= idge": >> =20 > > Why do we need a 'query' parameter here? It looks to me like > self.params.get("network") !=3D "bridge" > should suffice. > =20 The query parameter is to make sure the guests nics could work well whe= n=20 mixing redirections and public bridges. I don't know if this kind of=20 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 >> =20 >> def is_sshd_running(self, timeout=3D10): >> """Return True iff the guest's SSH port is responsive.""" >> address =3D self.get_address() >> port =3D 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=3Dtimeout) >> =20 > > Again, I will do my best to post my patches soon. > > =20 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 > =20 Thanks. Jason