[Avocado-devel] Bug: Once created VM object stays for all tests.
Andrei Stepanov
astepano at redhat.com
Thu Feb 2 16:48:46 UTC 2017
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> 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>> 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 <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> 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/4dfa801b/attachment.htm>
More information about the Avocado-devel
mailing list