env_process: Refactor {pre,post}process_vm_off_hook execution

[bgartzi]

This is part of the env_process {pre,post}process function refactoring. Now it was turn of the {pre,post}process_vm_off_hook execution steps.

I couldn't find any direct use-case in avocado-vt or tp-qemu so I could test the patch. However, as I found in #2106 and #2134 these hooks seem to be a part of the I2N plugin.

I tried to test mock the way the i2n.cmd_parser modifies the hooks through i2n.plugins.auto and i2n.plugins.manu. I did so by adding a few lines in avocado_vt.test that modified the values of the hooks. However, this would feel safer if you @pevogam could have a look and maybe check that it doesn't bring any unintended regression. Would you mind it?

Also, I'd appreciate it if you could have a look too, @YongxueHong.

Thanks in advance!

[bgartzi]

That would have been better if the checks passed. Sorry for the noise @pevogam, @YongxueHong. I will try fix this and get back to you again when it's done.

[pevogam]

Sure thing! I will take a look and come back to you.

[bgartzi]

Okay, @pevogam @YongxueHong, sorry again for the noise.

It looks that CI errors were raised due to a weird behavior in the python3.6 import system that was fixed on python3.7 and newer versions. It seems it was related to [0]. As simple as importing from virttest import env_process instead of import virttest.env_process as env_process.

Now the patch really looks like it's ready to be reviewed and tested.

[0] https://bugs.python.org/issue30024

[YongxueHong]

LGTM.

[pevogam]

Hi @bgartzi I tested this and it is a very harmless change. However, it raised some questions for those of us that are not fully aware of the current trends and what you are working on:

1. Is the same change meant to be done for the {pre|post}process_vm_on_hook cases and why isn't it included in this commit?
2. What is the overall goal of the current refactoring? To establish a new subpackage called test_setup that wraps up some functionality? Is there some RFC/blueprint/motivation somewhere so I can see details?

[bgartzi]

Hi @pevogam! Thanks for testing the patch. Glad to know it didn't cause any problem.

1. Yes, that change is also supposed to be done. However, I'm afraid it will be the last one in the refactoring patch series to be submitted. The main reason behind that is its relative position to the _setup_manager's do_setup and do_cleanup methods. The _setup_manager will run the registered Setupers setup methods in the same order they were registered and the cleanup methods in the inverse order. If we were to refactor the {pre,post}process_vm_on_hooks now, we would run them before any of the un-refactored setup steps, which would most probably introduce a regression. Then, IMO let's better wait until the rest of patches are merged so we can make sure the ...vm_on_hook is the last one registered and so its setup method is the last in the preprocess step and the cleanup method is the first in the postprocess step.
2. I'm afraid there's not an explicit RFC/blueprint explaining the motivation of this work. I will try to cover them though, from most important to less important IMO:
   • Making sure only the cleanup methods of those successful setup steps are ran. In other words, if a setup step fails, the consequent setup steps aren't run. However, in the current state, all the postprocess will be executed. That can lead to more complex environment errors to understand, or in the worst case scenario, a possible test environment corruption. Due to _setup_manager truncating the list of Setupers during do_setup, that issue would be avoided; only the right amount of cleanups would be run.
   • Running setup/cleanup steps of each class symmetrically in the preprocess/postprocess functions.
   • Trying to simplify env_process by reducing the amount of code preprocess and postprocess are holding explicitly there.
   • Although this one being a bit more subjective, grouping related pieces of logic (or Setuper classes) in an ordered fashion under virttest.test_setup submodules.

[pevogam]

Hi @pevogam! Thanks for testing the patch. Glad to know it didn't cause any problem.

1. Yes, that change is also supposed to be done. However, I'm afraid it will be the last one in the refactoring patch series to be submitted. The main reason behind that is its relative position to the `_setup_manager`'s [`do_setup`](https://github.com/avocado-framework/avocado-vt/blob/15fbf5302390aa8e13ffd235ef5f744b9e919b78/virttest/env_process.py#L1033) and [`do_cleanup`](https://github.com/avocado-framework/avocado-vt/blob/15fbf5302390aa8e13ffd235ef5f744b9e919b78/virttest/env_process.py#L1759) methods. The `_setup_manager` will run the registered `Setuper`s `setup` methods in the same order they were registered and the `cleanup` methods in the inverse order. If we were to refactor the `{pre,post}process_vm_on_hook`s now, we would run them before any of the un-refactored setup steps, which would most probably introduce a regression. Then, IMO let's better wait until the rest of patches are merged so we can make sure the `...vm_on_hook` is the last one registered and so its setup method is the last in the preprocess step and the cleanup method is the first in the postprocess step.

I see, I am glad this is kept in mind by you and a potential source of further migration or perhaps a bit of inspiration on the design of the current setup since post-vm-boot or pre-vm-shutdown stages are important stages of customization too.

2. I'm afraid there's not an explicit RFC/blueprint explaining the motivation of this work. I will try to cover them though, from most important to less important IMO:
   
   * Making sure only the `cleanup` methods of those successful `setup` steps are ran. In other words, if a `setup` step fails, the consequent `setup` steps aren't run. However, [in the current state](https://github.com/avocado-framework/avocado-vt/blob/15fbf5302390aa8e13ffd235ef5f744b9e919b78/avocado_vt/test.py#L314), all the [postprocess](https://github.com/avocado-framework/avocado-vt/blob/15fbf5302390aa8e13ffd235ef5f744b9e919b78/virttest/env_process.py#L1437) will be executed. That can lead to more complex environment errors to understand, or in the worst case scenario, a possible test environment corruption. Due to `_setup_manager` [truncating the list of `Setuper`s](https://github.com/avocado-framework/avocado-vt/blob/15fbf5302390aa8e13ffd235ef5f744b9e919b78/virttest/test_setup/core.py#L83-L89) during `do_setup`, that issue would be avoided; only the right amount of `cleanup`s would be run.
   * Running setup/cleanup steps of each class symmetrically in the preprocess/postprocess functions.
   * Trying to simplify `env_process` by reducing the amount of code `preprocess` and `postprocess` are holding explicitly there.
   * Although this one being a bit more subjective, grouping related pieces of logic (or `Setuper` classes) in an ordered fashion under `virttest.test_setup` submodules.

Great, I think a lot of this would also make sense to add in a commit message at the very end of your work (if not already in the beginning) or overarching branch topic so that the rest can also understand what we gain with this.

Other than that the changes look good to me.

[bgartzi]

Sounds fair @pevogam, I will probably add a little summary in each commit I'm submitting on this regard.

Thank you and @YongxueHong for the review!