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

Setup os dependent machine config extensions. #266

Merged
merged 2 commits into from Mar 10, 2023

Conversation

smuda
Copy link
Contributor

@smuda smuda commented Feb 28, 2023

- Description of the problem which is fixed/What is the use case

The machine config operator handles RHCOS and FCOS differently when effectuating machine config extensions.

On RHCOS it uses a hardcoded list which is then translated into the actual packages. In our case, sandboxed-containers are translated into kata-containers and then rpm-ostree is called.

On FCOS it just pipes the extensions directly into rpm-ostree, which means that in the current implementation it tries to install sandboxed-containers which doesn't exist in FCOS yum repo.

Fixes #267

- What I did

I added an OS detection mechanism and if it's RHCOS I use sandboxed-containers as extension, otherwise I use kata-containers.

In theory it's unfortunate that it's the OS of the operator that I use for guessing what OS the actual worker node has, but I've never heard of an OCP with FCOS or OKD with RHCOS so I think it's a reasonable assumption.

- How to verify it

  1. Install OKD.
  2. Enable the fcos.repo yum repo
  3. Build from source according to DEVELOPMENT.md.
  4. Install from OKD console.

- Description for the changelog

Support for choosing the right RPM package on OKD/FCOS.

@openshift-ci openshift-ci bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Feb 28, 2023
@openshift-ci
Copy link

openshift-ci bot commented Feb 28, 2023

Hi @smuda. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@bpradipt
Copy link
Contributor

bpradipt commented Mar 2, 2023

/ok-to-test

@openshift-ci openshift-ci bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 2, 2023
@bpradipt bpradipt requested review from pmores and jensfr March 2, 2023 12:41
Copy link
Contributor

@pmores pmores left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks @smuda !

@openshift-ci
Copy link

openshift-ci bot commented Mar 9, 2023

@smuda: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/sandboxed-containers-operator-e2e 9d07b7f link false /test sandboxed-containers-operator-e2e

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.

Copy link
Contributor

@bpradipt bpradipt left a comment

Choose a reason for hiding this comment

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

/lgtm
Thanks @smuda

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 10, 2023
@bpradipt bpradipt merged commit 448958b into openshift:main Mar 10, 2023
@smuda smuda deleted the different-rpm-ostree-per-os branch March 10, 2023 10:31
gkurz added a commit to gkurz/sandboxed-containers-operator that referenced this pull request Mar 17, 2023
PR openshift#266 added the possibility to use different names for the extension
depending on the CoreOS flavour presumably running on the host :
"kata-containers" by default and "sandboxed-containers" for RHCOS. This
is based on OS detection using os-release(5) files.

In order to make a good guess, this logic should be handed over the
os-release files of the host, otherwise arbitrary os-release files
from the controller image might be used instead.

This is exactly what happens in the case of Red Hat's OpenShift Sandboxed
Containers (OSC) : the controller image is based on RHEL8. It legitimately
fails the RHCOS detection heuristics and we end up trying to use the
"kata-containers" name that doesn't exist in RHCOS. Thus preventing
deployment of kata and putting the cluster in a degraded state.

Trying to make assumptions on the host OS isn't generally recommanded.
It is at best fragile and at worse potentially insecure if it requires
to expose host details inside containers. This isn't really a direction
that OSC is willing to take. Also, there is no real need for runtime
detection in the code : the CoreOS flavour is an invariant that can
be passed to the controller process when it starts.

Let's go for a more simple and robust solution : make it configurable
with an environment variable. This allows easy customization in the
manifest files and doesn't raise any security concern.

"kata-containers" remains the default so this should not change any
existing behavior for FCOS. OSC will adapt its downstream manifests to
use the RHCOS-friendly name.

Fixes: https://issues.redhat.com/browse/KATA-2079

Signed-off-by: Greg Kurz <groug@kaod.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OKD: Choosing the right machine config extension on FCOS
3 participants