Skip to content

[MCO-8] machineconfig: Use KernelArguments field instead of kernel-args file#5439

Merged
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
LorbusChris:mc-kargs
Dec 16, 2021
Merged

[MCO-8] machineconfig: Use KernelArguments field instead of kernel-args file#5439
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
LorbusChris:mc-kargs

Conversation

@LorbusChris
Copy link
Copy Markdown
Contributor

The MC KernelArguments field is the recommended way to set kernel args,
and should be used in new installs as opposed to writing kernel args to
the /etc/pivot/kernel-args file, which should be considered
deprecated.

This should be considered a stop-gap solution until in-Ignition kargs support has landed in the MCO.

/cc @sinnykumari @yuqi-zhang @craychee @cgwalters

/hold
for testing

@LorbusChris
Copy link
Copy Markdown
Contributor Author

I'll have to update the tests, too.
/label wip

@LorbusChris LorbusChris changed the title [MCO-8] machineconfig: Use KernelArguments field instead of kernel-args file WIP: [MCO-8] machineconfig: Use KernelArguments field instead of kernel-args file Dec 2, 2021
@openshift-ci openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 2, 2021
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Dec 2, 2021

@LorbusChris: The label(s) /label wip cannot be applied. These labels are supported: platform/aws, platform/azure, platform/baremetal, platform/google, platform/libvirt, platform/openstack, ga, tide/merge-method-merge, tide/merge-method-rebase, tide/merge-method-squash, px-approved, docs-approved, qe-approved, downstream-change-needed, backport-risk-assessed, cherry-pick-approved

Details

In response to this:

I'll have to update the tests, too.
/label wip

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.

@sinnykumari
Copy link
Copy Markdown
Contributor

Thanks for the PR, can you also add steps on how to test this?

@LorbusChris LorbusChris force-pushed the mc-kargs branch 2 times, most recently from 82df865 to 8185244 Compare December 2, 2021 17:40
@LorbusChris
Copy link
Copy Markdown
Contributor Author

I've updated the unit tests, but I don't think the kargs that are actually in use are gathered anywhere in CI, so that might have to be checked manually.

Copy link
Copy Markdown
Contributor

@staebler staebler left a comment

Choose a reason for hiding this comment

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

/approve

I will leave it up to the CoreOS team to give it a lgtm once they are satisfied with the test results.

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 3, 2021
@LorbusChris LorbusChris changed the title WIP: [MCO-8] machineconfig: Use KernelArguments field instead of kernel-args file [MCO-8] machineconfig: Use KernelArguments field instead of kernel-args file Dec 3, 2021
@openshift-ci openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 3, 2021
@cgwalters
Copy link
Copy Markdown
Member

I think using the old interface was semi-intentional as far as ensuring it was tested. But...OTOH, yes everyone should be using the MCO path now. So...
/approve

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Dec 7, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, staebler

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

The pull request process is described here

Details 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

The MC KernelArguments field is the recommended way to set kernel args,
and should be used in new installs as opposed to writing kernel args to
the `/etc/pivot/kernel-args` file, which should be considered
deprecated.

In order to test this change, set `hyperthreading: Disabled` in the
`install-config.yaml` and install a cluster. Once the installation has
finished, `oc get mc` should show the following two MachineConfig
objects among others:
- 99-master-disable-hyperthreading
- 99-worker-disable-hyperthreading
To verify hyperthreading was actually disabled one the nodes, access a
node via e.g. `oc debug node/<name>` and run
`cat /sys/devices/system/cpu/smt/active`. It should show `0`.
@LorbusChris
Copy link
Copy Markdown
Contributor Author

@sinnykumari I have added instructions for testing this on the commit message
/hold cancel

@openshift-ci openshift-ci Bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 9, 2021
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Dec 9, 2021

@LorbusChris: The following tests 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/e2e-libvirt 26868a7 link false /test e2e-libvirt
ci/prow/e2e-crc 26868a7 link false /test e2e-crc
ci/prow/e2e-aws-workers-rhel8 26868a7 link false /test e2e-aws-workers-rhel8
ci/prow/okd-e2e-aws-upgrade 26868a7 link false /test okd-e2e-aws-upgrade
ci/prow/e2e-aws-fips 26868a7 link false /test e2e-aws-fips
ci/prow/e2e-metal-single-node-live-iso 26868a7 link false /test e2e-metal-single-node-live-iso
ci/prow/e2e-metal-ipi-ovn-ipv6 26868a7 link false /test e2e-metal-ipi-ovn-ipv6

Full PR test history. Your PR dashboard.

Details

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.

@LorbusChris
Copy link
Copy Markdown
Contributor Author

/retest-required

@sinnykumari
Copy link
Copy Markdown
Contributor

Missed this PR earlier.
/lgtm

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Dec 16, 2021
@openshift-bot
Copy link
Copy Markdown
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

2 similar comments
@openshift-bot
Copy link
Copy Markdown
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Copy Markdown
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Copy Markdown
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

2 similar comments
@openshift-bot
Copy link
Copy Markdown
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Copy Markdown
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

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.

6 participants