-
Notifications
You must be signed in to change notification settings - Fork 5
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
ESP mount generator #382
ESP mount generator #382
Conversation
0281206
to
3218ec4
Compare
Without this, running `make check` without first running `make` fails since the tests depend on building eos-update-efi-uuid first. Prior to adding eos-update-efi-uuid, none of the top directory programs were generated, so there was no difference. https://phabricator.endlessm.com/T29930
Personally I think the `-v` output where it shows each test on a single line should be the default. Enabling debug logging by default is helpful for inspecting failed tests remotely. pytest still hides the output by default. The last one also seems like it should be the default - if you have a test that's expected to fail but succeeds, that should be considered a failure. https://phabricator.endlessm.com/T29930
Add an autouse pytest fixture to set environment variables as needed for tests. Currently this just takes over the `LANG=C.UTF-8` setting from `BaseTestCase`. https://phabricator.endlessm.com/T29930
3218ec4
to
6565c64
Compare
OK, so this is nearly a rewrite of the generator that was primarily driven by the need to handle the Windows dual boot case. The significant changes are:
I tested this on 4 different systems that are represented in the test data:
Earlier I wrote some instructions on how to test this by making a temporary ostree commit, but you can do it even easier. Since directories in the checkout are made on the fly, you can just make the
The |
6565c64
to
3b831e8
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.
Impressive work.
|
||
logger.debug(f'EFI variable {name} contents: {value}') | ||
|
||
# Systemd appends 3 nul bytes for some reason. If there are an odd |
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 curious to know what the reason is!
(From a less careful codebase I might wonder if this is the UTF-16-encoding of U+0000 (two nul bytes) followed by a C byte string terminator (1 nul byte).)
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 found the spot in the systemd code where it happens, but it was quite a while ago. There weren't really comments, but I think it was sort of what you're suggesting. Whether you try to parse as a C string or UTF-16, you'll find a trailing NUL. But it just has easily could have been a mistake that's essentially irrelevant the way systemd reads the values. The Unicode handling was entirely bespoke. I'll try to find it again when I get a chance.
I added the new changes as fixups so it would be easier to see the differences from the previous mega-commit. I'll squash them if you approve. |
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 great. Squash away!
This is intended to replace systemd-gpt-auto-generator for mounting the ESP. The various policies we need are difficult to capture there and would be hard to upstream. Instead, this program can capture all the products we support to ensure the ESP is mounted where and how we need it. It creates units in the normal generator directory, which mean they take precedence over the gpt-auto-generator units in the late generator directory. I've tested this on EOS with GPT, EOS with MBR, and PAYG. What's not currently supported is the Windows dual boot configuration where the root disk isn't really the root disk. https://phabricator.endlessm.com/T29930
1c178a7
to
323e7a6
Compare
This adds a systemd generator to mount the ESP. The intention is to supplant the ESP mounting currently handled by systemd-gpt-auto-generator since it does not contain the policy to handle all the systems EOS runs on. I've tested this (in a VM) on the following configurations:
eos-repartition-mbr
from the original GPT)I have not tested this on:
As we previously discussed, I'm sure this will get the dual boot case wrong. However, with this generator we can add the policy and tests for it. Trying to cram the policy into systemd-gpt-auto-generator would probably never work.
There's pretty extensive testing since I really don't want to regress any systems. You can test this on a real device by dropping it into
/etc/systemd/system-generators/
. Without the/efi
directory it only does anything useful on PAYG. You can do a full test by creating a temporary deployment with an/efi
directory:When done, redeploy the normal ostree:
https://phabricator.endlessm.com/T29930