Re: [Autotest] [PATCH] Test 802.1Q vlan of nic

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Oct 20, 2009 at 09:19:50AM -0400, Michael Goldish wrote:
> See comments below.

Hi all,
Thanks for your reply.
 
> ----- "Dor Laor" <dlaor@xxxxxxxxxx> wrote:
> 
> > On 10/15/2009 11:48 AM, Amos Kong wrote:
> > >
> > > Test 802.1Q vlan of nic, config it by vconfig command.
> > >    1) Create two VMs
> > >    2) Setup guests in different vlan by vconfig and test
> > communication by ping
> > >       using hard-coded ip address
> > >    3) Setup guests in same vlan and test communication by ping
> > >    4) Recover the vlan config
> > >
> > > Signed-off-by: Amos Kong<akong@xxxxxxxxxx>
> > > ---
> > >   client/tests/kvm/kvm_tests.cfg.sample |    6 +++
> > >   client/tests/kvm/tests/vlan_tag.py    |   73
> > +++++++++++++++++++++++++++++++++
> > >   2 files changed, 79 insertions(+), 0 deletions(-)
> > >   mode change 100644 =>  100755 client/tests/kvm/scripts/qemu-ifup
> > 
> > In general the above should come as an independent patch.
> > 
> > >   create mode 100644 client/tests/kvm/tests/vlan_tag.py
> > >
> > > diff --git a/client/tests/kvm/kvm_tests.cfg.sample
> > b/client/tests/kvm/kvm_tests.cfg.sample
> > > index 9ccc9b5..4e47767 100644
> > > --- a/client/tests/kvm/kvm_tests.cfg.sample
> > > +++ b/client/tests/kvm/kvm_tests.cfg.sample
> > > @@ -166,6 +166,12 @@ variants:
> > >           used_cpus = 5
> > >           used_mem = 2560
> > >
> > > +    - vlan_tag:  install setup
> > > +        type = vlan_tag
> > > +        subnet2 = 192.168.123
> > > +        vlans = "10 20"
> > 
> > If we want to be fanatic and safe we should dynamically choose subnet
> > and vlans numbers that are not used on the host instead of hard code
> > it.
> 
> For the sake of safety maybe we should start both VMs with -snapshot.
> Dor, what do you think?  Is it safe to start 2 VMs with the same disk image
> when only one of them uses -snapshot?

Setup the second VM with -snapshot is enough. The image can only be R/W by 1th VM.
 
> > > +        nic_mode = tap
> > > +        nic_model = e1000
> > 
> > Why only e1000? Let's test virtio and rtl8139 as well. Can't you
> > inherit the nic model from the config?
> 
> It's not just inherited, it's overwritten, because nic_model is defined
> later in the file in a variants block.  So this nic_model line has no
> effect.

No, this line is effective. If reserve this line, this case just test e1000, not the default 8139.
 
> > >
> > >       - autoit:       install setup
> > >           type = autoit
> > > diff --git a/client/tests/kvm/scripts/qemu-ifup
> > b/client/tests/kvm/scripts/qemu-ifup
> > > old mode 100644
> > > new mode 100755
> > > diff --git a/client/tests/kvm/tests/vlan_tag.py
> > b/client/tests/kvm/tests/vlan_tag.py
> > > new file mode 100644
> > > index 0000000..15e763f
> > > --- /dev/null
> > > +++ b/client/tests/kvm/tests/vlan_tag.py
> > > @@ -0,0 +1,73 @@
> > > +import logging, time
> > > +from autotest_lib.client.common_lib import error
> > > +import kvm_subprocess, kvm_test_utils, kvm_utils
> > > +
> > > +def run_vlan_tag(test, params, env):
> > > +    """
> > > +    Test 802.1Q vlan of nic, config it by vconfig command.
> > > +
> > > +    1) Create two VMs
> > > +    2) Setup guests in different vlan by vconfig and test
> > communication by ping
> > > +       using hard-coded ip address
> > > +    3) Setup guests in same vlan and test communication by ping
> > > +    4) Recover the vlan config
> > > +
> > > +    @param test: Kvm test object
> > > +    @param params: Dictionary with the test parameters.
> > > +    @param env: Dictionary with test environment.
> > > +    """
> > > +
> > > +    vm = []
> > > +    session = []
> > > +    subnet2 = params.get("subnet2")
> > > +    vlans = params.get("vlans").split()
> > > +
> > > +    vm.append(kvm_test_utils.get_living_vm(env, "%s" % params.get("main_vm")))
> 
> There's no need for the "%s" here.
> ...get_living_vm(env, params.get("main_vm"))) should work.
> 
> > > +    params_vm2 = params.copy()
> > > +    params_vm2['image_snapshot'] = "yes"
> > > +    params_vm2['kill_vm_gracefully'] = "no"
> > > +    params_vm2["address_index"] = int(params.get("address_index", 0))+1
> > > +    vm.append(vm[0].clone("vm2", params_vm2))
> > > +    kvm_utils.env_register_vm(env, "vm2", vm[1])
> > > +    if not vm[1].create():
> > > +        raise error.TestError("VM 1 create faild")
> > 
> > 
> > The whole 7-8 lines above should be grouped as a function to clone 
> > existing VM. It should be part of kvm autotest infrastructure.
> > Besides that, it looks good.
> 
> There's already a clone function and it's being used here.
> 
> Instead of those 7-8 lines, why not just define the VM in the config file?
> It looks like you're always using 2 VMs so there's no reason to do this in
> test code.  This should do what you want:
> 
> - vlan_tag:  install setup
>     type = vlan_tag
>     subnet2 = 192.168.123
>     vlans = "10 20"
>     nic_mode = tap
>     vms += " vm2"
>     extra_params_vm2 += " -snapshot"
>     kill_vm_gracefully_vm2 = no
>     address_index_vm2 = 1
> 
> The preprocessor then automatically creates vm2 and registers it in env.
> To use it in the test just do:
> 
> vm.append(kvm_test_utils.get_living_vm(env, "vm2"))
> 
> You can also use a parameter that tells the test which VM to use if you don't
> want the name "vm2" hardcoded into the test.
> Add something like this to the config file:
> 
>     2nd_vm = vm2
> 
> and in the test use params.get("2nd_vm") instead of "vm2" (just like you use
> "main_vm").

