On 2013-2-14 23:18, Chris Evich wrote:
On 02/06/2013 12:08 AM, liyang wrote:
Signed-off-by: Li Yang <[email protected]>
...
+def run_virsh_domjobabort(test, params, env):
+ """
+ Test command: virsh domjobabort.
+
+ The command can abort the currently running domain job.
+ 1.Prepare test environment,destroy or suspend a VM.
+ 2.When the libvirtd == "off", stop the libvirtd service.
+ 3.Do action to get a subprocess.
+ 4.Perform virsh domjobabort operation.
+ 5.Recover test environment.(If the libvirtd service is stopped
,start
+ the libvirtd service.)
+ 6.Confirm the test result.
+ """
I think we need a bit more explanation above, -or- more comments
below, because this test is not as simple as 1-6 makes it sound :)
I will add some more comments for this test..
+ vm_name = params.get("main_vm", "vm1")
+ vm = env.get_vm(vm_name)
+ start_vm = params.get("start_vm")
+ pre_vm_state = params.get("pre_vm_state", "start")
+ if start_vm == "no" and vm.is_alive():
+ vm.destroy()
You have a good explanation in your patch-email why you didn't use
"paused_after_start_vm". Can you put that into a comment here in the
code also :)
+ def get_subprocess(action, vm_name, file):
+ """
+ Get subprocess with vm_ref and other conditions.
+
+ @param: cmd : virsh command.
+ @param: guest_name : VM's name
+ @param: file_source : virsh command's file option.
+ """
+ if action == "managedsave" or action == "snapshot-create":
+ file = ""
+ command = "virsh %s %s %s" % (action, vm_name, file)
+ p = subprocess.Popen(command,shell=True,
stdout=subprocess.PIPE,
+ stderr=subprocess.PIPE)
+ return p
I talked with lmr about this, since this function is so closely tied
to virsh.domjobabort(). He suggested, and I agree that it's okay to
put this here for now. But we need to keep an eye out if this
functionality should get added into virsh.command() (or similar) in
the future. For now, since it's just for this one test, it's okay to
have it here (and also because aexpect has a hard-to-find FD leak bug
still driving lmr crazy). I would suggest updating the docstring to
be a little more descriptive. For example:
"""
Execute background virsh command, return subprocess w/o waiting for
exit()
"""
+ if libvirtd == "off":
+ libvirt_vm.libvirtd_stop()
I think having libvirtd running here is enough of an implied
requirement that we needn't test with it stopped. Plenty of other
tests confirm that "bad things happen" to virsh commands w/o libvirtd.
Removing it will also make the logic in this test simpler.
+ #Get the subprocess of VM
+ process = None
+ if job == "yes" and start_vm == "yes":
+ process = get_subprocess(action, vm_name, tmp_file)
This part needs a lot more commenting. It took me quite a while to
follow exactly what this test was doing. What's needed is an
explanation of "why" test needs a subprocess here.
Indeed, and my code habit is not good, I will pay attention to this.
Alternatively, clearing this up w/in the run_virsh_domjobabort()
docstring would be acceptable too. Because this is the 0.000001% of
tests where a not-yet-exited process is needed, we need to be careful
to alert other developers to what's happening.
Make sense?
+
+ ret = virsh.domjobabort(vm_ref, ignore_status=True)
+ status = ret.exit_status
As a future "todo", a good test of this domjobabort command would be
to see if it can cope with a defunct/zombie subprocess. Another good
one would be to test on a subprocess that's been stopped (sigstop).
+ #recover libvirtd service start
+ if libvirtd == "off":
+ libvirt_vm.libvirtd_start()
+ if pre_vm_state == "suspend":
+ vm.resume()
+ if process:
+ process.wait()
Do we need more recovery here? If the subprocess abort fails, then
process.wait() could conceivable wait forever. It would be good to
wrap this call inside a "wait_for" with a timeout. This way autotest
doesn't potentially get "stuck" for a long time.
Maybe I can use process.kill() to kill this process...
+
+ #check status_error
+ if status_error == "yes":
+ if status == 0:
+ raise error.TestFail("Run successfully with wrong
command!")
+ elif status_error == "no":
+ if status != 0:
+ raise error.TestFail("Run failed with right command")
Otherwise this is a good test, and fairly simple (once I understood
what was happening). I think a little work on comments/docstrings
will help a lot. Oh, and don't forget to create a topic issue ;)
Sorry for that I will add it right now...
--
Regards,
--------------------------------------------------
Li Yang
No. 6 Wenzhu Road, Nanjing, 210012, China
TEL：+86+25-86630566-8526
FUJITSU INTERNAL：7998-8526
FAX：+86+25-83317685
EMail：[email protected]
--------------------------------------------------
This communication is for use by the intended recipient(s) only and may contain
information that is privileged, confidential and exempt from disclosure under
applicable law. If you are not an intended recipient of this communication, you
are hereby notified that any dissemination, distribution or copying hereof is
strictly prohibited. If you have received this communication in error, please
notify me by reply e-mail, permanently delete this communication from your
system, and destroy any hard copies you may have printed
_______________________________________________
Virt-test-devel mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/virt-test-devel
