Invert state and listening logic #18738
Conversation
When waiting for a VM to boot up, we check for two indicators to before mounting volumes: is the vm "running" and is the qemu socket listening. In some cases, presumably race or system pressure, it is possible that the qemu socket may be listening but the vm (and specifically sshd) is not running yet. Fixes: containers#17403 [NO NEW TESTS NEEDED] Signed-off-by: Brent Baude <bbaude@redhat.com>
|
/hold |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: baude, vrothberg The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| @@ -661,7 +661,7 @@ func (v *MachineVM) Start(name string, opts machine.StartOptions) error { | |||
| return err | |||
| } | |||
| listening := v.isListening() | |||
| for state != machine.Running || !listening { | |||
There was a problem hiding this comment.
Tried this out. Every time I've been able to reproduce this error, listening after line 633 is true, which means that usually this condition isn't met and the loop is never used, even on failures.
|
LGTM, I still think this is worth merging as it fixes the condition. @Luap99 WDYT? |
|
Actually rethinking here, no that does not seem correct. What we want is to wait for the VM to be running and listening, the for loop will continue until the condition is false. Thus we need a condition that is false only if both conditions are true, so The bigger problem I see here is that we can potentially loop forever because there is no time-out. |
Has been a while since I used this boolean transformation but you are absolutely right. Hard to believe when reading the condition out loud. |
When waiting for a VM to boot up, we check for two indicators to before mounting volumes: is the vm "running" and is the qemu socket listening. In some cases, presumably race or system pressure, it is possible that the qemu socket may be listening but the vm (and specifically sshd) is not running yet.
Fixes: #17403
[NO NEW TESTS NEEDED]
Does this PR introduce a user-facing change?