-
Notifications
You must be signed in to change notification settings - Fork 77
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
Add a systemd generator to fixup Anaconda's /etc/fstab #417
Conversation
3bbf7ee
to
6a793c1
Compare
/// A workaround for https://github.com/ostreedev/ostree/issues/3193 | ||
/// as generated by anaconda. | ||
#[context("Updating /etc/fstab for anaconda+composefs")] | ||
pub(crate) fn fixup_etc_fstab(root: &Dir) -> Result<()> { |
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 have to say one really nice thing about pervasively using cap-std is that it makes it trivial to unit-test things like this in a super reliable fashion without accidentally trying to edit the system global /etc/fstab
when running cargo test
and also without going all the way to running containers. (Which we should do too of course...)
@henrywang we aren't testing anaconda in our integration tests yet, right? Do you know offhand how hard that'd be to wire up? A great thing to do here would be to change the tests to verify no systemd units fail across the board, which is the goal of this change. |
We have anaconda test in |
Ah OK. Yeah I think it could be an opt-in? Well...here's what I'd propose...let's land this change and then it will show up very quickly in the -dev images, and the workflow test runs against our -dev images and we can verify it works there? |
Exactly. |
@cgwalters One more question. Does anaconda support |
No, that's rhinstaller/anaconda#5197 |
6a793c1
to
4e7e665
Compare
This is a giant and hacky workaround for ostreedev/ostree#3193 The better real fix is probably in either systemd or anaconda (more realistically both) but let's paper over things here for now. Having code to run as a generator will likely be useful in the future anyways. Signed-off-by: Colin Walters <walters@verbum.org>
4e7e665
to
d46b072
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, love the tests!
@cgwalters Anaconda test passed with |
Yep cool, I no longer see the |
@cgwalters are there tracker issues for this? just curious to follow and maybe help on this one to have a proper fix |
Thanks for the ping, yes I didn't file a tracker for this because it's a bit unclear exactly where it could be fixed, but I'd love to have your help! I think the best fix is:
But there's more words in ostreedev/ostree#3193 (comment) |
I understand this is slightly different than just adding ‘ro’ like this patch does for the root filesystem 🤔 but I can poke around the anaconda code to achieve your fix and see what happens during some testing too. |
Add a systemd generator to fixup Anaconda's /etc/fstab
This is a giant and hacky workaround for
ostreedev/ostree#3193
The better real fix is probably in either systemd or anaconda
(more realistically both) but let's paper over things here for now.
Having code to run as a generator will likely be useful in the
future anyways.
Signed-off-by: Colin Walters walters@verbum.org