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.

Sign up for GitHub

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

Jump to bottom

Follow-ups to the extensions work #2519

Merged
merged 9 commits into from
Feb 3, 2021
Merged

Follow-ups to the extensions work #2519

merged 9 commits into from
Feb 3, 2021

Conversation

jlebon
Copy link
Member

@jlebon jlebon commented Jan 29, 2021 •
edited
Loading

See coreos/coreos-assembler#2028 (comment) for background. Testing this on RHCOS with:

repos:
  - rhel-8-nfv

extensions:
  usbguard:
    kind: os-extension
    packages:
      # notice how we don't have to specify deps here
      - usbguard
  kernel-rt:
    kind: os-extension
    architectures:
      - x86_64
    packages:
      - kernel-rt-core
      - kernel-rt-kvm
      - kernel-rt-modules
      - kernel-rt-modules-extra
      - kernel-rt-devel
  kernel-devel:
    kind: development
    packages:
      - kernel-devel
      - kernel-core
      - kernel-headers
      - kernel-modules
      - kernel-modules-extra
    match-base-evra: kernel

Works fine!

See individual commit messages for details.

jlebon added 2 commits January 29, 2021 17:54
@jlebon
core: Factor out function to set repos on pkgs
b611cd3 
And use a hash table to make it more efficient.

Prep for future patch.
@jlebon
core: Factor out function to download pkgs
b1b6304 
I want to be able to use this function without an `RpmOstreeContext`.

Prep for future patch.
@jlebon
Copy link
Member Author

jlebon commented Jan 29, 2021

Hmm actually, I think a cleaner approach than the kind: development thing is to just add a separate top-level key for this in the schema. So something like:

extensions:
  ...
packages:
  - kernel-core
  - kernel-headers
  ...

Makes it more generic than "development" (because honestly, this is very specific to the kernel), and makes it a bit clearer that rpm-ostree is just a glorified yumdownload for that bit.

@cgwalters
Copy link
Member

Hmm actually, I think a cleaner approach than the kind: development thing is to just add a separate top-level key for this in the schema. So something like:

I'm fine with that, but I think it'd also be enough to default kind: os-extension.

cgwalters
cgwalters approved these changes Jan 30, 2021
View reviewed changes
Copy link
Member

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically LGTM!

rust/src/extensions.rs Show resolved Hide resolved
@jlebon jlebon force-pushed the pr/extension-kind branch from cc197bb to 60c5010 Compare February 1, 2021 20:50
@jlebon jlebon changed the title WIP: Follow-ups to the extensions work Follow-ups to the extensions work Feb 1, 2021
@jlebon jlebon force-pushed the pr/extension-kind branch from 60c5010 to e1de4a5 Compare February 1, 2021 21:00
@jlebon
Copy link
Member Author

jlebon commented Feb 1, 2021

OK, updated and ready for review!

Hmm actually, I think a cleaner approach than the kind: development thing is to just add a separate top-level key for this in the schema. So something like:

So I went down that path a bit, but it felt hacky there too because it just looks out of place in a file called extensions.yaml which is passed to a command called rpm-ostree compose extensions. I'd rather just broaden the meaning of "extension" a bit to include that use case.

@jlebon jlebon force-pushed the pr/extension-kind branch from e1de4a5 to 7e32263 Compare February 1, 2021 21:51
@jlebon
Copy link
Member Author

jlebon commented Feb 2, 2021 

   basic-unified: error: mkdir(/home/jenkins/agent/workspace/hub-ci_coreos_rpm-ostree_PR-2519/compose-cache/cachedir/packages/): Read-only file system

Hmm, interesting... investigating.

jlebon added 7 commits February 2, 2021 17:11
@jlebon
core: Fix handling of local packages when downloading
f5c689d 
In the core context, this is redundant with `sort_packages` because it
won't put local packages in the `pkgs_to_download` array anyway, but we
want this check even if we call `rpmostree_download_packages` directly
and pass some packages which may be local.
@jlebon
extensions: Support enabling additional repos
1e3a2eb 
We want to be able to enable more repos than those in the treefile when
downloading extensions. In RHCOS for example, the `kernel-rt` packages
come from a separate repo.

But also, once we support "development" extensions, we want to support
the case where devel packages come from another repo.
@jlebon
extensions: Fix missing CxxResult
c01f862
@jlebon
app/compose: Add comment about pkgcache
2fee11a 
Gives a bit more info about how the extensions path is different from
the base treecompose.
@jlebon
rust/treefile: Drop unnecessary #[serde(rename)]
3313a66 
The key is already called `rpmdb`.
@jlebon
extensions: Add support for development extensions
6090476 
In RHCOS, we ship kernel development-related packages as an extension.
Those aren't really extensions that are meant to be layered onto the
host.  They're meant to be used in a build environment somewhere to
compile kernel modules.

This makes it very different from "OS extensions" in at least two
drastic ways:
1. we don't want to do any depsolving (e.g. we don't want to pull in
   `gcc` or something)
2. some of those packages may be present in the base already, but we
   still want to redownload them

Hesitated putting this functionality in rpm-ostree, but I think in the
end it cuts from the benefit of moving this code to rpm-ostree if we
can't entirely get rid of the Python script it obsoletes. Plus, being
able to use the `match-base-evr` is still really useful for this use
case.

Let's add a new `kind` key to support this. The traditional extensions
are called "OS extensions" and these new extensions are called
"development extensions".

The latter is not yet part of the state checksum, so change detection
doesn't work there. I think that's fine for now though because the
primary use case is the kernel, and there we want to match the base
version. So if the kernel changes, the base would change too. (Though
there's the corner case of adding a new package to the list while at the
same version...)
@jlebon
tests/compose.sh: Always rebuild supermin appliance
d39f880 
We always want the latest rpm-ostree binaries tested, so we need to
always rerun supermin.

Patch better viewed with whitespace ignored.
@jlebon jlebon force-pushed the pr/extension-kind branch from 7e32263 to d39f880 Compare February 2, 2021 22:14
@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

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 57edf7e into coreos:master Feb 3, 2021
@jlebon jlebon mentioned this pull request Feb 5, 2021
Merged
@jlebon jlebon deleted the pr/extension-kind branch April 23, 2023 23:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cgwalters cgwalters cgwalters approved these changes

@kelvinfan001 kelvinfan001 Awaiting requested review from kelvinfan001

@travier travier Awaiting requested review from travier

Assignees

@cgwalters cgwalters

Labels
approved lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jlebon @cgwalters @openshift-ci-robot @openshift-merge-robot