This is a HUGE configuration. Looking through it, I see you're using
Cartesian system a little, but maybe it could be leveraged more to
reduce configuration down to less than ~50 lines? Maybe not, which is
okay - it's just that large configuration like this is harder to
maintain. More comments inline...
On 07/11/2012 10:37 PM, tangchen wrote:
> Signed-off-by: Tang Chen<tanc...@cn.fujitsu.com>
> ---
> client/virt/subtests.cfg.sample | 123
> +++++++++++++++++++++++++++++++++++++++
> 1 file changed, 123 insertions(+)
>
> diff --git a/client/virt/subtests.cfg.sample b/client/virt/subtests.cfg.sample
> index 099f28e..36ac612 100644
> --- a/client/virt/subtests.cfg.sample
> +++ b/client/virt/subtests.cfg.sample
> @@ -307,6 +307,129 @@ variants:
> libvirtd = "off"
> status_error = "yes"
>
> + - virsh_attach_detach_interface:
> + type = virsh_attach_detach_interface
> + start_vm = no
> + at_options = ""
> + dt_options = ""
There's little value in abbreviating these names, just spell them out
fully to help code be self-documenting. i.e.:
attach_interface_options = ""
detach_interface_options = ""
It's more typing, but doesn't require comments/documentation to explain
what it's for.
> + vm_ref = name
Same suggestion here, a more descriptive name would be nice. Pretending
I'm a new user, and looking at debug log, I have no idea what "vm_ref"
means outside of the test-code context.
Looking at the code, it appears this is a multiple choice option, a good
way to handle these is descriptive names and a comment of example
values. For example, copying comment from your code:
# How to reference a VM.
# For example: name, id, uuid, and so on.
vm_reference_method = "name"
> + pre_vm_state = running
> + libvirtd_state = on
I lost track of who the patch was for, but...I remember suggesting that
we add both of these as global options to control virt_env_process
preprocess_vm() and postprocess_vm(). I've seen at least two (maybe
more) tests that want to control/check/change initial state of VM and
libvirt. Instead of writing these new for each test, it makes sense to
centralize.
Thoughts? Concerns? Can I help?
P.S. Off-hand, I _THINK_ test module code can actually call
postprocess_vm() if it needs to respond to resulting conditions. i.e. I
don't think there will be harm if autotest then calls this function
again (after test) - but I could be wrong.
> + status_error = no
> + no_attach = no
Again, the "no_attach" name here could be ambiguous (i.e. have multiple
meanings). I can't tell by looking what this option is used for. Even
looking at the test-module code, it's hard to see what this is for:
if test_cmd == "detach-interface" and no_attach != "yes":
...do some stuff...
But I don't see no_attach = "yes" elsewhere in configuration, so maybe
we don't need this no_attach option? It's your call, but let's at least
make it more descriptive name or put some comments.
> + delay = 10
Also could be ambiguous, delay what? Delay winning lottery, NOOOOOOO!
Delay paying too much taxes, YESSSSSSS! :D
> + test_times = 1
This is a confusing name, does it mean how many times to run the test?
Looking at code, this looks like numeric substitution for "nics"
parameter. New networking code doesn't require enumerating all "nics",
so I think this parameter can just be removed. Variants can then use:
nics = nic1 nic2 nic3 nic4
or
nics = nic1
If you need to set some specific details for some or all nics, you just
put it before the name. For example:
mac_nic1 = 01:02:03:04:05:06
See my comments in base.cfg.sample for how this works. Good news is,
code is already written :)
> + # Nic information.
> + device = interface
> + device_type = bridge
> + device_source = virbr0
> + device_targets = vnet1
> + auto_target = no
> + auto_nic_mac = yes
> + bus_type = virtio
Again, all these nic parameters are supported in new network code. It's
all optional, where if you don't specify then you get defaults. It also
supports heterogeneous environments, e.g. vm1_nic1 can be different from
vm2_nic2 even in the same test! Check out my comments in
base.cfg.sample and
https://github.com/autotest/autotest/wiki/KVMAutotest-Networking
Also, if we need to extend the new networking model to support testing
better, let's do it!
> + variants:
> + - attach_interface:
> + test_cmd = attach-interface
> + - detach_interface:
> + test_cmd = detach-interface
> + device_targets = ""
> + variants:
> + - no_vm_name:
> + vm_ref = ""
> + status_error = yes
> + - vm_id:
> + vm_ref = id
> + - hex_vm_id:
> + vm_ref = hex_id
> + status_error = yes
> + - invalid_vm_id:
> + vm_ref = invalid_id
> + invalid_vm_id = 9999
> + status_error = yes
> + - vm_name:
> + - num_vm_name:
> + vm_ref = new_name
> + new_vm_name = 12345678
> + - pound_vm_name:
> + vm_ref = invalid_vm_name
> + # The \ here is used as a escape character.
> + invalid_vm_name = "\#"
> + status_error = yes
This name with "#" character was a hotly debated bugzilla if I remember
correctly. It may have different behavior depending on version of
libvirt. This is specific to the "#" character in my memory, others
might be okay.
I'd suggest maybe find a different invalid character, one that has
always been invalid, even on older versions. For example, "@" or "$[]"
or something like that (check libvirt docs to be sure).
> + - dash_vm_name:
> + vm_ref = new_name
> + new_vm_name = "-vm1"
We should double-check if vm.rename will handle this case properly.
i.e. if the command properly escapes the non-option argument with "--"
or not.
> + - vm_suspend:
> + pre_vm_state = paused
This is another parameter we should see if we can make global in
preprocess_vm() and postprocess_vm().
> + - libvirtd_stop:
> + libvirtd_state = off
> + status_error = yes
> + - vm_uuid:
> + vm_ref = uuid
> + - invalid_vm_uuid:
> + vm_ref = invalid_vm_uuid
> + invalid_vm_uuid = 99999999-9999-9999-9999-999999999999
Instead of introducing another parameter for uuid, I think it's safe for
the test-module code to just hard-code this 999... value. Optionally
(to be very good test), it could also check to make sure there REALLY is
no VM defined with uuid = 9999... :)
My point is, there's little value in defining a new parameter that will
only be used in a single variant. It's good to see if this can be
combined or avoided somehow. Make sense?
> + status_error = yes
> + - vm_shutdown:
> + pre_vm_state = "shut off"
> + status_error = yes
> + - invalid_type:
> + device_type = xyz
> + status_error = yes
> + - invalid_option_1:
> + at_options = xyz
> + dt_options = xyz
> + status_error = yes
> + - invalid_option_2:
> + at_options = "--xyz"
> + dt_options = "--xyz"
> + status_error = yes
> + - invalid_option_3:
> + only detach_interface
> + dt_options = xyz
> + auto_nic_mac = no
> + status_error = yes
> + - invalid_source:
> + only attach_interface
> + device_source = xyz
> + status_error = yes
> + - target_vnet0:
> + only attach_interface
> + device_targets = vnet0
> + status_error = yes
> + - invalid_mac:
> + invalid_nic_mac = xyz
Interesting, this one will be tricky. The new networking code goes to
great lengths to make sure you CAN'T define a nic with an invalid mac
address. This will be difficult test variant to accommodate but I must
admit, it's actually a problem with the networking code, not your test.
How do you feel if we drop this invalid_mac test for now? We can add it
as an enhancement later, after I have chance to review how to make it
possible. I have sent myself a note to investigate more.
N/B: It's complex problem because networking code involves multiple
cache databases and pickling the test context into env file. If a mac
is invalid, it's hard for low-level code to know it's intended to be
wrong instead of cache or env corruption problem.
> + status_error = yes
> + - twice_same_mac:
> + test_times = 2
> + device_targets = vnet1 vnet2
> + nic_macs = 02:17:42:2F:99:01 02:17:42:2F:99:01
As opposed to invalid mac, this test should actually work fine with new
networking code. Just parameters here will need to be changed, so:
mac_nic1 = 02:17:42:2F:99:01
mac_nic2 = 02:17:42:2F:99:01
> + auto_nic_mac = no
> + status_error = yes
> + - invalid_script:
> + only attach_interface
> + at_options = "--script xyz"
> + status_error = yes
> + - loop_10:
> + only attach_interface
> + test_times = 10
> + auto_target = yes
> + - remote:
> + # Here, we test -c option. We first login into a remote
> + # host, and connect back to local host uri to test
> + # -c option. This is being improved.
> + remote_ip = REMOTE_HOST_IP
> + back_uri = qemu+ssh://LOCAL_HOSTNAME.EXAMPLE.COM/system
> + remote_user = root
> + remote_pwd = REMOTE_HOST_ROOT_PASSWORD
> + remote_prompt = #
> + - vm_running_persistent:
> + at_options = "--persistent"
> + dt_options = "--persistent"
> + - vm_shutdown_persistent:
> + at_options = "--persistent"
> + dt_options = "--persistent"
> + pre_vm_state = "shut off"
> +
> - module_probe:
> type = module_probe
> # You can specify your own module list, though it is not needed
> usually.
> -- 1.7.10.2
>
Whew! This must be close to longest configuration I've ever reviewed :)
I'm looking forward to v2! See if you can coordinate with vm.rename()
patch and we can do it all as once. If not, just do like you did and
make e-mail note of dependency - this was very helpful, thanks!
--
Chris Evich, RHCA, RHCE, RHCDS, RHCSS
Quality Assurance Engineer
e-mail: cevich + `@' + redhat.com o: 1-888-RED-HAT1 x44214
_______________________________________________
Autotest mailing list
Autotest@test.kernel.org
http://test.kernel.org/cgi-bin/mailman/listinfo/autotest
