Treecompose kernel cleanup #959

cgwalters · 2017-08-28T17:53:21Z

No description provided.

cgwalters · 2017-08-28T21:23:20Z

Mmm...this is even messier than I thought. Marking WIP.

cgwalters · 2017-08-29T19:51:54Z

Updated 🆕. There was a lot of internal churn here as I settled on things...there may be some artifacts from intermediate state.

One thing that clearly sucks is:

# ostree --repo=repo-build ls -X fedora/26/x86_64/atomic-host /usr/lib/modules/4.12.8-300.fc26.x86_64/vmlinuz /boot/vmlinuz-4.12.8-300.fc26.x86_64-c28965ed30a8c8d597f21dd47f854147a94720ed821aff6be56af3a4c7d82393 /usr/lib/ostree-boot/vmlinuz-4.12.8-300.fc26.x86_64-c28965ed30a8c8d597f21dd47f854147a94720ed821aff6be56af3a4c7d82393
-00755 0 0 7373400 { [(b'security.selinux', b'system_u:object_r:modules_object_t:s0')] } /usr/lib/modules/4.12.8-300.fc26.x86_64/vmlinuz
-00755 0 0 7373400 { [(b'security.selinux', b'system_u:object_r:boot_t:s0')] } /boot/vmlinuz-4.12.8-300.fc26.x86_64-c28965ed30a8c8d597f21dd47f854147a94720ed821aff6be56af3a4c7d82393
-00755 0 0 7373400 { [(b'security.selinux', b'system_u:object_r:lib_t:s0')] } /usr/lib/ostree-boot/vmlinuz-4.12.8-300.fc26.x86_64-c28965ed30a8c8d597f21dd47f854147a94720ed821aff6be56af3a4c7d82393

This means we now get 3 copies of both the kernel and initramfs due to separate SELinux labels 😿. It's tempting to just do usr_t for all of this.

Anyways, we now with boot_location: both (the default), we inject the kernel in all 3 places; the logic treats the other two as legacy. And /boot is always cloned/updated from /usr/lib/ostree-boot.

cgwalters · 2017-08-31T12:11:37Z

Folded #927 into this

cgwalters · 2017-09-01T14:32:59Z

bot, retest this please

cgwalters · 2017-09-01T15:56:15Z

Lifting WIP, should be ready for review 👁.

jlebon · 2017-09-05T18:53:09Z

src/libpriv/rpmostree-postprocess.c

+        if (!old_machine_id)
+          return FALSE;
+      }
+    if (strlen (old_machine_id) != 33)


This will segfault in the ENOENT case, right? Seems like this one and the trimming should be under the != NULL check.

jlebon · 2017-09-05T19:15:20Z

src/libpriv/rpmostree-kernel.c

+  int upto_index;
+  switch (dest)
+    {
+    case RPMOSTREE_FINALIZE_KERNEL_AUTO:


This confuses me; don't we always want to put the kernel in /usr/lib/ostree-boot as well?

Yes; we do/should - this was more about "update /boot only if we find a kernel there".

Ahh gotcha. OK, the implementation here is really tricky. Can we just drop the loop approach, factor out the unlink & linkat bits and just go through the two locations one at a time? Also, maybe a comment where the enum is declared for AUTO?

OK, reworked, and fixed some bugs along the way. Also did a git add of a test case I'd written that had been sitting in my tree for a while without me doing a git clean -dfx (lucky!).

We have 3 locations to find kernels now; I can't think of a reason to support placing kernels *only* in `/boot`. The original commit 15ecaac doesn't give a reason, and I certainly can't think of one now. This makes `legacy` be an alias for `both`, which should be fully compatible. Prep for further refactoring towards changing `new` to mean both `/usr/lib/ostree-boot` *and* `/usr/lib/modules`.

This helped me debug/fix the tests faster.

Prep for changing `boot_location: new` to use `/usr/lib/ostree-boot` and `/usr/lib/modules`. Rework our kernel postprocessing so that we unify the `boot_location` handling with initramfs generation. Instead of doing the initramfs first in postprocessing, we do it nearly last, after e.g. `etc` is renamed to `usr/etc`. This has some consequences, such as the fact that `run_bwrap_mutably()` is now called in both situations. In general, our handling of `etc` is inconsistent, although understandably so. As part of this, I finally got around to implementing the bit from systemd/systemd#4174 however suboptimal it is; need the unified core so we can cleanly ignore the posttrans like we do others. We intentionally keep the file around in the generated tree so that installing a kernel RPM per client doesn't try to do any of this either. This all gets folded together so that the logic for handling the bootloader gets simpler - in the Fedora case, we now know to find kernels in `/usr/lib/modules` and can ignore `/boot`.

jlebon · 2017-09-08T18:24:55Z

src/libpriv/rpmostree-kernel.c

+/* Given a @rootfs_dfd and path to kernel/initramfs that live in
+ * usr/lib/modules/$kver, possibly update @bootdir to use them.
+ * @bootdir should be one of either /usr/lib/ostree-boot or /boot.
+ * If @copy_if_not_found is set, we do the copy unconditionally,


The parameter is still called is_auto, though I prefer copy_if_not_found as well. :) Or maybe keep it reversed and name it something like only_if_found?

jlebon · 2017-09-08T18:37:24Z

src/libpriv/rpmostree-postprocess.c

+  /* This gets called both by treecompose, where in the non-unified path we just
+   * have /etc, and in kernel postprocessing where we have usr/etc.
+   */
+  if (fstatat (rootfs_fd, "etc", &stbuf, 0) < 0)


glnx_fstatat_allow_noent ?

jlebon · 2017-09-08T18:50:56Z

src/libpriv/rpmostree-kernel.c

  if (rebuild_from_initramfs)
    bwrap = rpmostree_bwrap_new (rootfs_dfd, RPMOSTREE_BWRAP_IMMUTABLE, error,
                                 "--ro-bind", "/etc", "/etc",
                                 NULL);
  else
    bwrap = rpmostree_bwrap_new (rootfs_dfd, RPMOSTREE_BWRAP_IMMUTABLE, error,
+                                 "--ro-bind", "./usr/etc", "/etc",


Minor/optional: this can just be usr/etc, right? (To be more consistent with other relative bindings we do elsewhere).

jlebon · 2017-09-08T18:52:12Z

src/libpriv/rpmostree-postprocess.c

+    return FALSE;
+
+  /* If the boot location includes /boot, we also need to copy /usr/lib/ostree-boot there */
+  switch (boot_location)


This can just be if (boot_location == RPMOSTREE_POSTPROCESS_BOOT_LOCATION_BOTH) right?

Yeah, but I usually like doing an explicit switch over enums so one gets warnings when adding a new case (and down the line we'll probably add a LOCATION_MODULES or so).

cgwalters · 2017-09-12T00:53:51Z

Saw this in testing with initramfs --enable and rebasing:

Generating initramfs... error: linkat: File exists

jlebon · 2017-09-12T13:15:08Z

LGTM! We might have to wait until the CentOS repos settle down though.
@rh-atomic-bot r+ 2974c67

We have 3 locations to find kernels now; I can't think of a reason to support placing kernels *only* in `/boot`. The original commit 15ecaac doesn't give a reason, and I certainly can't think of one now. This makes `legacy` be an alias for `both`, which should be fully compatible. Prep for further refactoring towards changing `new` to mean both `/usr/lib/ostree-boot` *and* `/usr/lib/modules`. Closes: #959 Approved by: jlebon

This helped me debug/fix the tests faster. Closes: #959 Approved by: jlebon

Prep for changing `boot_location: new` to use `/usr/lib/ostree-boot` and `/usr/lib/modules`. Rework our kernel postprocessing so that we unify the `boot_location` handling with initramfs generation. Instead of doing the initramfs first in postprocessing, we do it nearly last, after e.g. `etc` is renamed to `usr/etc`. This has some consequences, such as the fact that `run_bwrap_mutably()` is now called in both situations. In general, our handling of `etc` is inconsistent, although understandably so. As part of this, I finally got around to implementing the bit from systemd/systemd#4174 however suboptimal it is; need the unified core so we can cleanly ignore the posttrans like we do others. We intentionally keep the file around in the generated tree so that installing a kernel RPM per client doesn't try to do any of this either. This all gets folded together so that the logic for handling the bootloader gets simpler - in the Fedora case, we now know to find kernels in `/usr/lib/modules` and can ignore `/boot`. Closes: #959 Approved by: jlebon

rh-atomic-bot · 2017-09-12T13:15:13Z

⌛ Testing commit 2974c67 with merge 5335b92...

rh-atomic-bot · 2017-09-12T13:45:49Z

💔 Test failed - status-atomicjenkins

cgwalters · 2017-09-12T14:26:42Z

@rh-atomic-bot retry

This helped me debug/fix the tests faster. Closes: #959 Approved by: jlebon

Prep for changing `boot_location: new` to use `/usr/lib/ostree-boot` and `/usr/lib/modules`. Rework our kernel postprocessing so that we unify the `boot_location` handling with initramfs generation. Instead of doing the initramfs first in postprocessing, we do it nearly last, after e.g. `etc` is renamed to `usr/etc`. This has some consequences, such as the fact that `run_bwrap_mutably()` is now called in both situations. In general, our handling of `etc` is inconsistent, although understandably so. As part of this, I finally got around to implementing the bit from systemd/systemd#4174 however suboptimal it is; need the unified core so we can cleanly ignore the posttrans like we do others. We intentionally keep the file around in the generated tree so that installing a kernel RPM per client doesn't try to do any of this either. This all gets folded together so that the logic for handling the bootloader gets simpler - in the Fedora case, we now know to find kernels in `/usr/lib/modules` and can ignore `/boot`. Closes: #959 Approved by: jlebon

rh-atomic-bot · 2017-09-12T14:26:48Z

⌛ Testing commit 2974c67 with merge f113fc5...

rh-atomic-bot · 2017-09-12T15:11:36Z

☀️ Test successful - status-atomicjenkins
Approved by: jlebon
Pushing f113fc5 to master...

cgwalters · 2017-09-14T18:09:57Z

This breaks with anaconda UEFI: https://github.com/rhinstaller/anaconda/issues/1188

cgwalters · 2017-09-14T18:10:10Z

(with boot_location: new i mean)

cgwalters · 2017-11-27T20:34:40Z

Followup in https://pagure.io/fedora-atomic/issue/94

This ensures that hardlinking works. See https://bugzilla.redhat.com/show_bug.cgi?id=1526191 coreos/rpm-ostree#959 (comment) coreos/rpm-ostree#1142

Moved from: fedora-selinux/selinux-policy-contrib#43 This ensures that hardlinking works with `/usr/share/rpm` (once the contrib patch to make it `usr_t` is merged too). See https://bugzilla.redhat.com/show_bug.cgi?id=1526191 coreos/rpm-ostree#959 (comment) coreos/rpm-ostree#1142

This ensures that hardlinking works, once the corresponding base policy PR is merged: fedora-selinux/selinux-policy#209 See https://bugzilla.redhat.com/show_bug.cgi?id=1526191 coreos/rpm-ostree#959 (comment) coreos/rpm-ostree#1142

Moved from: fedora-selinux/selinux-policy-contrib#43 This ensures that hardlinking works with `/usr/share/rpm` (once the contrib patch to make it `usr_t` is merged too). See https://bugzilla.redhat.com/show_bug.cgi?id=1526191 coreos/rpm-ostree#959 (comment) coreos/rpm-ostree#1142

cgwalters changed the title ~~treecompose kernel cleanup~~ WIP: treecompose kernel cleanup Aug 28, 2017

cgwalters added the WIP label Aug 28, 2017

cgwalters force-pushed the boot-location branch from 8c3a60d to 02967c6 Compare August 29, 2017 19:42

cgwalters mentioned this pull request Aug 31, 2017

bin/compose: Suppress kernel.rpm's dracut run #927

Closed

cgwalters force-pushed the boot-location branch from 02967c6 to 1e04920 Compare September 1, 2017 15:24

cgwalters changed the title ~~WIP: treecompose kernel cleanup~~ Treecompose kernel cleanup Sep 1, 2017

cgwalters removed the WIP label Sep 1, 2017

cgwalters mentioned this pull request Sep 1, 2017

Failed to rebase or upgrade with new kernel 4.x #966

Closed

jlebon reviewed Sep 5, 2017

View reviewed changes

cgwalters force-pushed the boot-location branch from 1e04920 to 9b03d1b Compare September 7, 2017 15:24

cgwalters added 3 commits September 8, 2017 10:27

tests/compose: Add env variables to force caching and fast fail

fdecebc

This helped me debug/fix the tests faster.

cgwalters force-pushed the boot-location branch from 9b03d1b to 4aad7d1 Compare September 8, 2017 17:50

jlebon reviewed Sep 8, 2017

View reviewed changes

cgwalters added 2 commits September 11, 2017 19:56

fixup! Rework treecompose kernel processing

8f931b2

fixup! Rework treecompose kernel processing

60b3b73

fixup! Rework treecompose kernel processing

2974c67

rh-atomic-bot pushed a commit that referenced this pull request Sep 12, 2017

tests/compose: Add env variables to force caching and fast fail

421e7f5

This helped me debug/fix the tests faster. Closes: #959 Approved by: jlebon

jlebon added the reviewed/approved label Sep 12, 2017

rh-atomic-bot pushed a commit that referenced this pull request Sep 12, 2017

tests/compose: Add env variables to force caching and fast fail

93d3fc6

This helped me debug/fix the tests faster. Closes: #959 Approved by: jlebon

rh-atomic-bot closed this in 3ff4858 Sep 12, 2017

cgwalters mentioned this pull request Sep 14, 2017

Set KERNEL_INSTALL_NOOP when composing and during scripts #448

Closed

cgwalters mentioned this pull request Jan 8, 2018

rpm: Label rpmdb consistently usr_t on ostree/Atomic systems fedora-selinux/selinux-policy-contrib#43

Merged

cgwalters mentioned this pull request Jan 23, 2018

kernel/files.fc: Label /usr/lib/sysimage as usr_t fedora-selinux/selinux-policy#209

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Treecompose kernel cleanup #959

Treecompose kernel cleanup #959

cgwalters commented Aug 28, 2017

cgwalters commented Aug 28, 2017

cgwalters commented Aug 29, 2017

cgwalters commented Aug 31, 2017

cgwalters commented Sep 1, 2017

cgwalters commented Sep 1, 2017

jlebon Sep 5, 2017

jlebon Sep 5, 2017

cgwalters Sep 7, 2017

jlebon Sep 7, 2017

cgwalters Sep 8, 2017

jlebon Sep 8, 2017

jlebon Sep 8, 2017

jlebon Sep 8, 2017

jlebon Sep 8, 2017

cgwalters Sep 11, 2017

cgwalters commented Sep 12, 2017 •

edited

Loading

jlebon commented Sep 12, 2017

rh-atomic-bot commented Sep 12, 2017

rh-atomic-bot commented Sep 12, 2017

cgwalters commented Sep 12, 2017

rh-atomic-bot commented Sep 12, 2017

rh-atomic-bot commented Sep 12, 2017

cgwalters commented Sep 14, 2017

cgwalters commented Sep 14, 2017

cgwalters commented Nov 27, 2017

Treecompose kernel cleanup #959

Treecompose kernel cleanup #959

Conversation

cgwalters commented Aug 28, 2017

cgwalters commented Aug 28, 2017

cgwalters commented Aug 29, 2017

cgwalters commented Aug 31, 2017

cgwalters commented Sep 1, 2017

cgwalters commented Sep 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgwalters commented Sep 12, 2017 • edited Loading

jlebon commented Sep 12, 2017

rh-atomic-bot commented Sep 12, 2017

rh-atomic-bot commented Sep 12, 2017

cgwalters commented Sep 12, 2017

rh-atomic-bot commented Sep 12, 2017

rh-atomic-bot commented Sep 12, 2017

cgwalters commented Sep 14, 2017

cgwalters commented Sep 14, 2017

cgwalters commented Nov 27, 2017

cgwalters commented Sep 12, 2017 •

edited

Loading