Yes, this is better.

> > > +
> > > +    for i in range(2):
> > > +        session.append(kvm_test_utils.wait_for_login(vm[i]))
> > > +
> > > +    try:
> > > +        vconfig_cmd = "vconfig add eth0 %s;ifconfig eth0.%s %s.%s"
> > > +        # Attempt to configure IPs for the VMs and record the
> > results in
> > > +        # boolean variables
> > > +        # Make vm1 and vm2 in the different vlan
> > > +
> > > +        ip_config_vm1_ok =
> > (session[0].get_command_status(vconfig_cmd
> > > +                                   % (vlans[0], vlans[0], subnet2,
> > "11")) == 0)
> > > +        ip_config_vm2_ok =
> > (session[1].get_command_status(vconfig_cmd
> > > +                                   % (vlans[1], vlans[1], subnet2,
> > "12")) == 0)
> > > +        if not ip_config_vm1_ok or not ip_config_vm2_ok:
> > > +            raise error.TestError, "Fail to config VMs ip address"
> > > +        ping_diff_vlan_ok = (session[0].get_command_status(
> > > +                             "ping -c 2 %s.12" % subnet2) == 0)
> > > +
> > > +        if ping_diff_vlan_ok:
> > > +            raise error.TestFail("VM 2 is unexpectedly pingable in
> > different "
> > > +                                 "vlan")
> > > +        # Make vm2 in the same vlan with vm1
> > > +        vlan_config_vm2_ok = (session[1].get_command_status(
> > > +                              "vconfig rem eth0.%s;vconfig add eth0
> > %s;"
> > > +                              "ifconfig eth0.%s %s.12" %
> > > +                              (vlans[1], vlans[0], vlans[0],
> > subnet2)) == 0)
> > > +        if not vlan_config_vm2_ok:
> > > +            raise error.TestError, "Fail to config ip address of VM
> > 2"
> > > +
> > > +        ping_same_vlan_ok = (session[0].get_command_status(
> > > +                             "ping -c 2 %s.12" % subnet2) == 0)
> > > +        if not ping_same_vlan_ok:
> > > +            raise error.TestFail("Fail to ping the guest in same
> > vlan")
> > > +    finally:
> > > +        # Clean the vlan config
> > > +        for i in range(2):
> > > +            session[i].sendline("vconfig rem eth0.%s" % vlans[0])
> > > +            session[i].close()
> 
> Please use get_command_output() or get_command_status() instead of
> sendline() when possible.  get_command_output() waits for the shell prompt
> to return so we know the guest received the command.  sendline() just sends
> a string to the session without waiting, so the close() that follows might
> kill the session before it receives or processes the command.

Agree with you.
When I test this case, the original get_command_status() always cause special read problem, so I use sendline().

I'll replace sendline() with get_command_status() later.
 
> Other than these minor issues the test looks good.

I'll re-send another patch later. Thanks again!

-- 
Amos Kong
Quality Engineer
Raycom Office(Beijing), Red Hat Inc.
Phone: +86-10-62608183
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux