Code review comment for ~thibf/uvtool:thibf/uvt-kvm_wait_system_booting

Revision history for this message
Robie Basak (racb) wrote :

Thanks, this looks much closer to what I'd like.

> But it may make sense in the future to always retries for generic errors until timeout occur.

"uvt-kvm ssh some-vm false" must not retry though, to mirror the behaviour of "ssh some-ip false". In other words, wherever possible it should pass through the return status of the command it wrapped, and I'm not sure how we could differentiate that from the ssh itself failing effectively enough. See below for more on this!

> I enabled retries only when we find the matching string which is the best in the case of the linked issue.

Therefore I think this behaviour is correct.

Above I asked about how we might differentiate a failure of ssh itself. That caused me to look up what ssh defines its return value to be, and it says 255 on failure. Then I tested /etc/nologin, and found that it does treat that as an ssh failure and the ssh command exits 255 in that case. Sorry I hadn't spotted this before, but perhaps this is better than a string match?

I'm open to opinions on this. Would retrying on 255 from ssh during the wait command be better to do this instead of a string match, or should we do both, or just the string match? I think I favour just the return value match, as ssh failure as opposed to command failure seems cleaner, and being restricted to main_wait_remote() it seems like this logic would fit what wait "is". But have I missed any other case that we need to consider?

One further comment inline.

review: Needs Fixing

« Back to merge proposal