Ok. I do not agree with this approach. (Calling .create() on old VM object. I still do not get the reason for doing this.) You can see how much efforts it took to find the source of the bug.
Nonetheless, I would provide a very simple solution: add next two lines: + # Drop old Spice options. New Spice options will be taken from self.params + self.spice_options = {} just before: for skey in spice_keys: value = params.get(skey, None) if value: logging.warn("Add: %s, %s", skey, value) self.spice_options[skey] = value What do you think about this solution? On Thu, Feb 2, 2017 at 5:35 PM, Lukáš Doktor <ldok...@redhat.com> wrote: > Dne 2.2.2017 v 15:07 Andrei Stepanov napsal(a): > >> 1. >> >> 2017-02-02 13:23:59,568 job L0356 INFO | >> vt.setup.keep_guest_running False >> >> OK, this simplifies thing and the VM object should always be dead when > obtained from env (this means the `needs_restart` is not used and I don't > need to care about it for now) > > 2. >> >> We call vm.create( ... params ....) line ~ 170 - 180 on old VM object. >> This is our mistake. >> >> This is not a mistake. Calling `vm.create` with different params is > (according to definition) perfectly valid usage and several tests are using > it to re-create machine throughout the test execution. If the > `VM.spice_options` don't support it correctly, that is a different question > and that is what needs to be adjusted. I went through the sources and I > think I see one of the possible issues causing that. When the `display == > spice` in `params` the spice keys are mirrored to `VM.spice_options` and > then they are used instead of the `params` options. I don't know the > history but this seems unacceptable to me, because basically this: > > 1. all settings for VM are in params > 2. during `VM.make_create_command` some CONFIGS are mirrored to > `VM.spice_options` > 3. other DYNAMIC values are added to `VM.spice_options` > 4. let's recreate the machine by VM.create(params=params) > 5. during `VM.make_create_command` new CONFIGS are mirrored to > `VM.spice_options` while previous CONFIG options are preserved as well as > DYNAMIC params > 6. new crippled machine is created > > My issue here is that the `VM.spice_options` combines CONFIG and DYNAMIC > params. I don't know why but this itself is not a good idea and instead of > `self.spice_options` in `add_spice()` `params.get()` should be used to get > configuration and elsewhere where you are asking about the actual values of > the ports `self.spice_options` should be used. That way with new params > it'd assign new ports and it would be not spoiled by `self.spice_options`, > therefor the machine would be started with correct fresh values. On the > other hand the `self.spice_options` would not be consistent as they would > possibly contain outdated information. > > To avoid the problem with outdated `self.spice_options` you can say they > are basically a cache with the current values and you need to treat it that > way. Instead of copying the values all the time you need to use local > variable inside `VM.make_create_command`, report the new content and > override the content in `VM.create`. > > There is still one thing to decide, whether `spice_options` are dynamic > (therefor different port matters) or whether they are static (therefor > different port forces the machine to be re-created). If they are dynamic, > than you should treat them similarly as `self.redirs` are. If not then you > should just wipe them during `make_create_command` as they are basically > just a cache, anyway this is important for `VM.needs_restart` which is not > in question for now (will probably be later when we fix this issue). > > Anyway to wrap it up I don't think the env is broken. It re-uses the old > VM object and creates a new one during `VM.create` which is, according to > definition, a correct usage. If this does not behaves correctly than the > `spice` handling inside `VM.create()` (or `VM.make_create_command`) is not > compatible with the definition and it worked only because nobody needed to > change those options between `VM.create()` calls. Would you please verify > this hypothesis is correct? I haven't been involved with `spice` much so > I'm not an expert there. I only know how `VM.create` should behave. > > Kind regards, > Lukáš > > >> For example >> ---------------- >> >> VM object from previous test already has options: >> >> self.spice_options = {} >> >> Go to : qemu_vm.py Line: ~~ 2028 >> >> for skey in spice_keys: >> value = params.get(skey, None) >> if value: >> logging.warn("Add: %s, %s", skey, value) >> self.spice_options[skey] = value <-------- >> If next test doesn't define Spice params than params from previous test >> remain. We do not flush self.spice_options. >> >> We do not flush all old VM.xxxxxxxx members. And sometimes, they are >> taken from previous tests. >> >> As a result VM sometimes gets wrongs cmdline. >> >> >> On Thu, Feb 2, 2017 at 1:56 PM, Lukáš Doktor <ldok...@redhat.com >> <mailto:ldok...@redhat.com>> wrote: >> >> Hello Andrei, >> >> first, can you please confirm you are using the `keep_guest_running` >> to minimize the environment differences. >> >> Then to your reproducer, I'm not sure how to trigger it. I use a >> modified `boot` test where I run the pre-process twice with modified >> params. This way I get your "Old vm is destroyed, but, it is still >> present in env." message, but this message only means the instance >> is reused. It does not mean it is used to boot the machine. The >> important part is that `start_vm` is set to `True` which means that >> around line `173` the old `params` are replaced with the new fresh >> ones so at least in my understanding it should work properly. Anyway >> mistakes happen so would you please give me a simple reproducer or >> at least more info about where this does not work. >> >> Regards, >> Lukáš >> >> >> Dne 2.2.2017 v 12:53 Andrei Stepanov napsal(a): >> >> Lukáš Hi! >> >> I added next debug code: >> >> diff --git a/virttest/env_process.py b/virttest/env_process.py >> index d05976e..64c78ac 100644 >> --- a/virttest/env_process.py >> +++ b/virttest/env_process.py >> @@ -162,6 +162,12 @@ def preprocess_vm(test, params, env, name): >> vm.devices = None >> start_vm = True >> old_vm.destroy(gracefully=gracefully_kill) >> + for key1 in env.keys(): >> + vm1 = env[key1] >> + if not isinstance(vm1, virt_vm.BaseVM): >> + continue >> + if vm1.name <http://vm1.name> >> <http://vm1.name> == old_vm.name <http://old_vm.name> >> <http://old_vm.name>: >> + logging.warn("Old vm is destroyed, >> but, it >> is still present in env.") >> update_virtnet = True >> >> if start_vm: >> >> >> >> Than logs have message: "Old vm is destroyed, but, it is still >> present >> in env." >> >> So, VM was destroyed, VM object is still in env. >> >> Let's go to line 690 in the same file: >> >> if vm.name <http://vm.name> <http://vm.name> not in >> requested_vms: >> >> VM for next test has the same name. >> >> As a result: next test uses VM object from previous test. VM is >> started >> using params from previous test. >> >> And this behavior is serious bug. >> >> >> On Wed, Feb 1, 2017 at 3:05 PM, Lukáš Doktor <ldok...@redhat.com >> <mailto:ldok...@redhat.com> >> <mailto:ldok...@redhat.com <mailto:ldok...@redhat.com>>> wrote: >> >> Hello Andrei, >> >> if this happens than there is something really wrong because >> Avocado >> should re-create the VM for 2 reasons: >> >> 1) by default VMs are not shared between tests (can be >> enabled in >> cfg by setting `keep_guest_running = True` in `vt.setup` >> section) >> 2) when the params of the existing VM and the current params >> are >> different, it's recreated. >> >> The (2) is checked in `virttest.env_process` on line `159` >> where it >> executes `vm.needs_restart`. The implementation of this >> function is >> defined mainly in `virttest.virt_vm` and unless overridden >> it uses >> the `virttest.virt_vm.make_create_command` to compare the >> original >> and the new command line to create the VM. If they are the >> same it >> reuses the VM (when (1) is enabled), otherwise it destroys >> the old >> VM and creates a new one. >> >> The question is how different your machines are. The >> `make_create_command` does not compares the extra dynamic >> stuff like >> migration. More info about this can be found in >> `virttest.qemu_vm.create()` function (if using qemu as a >> backend). >> >> Would you please (publicly or in private) share more details >> I might >> be able to identify why the machine is not being re-created. >> >> Regards, >> Lukáš >> >> Dne 31.1.2017 v 18:15 Andrei Stepanov napsal(a): >> >> Hi. >> >> It seems that avocado-vt has a serious bug. >> >> Test case: run a few avocado-vt tests in a bunch. For >> example >> two tests. >> Test1 starts just right after Test2. >> >> Test1. >> Test2. >> >> Test1 & Test2 use the same name for VM in cartesian >> configs: >> vms = guest >> >> Other options for VM() objects are different, for >> example port >> VNC port, >> some device config, etc.... >> >> The problem is that: KVM from Test2 uses VM() object >> that was >> created >> for Test1. >> >> For Test2: >> virttest/env_process.py: >> >> def preprocess_vm(test, params, env, name): >> >> vm = env.get_vm(name) <--- returns VM that was >> created >> for Test1. >> create_vm == False >> >> It can be fixed by: >> >> diff --git a/virttest/env_process.py >> b/virttest/env_process.py >> index d05976e..7c08df4 100644 >> --- a/virttest/env_process.py >> +++ b/virttest/env_process.py >> @@ -687,9 +687,8 @@ def preprocess(test, params, env): >> vm = env[key] >> if not isinstance(vm, virt_vm.BaseVM): >> continue >> - if vm.name <http://vm.name> <http://vm.name> >> <http://vm.name> not in >> requested_vms: >> - vm.destroy() >> - del env[key] >> + vm.destroy() >> + del env[key] >> >> if (params.get("auto_cpu_model") == "yes" and >> vm_type == "qemu"): >> >> >> Could you please confirm that bug exists in real? >> >> >> >> >> >> >