-
Notifications
You must be signed in to change notification settings - Fork 23
Do not use sudo FQDN lookups #199
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
dc15c88 to
16bae24
Compare
80 lines :-) |
Test jobs for commit 16bae24 |
|
|
Presumably that's for the entire PR, which also adds actual guest tests and some other bits (see individual commits)? I'm picking out only the mechanism to implement qemu guest tests itself in my <100 line claim. |
In a future change I want to pass around another value as well as the pexpect.spawn object, so make the fixture return a SimpleNamespace that has the spawn object under that instead of it being the direct value. Change the existing test accordingly. This should result in no functional change to the test suite. Signed-off-by: Robie Basak <[email protected]>
Since qemu booting is slow and we want fast developer iteration, we make the optimisation compromise that we will not reset the qemu vm test fixture from a fresh image for every test. Most tests should not collide with each other. If we think a new test will do that and we want to make the compromise of giving it an isolated environment for a slower test suite, we can deal with that then. Signed-off-by: Robie Basak <[email protected]>
This allows us to write tests that run natively on the guest, and qemu_test.py will invoke them using qemu. Signed-off-by: Robie Basak <[email protected]>
If DNS is misconfigured in the deployment environment, sudo may hang while it times out on DNS requests, which is not desirable for the majority of our users. Add a test that identifies if sudo is using DNS. See: qualcomm-linux#193 There isn't an easy and obvious method of testing "did sudo use DNS" that (ideally) is independent of the DNS configuration in the test environment. This is hopefully unintrusive and reasonably flexible. The catch is that if we use sudo-rs in the future then it might stop working if Rust doesn't use libc. But this seems better than nothing. To make it easier to ensure that this test is correctly testing what we need, I've separated the test from the fix using xfail. This way, before the issue is fixed, the test suite passing means that the test is correctly detecting the issue. I'll then introduce the fix in the next change. Signed-off-by: Robie Basak <[email protected]>
Fixes: qualcomm-linux#193 Signed-off-by: Robie Basak <[email protected]>
16bae24 to
deddc35
Compare
Test jobs for commit deddc35 |
lool
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I have some doubts about the architecture of this approach:
- using expect was necessary for the login interaction, but seems generally brittle and to be avoided; I guess that's only for the bootstraping phase
- the dependencies on downloading packages and changing the rootfs are kind of heavy
- I would personally prefer if the tests were driven from the host towards the DUT rather than installing an on device framework and I'm not a big fan of the gdb usage here, but there's a limit to what we can do there; in this case, I guess we could set the DNS and check if there are packets coming its way, this would also work with sudo-rs
Anyway, if this works, it is understandable enough that I feel I could debug it if needed and the python and testing code is clean; I've left a few comments to your discretion :)
| def vm(): | ||
| """A pexpect.spawn object attached to the serial console of a VM freshly | ||
| booting with a CoW base of disk-ufs.img""" | ||
| # Since qemu booting is slow and we want fast developer iteration, we make the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should that comment be a couple of lines higher, near the scope definition for the fixture?
| # We use apt-get -U here and the apt_dependencies fixture in | ||
| # qemu_guest_test.py relies on this. | ||
| SCRIPT = f"""sudo -i sh <<EOT | ||
| apt-get install -Uy --no-install-recommends python3-pytest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's quite unfortunate to introduce a network dependency :-(
I also don't like that we're changing the rootfs, but I guess we need to bring the tests somehow.
Should we just add this test runner to the image?
| "-x", | ||
| gdb_commands_file.name, | ||
| "--args", | ||
| "sudo", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little bit surprized that this works; isn't sudo SUID and losing its SUID bit when run under GDB? I guess all tests run as root and this is sudo without SUID launched from the tests launched from a sudo shell.
| mkdir qcom-deb-images | ||
| mount -t 9p qcom-deb-images qcom-deb-images | ||
| cd qcom-deb-images | ||
| py.test-3 -vvm guest ci/qemu_guest_test.py && echo "{SUCCESS_NOTICE}" || echo "{FAILURE_NOTICE}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm tests running as root; I guess we can revisit this when we need a non-root test
Details are in the individual commit messages.