Updating behaviour on concurrent access to a pending VM shutdown oper… - #876
Updating behaviour on concurrent access to a pending VM shutdown oper…#876ekdama wants to merge 3 commits into
Conversation
marmarek
left a comment
marmarek
left a comment
There was a problem hiding this comment.
Looks like kill() could use a similar change (if you're up for it)
| """ | ||
|
|
||
| if self.is_halted(): | ||
| pending = self.is_halted() and not self.__waiter is None |
There was a problem hiding this comment.
This could use a comment explaining what case it handles (in-progress shutdown that is already at the storage stopping stage). Or if not a comment, at least include it in the commit message.
Yes, it makes sense. |
|
I think |
|
Sorry, I had some personal issues this week and had to be AFK. I believe that I can get the requested changes done in the following days. |
| raise qubes.exc.QubesVMNotStartedError(self) | ||
|
|
||
| try: | ||
| await self.fire_event_async( |
There was a problem hiding this comment.
What happens if a second shutdown is called on domain-pre-shutdown? Can you write a test that does that? I'd like to test if pending on the second shutdown request will be False, in case domain is not halted yet.
|
IMO I think that's the current behavior, but some test for it could be useful. |
|
With the pending condition, the integration test looped firing domain-pre-shutdown events. About kill, how should I handle it? I made these small changes, but I don't see a need since it does not have a wait option. |
| """The best option is to fire the domain-pre-shutdown after self.__waiter check and assignment, to avoid race conditions and undesired synchronization behaviour.""" | ||
| if self.__waiter is None: | ||
| self.__waiter = asyncio.get_running_loop().create_future() | ||
| await self.fire_event_async( |
There was a problem hiding this comment.
If the domain-pre-shutdown fails (is rejected), the self.__waiter will continue to be None
There was a problem hiding this comment.
Can I clean self.__waiter in the except: block? This would avoid creating a __waiter in an unsuccesful operation.
Or else would be better creating a try except block wrapping 1681 to 1684?
There was a problem hiding this comment.
Let's actually allow domain-pre-shutdown to repeat depending on the stage of shutdown.
There was a problem hiding this comment.
Weakening the pending condition by just checking self.__waiter could make domain-pre-shutdown repeatable. Could it work?
| @@ -0,0 +1 @@ | |||
| 2 | |||
There was a problem hiding this comment.
Some files included in tests-data by mistake?
There was a problem hiding this comment.
Yes, I am correcting this and linting mistakes as of now. This also broke some test. I will eventually squash all commits.
| if timeout is None: | ||
| timeout = self.shutdown_timeout | ||
| try: | ||
| await asyncio.wait_for(waiter, timeout=timeout) |
There was a problem hiding this comment.
One side-effect of calling this function again, is that it doesn't connect to the previous timeout, thus if it is hangs here, it will timeout later, possibly raising timeout twice.
There was a problem hiding this comment.
Let's check pending also there?
There was a problem hiding this comment.
elif not pending: elif is_running:
We actually want to wait for the full shutdown, independent if it is report as running or it is the second call.
There was a problem hiding this comment.
Yes sorry, I was confused. is_running and not pending are not aimed to the timeout call but to avoid calling libvirt_domain.destroy() or other domain specific code more than once, as I believe is the expected behaviour.
Then the waiter is acquired, not concerned by the current VM state (except the initial is_halted check).
3 participants
Closes QubesOS/qubes-issues#11076
Now, the behaviours that could make a concurrent call (like the Halted check), waiting for a shutdown of the VM through the
qvm-shutdown --waitcommand will also test if there is a waiter present in the VM.The waiter job is to act as a synchronization primitive, when it is finally consumed all the concurrent shutdown request will exit, if the wait flag is enabled.
Is the event firing handled correctly? I only throw the pre-shutdown event when there is not a pending operation.