[Avocado-devel] Bug: Once created VM object stays for all tests.
Lukáš Doktor
ldoktor at redhat.com
Thu Feb 2 17:23:56 UTC 2017
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 --------------
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/3fd483c7/attachment.sig>
More information about the Avocado-devel
mailing list