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

Kill a paused qube before system shutdown #616

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

Conversation

alimirjamali
Copy link
Contributor

@marmarek
Copy link
Member

marmarek commented Sep 3, 2024

This doesn't look right. If a qube was paused, it was likely done for a reason, it shouldn't execute anything without explicit user action. Maybe kill paused qubes on shutdown instead?

@alimirjamali
Copy link
Contributor Author

Maybe kill paused qubes on shutdown instead?

This sounds more dangerous. What if the qube had open applications with proper auto-save on shutdown mechanism. Killing qube might lead to serious data-loss. Having said that, killing disposables might be a legitimate option.

@marmarek
Copy link
Member

marmarek commented Sep 3, 2024

Ideally, we'd ask the user, but I don't think we have any way to do that. The autosave is one scenario, but if user wanted to interact with such a qube, why it was paused? Another scenario could be some misbehaving qube paused to limit damage - unpausing it would be very wrong here.

@alimirjamali
Copy link
Contributor Author

alimirjamali commented Sep 3, 2024

So properly killing it would be the way. I will look into libvirt_domain to find the clean way to kill a paused domain. libvirt_domain.shutdown() fails to do it for suspended domains.

p.s.: And documentation should be updated that paused domain will be killed on shutdown.

@marmarek
Copy link
Member

marmarek commented Sep 3, 2024

See what qvm-kill does. BTW, I think killing should be only during system shutdown. Normal qvm-shutdown should not automatically change to killing on a paused qube but maybe show an error or some other message?

alimirjamali added a commit to alimirjamali/qubes-core-admin-client that referenced this pull request Sep 3, 2024
@alimirjamali
Copy link
Contributor Author

@marmarek It is done. qvm-pause documentation is also updated to reflect this behaviour. Considering the fact that the existing behaviour is also indeed killing of paused qubes on shutdown, nothing is really changed here. It is only cleaner and much faster.

@alimirjamali
Copy link
Contributor Author

Normal qvm-shutdown should not automatically change to killing on a paused qube but maybe show an error or some other message?

Ops. Just saw this. I will work on it.

Copy link

codecov bot commented Sep 3, 2024

Codecov Report

Attention: Patch coverage is 0% with 5 lines in your changes missing coverage. Please review.

Project coverage is 69.31%. Comparing base (68070df) to head (8344dad).

Files with missing lines Patch % Lines
qubes/vm/qubesvm.py 0.00% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #616      +/-   ##
==========================================
- Coverage   69.33%   69.31%   -0.03%     
==========================================
  Files          58       58              
  Lines       11946    11950       +4     
==========================================
  Hits         8283     8283              
- Misses       3663     3667       +4     
Flag Coverage Δ
unittests 69.31% <0.00%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

alimirjamali added a commit to alimirjamali/qubes-core-admin-client that referenced this pull request Sep 3, 2024
@alimirjamali alimirjamali changed the title Unpause a paused qube before shutdown Kill a paused qube before system shutdown Sep 3, 2024
@alimirjamali alimirjamali force-pushed the Issue-8580 branch 2 times, most recently from 37c9ee9 to 2ff9968 Compare September 3, 2024 11:34
@alimirjamali
Copy link
Contributor Author

@marmarek I believe it is done properly.

Trying to shutdown a paused qube will raise proper error (instead of doing nothing).
Shutdown with --force will destroy it (instead of waiting).

Please let me know if you are OK with it, so I could proceed with updating unittests.

This is the screenshot of the error:
shutdown_paused

@alimirjamali
Copy link
Contributor Author

Reason for amendment: Making Pylint happy(er)

@marmarek
Copy link
Member

marmarek commented Sep 3, 2024

Please let me know if you are OK with it, so I could proceed with updating unittests.

Yes, this looks reasonable

@alimirjamali
Copy link
Contributor Author

It appears that CI/CD is broken at the moment.

I do not have access to PipelineRetry

@alimirjamali
Copy link
Contributor Author

OK. Codecov is advising on the necessary unittests. I have a crude idea on the required unittest. Might be wrong

@alimirjamali alimirjamali force-pushed the Issue-8580 branch 2 times, most recently from 059171e to be197f0 Compare September 5, 2024 07:06
@alimirjamali
Copy link
Contributor Author

@marmarek It appears that none of the basic integration unittests are executed. I am having trouble to find the proper unittest to update for this.

@marmarek
Copy link
Member

marmarek commented Sep 5, 2024

Integration tests are not running in gitlab-ci, they are running only in openqa, see https://www.qubes-os.org/doc/automated-tests/

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.

Shutting down or rebooting dom0 while VMs are paused takes a very long time
2 participants