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

Change vm.destroy gracefulness for performance when discarding vms #3798

Merged
merged 1 commit into from
Dec 5, 2023

Conversation

pevogam
Copy link
Contributor

@pevogam pevogam commented Nov 20, 2023

During environment setup we discard vms that are not going to participate in a test but doing so gracefully results in a large timeout (60 seconds by default) for each one and slows down the test and overall test run significantly. As the discarded vm will either be fully reconstructed or restored from a clean state when it is needed again by a test, graceful shutdown is not needed and ridding ourselves of it will improve performance for free.

@pevogam
Copy link
Contributor Author

pevogam commented Nov 20, 2023

@luckyh Hopefully this is an easy diff and it seems reasonable to you.

@luckyh
Copy link
Contributor

luckyh commented Nov 24, 2023

Hi @pevogam, thanks for bringing this up! I just have a concern to the following point.

As the discarded vm will either be fully reconstructed or restored from a clean state when it is needed again by a test ...

However, the guest os images might continue to be used for the rest tests. As unclean shutdowns may trigger the guest encountering into the rescue mode (especially for Windows guests), without any protection being taken (e.g. using snapshots) those tests may fail to run into the end which would be not expected by the users.

So I'd suggest that, if we exactly want such a tweak to be applied, we'd better either

  • make the behavior configurable by a param and tell the users about the risk

or

  • do ungraceful shutdowns only if there is any protection been activated to the os images

Let me know your thoughts.

@pevogam
Copy link
Contributor Author

pevogam commented Nov 25, 2023

Hi @pevogam, thanks for bringing this up! I just have a concern to the following point.

As the discarded vm will either be fully reconstructed or restored from a clean state when it is needed again by a test ...

However, the guest os images might continue to be used for the rest tests. As unclean shutdowns may trigger the guest encountering into the rescue mode (especially for Windows guests), without any protection being taken (e.g. using snapshots) those tests may fail to run into the end which would be not expected by the users.

So I'd suggest that, if we exactly want such a tweak to be applied, we'd better either

* make the behavior configurable by a param and tell the users about the risk

Indeed I guess you are right about the images, I originally thought the vms are always reconstructed on top of black images since anything else might not isolate tests well enough but I was wrong here. So I pushed changes making this configurable, let me know what you think.

virttest/env_process.py Outdated Show resolved Hide resolved
During environment setup we discard vms that are not going to
participate in a test but doing so gracefully results in a large
timeout (60 seconds by default) for each one and slows down the
test and overall test run significantly. As for some plugins the
discarded vm will be fully restored from a clean state when
it is needed again by a test, graceful shutdown is not always
needed and ridding ourselves of it will improve performance for
free in these cases. All default cases can still not do this to
keep their reused images clean.

Signed-off-by: Plamen Dimitrov <[email protected]>
@pevogam
Copy link
Contributor Author

pevogam commented Dec 4, 2023

@YongxueHong @PaulYuuu Would you like to review this?

@luckyh
Copy link
Contributor

luckyh commented Dec 5, 2023

Thanks all.

@luckyh luckyh merged commit be4af65 into avocado-framework:master Dec 5, 2023
49 checks passed
@pevogam pevogam deleted the faster-vm-cleanup branch December 7, 2023 05:41
pevogam added a commit to pevogam/avocado-i2n that referenced this pull request Jan 26, 2024
An extra pull request was merged before the LTS release that allows
for faster vm shutdowns in specific cases like ours:

avocado-framework/avocado-vt#3798
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.

3 participants