-
Notifications
You must be signed in to change notification settings - Fork 158
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
40ignition-ostree: add ignition-ostree-mount-state-overlays.service #2838
Conversation
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.
mostly LGTM - a few comments
do_mount() { | ||
for overlay in /usr/lib/opt /usr/local; do | ||
escaped=$(systemd-escape --path "${overlay}") | ||
overlay_dirs=/sysroot/var/ostree/state-overlays/${escaped} |
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.
Since we only operate on one overlay dir at a time maybe:
overlay_dirs=/sysroot/var/ostree/state-overlays/${escaped} | |
overlay_dir=/sysroot/var/ostree/state-overlays/${escaped} |
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 plural because that dir contains work
and upper
. This matches the wording in e.g. https://github.com/ostreedev/ostree/blob/da892140657bfb67d475f80395af97af96b1b6af/src/ostree/ot-admin-builtin-state-overlay.c#L48.
need_relabeling=0 | ||
if [ ! -d "${overlay_dirs}" ]; then | ||
mkdir -p "${overlay_dirs}"/{upper,work} | ||
coreos-relabel /var/ostree |
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 this gets executed multiple times.. any reason that needs to happen or can we just move coreos-relabel /var/ostree
to the need_relabeling
if down on line 34?
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.
Changed the approach for this!
# if state overlays are disabled, there's nothing to do | ||
if ! jq -e '.["opt-usrlocal-overlays"]' /sysroot/usr/share/rpm-ostree/treefile.json; then | ||
exit 0 | ||
fi |
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.
feels a bit icky to dig into the treefile to determine if this is supported but I guess it's the only way and we probably have precedent for doing something like this in the past?
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.
A better source of truth for this is actually whether the systemd services are enabled. I changed it to checked that (which actually also matches some rpm-ostree code).
The new experimental `opt-usrlocal-overlays` treefile knob[1] allows users to overlay packages with `/opt`/`/usr/local` content (and eventually, rebase to container images with content in those places). However, booting a base compose with this knob will break Ignition configs that currently write data in e.g. `/opt` since that now points to `/usr/lib/opt`, which is of course read-only. We need to assemble the state overlays from the initramfs so that those configs keep working seamlessly. This is a no-op if state overlays are off (status quo), but it'll make it easier for people and CI to test the feature. This is a tiny part of the required bits if we want to eventually turn this on in FCOS/RHCOS. The other major part is migrating existing systems. [1] coreos/rpm-ostree#4728
d81422d
to
de52667
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
The new experimental
opt-usrlocal-overlays
treefile knob[1] allows users to overlay packages with/opt
//usr/local
content (and eventually, rebase to container images with content in those places).However, booting a base compose with this knob will break Ignition configs that currently write data in e.g.
/opt
since that now points to/usr/lib/opt
, which is of course read-only.We need to assemble the state overlays from the initramfs so that those configs keep working seamlessly.
This is a no-op if state overlays are off (status quo), but it'll make it easier for people and CI to test the feature.
This is a tiny part of the required bits if we want to eventually turn this on in FCOS/RHCOS. The other major part is migrating existing systems.
[1] coreos/rpm-ostree#4728