ostree: move admindir to /etc/alternatives.admindir #135

vrothberg · 2024-07-26T09:36:45Z

ostree container commit wipes /var and thereby erases the data in /var/lib/alternatives; the directory used to store configs/symlinks of alternatives.

/var is not a good place for storing such configs since it won't receive updates on bootc images either. We need to move to another location.

Hence, use /etc/alternatives.admindir when running on an ostree-based system unless /var/lib/alternatives is already present; a user may have worked around the problem. This way we enable alternatives to work on ostree-based systems without breaking backwards compat.

Fixes: #9

vrothberg · 2024-07-26T10:17:06Z

Dockerfile to test:

FROM quay.io/fedora/fedora-bootc:40
COPY alternatives /usr/sbin/alternatives
RUN dnf -y install golang
RUN ls /etc/alternatives.admindir
RUN go help > /dev/null

alternatives.8

chkconfig.spec

alternatives.c

travier · 2024-07-29T11:03:42Z

I still think it would be better to do a migration as that would fix things for all systems once instead of us having to write migration scripts and ship them in Fedora CoreOS, Fedora Atomic Desktops, Fedora IoT, etc.

But this LGTM, although I've not tested it.

vrothberg · 2024-07-29T12:12:47Z

I still think it would be better to do a migration as that would fix things for all systems once instead of us having to write migration scripts and ship them in Fedora CoreOS, Fedora Atomic Desktops, Fedora IoT, etc.

I am not opposed to the migration proposal. @cgwalters WDYT?

cgwalters · 2024-07-29T15:07:15Z

I definitely like the idea of the migration too, avoiding any drift between image and package mode - the migration would also work more nicely for non-ostree image based systems too.

Though of course, a notable complexity here is that such a migration would need to be a systemd unit - it couldn't be done in %post for image based systems (though it could on package based ones).

alternatives.c

alternatives-migration

vrothberg · 2024-07-30T13:08:50Z

Dockerfile for testing ostree container:

FROM quay.io/fedora/fedora-bootc:40
COPY alternatives /usr/sbin/alternatives
RUN dnf -y install golang
RUN ls /etc/alternatives /etc/alternatives.admindir
RUN go help > /dev/null

Dockerfile to make sure that the admindir gets created even on package mode:

FROM fedora:40
COPY alternatives /usr/sbin/alternatives
RUN rm -rf /var/lib/alternatives
RUN dnf -y install golang
RUN ls /etc/alternatives /var/lib/alternatives
RUN go help > /dev/null

To test the systemd service, build the Dockerfule, run a detached container and podman-exec into it:

FROM quay.io/fedora/fedora-bootc:40
COPY ./systemd/alternatives-migration.service /usr/lib/systemd/system/
COPY alternatives-migration /usr/libexec/

Makefile

lnykryn · 2024-07-30T13:23:43Z

Honestly, I don't like the idea of moving those files on existing systems. It feels really fragile. If we want to move the db elsewhere, it should be done only on new installations and support reading from both directories.

vrothberg · 2024-07-30T13:26:21Z

Honestly, I don't like the idea of moving those files on existing systems. It feels really fragile. If we want to move the db elsewhere, it should be done only on new installations and support reading from both directories.

I am happy to remove the migration part if you wish. We decided against moving the DB on new installations as it may break existing users. Let's say one machine upgrades to RHEL X.Y and another would does a fresh install, the paths would differ. We also wanted to make sure that users depending on /var/lib/alternatives (for whatever reason) are not impacted.

vrothberg · 2024-07-30T13:30:56Z

@travier @cgwalters can you chime in?

cgwalters · 2024-07-30T14:30:00Z

I think as far as a migration it seems doable across a RHEL major (and a fedora major)...the chance for breakage if we did it seems pretty low.

A migration would clearly be useful to be configurable - i.e. we can land the code, have it off by default and opt-in to start). So here's my proposal

