-
Notifications
You must be signed in to change notification settings - Fork 46
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
Adds config test for required kernel modules #604
Conversation
5c6c0f2
to
6f054d7
Compare
Testing for this pr is currently blocked on #606 , the new kernel metapackage being tested here is not yet on apt-test.freedom.press |
@emkll You're right that #606 deserves immediate attention (looking into it now) but technically it needn't block here. The ko filepath being tested here has always been present—for each kernel version, and the kernel version has not changed recently—and its absence will result in a non-working GUI. I've amended the plan to point this out. Fine to wait on review here, I'd just like to make sure that if the GUI is broken in a way we can observe, we have a corresponding test—otherwise, the config tests say "Everything's fine!" when the AppVMs are unusable. |
#606 is re-closed, so ready for testing here. |
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.
Thanks @conorsch for the tests! Looks like the tests are failing for me locally to to string/bytes comparison (see inline).
tests/test_vms_exist.py
Outdated
u2mfn_filepath = "/usr/lib/modules/{}/updates/dkms/u2mfn.ko".format(EXPECTED_KERNEL_VERSION) | ||
# cmd will raise exception if file not found | ||
stdout, stderr = vm.run("sudo test -f {}".format(u2mfn_filepath)) | ||
assert stdout == "" |
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.
Looks like adding .decode("utf-8")
to stdout/stderr resolves the tests for me locally
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.
Thanks, you're right! Updated the string checks ""
-> b""
, now passing for me.
The u2mfn kernel module, built by dkms, is required for graphical support in VMs. Let's ensure that the kernel object is present for the active kernel, otherwise graphical displays will be garbled.
6f054d7
to
67f71cb
Compare
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.
lgtm !
Status
Ready for review.
Description of Changes
These changes add new config tests for VM state, ensuring that the u2mfn kernel module, required for graphical environments in Qubes, was built accurately. Without these tests, the graphical display problems reported in #590 wouild not result in a test suite failure. Now the tests should fail accordingly.
Testing
See detailed test plan in freedomofpress/securedrop-builder#189 to validate the logic changes in the new metapackage. For reviewing this PR, a fully passing
make test
is sufficient. Note that you may see a failing test such as #583 , which is unrelated.Additionally, it's probably worth confirming that removing the filepath
/usr/lib/modules/4.14.186-grsec-workstation/updates/dkms/u2mfn.ko
from a TemplateVM causes the new test to fail, although be aware that removing that file will break the GUI as described in #590Checklist
If you have made code changes
make flake8
) passes in the development environment (this box maybe left unchecked, as
flake8
also runs in CI)If you have made changes to the provisioning logic
All tests (
make test
) pass indom0
of a Qubes installThis PR adds/removes files, and includes required updates to the packaging
logic in
MANIFEST.in
andrpm-build/SPECS/securedrop-workstation-dom0-config.spec