PHD: improve Windows reliability #651
Conversation
|
I pushed ffc0178 to resolve the Clippy issue, but GH doesn't seem to be picking it up into the PR 🙃 If this doesn't resolve itself soon I'll do an amend/force-push to try to get things moving. |
| CommandSequenceEntry::wait_for("C:\\Windows\\system32>"), | ||
| CommandSequenceEntry::write_str("cls\r"), | ||
| CommandSequenceEntry::Sleep(Duration::from_secs(5)), |
There was a problem hiding this comment.
five seconds is kind of a longish time to add to every test in the test suite...are we sure we need to wait this long?
There was a problem hiding this comment.
I added a facility to check for command line responsiveness more dynamically in 68607a8. The start sequence will now try to enter a command and wait for a short while for it to be echoed; if that fails, it will retry (with backoff) until the desired output shows up (or the overarching boot sequence times out).
| @@ -148,3 +152,23 @@ fn make_absolute_cursor_position(col: usize, row: usize) -> Change { | |||
| y: Position::Absolute(row), | |||
| } | |||
| } | |||
|
|
|||
| fn trace_buffer_contents(contents: &str) { | |||
| CommandSequenceEntry::wait_for("C:\\Windows\\system32>"), | ||
| CommandSequenceEntry::write_str("cls\r"), | ||
| CommandSequenceEntry::Sleep(Duration::from_secs(5)), |
There was a problem hiding this comment.
I added a facility to check for command line responsiveness more dynamically in 68607a8. The start sequence will now try to enter a command and wait for a short while for it to be echoed; if that fails, it will retry (with backoff) until the desired output shows up (or the overarching boot sequence times out).
| /// Attempt to establish consistent echoing of characters to the guest | ||
| /// serial console by typing `send` and then waiting for `expect` to appear | ||
| /// within the supplied `timeout`. | ||
| EstablishConsistentEcho { |
There was a problem hiding this comment.
neat!
is there any particular reason we allow using arbitrary strings for this operation? could we just always use a single character, or is there a reason we might want to do this with multiple strings?
There was a problem hiding this comment.
The string's arbitrary. It could be just a single character, though if the caller picks something that might already be in the back buffer at the time it requests this operation, it will want to issue a ClearBuffer command first to avoid spurious matching. (The way I look at this operation is that it gives guests a way to tell the framework to repeat a pair of WriteStr/WaitFor commands until the WaitFor is satisfied; the flexibility in specifying the sent/expected strings is accidental, not essential, and is mostly there to provide an analogy to the Write/Wait commands.)
There was a problem hiding this comment.
Ah, yeah, the need to ensure that the string isn't already in the buffer makes sense. 👍
Tighten a few bolts to increase the reliability of Windows tests: - In the Windows boot sequence, wait 5 seconds after the initial command prompt appears before entering a newline and continuing with the boot sequence. This helps to mitigate an apparent race condition in which characters typed too quickly into a brand new command line session get eaten. (Previously PHD attempted to mitigate this with a `cls` command, but this isn't reliable enough.) - Explicitly run `mode con` to set the console width and height before launching Cygwin. Without this, Cygwin on Server 2022 believes its terminal is 30 columns wide, which causes it to wrap commands in unexpected places that break echo detection. - Shorten the command prompt from "Administrator@PHD-WINDOWS:$ " to "$ " to reduce command wraparound for Server 2016/2019 guests. - Teach the raw buffer backend to honor "move cursor to the right" control commands. On Server 2022, Cygwin sometimes issues these instead of printing spaces when continuing a multiline command onto a new line, so this is needed to make echoing work reliably. Also add some tracing to this backend to help debug these kinds of problems.
gjcolombo
force-pushed
the
gjcolombo/phd/win-reliability
branch
from
5eb8cd0 to
30e0ce3
Compare
|
I suspect the migrate-from-base tests barfed on the last run because this branch was previously based on a commit from before #639 landed. Let's see how the force-pushed branch fares. |
|
That's more like it. |
Tweak a handful of things to make Windows tests more (though not perfectly) reliable:
C:\Windows\System32>prompt is displayed and the time the next command is issued. This appears to reduce the chance that the first command's keystrokes will be eaten.modecommand to set the console session's row/column count explicitly before launching Cygwin. This avoids a problem where Cygwin thought it was on a 30-column terminal and was wrapping lines unexpectedly.Administrator@PHD-WINDOWS:$to just$. The test doesn't need the user/host name cue and this reduces the likelihood of a line wrapping unexpectedly on WS2016/2019.Tested with WS2022, WS2019, and Debian 11 guests.