Add a mechanism to opt-in to the new location (env ALTERNATIVES_STATEDIR=/etc/alternatives.admindir or just ALTERNATIVES_STATEDIR_NEW=1? Or maybe again mkdir /etc/alternatives.admindir before installing)
Change the code so that the new admindir, even if configured on is only used if the old /var path is nonexistent/inactive
The migration code would be a separate opt-in (build-time option seems viable, then we can flip that on in just rhel10 and fedora 42's spec files and/or start by doing just the migration in e.g. fedora-bootc/FCOS/etc. to start)

vrothberg · 2024-07-30T14:42:42Z

I think as far as a migration it seems doable across a RHEL major (and a fedora major)...the chance for breakage if we did it seems pretty low.

A migration would clearly be useful to be configurable - i.e. we can land the code, have it off by default and opt-in to start). So here's my proposal
* Add a mechanism to opt-in to the new location (`env ALTERNATIVES_STATEDIR=/etc/alternatives.admindir` or just `ALTERNATIVES_STATEDIR_NEW=1`? Or maybe again `mkdir /etc/alternatives.admindir` before installing)

The latter would be my preference.

* Change the code so that the new admindir, even if configured on is only used if the old `/var` path is nonexistent/inactive

That would be doable. The code still prefers /var if it exists..

* The migration code would be a separate opt-in (build-time option seems viable, then we can flip that on in just rhel10 and fedora 42's spec files _and/or_ start by doing just the migration in e.g. fedora-bootc/FCOS/etc. to start)

I think it could be opt-in by default by just shipping the system service but not enabling it. So fedora-bootc etc. can enable it if desired.

But I personally was already happy with the previous PR who just did its magic on OSTree systems. If we want to add more to it, let's dicuss and agree on the behavior. I don't want to burn too much time.

cgwalters · 2024-07-30T17:12:44Z

Yeah, I hear you. I'm OK keeping this migration code around in a draft PR, and then we can continue to do the super targeted change?

But...I think the messy thing though is these are kind of entangled...if we change this code to detect /run/ostree-booted, in theory it could break non-bootc ostree systems where the user legitimately configured alternatives locally?

Broken out of PR fedora-sysv#135. Work can be continued/picked up at a later point in time. Signed-off-by: Valentin Rothberg <[email protected]>

vrothberg · 2024-07-31T08:02:28Z

Yeah, I hear you. I'm OK keeping this migration code around in a draft PR, and then we can continue to do the super targeted change?

Moved the current state into #139.

But...I think the messy thing though is these are kind of entangled...if we change this code to detect /run/ostree-booted, in theory it could break non-bootc ostree systems where the user legitimately configured alternatives locally?

Can you elaborate? The code will only use /etc/alternatives.admindir if /var/lib/alternatives doesn't exist, we are running on an OSTree host. It won't switch if the --admindir flag is used.

alternatives.c

vrothberg · 2024-08-01T07:41:29Z

Good to go from my end.
@cgwalters @travier @lnykryn @ldv-alt WDYT?

Heads up: I'll be on PTO next week.

travier · 2024-08-01T08:59:22Z

Not tested but LGTM. My position is still #135 (comment) & #134 (comment). Might be good to make it a change request for Fedora 42.

lnykryn · 2024-08-01T09:39:50Z

I am OK with the concept. However, I still want to debug what is going on with 6d6b96f, and some small bits in the code bother me. But to save everyone's time I can just force-push that here.

vrothberg · 2024-08-01T10:52:35Z

I am OK with the concept. However, I still want to debug what is going on with 6d6b96f, and some small bits in the code bother me. But to save everyone's time I can just force-push that here.

Sounds great, thanks! Feel free to take over this PR.

Signed-off-by: Valentin Rothberg <[email protected]>

dtardon · 2024-08-05T12:39:52Z

alternatives.c

@@ -1454,12 +1466,26 @@ int main(int argc, const char **argv) {
        }
    }

-    if (stat(altDir, &sb) || !S_ISDIR(sb.st_mode) || access(altDir, F_OK)) {
+    if (!dirExists(altDir)) {


The access() check should remain, I think.

We discussed this with @msekletar, and I don't think it is needed. With F_OK, it will just check if it exists, but that is already done through stat. If there had been W_OK, that would have been a different story.

True... But then it'd be better to remove it in a separate commit.

dtardon · 2024-08-05T12:49:17Z

alternatives.c

+                fprintf(stderr, _("failed creating admindir: %s\n"), strerror(errno));
+                exit(2);
+            }
+        }


access() check missing here too, in both branches.

Best to do that just once after the whole if block, I think (that is, beyond line 1491). Or even change the else if at 1488 back to if. It could mean a duplicate existence check, but I don't think this code is performance-critical... 😉

