Minor test fixes to improve debuggability and readability #4862
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
roypat
added
the
Status: Awaiting review
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4862 +/- ##
=======================================
Coverage 84.07% 84.07%
=======================================
Files 251 251
Lines 28052 28052
=======================================
Hits 23586 23586
Misses 4466 4466
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
pb8o
reviewed
Oct 21, 2024
Print microvm logs and stack backtraces whenever running a command via SSH inside the guest fails (instead of only doing this if `wait_for_ssh_up` fails). To this by adding an `on_error` hook that gets called from `SSHConnection.check_output` if running the SSH command fails. Potentially, this could later be extended to automatically snapshot the microvm inside the hook, but for now, let's only print some data. Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
If the exception ends up being fatal, then it will get printed in full anyway, and it contains the entire log message anyway, and we just end up with duplicate messages. If its not fatal (e.g. caught), then its arguably up to the caller to decide whether it should be logged. Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
roypat
force-pushed
the
test-debug-improvements
branch
from
a63bf07 to
e41a36e
Compare
kalyazin
reviewed
Oct 21, 2024
roypat
force-pushed
the
test-debug-improvements
branch
from
e41a36e to
9e6593d
Compare
Only keep a single "snapshot" variable outside the loop Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
Exit code 255 is already used by the SSH client to communicate connection errors [1], meaning if the remote command also exits with 255 in failure cases, this will be indistinguishable from a connection error. Use exit code 1 instead in `check_guest_connections`. [1]: https://man.openbsd.org/ssh#EXIT_STATUS Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
Even if one child process failed, still call `wait` to ensure all other child vsock_helper processes also exit, to ensure clean socket teardown/etc. Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
After we're done with a socket, close it, to avoid leaking resources. We do not need to rebuild the rootfs for this, this specific binary is compiled on the fly and SCP'd into the microVM. Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
The main reason we have timeouts is so that we can get some useful debugging information in case they do happen. This means the timesouts only need to be strict enough to avoid running into the pytest timeout instead. 10s/1s however still gets us spurious failures, since in the non-performance tests contention on host networking resources (we use a TON of networking namespaces) is pretty bad. So increase the timeouts to something that still allows us to not run into the pytest timeout, while hopefully returning spurious failures once and for all. Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
roypat
force-pushed
the
test-debug-improvements
branch
from
7c54859 to
8cfed0d
Compare
kalyazin
approved these changes
Oct 22, 2024
pb8o
approved these changes
Oct 22, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes
Reason
License Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license. For more information on following Developer
Certificate of Origin and signing off your commits, please check
CONTRIBUTING.md.PR Checklist
PR.
CHANGELOG.md.TODOs link to an issue.contribution quality standards.
rust-vmm.