Fix recreating VM defined by devices issue:1331 #1348
Conversation
|
Hi Jiří, I like the idea. Please take a look at my questions... |
|
@jzupka there is one big problem with this. In case the fds or whatever changes, make_create_command returns different output and VM will be recreated with fresh params. So all the effort would be wasted. Instead of all this handling we could simply detect the different fds/ports and delete On the other hand if we really want to keep the VM we could update these params in later part of preprocess. |
|
Actually I followed the env_preprocess and the simplest solution and actually even bugfix (probably fixes the issue #1331 too) is to remove self.devices here: diff --git a/virttest/virt_vm.py b/virttest/virt_vm.py
index 689b807..1985ae8 100644
--- a/virttest/virt_vm.py
+++ b/virttest/virt_vm.py
@@ -587,6 +587,7 @@ class BaseVM(object):
logging.debug("\t" + str(self.virtnet))
logging.debug("\t!=")
logging.debug("\t" + str(other_virtnet))
+ self.devices = None
return True
else:
logging.debug(
|
|
Well and the safest way around this would be to skip optimization and recreate devices always in case vm is recreated by env: diff --git a/virttest/env_process.py b/virttest/env_process.py
index e129110..0bcb2c6 100644
--- a/virttest/env_process.py
+++ b/virttest/env_process.py
@@ -113,6 +113,7 @@ def preprocess_vm(test, params, env, name):
if vm.needs_restart(name=name,
params=params,
basedir=test.bindir):
+ self.devices = None
start_vm = True
old_vm.destroy(gracefully=gracefully_kill,
free_mac_addresses=False)
|
|
New version of patch for commnets in commit https://github.com/jzupka/virt-test/commit/db9fe97f7fea61b184df2470c99c8b5e4f65c729 Unfortunately the changes that you suggest are not applicable because self.devices needs to be use in some cases. Hence some changes were done weeks ago. It allows moving and making special changes in special cases. For example during migration when I need to delete some device from VM before migration. This changes allows move vm without ugly hacking in env_preproces or what ever. Something like persistent and executable vm instance movable over network without changes. |
|
After talking with @ldoktor added vm.device=None. For avoid errors between tests. Thanks for ideas. |
|
@ldoktor yes there is another bug. I'll fix it and I send another patch |
|
for testing: sudo AUTOTEST_PATH=../../../ ./run -v -t qemu --no-downloads -g Fedora.19.x86_64 --verbose -m 512 --qemu_sandbox=off --no-cleanup --tests="boot shutdown" -k --keep-image-between-tests --nettype=user --machine-type=q35 --keep-guest-running |
|
new patch fix problems from commit https://github.com/jzupka/virt-test/commit/26a95943ef8808cc28f60446d0355702c749b462 |
|
Hi Jiří, another maybe stupid question, do we really need the info about those dynamic parameters? I mean is there a benefit of having them stored and not always generated during vm.create() function accordingly to the current params? It would be perfectly clean and simple and currently I'm not able to think of the possible drawback (comparison could be tricky, but solvable with the special device). The number of required modification would be minimal. |
|
Hi Lukáš, thank you for your reaction. I think there is also another solution. If I have special device for everything what is someway strange and when you suggest wiring there "ok mac addr and fds and what ever will not be compared any more". Then there could be another way how to do this. _Simply don't compare dynamic things at all._ I wanted to avoid this solution because I afraid of removing full compare function. However, if you suggest create special object for every dev then we can remove full compare function and let there only compare without dynamic parameters. With using this solution it will be clean. What you think about this solution? We both want find someway how to fix this problem. I prefer general solution. I don't like wiring of something if I don't really afraid of speed and it is really not necessary do this. If you really want do wired solution with replacement of vritnet then somebody else do this. I don't want spend mi time on this in this case. |
|
Hi Jiří, it seems to work fine. It even fixes the unnecessarily recreation on MAC addr change (two I believe this is ready for inclusion, even thought I'd prefer to remove all Acked-by: Lukáš Doktor [email protected] |
|
@lmr It worked for ldoktor. Please could you take a look on that too. |
Allows to search specific device in qemu container by filter for params. Created due to searching in the QStringDevices. Signed-off-by: Jiří Župka <[email protected]>
|
OK I tested this patch and still works as expected. Acked-by: Lukáš Doktor [email protected] |
|
^^ note there was one line which needed re-indentation, @jzupka, I hope you fixed it already... |
This patch make qdevice more verbose during qcontainer comparison.
Also this patch add support for dynamic params in general devices
like QStringDevice and QCustomDevice and for devices classed inherited from them.
It sets cmdline with and without dynamic params for QStringDevice.
dev = QStringDevice(dev_type="xxx", cmdline="xxx:dynamicparam",
cmdline_nd="xxx:DYN")
It allows specification which param is dynamic for QCustomDevice
dev = QCustomDevice(dev_type="xxx")
dev.set_param(name, value, something, dynamic True/False) default is true.
Comparison of qcontainer using == works as usual it compare all params
without dynamic params.
Some of qemu devices needs some kind of preparation before
start of qemu vm. Unfortunately there preparatory work are
done during creation of devices. For example network tap devices,
port redir, etc.
It should be divided in some later patch.
Signed-off-by: Jiří Župka <[email protected]>
|
@ldoktor Indentation fixed. Thanks for review :-) |
|
@jzupka, @ldoktor, I've rebased this and started testing... I've got a python core dump running the default qemu set, first test: |
|
Well I haven't encounter this problem. Today I tested more tests and still without any problems running the same set of tests as you have... btw @jzupka, it needs rebase again :-( |
|
I did make a rebase, I can push the branch with the rebase. Unfortunately, the 'stack smashing' bug is one of the scariest I've seen in recent times, so we need to figure out where it comes from, and if it's some bug in the python version my machine (Fedora 20) runs. |
|
Ok, I found out that this was due to https://bugzilla.redhat.com/show_bug.cgi?id=1068664 And not @jzupka's code. Phew :) The code looks good to me, I'm going to push #1470 |
|
Well I found one possible issue in qcontainer, @jzupka please take look on it. |
virt: qcontainer adds possibility to find device by params.
qemu: Fix recreating VM defined by devices
It should solve problems in issue
#1331