I already talked about the acccess and I like performant code :-)

alternatives.c

chkconfig.spec

alternatives.c

`ostree container commit` wipes /var and thereby erases the data in /var/lib/alternatives; the directory used to store configs/symlinks of alternatives. /var is not a good place for storing such configs since it won't receive updates on bootc images either. We need to move to another location. Hence, use /etc/alternatives.admindir when running on an ostree-based system unless /var/lib/alternatives is already present; a user may have worked around the problem. This way we enable alternatives to work on ostree-based systems without breaking backwards compat. Fixes: fedora-sysv#9 Signed-off-by: Valentin Rothberg <[email protected]>

dtardon

LGTM

vrothberg · 2024-08-12T07:07:20Z

Thanks! Do we need to cut a new upstream release to get the changes into Fedora/RHEL?

lnykryn · 2024-08-12T07:13:12Z

@jamacku already did that 81cda82

This is seen on rpm-ostree based system during uninstall: ``` Running scriptlet: sssd-client-2.9.5-4.el9.x86_64 9/9 admindir /var/lib/alternatives invalid error: %preun(sssd-client-2.9.5-4.el9.x86_64) scriptlet failed, exit status 2 ``` This should be fixed by fedora-sysv/chkconfig#135 but let's avoid hard failing here anyway.

This is seen on rpm-ostree based system during uninstall: ``` Running scriptlet: sssd-client-2.9.5-4.el9.x86_64 9/9 admindir /var/lib/alternatives invalid error: %preun(sssd-client-2.9.5-4.el9.x86_64) scriptlet failed, exit status 2 ``` This should be fixed by fedora-sysv/chkconfig#135 but let's avoid hard failing here anyway. Reviewed-by: Pavel Březina <[email protected]> Reviewed-by: Tomáš Halman <[email protected]>

vrothberg mentioned this pull request Jul 26, 2024

ostree: move admindir to /etc/alternatives.admindir #134

Closed

vrothberg force-pushed the ostree-alternatives branch from e9ade9e to 87cdffc Compare July 26, 2024 10:19

vrothberg commented Jul 26, 2024

View reviewed changes

alternatives.8 Outdated Show resolved Hide resolved

chkconfig.spec Outdated Show resolved Hide resolved

cgwalters reviewed Jul 26, 2024

View reviewed changes

alternatives.c Outdated Show resolved Hide resolved

alternatives.c Outdated Show resolved Hide resolved

vrothberg force-pushed the ostree-alternatives branch from 87cdffc to cae669c Compare July 29, 2024 08:20

vrothberg mentioned this pull request Jul 29, 2024

[please ignore] CI test #138

Closed

lnykryn requested changes Jul 30, 2024

View reviewed changes

vrothberg force-pushed the ostree-alternatives branch from cae669c to d52444f Compare July 30, 2024 13:04

github-advanced-security bot found potential problems Jul 30, 2024

View reviewed changes

alternatives-migration Fixed Show fixed Hide fixed

alternatives-migration Fixed Show fixed Hide fixed

vrothberg commented Jul 30, 2024

View reviewed changes

Makefile Outdated Show resolved Hide resolved

vrothberg force-pushed the ostree-alternatives branch from d52444f to abd7e11 Compare July 30, 2024 13:23

vrothberg force-pushed the ostree-alternatives branch from abd7e11 to 946309e Compare July 30, 2024 14:32

vrothberg added a commit to vrothberg/chkconfig that referenced this pull request Jul 31, 2024

Proposal: migrate admindir

120ec3d

Broken out of PR fedora-sysv#135. Work can be continued/picked up at a later point in time. Signed-off-by: Valentin Rothberg <[email protected]>

vrothberg mentioned this pull request Jul 31, 2024

Proposal: migrate admindir #139

Closed

vrothberg force-pushed the ostree-alternatives branch 2 times, most recently from 7e7d3cb to aed657e Compare July 31, 2024 08:56

ldv-alt suggested changes Jul 31, 2024

View reviewed changes

alternatives.c Outdated Show resolved Hide resolved

alternatives.c Outdated Show resolved Hide resolved

alternatives.c Outdated Show resolved Hide resolved

vrothberg force-pushed the ostree-alternatives branch from aed657e to 87a94ac Compare July 31, 2024 12:16

