ephemeral run: Add timeout to status monitor to prevent indefinite hang#300
ephemeral run: Add timeout to status monitor to prevent indefinite hang#300gursewak1997 wants to merge 1 commit into
Conversation
cgwalters
left a comment
cgwalters
left a comment
There was a problem hiding this comment.
A full 60s delay is a poor UX. I think it'd be better to do vsock and ssh polling in parallel - if vsock gives us some data, then we can stop ssh polling (until we see startup complete). But if (as in this case) vsock doesn't give us anything but we succeed at logging in through ssh - great.
Or, since vsock isn't reliable...we could just drop it.
| // main loop. Without this, BufRead::lines() blocks indefinitely when the | ||
| // monitor stops emitting updates (e.g. vsock notifications silently fail). | ||
| let (tx, rx) = std::sync::mpsc::channel::<std::io::Result<String>>(); | ||
| std::thread::spawn(move || { |
There was a problem hiding this comment.
I know this is a larger refactoring, but I think it'd be better to use async rust for this stuff.
(I wouldn't be surprised if we were calling a blocking sync function from async)
There was a problem hiding this comment.
Happy to do that as a follow-up, but keeping this PR scoped to the concurrency fix with thread+channel since the infrastructure is already sync.
12f86e9 to
4c6f698
Compare
| debug!("Waiting for systemd to initialize..."); | ||
| // Drain any available status updates from the monitor (non-blocking). | ||
| if let Some(ref rx) = monitor_rx { | ||
| while let Ok(line) = rx.try_recv() { |
There was a problem hiding this comment.
But this is going to burn 100% of a CPU core busy polling right?
That's the kind of thing I mean in that while yes, it's a larger refactor to do async, it's also tricky to do this efficiently and properly without async.
If we're putting off that refactor, it may be cleanest to have the overall timeout and ssh subprocess polling be their own threads that writes to the channel, then this parent thread waits on events.
Previously wait_for_vm_ssh() blocked indefinitely on the vsock status monitor when boot notifications silently failed. Instead, run the vsock monitor and SSH polling concurrently — whichever succeeds first wins. This avoids the hang without sacrificing boot progress reporting on systems where vsock works. Closes: #279 Assisted-by: AI Signed-off-by: gursewak1997 <gursmangat@gmail.com>
4c6f698 to
84e7daa
Compare
2 participants
Previously wait_for_vm_ssh() blocked indefinitely on the vsock status monitor when boot notifications silently failed. Instead, run the vsock monitor and SSH polling concurrently; whichever succeeds first wins. This avoids the hang without sacrificing boot progress reporting on systems where vsock works.
Closes: #279