[Avocado-devel] Bug: Once created VM object stays for all tests.
Lukáš Doktor
ldoktor at redhat.com
Thu Feb 2 16:35:00 UTC 2017
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 --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 502 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/avocado-devel/attachments/20170202/d2222854/attachment.sig>
More information about the Avocado-devel
mailing list