vrothberg force-pushed the ostree-alternatives branch 2 times, most recently from c000aa0 to 0ab434f Compare July 31, 2024 12:29

update po/chkconfig.pot

2f62c19

Signed-off-by: Valentin Rothberg <[email protected]>

lnykryn force-pushed the ostree-alternatives branch from 0ab434f to 40e9f75 Compare August 5, 2024 11:47

jamacku mentioned this pull request Aug 5, 2024

Previously built Copr PR packages used for buildroot initialization packit/packit-service#2432

Closed

dtardon suggested changes Aug 5, 2024

View reviewed changes

lnykryn force-pushed the ostree-alternatives branch from 40e9f75 to 5502982 Compare August 5, 2024 14:08

ldv-alt reviewed Aug 5, 2024

View reviewed changes

alternatives.c Outdated Show resolved Hide resolved

lnykryn force-pushed the ostree-alternatives branch from 5502982 to 538dc7e Compare August 5, 2024 14:35

dtardon approved these changes Aug 7, 2024

View reviewed changes

lnykryn merged commit 4224f51 into fedora-sysv:main Aug 7, 2024
9 of 13 checks passed

vrothberg deleted the ostree-alternatives branch August 12, 2024 07:06

vrothberg mentioned this pull request Aug 30, 2024

BUG: Some symlinks missing from the OCI images (Can't add printer via SMB) ublue-os/main#77

Closed

travier mentioned this pull request Oct 24, 2024

Migrate existing systems to iptables-nft and remove iptables-legacy coreos/fedora-coreos-tracker#1818

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ostree: move admindir to /etc/alternatives.admindir #135

ostree: move admindir to /etc/alternatives.admindir #135

vrothberg commented Jul 26, 2024

vrothberg commented Jul 26, 2024 •

edited

Loading

travier commented Jul 29, 2024

vrothberg commented Jul 29, 2024

cgwalters commented Jul 29, 2024

vrothberg commented Jul 30, 2024

lnykryn commented Jul 30, 2024

vrothberg commented Jul 30, 2024 •

edited

Loading

vrothberg commented Jul 30, 2024

cgwalters commented Jul 30, 2024

vrothberg commented Jul 30, 2024 •

edited

Loading

cgwalters commented Jul 30, 2024

vrothberg commented Jul 31, 2024

vrothberg commented Aug 1, 2024

travier commented Aug 1, 2024

lnykryn commented Aug 1, 2024

vrothberg commented Aug 1, 2024

dtardon Aug 5, 2024

lnykryn Aug 5, 2024

dtardon Aug 5, 2024

dtardon Aug 5, 2024

lnykryn Aug 5, 2024

dtardon left a comment

vrothberg commented Aug 12, 2024

lnykryn commented Aug 12, 2024

ostree: move admindir to /etc/alternatives.admindir #135

ostree: move admindir to /etc/alternatives.admindir #135

Conversation

vrothberg commented Jul 26, 2024

vrothberg commented Jul 26, 2024 • edited Loading

travier commented Jul 29, 2024

vrothberg commented Jul 29, 2024

cgwalters commented Jul 29, 2024

vrothberg commented Jul 30, 2024

lnykryn commented Jul 30, 2024

vrothberg commented Jul 30, 2024 • edited Loading

vrothberg commented Jul 30, 2024

cgwalters commented Jul 30, 2024

vrothberg commented Jul 30, 2024 • edited Loading

cgwalters commented Jul 30, 2024

vrothberg commented Jul 31, 2024

vrothberg commented Aug 1, 2024

travier commented Aug 1, 2024

lnykryn commented Aug 1, 2024

vrothberg commented Aug 1, 2024

dtardon Aug 5, 2024

Choose a reason for hiding this comment

lnykryn Aug 5, 2024

Choose a reason for hiding this comment

dtardon Aug 5, 2024

Choose a reason for hiding this comment

dtardon Aug 5, 2024

Choose a reason for hiding this comment

lnykryn Aug 5, 2024

Choose a reason for hiding this comment

dtardon left a comment

Choose a reason for hiding this comment

vrothberg commented Aug 12, 2024

lnykryn commented Aug 12, 2024

vrothberg commented Jul 26, 2024 •

edited

Loading

vrothberg commented Jul 30, 2024 •

edited

Loading

vrothberg commented Jul 30, 2024 •

edited

Loading