Skip to content

initramfs: Fix using local /etc when also replacing kernel#1998

Merged
openshift-merge-robot merged 7 commits intocoreos:masterfrom
jlebon:pr/dracut-override-replace-prep
Feb 27, 2020
Merged

initramfs: Fix using local /etc when also replacing kernel#1998
openshift-merge-robot merged 7 commits intocoreos:masterfrom
jlebon:pr/dracut-override-replace-prep

Conversation

@jlebon
Copy link
Member

@jlebon jlebon commented Feb 26, 2020

Instead of basing our decision to use the local /etc on whether we're
using dracut --rebuild, base it directly on a boolean parameter.

This is relevant in the client-side when initramfs regeneration is
requested as well as a kernel override. In such cases, we do want to use
the local /etc, but we'd skip that path because we didn't also use
dracut --rebuild.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1806588

We don't support `/etc/dnf/dnf.conf`, so tell libdnf to not look for it.
This squashes warnings from libdnf (which turn into hard errors when
hacking locally with `G_DEBUG=fatal-warnings`).

As mentioned in the comment, the reason for putting this in `main.c` is
that it controls a global variable, which is used in a few places in
libdnf. So rather than duplicating this across callsites, just set it
upfront. And yeah... we should improve that API.
E.g. if it contains spaces as is the case when one does
`rpm-ostree initramfs --arg=-I --arg='/file1 /file2'`.
@jlebon
Copy link
Member Author

jlebon commented Feb 26, 2020

This is split out from #1997 and is all that's technically needed to fix https://bugzilla.redhat.com/show_bug.cgi?id=1806588. Though I think we should do something like #1997 too.

Keeping as draft until I add tests here.

@jlebon
Copy link
Member Author

jlebon commented Feb 26, 2020

To fix Prow: coreos/coreos-assembler#1154

@jlebon jlebon force-pushed the pr/dracut-override-replace-prep branch from f09b77c to d286b87 Compare February 26, 2020 20:24
@jlebon jlebon marked this pull request as ready for review February 26, 2020 20:24
@jlebon
Copy link
Member Author

jlebon commented Feb 26, 2020

OK, added a test for this. Ready to review!

@cgwalters
Copy link
Member

In such cases, we don't use dracut --rebuild because no longer have access to the original initramfs.

We do, it's in the base tree.

So my vague recollection here is that the current behavior was somewhat intentional - the concept is that e.g. override replace for kernel should just get you new kernel modules, but otherwise behave the same as a server side compose.

We do distinguish in the interface/code today between rpm-ostree initramfs --enable and rpm-ostree override replace.

Isn't this something more like we should look at the origin and see whether initramfs regeneration was requested and if so use the local /etc?

@jlebon
Copy link
Member Author

jlebon commented Feb 26, 2020

We do, it's in the base tree.

OK, sure :), but it's not super handy. We'd have to re-check it out somewhere under the temp rootfs so dracut can access it. Actually, a first iteration of this patch was to keep the base initramfs under /usr/lib/sysimage and using that. But anyway, it's hard to talk about this without talking about #1997: my conclusion was that using --rebuild isn't what we really want.

So my vague recollection here is that the current behavior was somewhat intentional - the concept is that e.g. override replace for kernel should just get you new kernel modules, but otherwise behave the same as a server side compose.

We do distinguish in the interface/code today between rpm-ostree initramfs --enable and rpm-ostree override replace.

Isn't this something more like we should look at the origin and see whether initramfs regeneration was requested and if so use the local /etc?

Hmm yeah, I think that makes sense. Will tweak!

@jlebon jlebon force-pushed the pr/dracut-override-replace-prep branch from d286b87 to 341b4e7 Compare February 26, 2020 20:50
@cgwalters
Copy link
Member

Yeah, I think I agree with #1997

@jlebon jlebon force-pushed the pr/dracut-override-replace-prep branch from 341b4e7 to 38adeb7 Compare February 26, 2020 20:53
@jlebon jlebon changed the title initramfs: Always regenerate with client's local /etc initramfs: Fix using local /etc when also replacing kernel Feb 26, 2020
@jlebon
Copy link
Member Author

jlebon commented Feb 26, 2020

OK, updated! ⬆️

@jlebon jlebon force-pushed the pr/dracut-override-replace-prep branch from 38adeb7 to 6fe9bcc Compare February 26, 2020 20:54
@cgwalters
Copy link
Member

Nice, thanks!
/lgtm

Instead of basing our decision to use the local `/etc` on whether we're
using `dracut --rebuild`, base it directly on a boolean parameter.

This is relevant in the client-side when initramfs regeneration is
requested as well as a kernel override. In such cases, we do want to use
the local `/etc`, but we'd skip that path because we didn't also use
`dracut --rebuild`.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1806588
We're hitting issues with packages getting tagged out of the pool:
https://pagure.io/releng/issue/9281

This in turn means we can't reliably recompose older builds right now,
which breaks our CI. For now at least, let's compose from the latest.
(Note we were already also composing the latest FCOS in the vmcheck
branch.)
@jlebon jlebon force-pushed the pr/dracut-override-replace-prep branch from 6fe9bcc to 52bec19 Compare February 26, 2020 21:45
@jlebon
Copy link
Member Author

jlebon commented Feb 26, 2020

Had to push another commit here to work around https://pagure.io/releng/issue/9281. Though I also took the opportunity to drop a stale comment I had added.

@cgwalters
Copy link
Member

/lgtm

It's the latest, and matches the rest of the host we're running on. But
also, pulling f30 is hitting 503s from the Fedora registry:

https://pagure.io/releng/issue/9282
@jlebon
Copy link
Member Author

jlebon commented Feb 26, 2020

OK, had to add yet another commit here to fix testing. 😢 Hopefully this is the one!

@cgwalters
Copy link
Member

/lgtm

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, jlebon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit 1cf0d55 into coreos:master Feb 27, 2020
@yanirq
Copy link

yanirq commented Mar 1, 2020

@jlebon I assume this should also work for initramfs --enable --arg=-I --arg="/etc/foo.conf /etc/bar.conf" right ?

@jlebon
Copy link
Member Author

jlebon commented Mar 2, 2020

@yanirq Yup, I could've probably tweaked the config to use that. But note: https://bugzilla.redhat.com/show_bug.cgi?id=1806588#c6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants