Thank you very much for your help! On Thu, Feb 2, 2017 at 6:49 PM, Lukáš Doktor <[email protected]> wrote:
> Sure, the thing is there are some high priority tasks on my TODO list, so > let me create https://trello.com/c/EkWc1DC0/910-fix-the-vm-spice-options-u > sage-to-allow-vm-create-with-different-params and I'll do my best to > squeeze it in and work on it soon. I don't see it as a complex task (as I > have some knowledge of this part of the code) but I don't know when it'll > end up on my TODO list. So if you have some free time and the card is still > not in WiP backlog you can try fixing it yourself. > > Lukáš > > Dne 2.2.2017 v 18:23 Lukáš Doktor napsal(a): > > Unfortunately it's not that simple, because the `make_create_command` is >> called with various params during the life of the VM instance. The >> persistent changes need to be done in `VM.create`. The example can be >> taken from `VM.devices` handling, basically: >> >> 1. VM.devices = None >> 2. in VM.make_create_command local variable `devices` is used >> 3. in VM.create the `self.devices` is overwritten by the reported >> `devices` from the `VM.make_create_command` (because we are actually >> modifying params of a clone) >> >> The same treatment should work for `spice_options` as well. This is the >> simple part, now in order to properly support `needs_restart` (which is >> actually optional and we could live without `spice` options to cause >> false-positives (false-negatives are unacceptable, though)) you need to >> decide whether some dynamic data (eg. ports) should be preserved when >> creating the `make_create_command`. The example is `self.redirs` which >> is reused by `make_create_command`. >> >> Anyway as I said this second part is optional and can be left for >> someone interested in reusing VMs with spice in multiple tests (which is >> exactly what you do not want to do...). >> >> Does this sound good to you? >> Lukáš >> >> PS: I don't say this is the optimal solution, but it exists for so long >> that no one sane would try to rewrite it with a different approach so >> I'd suggest just copy&paste the solution already used in code rather >> than inventing something clearer (like a better `VM.needs_restart` >> method). >> >> >> Dne 2.2.2017 v 17:48 Andrei Stepanov napsal(a): >> >>> 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 <[email protected] >>> <mailto:[email protected]>> 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 <[email protected] >>> <mailto:[email protected]> >>> <mailto:[email protected] <mailto:[email protected]>>> 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> >>> <http://vm1.name> == old_vm.name <http://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> >>> <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 >>> <[email protected] <mailto:[email protected]> >>> <mailto:[email protected] <mailto:[email protected]>> >>> <mailto:[email protected] <mailto:[email protected]> >>> <mailto:[email protected] <mailto:[email protected]>>>> 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> >>> <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? >>> >>> >>> >>> >>> >>> >>> >>> >> >
