On Tue, Oct 20, 2009 at 11:19 AM, Michael Goldish <mgoldish@xxxxxxxxxx> wrote:
> See comments below.
>
> ----- "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?
>
>> > + 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.
>
>> >
>> > - 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").
>
>> > +
>> > + 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.
>
> Other than these minor issues the test looks good.
Ok Michael, I found another small things that I was correcting here,
so I will send soon a version addressing your comments and Dor's.
>>
>> _______________________________________________
>> Autotest mailing list
>> Autotest@xxxxxxxxxxxxxxx
>> http://test.kernel.org/cgi-bin/mailman/listinfo/autotest
> _______________________________________________
> Autotest mailing list
> Autotest@xxxxxxxxxxxxxxx
> http://test.kernel.org/cgi-bin/mailman/listinfo/autotest
>
--
Lucas
--
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
