Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor host dmesg verification into a Setuper #3994

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bgartzi
Copy link
Contributor

@bgartzi bgartzi commented Sep 3, 2024

The main goal of this patch is to add a new Setuper subclass named VerifyHostDMesg, then use it in the env_process {pre,post}process methods setup_manager.

Meanwhile, I'm renaming the virttest.test_setup.os_posix.py file into virttest.test_setup.host_config, as that would help maintaining the number of submodules under virttest.test_setup under a reasonable amount.

ID: 2437

@bgartzi
Copy link
Contributor Author

bgartzi commented Sep 3, 2024

Hi @YongxueHong,

Yet another env_process refactoring patch. Could you review it please?

Note that I'm also renaming one of the test_setup submodules from virttest.test_setup.os_posix into virttest.test_setup.host_config. I'm doing that in this patch as I thought setupers of the ULimitConfig and VerifyhostDmesg sort could "live together" in a submodule. That would also help maintaining the number of submodules under virttest.test_setup under a reasonable amount.

I guess there might be some concerns about changing the virttest API (from its user point of view). However, test_setup.os_posix is relatively new (I created it in another env_process refactoring patch) and AFAIC there aren't direct imports of that submodule apart from virttest.env_process. Moreover, I don't think it would make much sense to use it outside env_process, as it will already use the classes from such submodule to configure the environment.

What do you think about that? Are you okay with that? should we rather create another submodule under virttest.test_setup to keep VerifyHostDMesg? Should we just add it into os_posix although it wouldn't quite match it's submodule name's purpose?

Thanks a lot!

@YongxueHong
Copy link
Contributor

Hi @YongxueHong,

Yet another env_process refactoring patch. Could you review it please?

Note that I'm also renaming one of the test_setup submodules from virttest.test_setup.os_posix into virttest.test_setup.host_config. I'm doing that in this patch as I thought setupers of the ULimitConfig and VerifyhostDmesg sort could "live together" in a submodule. That would also help maintaining the number of submodules under virttest.test_setup under a reasonable amount.

Hi @bgartzi
Sorry for replying so late, and thanks for sharing your thoughts on enhancement. As you mentioned there must be amount of modules in the future, we might organize those with a clear layout for maintenance.
Regarding virttest.test_setup.host_config, I think it is a bit general since the major of env_process is to configure the host, such as networking, storage, requirement_check and so on, indeed, those did some operations on the host from a different perspective. so I think the host_config is a bit inappropriate.
The above is just my understanding, please let me know your thoughts. Thanks.

I guess there might be some concerns about changing the virttest API (from its user point of view). However, test_setup.os_posix is relatively new (I created it in another env_process refactoring patch) and AFAIC there aren't direct imports of that submodule apart from virttest.env_process. Moreover, I don't think it would make much sense to use it outside env_process, as it will already use the classes from such submodule to configure the environment.

What do you think about that? Are you okay with that? should we rather create another submodule under virttest.test_setup to keep VerifyHostDMesg? Should we just add it into os_posix although it wouldn't quite match it's submodule name's purpose?

Regarding the os_posix module, I think it aims at the setuppers which is responsible for functionalities that are specifically available for operating systems that follow the standard, while the dmesg is a tool used in Unix-like operating systems to display kernel-related messages.
And from the implementation of verify_host_dmesg, I think it looks like a step to check the host kernel message whether it is satisfied to run with the current kernel environment.
Therefore, I prefer to create another submodule for it.
Thanks.

Thanks a lot!

@bgartzi
Copy link
Contributor Author

bgartzi commented Sep 18, 2024

Hi @bgartzi Sorry for replying so late, and thanks for sharing your thoughts on enhancement. As you mentioned there must be amount of modules in the future, we might organize those with a clear layout for maintenance. Regarding virttest.test_setup.host_config, I think it is a bit general since the major of env_process is to configure the host, such as networking, storage, requirement_check and so on, indeed, those did some operations on the host from a different perspective. so I think the host_config is a bit inappropriate. The above is just my understanding, please let me know your thoughts. Thanks.

Looking back at this, I agree with you.

Regarding the os_posix module, I think it aims at the setuppers which is responsible for functionalities that are specifically available for operating systems that follow the standard, while the dmesg is a tool used in Unix-like operating systems to display kernel-related messages. And from the implementation of verify_host_dmesg, I think it looks like a step to check the host kernel message whether it is satisfied to run with the current kernel environment. Therefore, I prefer to create another submodule for it. Thanks.

It will just make the test fail if unexpected kernel messages are found, so they are notified and investigated.

I'm separating both. I will move VerifyHostDMesg into virttest.test_setup.verify, as other steps in the pre/post process functions are also tools used to verify that everything went properly during the test run. Perhaps there's a better term to name the submodule, but I'm not able to come up with one now.

Host dmesg can be checked if the test configuration requires to do so.
It will be in fact be checked both before and after running the test.

However, that was done directly in the preprocess and postprocess
functions in virttest.env_process. Write a Setuper subclass that
implements those in setup/cleanup methods and register the setuper in
the env_process setup_manager.

Signed-off-by: Beñat Gartzia Arruabarrena <[email protected]>
@bgartzi bgartzi force-pushed the env_process_refactoring-host_dmesg_verification branch from fa3145d to 2894cc5 Compare September 18, 2024 10:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants