Sounds good.
Let's skip 'optional part'. + Leave a comment on a bright spot: VMs with
Spice doesn't support reusing.
But, I really do not know how to write it. I do not understand
'needs_restart' testcase. My justification would be pale and incomplete.
Could I ask you to write it?
About: make_create_command()
I proposed to add:
+ # Drop old Spice options. New Spice options will be taken from self.params
+ self.spice_options = {}
before:
for skey in spice_keys:
It is a part of make_create_command().
Is it a right solution?
On Thu, Feb 2, 2017 at 6:23 PM, Lukáš Doktor <ldok...@redhat.com> wrote:
> 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 <ldok...@redhat.com
>> <mailto:ldok...@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 <ldok...@redhat.com
>> <mailto:ldok...@redhat.com>
>> <mailto:ldok...@redhat.com <mailto:ldok...@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
>> <ldok...@redhat.com <mailto:ldok...@redhat.com>
>> <mailto:ldok...@redhat.com <mailto:ldok...@redhat.com>>
>> <mailto:ldok...@redhat.com <mailto:ldok...@redhat.com>
>> <mailto:ldok...@redhat.com <mailto:ldok...@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?
>>
>>
>>
>>
>>
>>
>>
>>
>
