OCPNODE-3372: Update defaultRuntime doc to show options and set default to crun #2370

ngopalak-redhat · 2025-06-17T04:34:08Z

This PR addresses an issue in the generated documentation for the defaultRuntime option

Previously, the documentation at https://docs.redhat.com/en/documentation/openshift_container_platform/4.18/html/machine_apis/containerruntimeconfig-machineconfiguration-openshift-io-v1 did not list crun and runc as valid options for defaultRuntime.

While crun became the default runtime in OpenShift 4.18 (as per release notes: https://docs.redhat.com/en/documentation/openshift_container_platform/4.18/html/release_notes/ocp-4-18-release-notes), the internal constant reflecting this default was not updated, though the functionality itself was correct.

This change updates the internal documentation comments and the relevant constant to accurately reflect:

Valid options: crun and runc
Default runtime: crun

Based on the review I have also added enum validation so that the failure can be caught earlier. Unit tests are added to ensure that it doesn't break any existing functionality.

openshift-ci-robot · 2025-06-17T04:34:12Z

@ngopalak-redhat: This pull request references OCPNODE-3372 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.20.0" version, but no target version was set.

In response to this:

This PR addresses an issue in the generated documentation for the defaultRuntime option

Previously, the documentation at https://docs.redhat.com/en/documentation/openshift_container_platform/4.18/html/machine_apis/containerruntimeconfig-machineconfiguration-openshift-io-v1 did not list crun and runc as valid options for defaultRuntime.

While crun became the default runtime in OpenShift 4.18 (as per release notes: https://docs.redhat.com/en/documentation/openshift_container_platform/4.18/html/release_notes/ocp-4-18-release-notes), the internal constant reflecting this default was not updated, though the functionality itself was correct.

This change updates the internal documentation comments and the relevant constant to accurately reflect:

Valid options: crun and runc
Default runtime: crun

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 openshift-eng/jira-lifecycle-plugin repository.

openshift-ci · 2025-06-17T04:34:12Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

openshift-ci · 2025-06-17T04:34:14Z

Hello @ngopalak-redhat! Some important instructions when contributing to openshift/api:
API design plays an important part in the user experience of OpenShift and as such API PRs are subject to a high level of scrutiny to ensure they follow our best practices. If you haven't already done so, please review the OpenShift API Conventions and ensure that your proposed changes are compliant. Following these conventions will help expedite the api review process for your PR.

ngopalak-redhat · 2025-06-17T04:42:25Z

/assign @sairameshv @haircommander

ngopalak-redhat · 2025-06-17T05:14:00Z

/test lint

ngopalak-redhat · 2025-06-17T06:11:25Z

/retest

ngopalak-redhat · 2025-06-17T11:56:36Z

/retest-required

ngopalak-redhat · 2025-06-17T12:44:07Z

/test images

ngopalak-redhat · 2025-06-17T12:45:16Z

/test minor-images

ngopalak-redhat · 2025-06-17T13:06:42Z

/test images

ngopalak-redhat · 2025-06-17T13:15:37Z

/test minor-images

ngopalak-redhat · 2025-06-17T13:27:03Z

/retest-required

ngopalak-redhat · 2025-06-17T15:38:12Z

/test e2e-azure

ngopalak-redhat · 2025-06-17T15:40:20Z

/test okd-scos-e2e-aws-ovn

ngopalak-redhat · 2025-06-18T02:35:47Z

/test minor-e2e-upgrade-minor

ngopalak-redhat · 2025-06-18T02:37:02Z

/test okd-scos-e2e-aws-ovn

machineconfiguration/v1/types.go

ngopalak-redhat · 2025-06-25T13:46:55Z

Because the logic previously existed in MCO that would cause failures if this isn't set correctly it should be fine to restrict this validation at admission time.

That being said, we should ensure that we properly test that this change ratchets correctly. Here is an example of a ratcheting validation test:

api/example/v1/tests/stableconfigtypes.example.openshift.io/AAA_ungated.yaml

Lines 258 to 281 in c3cf9bb

- name: Should allow changing other fields when a persisted value is no longer valid

initialCRDPatches:

- op: remove

path: /spec/versions/0/schema/openAPIV3Schema/properties/spec/properties/nonZeroDefault/minimum # Minimum is normally 8

initial: |

apiVersion: example.openshift.io/v1

kind: StableConfigType

spec:

nonZeroDefault: 1

immutableField: foo

updated: |

apiVersion: example.openshift.io/v1

kind: StableConfigType

spec:

nonZeroDefault: 1

immutableField: foo

optionalImmutableField: foo

expected: |

apiVersion: example.openshift.io/v1

kind: StableConfigType

spec:

nonZeroDefault: 1

immutableField: foo

optionalImmutableField: foo

Addressed by adding ratchets test

everettraven · 2025-06-25T14:13:56Z

...v1/tests/containerruntimeconfigs.machineconfiguration.openshift.io/DefaultRuntimeConfig.yaml

+        apiVersion: machineconfiguration.openshift.io/v1
+        kind: ContainerRuntimeConfig
+        spec: 
+          containerRuntimeConfig:


Should defaultRuntime be set to the empty string value here?

@everettraven I've reverted the omitempty removal and kept defaultRuntime as a non-pointer type.

My reasoning is:

MCO handles both unset and empty string values for defaultRuntime as a default to crun.

Changing it to a pointer would require extensive code changes. May be across MCO and API repos.

Keeping omitempty ensures the field is omitted when unset, avoiding a confusing empty string for users.

If a user explicitly sets it to an empty string, it will be included in the manifest, which is acceptable as MCO handles it.

MCO handles both unset and empty string values for defaultRuntime as a default to crun.

Without a pointer, MCO cannot tell the difference between unset and the empty string.

Does the empty string mean anything to crun?

Keeping omitempty ensures the field is omitted when unset, avoiding a confusing empty string for users.

This is called discoverability in our APIs. When a controller generates a configuration API object, we try to generate a complete set of fields so that a user can easily see from the object, where there are options they may not have known about (they can discover the new options).

If a user explicitly sets it to an empty string, it will be included in the manifest, which is acceptable as MCO handles it.

But it won't round trip. As soon as a structured client writes the object, the field will be removed from etcd and future reads will not render the "" value. This means that an unstructured apply would never reach a stable state when the controller is responding to the writes from the unstructured client.

Either remove omitempty, or remove "" from the enum of valid values. Removing an enum value is a breaking change btw, so if this is already shipped and the enum already allows "", you should remove omitempty as this is the least breaking change.

Thanks @JoelSpeed . I have removed omitempty and updated test cases.

@JoelSpeed can you review? Happy to address any comments you have

These are unstructured calls, unless there's an explicit default, I wouldn't expect the defaultRuntime to be added as part of this test

ngopalak-redhat · 2025-07-01T01:12:55Z

/retest-required

JoelSpeed · 2025-07-01T10:24:31Z

...guration/v1/tests/containerruntimeconfigs.machineconfiguration.openshift.io/AAA_ungated.yaml

@@ -3,7 +3,7 @@ name: "ContainerRuntimeConfig"
 crdName: containerruntimeconfigs.machineconfiguration.openshift.io
 tests:
  onCreate:
-    - name: Should be able to create a minimal ContainerRuntimeConfig
+    - name: Should be able to create a minimal ContainerRuntimeConfig and default runtime set


Don't modify the existing case, add a new case

JoelSpeed · 2025-07-01T10:27:00Z

...v1/tests/containerruntimeconfigs.machineconfiguration.openshift.io/DefaultRuntimeConfig.yaml

+        apiVersion: machineconfiguration.openshift.io/v1
+        kind: ContainerRuntimeConfig
+        spec: 
+          containerRuntimeConfig:


These are unstructured calls, unless there's an explicit default, I wouldn't expect the defaultRuntime to be added as part of this test

JoelSpeed · 2025-07-01T10:28:37Z

...v1/tests/containerruntimeconfigs.machineconfiguration.openshift.io/DefaultRuntimeConfig.yaml

+    - name: Should be able to update from runc to crun with other parameters
+      initialCRDPatches:
+        - op: remove
+          path: /spec/versions/0/schema/openAPIV3Schema/properties/spec/properties/containerRuntimeConfig/properties/defaultRuntime/enum


Why remove the enum on this one? runc is a valid choice no?

JoelSpeed · 2025-07-01T10:28:49Z

...v1/tests/containerruntimeconfigs.machineconfiguration.openshift.io/DefaultRuntimeConfig.yaml

+    - name: Should not be able to update from default to invalid value
+      initialCRDPatches:
+        - op: remove
+          path: /spec/versions/0/schema/openAPIV3Schema/properties/spec/properties/containerRuntimeConfig/properties/defaultRuntime/enum


Why remove the enum on thius one, "" is a valid choice no?

JoelSpeed · 2025-07-01T10:29:42Z

machineconfiguration/v1/types.go

+	// When omitted, this means no opinion and the platform is left to choose a reasonable default,
+	// which is subject to change over time. Currently, the default is `crun`.
+	// +kubebuilder:validation:Enum=crun;runc;""
+	// +kubebuilder:default:=""


So this is interesting, this creates discoverability even from unstructured clients, we haven't done this before

What's the motivation?

JoelSpeed · 2025-07-01T10:36:52Z

machineconfiguration/v1/types.go

+	// When set to `crun`, OpenShift will use crun to execute the container
+	// When omitted, this means no opinion and the platform is left to choose a reasonable default,
+	// which is subject to change over time. Currently, the default is `crun`.
+	// +kubebuilder:validation:Enum=crun;runc;""


I'm not convinced we actually want to add "" to the enum here. Are we aiming for discoverability? Who creates this object? Is it generally created by a controller/structured client? Or do we expect end users to create these?

@JoelSpeed Before diving into other comments, let's clarify the defaultRuntime field. Our primary goal is to inform users that only 'runc' and 'crun' are valid options, as they currently aren't aware. We also expect users to directly modify this field to change the node's underlying runtime.

Removing omitempty and handling "" as an invalid value could break existing user configurations if they previously set defaultRuntime to "".

Given this, I see these paths forward:

Skip Enum Validation for now: Document 'runc' and 'crun' as the allowed values, and let MCO manage it as before.

Remove "" and omitempty (breaking change): Proceed with stricter validation, accepting this might break a small number of existing users who explicitly set ""."

The current change which uses "" as an enum option so that the existing users aren't broken.

Joel and I discussed this a bit further offline, and it looks like I missed the nuance of this field already having been set as omitempty previously meaning that explicitly setting the empty value ("") and completely omitting the field are treated exactly the same by the kube-apiserver (i.e users aren't broken if the enum doesn't include "").

That means we should:

Remove the "" value from the enum

Remove the default marker

Add omitempty back

Keep the messaging about default behavior when omitted in the GoDoc

My apologies for the confusion here.

@ngopalak-redhat What is the difference between this field being omitted (not present at all), and ""? Is there a semantic difference?

As far as I can tell from the docs, it mentions only runc and crun as valid values, https://docs.redhat.com/en/documentation/openshift_container_platform/4.18/html/machine_configuration/machine-configs-custom

@everettraven When we remove the "" option in the enum and keep omitempty, it leads to an error if a customer has previously set defaultRuntime to "". You can see the specific error below:

Type: "FieldValueNotSupported", Message: "Unsupported value: \"\": supported values: \"crun\", \"runc\"", Field: "spec.containerRuntimeConfig.defaultRuntime",

My take is that this breakage should be acceptable. We've never actually instructed customers to set defaultRuntime to an empty string. Therefore, any existing configurations with "" are likely not intended
What are your thoughts?

@JoelSpeed crun is the default now. Omitting or "" for defaultRuntime will result in choosing crun.

it leads to an error if a customer has previously set defaultRuntime to ""

Which version of OpenShift did you test this with? Ratcheting configuration should allow the previous value to be persisted on the latest few versions of OpenShift.

Therefore, any existing configurations with "" are likely not intended
What are your thoughts?

My concern is that, the way the field is structured today, allows an unstructured client (think kubectl apply) to set the field to "", but a structured client (think the controller) who updates the object, will strip the value (because of the omitempty). This means that "" doesn't round trip currently, and anyone who has tried to set the value to "", I expect has seen it removed after they have tried to set it. In theory that could be tested.

@JoelSpeed crun is the default now. Omitting or "" for defaultRuntime will result in choosing crun.

This implies to me that the value "" should not be in the enum. Though please don't update right now, @deads2k had some comments about this and I'll talk this through with him tomorrow

Lets continue with this and remove "". We can't see anywhere that we told users to do this, and if they did, it wouldn't round trip through a structured client today either

@JoelSpeed I have removed "". Updated the PR. Please review

… set default to crun

…ommitted value handled in MCO

…MCO with additional test cases

…O with additional test cases

...guration/v1/tests/containerruntimeconfigs.machineconfiguration.openshift.io/AAA_ungated.yaml

JoelSpeed · 2025-07-10T10:14:27Z

...v1/tests/containerruntimeconfigs.machineconfiguration.openshift.io/DefaultRuntimeConfig.yaml

+        kind: ContainerRuntimeConfig
+        spec:
+          containerRuntimeConfig:
+            defaultRuntime: crun


The point of removing the enum in the ractheting tests is to set the value to something invalid. Set this to docker in initial`, then create three different updated, One to change another field and show it remains, one to show it can be removed, one to update it to a valid value.

JoelSpeed · 2025-07-10T10:15:32Z

...v1/tests/containerruntimeconfigs.machineconfiguration.openshift.io/DefaultRuntimeConfig.yaml

+        spec:
+          containerRuntimeConfig:
+            logLevel: info
+    - name: Should be able to update from runc to crun with other parameters


You don't need to remove the enum from the initial CRD validation for this test? You're not actually testing ratcheting unless the initial value has something that is not valid

JoelSpeed · 2025-07-10T10:16:05Z

...v1/tests/containerruntimeconfigs.machineconfiguration.openshift.io/DefaultRuntimeConfig.yaml

+    - name: Should be able to update other parameters with invalid defaultRuntime
+      initialCRDPatches:
+        - op: remove
+          path: /spec/versions/0/schema/openAPIV3Schema/properties/spec/properties/containerRuntimeConfig/properties/defaultRuntime/enum
+      initial: |
+        apiVersion: machineconfiguration.openshift.io/v1
+        kind: ContainerRuntimeConfig
+        spec:
+          containerRuntimeConfig:
+            defaultRuntime: docker
+            logLevel: fatal
+      updated: |
+        apiVersion: machineconfiguration.openshift.io/v1
+        kind: ContainerRuntimeConfig
+        spec:
+          containerRuntimeConfig:
+            defaultRuntime: docker
+            logLevel: info
+      expected: |
+        apiVersion: machineconfiguration.openshift.io/v1
+        kind: ContainerRuntimeConfig
+        spec:
+          containerRuntimeConfig:
+            defaultRuntime: docker
+            logLevel: info


This is a correct use of ratcheting, but you also need to show you can transition from an invalid to valid value, and from an invalid value to absent

…CO with additional test cases

JoelSpeed

/lgtm

openshift-ci · 2025-07-11T13:07:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: haircommander, JoelSpeed, ngopalak-redhat

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~~ [JoelSpeed]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot · 2025-07-11T13:51:51Z

/retest-required

Remaining retests: 0 against base HEAD 674ad74 and 2 for PR HEAD 9334ab7 in total

openshift-ci-robot · 2025-07-11T18:36:58Z

/retest-required

Remaining retests: 0 against base HEAD 674ad74 and 2 for PR HEAD 9334ab7 in total

openshift-ci-robot · 2025-07-14T16:57:27Z

/retest-required

Remaining retests: 0 against base HEAD 674ad74 and 2 for PR HEAD 9334ab7 in total

openshift-ci · 2025-07-14T20:33:25Z

@ngopalak-redhat: 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-azure	`9334ab7`	link	false	`/test e2e-azure`
ci/prow/e2e-aws-serial-2of2	`9334ab7`	link	true	`/test e2e-aws-serial-2of2`

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-sigs/prow repository. I understand the commands that are listed here.

openshift-ci-robot · 2025-07-15T12:46:51Z

/retest-required

Remaining retests: 0 against base HEAD 674ad74 and 2 for PR HEAD 9334ab7 in total

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jun 17, 2025

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 Jun 17, 2025

openshift-ci bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jun 17, 2025

ngopalak-redhat marked this pull request as ready for review June 17, 2025 04:35

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 Jun 17, 2025

openshift-ci bot requested review from cheesesashimi and yuqi-zhang June 17, 2025 04:36

openshift-ci bot assigned haircommander and sairameshv Jun 17, 2025

ngopalak-redhat force-pushed the ngopalak/OCPNODE-3372_defaultRuntime branch from 0684334 to 6cf242b Compare June 17, 2025 05:28

ngopalak-redhat force-pushed the ngopalak/OCPNODE-3372_defaultRuntime branch from 6cf242b to 4a4c7f8 Compare June 17, 2025 09:26

openshift-ci bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jun 17, 2025

everettraven reviewed Jun 18, 2025

View reviewed changes

machineconfiguration/v1/types.go Outdated Show resolved Hide resolved

machineconfiguration/v1/types.go Show resolved Hide resolved

everettraven reviewed Jun 25, 2025

View reviewed changes

ngopalak-redhat requested review from everettraven and JoelSpeed June 27, 2025 14:55

JoelSpeed reviewed Jul 1, 2025

View reviewed changes

ngopalak-redhat added 9 commits July 10, 2025 09:30

OCPNODE-3372: Update defaultRuntime doc to show options available and…

4a55a1f

… set default to crun

OCPNODE-3372: Add ratcheting validation tests for defaultRuntime

e872173

OCPNODE-3372: Remove omitempty from defaultRuntime as its handled in MCO

f4f3ec3

OCPNODE-3372: Revert omitempty from defaultRuntime. Empty string and …

a8cc7d0

…ommitted value handled in MCO

OCPNODE-3372: Remove omitempty from defaultRuntime as its handled in …

dbd863c

…MCO with additional test cases

OCPNODE-3372: Remove omitempty from defaultRuntime as its handled in …

b014c62

…MCO with additional test cases

OCPNODE-3372: Remove omitempty from defaultRuntime as its handled in …

375ed95

…MCO with additional test cases

OCPNODE-3372: Readd omitempty for defaultRuntime as its handled in MC…

5a48948

…O with additional test cases

OCPNODE-3372: Readd omitempty for defaultRuntime as its handled in MC…

73dfbf9

…O with additional test cases

ngopalak-redhat force-pushed the ngopalak/OCPNODE-3372_defaultRuntime branch from 81e5b29 to 73dfbf9 Compare July 10, 2025 04:01

ngopalak-redhat requested a review from JoelSpeed July 10, 2025 04:04

JoelSpeed reviewed Jul 10, 2025

View reviewed changes

OCPNODE-3372: Re-add omitempty for defaultRuntime as its handled in M…

9334ab7

…CO with additional test cases

ngopalak-redhat requested a review from JoelSpeed July 11, 2025 09:40

JoelSpeed reviewed Jul 11, 2025

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 11, 2025

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 11, 2025

OCPNODE-3372: Update defaultRuntime doc to show options and set default to crun #2370

Are you sure you want to change the base?

OCPNODE-3372: Update defaultRuntime doc to show options and set default to crun #2370

Conversation

ngopalak-redhat commented Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Jun 17, 2025 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci bot commented Jun 17, 2025

Uh oh!

openshift-ci bot commented Jun 17, 2025

Uh oh!

ngopalak-redhat commented Jun 17, 2025

Uh oh!

ngopalak-redhat commented Jun 17, 2025

Uh oh!

ngopalak-redhat commented Jun 17, 2025

Uh oh!

ngopalak-redhat commented Jun 17, 2025

Uh oh!

ngopalak-redhat commented Jun 17, 2025

Uh oh!

ngopalak-redhat commented Jun 17, 2025

Uh oh!

ngopalak-redhat commented Jun 17, 2025

Uh oh!

ngopalak-redhat commented Jun 17, 2025

Uh oh!

ngopalak-redhat commented Jun 17, 2025

Uh oh!

ngopalak-redhat commented Jun 17, 2025

Uh oh!

ngopalak-redhat commented Jun 17, 2025

Uh oh!

ngopalak-redhat commented Jun 18, 2025

Uh oh!

ngopalak-redhat commented Jun 18, 2025

Uh oh!

Uh oh!

Uh oh!

ngopalak-redhat commented Jun 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ngopalak-redhat commented Jul 1, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

everettraven Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ngopalak-redhat commented Jun 17, 2025 •

edited

Loading

openshift-ci-robot commented Jun 17, 2025 •

edited by openshift-ci bot

Loading

everettraven Jul 1, 2025 •

edited

Loading