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 kernel arg tuneable allowlist for FCOS #1925

Merged
merged 1 commit into from Jul 16, 2020

Conversation

LorbusChris
Copy link
Member

This is required for Fedora CoreOS installs, which set
mitigations=auto,nosmt and may enable cgroups v2
in the future so it has to be allowed to disable it explicitly.

/cc @runcom @vrutkovs

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 14, 2020
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 14, 2020
@openshift openshift deleted a comment from openshift-ci-robot Jul 14, 2020
@openshift openshift deleted a comment from openshift-ci-robot Jul 14, 2020
@cgwalters
Copy link
Member

This is a super messy topic. First, I think we should recommend using kernelArgs in MachineConfig today via additional manifests provided to the installer instead of the /etc/pivot interface. The installer should switch to doing that instead.

However...what you really want for this specific case is the ability to remove kernel arguments (I am not sure that one can easily re-enable SMT by appending). We could add such a thing to MachineConfig...or we could add a high level hyperThreading: true value just like the install config has.

Another path would be for OKD to always remove the migitations=auto,nosmt value by default (i.e. making OKD match OCP/RHCOS) and then the existing interfaces exposed for this by the installer and the MCO to disable it again would work.

This is required for Fedora CoreOS installs, which set
`mitigations=auto,nosmt` and may enable cgroups v2
in the future so it has to be allowed to disable it explicitly.
@LorbusChris
Copy link
Member Author

/retest

@LorbusChris
Copy link
Member Author

@cgwalters what we've been doing so far is this: openshift/installer@5814b13#diff-41963c416a97dfbe789cafbd4a347a78R217 -- is what you're saying that the removal doesn't work?

I agree that we should switch to using MC kernelArgs for this instead of pivot - will look into it.

@cgwalters
Copy link
Member

@cgwalters what we've been doing so far is this: openshift/installer@5814b13#diff-41963c416a97dfbe789cafbd4a347a78R217 -- is what you're saying that the removal doesn't work?

That code might work indeed. I'm not opposed to this PR to be clear, but I'd like to consider the /etc/pivot interface very deprecated, and would prefer one of the options above.

@LorbusChris
Copy link
Member Author

@cgwalters @runcom this PR is keeping the status quo, and adding to master what we do on the fcos branch already.
If you're not entirely opposed, I'd like to merge this, as it's among the last pieces missing before we can drop the fork entirely.
And I'll file a card for moving from pivot to MC's kernelArguments.

@runcom
Copy link
Member

runcom commented Jul 15, 2020

I agree with Colin about deprecating that interface and also ok with this PR as well to allow a safe iteration towards porting the working FCOS branch patches here. As long as we track the deprecation with a card I’m fine, that card will likely drop nosmt for rhcos as well, I’ll leave to Colin to approve if the plan sounds good 👍

@LorbusChris
Copy link
Member Author

/retest

@sinnykumari
Copy link
Contributor

I don't have full context of how fcos branch of installer and MCO works today. For master branch we have kargs support added for both day1 and day2 . It should be easy to validate for OKD with the doc https://github.com/openshift/machine-config-operator/blob/master/docs/MachineConfiguration.md#KernelArguments. As Colin mentioned, applying a karg through MachineConfigs has advantage of deleting them further as well.

While working on PR #1850 which involves lot of changes during pivot, I was thinking of the possibility of cleaning up tunable kargs which we support during pivot.

@LorbusChris
Copy link
Member Author

@sinnykumari OCP and OKD installer currently both use the pivot interface to add the kargs (they don't differ in the way they do it, they just set different ones).

@cgwalters
Copy link
Member

For now
/approve
and code looks good to me.

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 15, 2020
@LorbusChris
Copy link
Member Author

/retest

@sinnykumari
Copy link
Contributor

@sinnykumari OCP and OKD installer currently both use the pivot interface to add the kargs (they don't differ in the way they do it, they just set different ones).

right. I was saying that same should be then possible to achieve through MachineConfig during cluster install. But yeah, we can discuss this in the card suggested in #1925 (comment)

@sinnykumari
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 16, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, LorbusChris, 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:
  • OWNERS [cgwalters,sinnykumari]

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 1f745c3 into openshift:master Jul 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants