Skip to content
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 cosa buildextend-extensions #2028

Merged
merged 7 commits into from
Feb 23, 2021

Conversation

jlebon
Copy link
Member

@jlebon jlebon commented Jan 26, 2021

This elevates extensions to be "first-class" artifacts. We include a
tarball of the extensions in the build dir, as well as details in
meta.json.

This command replaces the download-extensions script and leverages
rpm-ostree's new support for extension composes:

coreos/rpm-ostree#2439

@openshift-ci-robot
Copy link

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@jlebon
Copy link
Member Author

jlebon commented Jan 26, 2021

This still needs more testing and splitting out some prep patches, as
well as porting oscontainer to actually make use of it and adapting it to add the labels in openshift/os#409 (comment).

(And yes, I realize buildextend-extensions sounds funny, but I wanted to highlight that it's exactly like other buildextend-* commands!)

@jlebon
Copy link
Member Author

jlebon commented Jan 28, 2021

Added a bunch more commits to migrate oscontainer and implement openshift/os#409 (comment)!

This is what the labels look like right now:

$ sudo buildah --root=$PWD/tmp/containers-storage inspect localhost/my-os-container:33.20210127.dev.1
...
            "Labels": {
                "com.coreos.os-extensions": "tracers",
                "com.coreos.ostree-commit": "f3feb7783aa4db4352ab7d1525a12f206b0668e5c456513e710ba3872525621c",
                "com.coreos.redhat-coreos-commit": "ce32a8399c708da421ff6e4acfee686b226143b9",
                "com.coreos.rpm.ignition": "2.9.0-2.git1d56dc8.fc33.x86_64",
                "com.coreos.rpm.kernel": "5.10.9-201.fc33.x86_64",
                "com.coreos.rpm.ostree": "2020.8-1.fc33.x86_64",
                "com.coreos.rpm.rpm-ostree": "2020.10-1.fc33.x86_64",
                "com.coreos.rpm.runc": "2:1.0.0-279.dev.gitdedadbf.fc33.x86_64",
                "com.coreos.rpm.strace": "5.10-1.fc33.x86_64",
                "com.coreos.rpm.systemd": "246.7-1.fc33.x86_64",
                "io.buildah.version": "1.18.0",
                "io.openshift.build.version-display-names": "machine-os=Fedora",
                "io.openshift.build.versions": "machine-os=33.20210127.dev.1",
                "version": "33.20210127.dev.1"
            }

Hitting a few complications on RHCOS:

  • kernel-rt comes from a separate repo entirely than what we enable in the base compose (rhel-8-nfv). So I think we need to add support for enabling additional repos in extensions.yaml. Alternatively, we could probably just enable the repo for treecomposes too; I don't think there's anything in there that would change the base layer. But I think having rpm-ostree itself support this makes sense anyway.

  • A bunch of the packages in https://github.com/openshift/os/blob/294402461ffa20a8f92b8e2fd2467ce0f708e205/extensions.yaml#L19-L23 are already in the base (obviously kernel-core, but a few others too because of https://github.com/openshift/os/blob/294402461ffa20a8f92b8e2fd2467ce0f708e205/manifest.yaml#L196). Those AFAIK are actually used to build kernel modules, so they're not strictly extensions that the MCO would layer. (IOW, it makes sense to e.g. redownload the kernel-core RPM even if it's already in the base). It wouldn't be hard to adapt rpm-ostree for these of course, but a major goal of this work is to have the MCO use extensions.json only as the source of truth for what extensions are supported (and mapping them to packages). But this wouldn't really be a desirable extension we'd want to support.

    Hmm, I think we can add a devel: true key to the schema which rpm-ostree can use to understand that this isn't a strict OS extension, and the MCO can also skip over it.

jlebon added a commit to jlebon/os that referenced this pull request Feb 4, 2021
With coreos/rpm-ostree#2439 and
coreos/coreos-assembler#2028, support for
extensions is now more robust.

The extensions.yaml file is now fed directly to rpm-ostree. We just need
to do some minor tweaks:
- drop listing usbguard deps, rpm-ostree fetches them automatically
- mark `kernel-devel` as a development extension; this will signal the
  MCO that it's not intended to be directly layered on the host
- use `match-base-evr: kernel` to enforce that the kernel development
  packages have the same EVR as the base kernel package in the OSTree
@sinnykumari
Copy link
Contributor

This is what the labels look like right now:
"Labels": {
"com.coreos.os-extensions": "tracers",

LGTM

* `kernel-rt` comes from a separate repo entirely than what we enable in the base compose (`rhel-8-nfv`). So I think we need to add support for enabling additional repos in `extensions.yaml`. Alternatively, we could probably just enable the repo for treecomposes too; I don't think there's anything in there that would change the base layer. But I think having rpm-ostree itself support this makes sense anyway.

Enabling it in treecompose won't be an issue today but who knows the future (like more extensions coming from additional repos) and these additional repos are managed by someone else. It can be an overhead to make sure that these repos don't contain any base layer packages.

* A bunch of the packages in https://github.com/openshift/os/blob/294402461ffa20a8f92b8e2fd2467ce0f708e205/extensions.yaml#L19-L23 are already in the base (obviously `kernel-core`, but a few others too because of https://github.com/openshift/os/blob/294402461ffa20a8f92b8e2fd2467ce0f708e205/manifest.yaml#L196). Those AFAIK are actually used to build kernel modules, so they're not strictly extensions that the MCO would layer. (IOW, it makes sense to e.g. redownload the `kernel-core` RPM even if it's already in the base). It wouldn't be hard to adapt rpm-ostree for these of course, but a major goal of this work is to have the MCO use `extensions.json` only as the source of truth for what extensions are supported (and mapping them to packages). But this wouldn't really be a desirable extension we'd want to support.
  Hmm, I think we can add a `devel: true` key to the schema which rpm-ostree can use to understand that this isn't a strict OS extension, and the MCO can also skip over it.

Yes, a clear distinction would be needed to make sure what packages should be installed as part of an extension. Instead of devel:true, perhaps putting devel packages into a field like additional-packages: (I am bad at naming) would be more readable and MCO will not read that section. Something like:

kernel-devel:
    packages:
      - kernel-devel
      - kernel-headers
    additional-packages:
     - kernel-core
      - kernel-modules
      - kernel-modules-extr

One question that I have is, do we create extensions.yaml during compose process? Reason of asking this is consider following case:
We have libqb package mentioned in extensions.yaml (https://github.com/openshift/os/blob/294402461ffa20a8f92b8e2fd2467ce0f708e205/extensions.yaml#L12) that we will install as part of usbguard extension. Now, one of the base package started pulling libqb as dependency. This means, baseos needs now libqb. Now, user wants to uninstall installed usbguard extension, MCO will read the extensions.yaml (or equivalent json) file and run rpm-ostree remove usbguard libqb protobuf . I believe rpm-ostree will fail with some error?

@sinnykumari
Copy link
Contributor

For handling kernel switch, one useful information for MCO would be to know what all kernel packages are installed for default kernel. Today, we hardcode that information in MCO https://github.com/openshift/machine-config-operator/blob/master/pkg/daemon/update.go#L1158 . Adding it in extension.yaml will make us stop hardcoding these package list.

baseos-kernel:
  packages:
      - kernel
      - kernel-core
      - kernel-modules
      - kernel-modules-extra

@sinnykumari
Copy link
Contributor

sinnykumari commented Feb 5, 2021

One question that I have is, do we create extensions.yaml during compose process? Reason of asking this is consider following case:
We have libqb package mentioned in extensions.yaml (https://github.com/openshift/os/blob/294402461ffa20a8f92b8e2fd2467ce0f708e205/extensions.yaml#L12) that we will install as part of usbguard extension. Now, one of the base package started pulling libqb as dependency. This means, baseos needs now libqb. Now, user wants to uninstall installed usbguard extension, MCO will read the extensions.yaml (or equivalent json) file and run rpm-ostree remove usbguard libqb protobuf . I believe rpm-ostree will fail with some error?

Thinking about this a bit more, I think what would make more sense is specify only direct packages that we would need to install in order to enable an extension and add dependencies into deps section.

extensions:
  # https://github.com/coreos/fedora-coreos-tracker/issues/326
  usbguard:
    packages:
      - usbguard
    deps:
      - libqb
      - protobuf
  kernel-devel:
    packages:
      - kernel-devel
      - kernel-headers
    deps:
      - ...

The way MCO enables today usbguard is run rpm-ostree install usbguard. rpm-ostree installs needed deps from enabled rhocs-extensions.repo.
I think during building OSContainer, cosa would need to call rpm-ostree to libsolv missing dependencies from baseos lockfile list, download missing packages into extensions/ dir and add it in deps (if we want to maintain deps) section

jlebon added a commit to jlebon/os that referenced this pull request Feb 5, 2021
With coreos/rpm-ostree#2439 and
coreos/coreos-assembler#2028, support for
extensions is now more robust.

The extensions.yaml file is now fed directly to rpm-ostree. We just need
to do some minor tweaks:
- drop listing usbguard deps, rpm-ostree fetches them automatically
- split the devel bits into two extensions: one is an OS extension which
  contains the devel and headers packages, and the other is a
  development extension containing kernel packages already in the base
  (marking it as a development extension will signal the MCO that it's
  not intended to be directly layered on the host)
- use `match-base-evr: kernel` on those to enforce that the kernel
  development packages have the same EVR as the base kernel package in
  the OSTree
@cgwalters
Copy link
Member

For handling kernel switch, one useful information for MCO would be to know what all kernel packages are installed for default kernel.

Mmm I'd like to bake that flow a bit more into rpm-ostree, perhaps something like rpm-ostree install --uninstall-kernel ./kernel-rt*.rpm or so?

@jlebon
Copy link
Member Author

jlebon commented Feb 5, 2021

Sorry, I hadn't updated this comment with a link to coreos/rpm-ostree#2519.

Thinking about this a bit more, I think what would make more sense is specify only direct packages that we would need to install in order to enable an extension and add dependencies into deps section

Yes exactly, except we don't even need to track dependencies anymore. Check out how in openshift/os#496, we don't need to specify usbguard deps at all. At compose time, rpm-ostree automatically also downloads dependencies needed, which on the client-side rpm-ostree will automatically pull in as well. As far as the MCO is concerned, it should just be able to install/uninstall the top-level packages.

Since each extension compose is specific to an OSTree commit, if a dep becomes a base pkg, rpm-ostree would just stop shipping that dep in the extensions repo. As long as the top-level packages themselves aren't in the base, it should be fine (and rpm-ostree checks for this at compose time).

Yes, a clear distinction would be needed to make sure what packages should be installed as part of an extension. Instead of devel:true, perhaps putting devel packages into a field like additional-packages: (I am bad at naming) would be more readable and MCO will not read that section.

Hmm interesting idea, I've toyed with something like that a bit in coreos/rpm-ostree#2519. In the end though, the kernel devel situation is so peculiar (and just hacky) that any way of generalizing will look awkward. What I ended up doing in coreos/rpm-ostree#2519 is to have two separate kinds of extensions: development and os-extension. The latter is what the MCO can rpm-ostree install, while the former is meant to be completely ignored by the MCO.

@sinnykumari
Copy link
Contributor

For handling kernel switch, one useful information for MCO would be to know what all kernel packages are installed for default kernel.

Mmm I'd like to bake that flow a bit more into rpm-ostree, perhaps something like rpm-ostree install --uninstall-kernel ./kernel-rt*.rpm or so?

Even better if we can bake in something like --uninstall-kernel option into rpm-ostree. I think whole operation would look like rpm-ostree override --uninstall-kernel --install kernel-rt-core --install kernel-rt-modules ...

@cgwalters
Copy link
Member

OK moved that to coreos/rpm-ostree#2542

@jlebon jlebon force-pushed the pr/extensions branch 2 times, most recently from c559735 to 724fa43 Compare February 5, 2021 17:14
@jlebon
Copy link
Member Author

jlebon commented Feb 5, 2021

OK, here's how the labels will actually look with this + openshift/os#496:

"Labels": {
    "architecture": "x86_64",
    ...
    "com.coreos.os-extensions": "usbguard;kernel-rt;kernel-devel",
    "com.coreos.ostree-commit": "2f2468560c142d62af41972c5dd2336993424e62866383157e85be2e7aa20da5",
    "com.coreos.redhat-coreos-commit": "ab02937e3944f53ddc0bac854c24f70b9d8c8c39",
    "com.coreos.rpm.ignition": "2.9.0-2.rhaos4.7.git1d56dc8.el8.x86_64",
    "com.coreos.rpm.kernel": "4.18.0-240.10.1.el8_3.x86_64",
    "com.coreos.rpm.kernel-rt-core": "4.18.0-240.10.1.rt7.64.el8_3.x86_64",
    "com.coreos.rpm.ostree": "2020.7-1.el8.x86_64",
    "com.coreos.rpm.rpm-ostree": "2020.7-1.el8_3.x86_64",
    "com.coreos.rpm.runc": "1.0.0-81.rhaos4.6.git5b757d4.el8.x86_64",
    "com.coreos.rpm.strace": "5.1-1.el8.x86_64",
    "com.coreos.rpm.systemd": "239-41.el8_3.1.x86_64",
    ...
    "version": "47.83.202101251959-0"
}

@jlebon jlebon marked this pull request as ready for review February 5, 2021 17:15
@jlebon
Copy link
Member Author

jlebon commented Feb 5, 2021

This is ready for review but needs new rpm-ostree.
/hold

@sinnykumari
Copy link
Contributor

Sorry, I hadn't updated this comment with a link to coreos/rpm-ostree#2519.

ah, I missed it. Sweet, this is much cleaner and better!

Also can't stop thinking that kernel-rt in reality is not an extension, instead it is a kernel replacement. Perhaps we should add it into kind: kernel-replacement (again my poor naming :)

One minor change I would say is drop kernel-rt-devel from direct package list, so far we are not installing it with RT kernel switch (https://github.com/openshift/machine-config-operator/blob/master/pkg/daemon/update.go#L1159) . Maybe we can add it in development kind which would need a bit readjustment in yaml levels. Perhaps something like:

repos:
  - rhel-8-nfv

extensions:
  os-extension:
    usbguard:
      packages:
      - usbguard
    kernel-devel:
      packages:
      - kernel-devel
      - kernel-headers
  kernel-replacement:
    kernel-rt:
      architectures:
      - x86_64
      packages:
      - kernel-rt-core
      - kernel-rt-kvm
      - kernel-rt-modules
      - kernel-rt-modules-extra
  development:
    kernel:
      architectures:
      - x86_64
      packages:
      - kernel-devel
      - kernel-core
      - kernel-headers
      - kernel-modules
      - kernel-modules-extra
      match-base-evra: kernel
    kernel-rt:
       packages:
       - kernel-rt-core
       - kernel-rt-kvm
       - kernel-rt-modules
       - kernel-rt-modules-extra
       - kernel-rt-devel

@cgwalters
Copy link
Member

Also can't stop thinking that kernel-rt in reality is not an extension, instead it is a kernel replacement. Perhaps we should add it into kind: kernel-replacement (again my poor naming :)

I agree with this. Probably in fact what we should do is generate two base OSTree commits - i.e. they show up as separate container images or so, like machine-os-content and machine-os-content-rt or so. This came up in the initial discussions but basically the reason we didn't do that originally is it crosses the whole build system and into the MCO.

@miabbott
Copy link
Member

Thanks for working this out. I think this is a great improvement from the current state of things.

I left some thoughts about future refinements that could be pursued later.

/approve

Prep for coreos#2028. (Had to
split this out because Prow tests don't rebuild the container from
scratch but use the buildroot image.)
@jlebon
Copy link
Member Author

jlebon commented Feb 22, 2021

Ahh right, to satisfy Prow we need to decouple the dep addition: #2051.

jlebon added a commit to jlebon/os that referenced this pull request Feb 22, 2021
With coreos/rpm-ostree#2439 and
coreos/coreos-assembler#2028, support for
extensions is now more robust.

The extensions.yaml file is now fed directly to rpm-ostree. We just need
to do some minor tweaks:
- drop listing usbguard deps, rpm-ostree fetches them automatically
- split the devel bits into two extensions: one is an OS extension which
  contains the devel and headers packages, and the other is a
  development extension containing kernel packages already in the base
  (marking it as a development extension will signal the MCO that it's
  not intended to be directly layered on the host)
- use `match-base-evr: kernel` on those to enforce that the kernel
  development packages have the same EVR as the base kernel package in
  the OSTree

The `cosa upload-oscontainer` command now also supports
`oscontainer.yaml` specifying "important" packages whose EVRA are
included directly as a label on the resulting image.
Make it more generic. Prep for using it for other things.
This is more robust than `$0` in the case where we're sourced from a
script that's *not* in `/usr/lib/coreos-assembler`.
This elevates extensions to be "first-class" artifacts. We include a
tarball of the extensions in the build dir, as well as details in
`meta.json` (this can be used for example by the release browser).

This command will replace the `download-extensions` script. A major
advantage is that it leverages rpm-ostree's new support for extension
composes:

coreos/rpm-ostree#2439

One awkward bit is that our Python code has to call back into the shell
library because we need to make use of the `runvm` path in the
unprivileged case, since that's where the cache is located.
Use the tarball produced by `buildextend-extensions` as the source of
truth for extensions and drop `download-extensions` now that we no
longer need it.
This label contains a semi-colon-separated list of extensions baked in
the oscontainer. This will be used by the humans and the MCO for
validation:

openshift/os#409 (comment)
These labels allow one to query the RPM version of a subset of packages
without having to download them. See also:

openshift/os#409 (comment)
@jlebon
Copy link
Member Author

jlebon commented Feb 22, 2021

Latest rpm-ostree is in cosa now, so we can unhold this!
/hold cancel

In this latest iteration, I tweaked things to make the list of important packages come from the oscontainer.yaml as inspired by @miabbott's comment above. You can see what this looks like in openshift/os#496.

Requires: #2051

openshift-merge-robot pushed a commit that referenced this pull request Feb 22, 2021
Prep for #2028. (Had to
split this out because Prow tests don't rebuild the container from
scratch but use the buildroot image.)
@jlebon
Copy link
Member Author

jlebon commented Feb 22, 2021

/retest

1 similar comment
@jlebon
Copy link
Member Author

jlebon commented Feb 22, 2021

/retest

@jlebon
Copy link
Member Author

jlebon commented Feb 22, 2021

Yay, green CI on this now! (Although... CI isn't actually testing extensions at all. Would be nice to have it run an RHCOS compose here now that we can do that.)

@miabbott
Copy link
Member

/lgtm

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [ashcrow,cgwalters,jlebon,miabbott]

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 32b6983 into coreos:master Feb 23, 2021
jlebon added a commit to jlebon/os that referenced this pull request Feb 23, 2021
With coreos/rpm-ostree#2439 and
coreos/coreos-assembler#2028, support for
extensions is now more robust.

The extensions.yaml file is now fed directly to rpm-ostree. We just need
to do some minor tweaks:
- drop listing usbguard deps, rpm-ostree fetches them automatically
- split the devel bits into two extensions: one is an OS extension which
  contains the devel and headers packages, and the other is a
  development extension containing kernel packages already in the base
  (marking it as a development extension will signal the MCO that it's
  not intended to be directly layered on the host)
- use `match-base-evr: kernel` on those to enforce that the kernel
  development packages have the same EVR as the base kernel package in
  the OSTree

The `cosa upload-oscontainer` command now also supports
`oscontainer.yaml` specifying "important" packages whose EVRA are
included directly as a label on the resulting image.
@jlebon jlebon deleted the pr/extensions branch April 22, 2023 23:34
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.

7 participants