-
Notifications
You must be signed in to change notification settings - Fork 413
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
Support installing extensions shipped by RHCOS #1850
Conversation
/skip e2e-ovn-step-registry |
b164617
to
0ceda75
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a few minor comments. love that this is super clear and easy to follow ❤️
0fd93b7
to
3693dc8
Compare
To install extensions on RHCOS host, I am pulling mounting OSContainer and then creating a repo in /etc/yum.repos.d/ which points to mounted OSContainer path (https://github.com/openshift/machine-config-operator/pull/1850/files#diff-06961b075f1753956d802ba954d2cfb5R820) . And then run
Looking at audit message on the applied host, I don't see any selinux denial message Do we need to implement something like coreos/rpm-ostree#1732 for Hacky option I see is copy the extensions/ from mounted container into somewhere (like /var/tmp/) and delete after applying extensions. @cgwalters @jlebon thoughts? |
OK I think the answer here is that currently for the ostree repo, what we're doing is opening a file descriptor and sending it to the daemon over DBus. That will work regardless of any mount namespaces in effect. When we invoked Possible fixes:
|
Hmm, I'm confused. Doesn't the |
3rd option sound promising to me. I have limited knowledge, so I am going to ask some silly questions -
|
We have two copies of that code now, one which runs on the host and one which runs in the MCD that was added for kernel-rt. I think we need to deduplicate, and we can do so conveniently after #1766 lands! |
Ahh gotcha, I see it now. I was just grepping for Re. possible fixes suggested in #1850 (comment), my vote personally is for (2) short-term until (3). |
Since #1766 is already in, I am going to give more thoughts and rework extensions feature + existing kernel-rt work to run in the context of host |
Yeah I think having the kernel-rt logic run consistently on the host is really a pre-requisite for this. I think actually today what we can do is serialize the Hmm right I just realized the reason the kernel-rt code is working today is because the Once we have https://bugzilla.redhat.com/show_bug.cgi?id=1839065 (which might even happen in 8.2.z) I think we can drop all of the logic for writing our binary to the host in the MCD case and just run everything from the MCD which would obviously be a huge cleanup. |
+1 I was also thinking about early validation of extensions args i.e. when we render the applied MachineConfig. I believe this would require another podman mount of OSContainer to look into available extensions :/
right
that will be nice. If I understand correctly we will still need to pull m-c-d binary during firstboot, right? |
Oh yeah, that's a good point. Hmm. Probably the MCC container could pull the oscontainer once when it starts up and then and cache a mapping of Or I guess we could just maintain a static list of allowed extensions in the MCO too. There shouldn't be too many to start.
Yeah, because we don't support really ancient kubelets joining the cluster, we don't want any workloads to land before OS updates etc. |
Maybe we should be including a JSON file or the like in machine-os-content that provides the list of supported extensions? Since we are responsible for sticking the right RPMs and deps into the container, seems like we are in the best position to enumerate the supported extensions per oscontainer/release. Could even go as far as providing a supported and unsupported list, if we were to ever decide to drop support for an extension. |
agree, if we want to extend this to supported/unsupported list then machine-os-content seems to be the right place to provide that detail. For now I think it should be fine for MCO to consume extensions as it is because we ship only those extra packages in machine-os-content which we want users to install. #1766 makes things really flexible to update MCD behavior whenever neededd |
The new v2 layout https://gitlab.cee.redhat.com/coreos/redhat-coreos/-/merge_requests/952 aims to have "enumerate extensions" simply use the filesystem. Basically:
Not opposed to a JSON list, but keep in mind the rpm-md repodata is already a "list of packages" (just in XML form). We'd be inventing "rpm metadata in JSON form"? Anything we change inside the container though isn't solving the issue that validating a MachineConfig fragment requires fetching and unpacking the container. |
I missed this association when that MR went by. That kind of clear relationship is what I was thinking about when I suggested the JSON file approach. |
I think this is fine to start, though IMO there's definitely an argument for having the flexibility that some metadata layer affords us. For example, if we want extension "foobar" to actually result in the installation of two related, but not necessarily dependent packages. Or if we want to change how extension "foobar" is implemented at the RPM level across an update. I think actually this might help with the Recommends issue as well. If we solve that and turn it off by default both server-side and client-side, then we'll need a way to pull in recommended packages which no longer get pulled in when it makes sense (and without globally turning on weak deps again client-side). |
3693dc8
to
470f515
Compare
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sinnykumari 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 |
470f515
to
c6100e6
Compare
I wanted to revive the discussion about providing some kind of metadata about available extensions in In a real-time meeting with @zvonkok @mrunalp @darkmuggle (re: openshift/enhancements#357) we discussed adding additional annotations to the It was brought up that we could go a step further and also enumerate the available extensions in an annotation on the |
This sounds like it needs more thoughts and discussion. To avoid getting this lost in this lengthy PR update, I think creating a new issue with relevant information would be nice. |
e2dabf9
to
a761209
Compare
/retest |
Moved to openshift/os#409 |
In the latest PR, I have moved kernel-rt and extensions processing into host context. It installs needed packages from coreos-extensions repo which we again mount in the host context so that it is accessible to rpm-ostree. It seems rpm-ostree still has some issues accessing the extensions repo path which points to already mounted OSContainer. Journal log of rpm-ostreed service while installing usbguard extensions at cluster install time -
Above issue has been seen while using latest installer where bootimage contains OSTree version 46.82.202007152240-0, rpm-ostree-2020.2-2.el8.x86_64, ostree-2020.3-3.el8.x86_64) Before moving to latest installer, I have been using older installer where bootimage contains OSTree version 45.81.202005200134-0, rpm-ostree-2019.6-8.el8.x86_64, ostree-2019.6-2.el8.x86_64 . With this OS update, extensions apply and kernel-rt switch works fine. Don't see any selinux denial in audit log. @cgwalters @jlebon Can it be related to some recent rpm-ostree update? |
- Save old and new MachineConfig into json file and process later on host - Rework the existing PullAndRebase() to run extensions in host context - Add m-c-d subcommand mount-container which we run in host context. Here we pull OSContainer, create a container and mount it. We also save the created container name and mount location in /run which we will be used later by MCD to rebase the OS and apply extensions
Install kernel-rt packages from coreos-extensions repo we created earlier. This saves us from searching explicitly for kernel-rt packages in the OSContainer. Also simplifies updating kernel-rt packages
a761209
to
f133530
Compare
Had a chat with Colin and Jonathan about recent issue #1850 (comment). Issue is happening because:
Solution:
We will explore option 2 first and if it doesn't work we will go for option 1 Until we fix this |
While going down the road of We can close this PR later on. |
@sinnykumari: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
var containerName string | ||
if containerName, err = daemon.ReadFromFile(constants.MountedOSContainerName); err != nil { | ||
return err | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I may be missing the point of this re-declaration of containerName
.
Don't we want it to be available below, outside of the condition?
containerName, err = daemon.ReadFromFile(constants.MountedOSContainerName)
if err != nil {
return err
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
meh.. I just realized there's a new PR 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will close this once we merge the other one.
Earlier to perform OS update we were pulling OS image and mounting the content using podman. We were performing OS update in host context because of selinux constraints on mounted container. Also, for rt-kernel switch we were pulling OS image again. With coreos extensions support, we require extensions rpm which is available in os container. We tried different approach to solve problems like minimizing container image pull, host/container context switch, selinux permission on mounted container and rpm-ostreed behavior. See openshift#1850 Finally, using `oc image extract` and skopeo to inspect container image solves our problems. With this we are also getting rid of using mco-pivot systemd service to rebase OS.
Alternative implementation of extensions in #1941 has been merged, closing this PR. |
enhancement doc- openshift/enhancements#317
host context. Here we pull OSContainer, create a container and mount it.
We also save the created container name and mount location in /run
which we will be used later by MCD to rebase the OS and apply extensions
This saves us from searching explicitly for kernel-rt packages in the OSContainer.
Also simplifies updating kernel-rt packages