[Avocado-devel] Bug: Once created VM object stays for all tests.
Andrei Stepanov
astepano at redhat.com
Thu Feb 2 17:52:53 UTC 2017
Thank you very much for your help!
On Thu, Feb 2, 2017 at 6:49 PM, Lukáš Doktor <ldoktor at redhat.com> 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 <ldoktor at redhat.com
>>> <mailto:ldoktor at 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 <ldoktor at redhat.com
>>> <mailto:ldoktor at redhat.com>
>>> <mailto:ldoktor at redhat.com <mailto:ldoktor at 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>
>>> <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
>>> <ldoktor at redhat.com <mailto:ldoktor at redhat.com>
>>> <mailto:ldoktor at redhat.com <mailto:ldoktor at redhat.com>>
>>> <mailto:ldoktor at redhat.com <mailto:ldoktor at redhat.com>
>>> <mailto:ldoktor at redhat.com <mailto:ldoktor at 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>
>>> <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?
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/avocado-devel/attachments/20170202/0d986e8a/attachment.htm>
More information about the Avocado-devel
mailing list