CORS-4235: Machine Management API Field & Feature Gate #10402
CORS-4235: Machine Management API Field & Feature Gate #10402patrickdillon wants to merge 2 commits intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@patrickdillon: This pull request references CORS-4235 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 story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
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. |
WalkthroughAdds a MachinePool Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.3)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
| // Using this field for control plane machines requires the ClusterAPIControlPlaneInstall feature gate. | ||
| // Using this field for compute machines requires the ClusterAPIComputeInstall feature gate. | ||
| // Supported platforms: aws | ||
| // Default is ClusterAPI. |
There was a problem hiding this comment.
once this gate is promoted we can remove these comments, but we will add platform-specific gates for new platforms.
|
CAPI compute PR is growing too big, so I will break it into smaller PRs. I can post the larger PR for reference, but I'm still working through some pieces. |
|
cc @damdo @JoelSpeed |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/types/machinepools.go`:
- Around line 132-141: The new Management field is incorrectly defaulting to
ClusterAPI; change its kubebuilder default so omitted values preserve today's
MachineAPI behavior by either removing the +kubebuilder:default=ClusterAPI line
or replacing it with +kubebuilder:default=MachineAPI for the Management
MachineManagementAPI `management` field in pkg/types/machinepools.go; ensure the
validation enum still allows both ClusterAPI and MachineAPI and that any related
logic/tests expecting MachineAPI remain unchanged.
In `@pkg/types/validation/featuregates.go`:
- Around line 48-51: The gate currently only inspects c.Compute[0] and reports a
non-indexed path; update the FeatureGateClusterAPIComputeInstall check to
iterate over all compute pools in c.Compute, trigger the condition if any pool's
Management equals types.ClusterAPI, and set the Field to an indexed path for the
offending pool (use the loop index to build a field path like
"compute[i].management" via
field.NewPath("compute").Index(i).Child("management") or equivalent). Ensure you
reference the existing symbols
FeatureGateName/features.FeatureGateClusterAPIComputeInstall,
Condition/c.Compute, types.ClusterAPI, and field.NewPath("compute",
"management") when making the change.
In `@pkg/types/validation/installconfig.go`:
- Line 1654: The error message for rejected ClusterAPI management feature-gate
configs is using a generic template that directs users to enable
TechPreviewNoUpgrade or CustomNoUpgrade; update the code path that appends
validateMachinePoolFeatureGates (the call to validateMachinePoolFeatureGates and
the template it returns) so that when the failing gate is the ClusterAPI
management gate it emits a gate-specific remediation message recommending
DevPreviewNoUpgrade (or the correct gate) instead of the shared
TechPreview/Custom suggestion; locate and adjust the message construction inside
validateMachinePoolFeatureGates (or the shared template function it calls) to
return the ClusterAPI-specific guidance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 63004412-efc9-4cf4-996c-f6da0433245e
📒 Files selected for processing (4)
pkg/types/machinepools.gopkg/types/validation/featuregate_test.gopkg/types/validation/featuregates.gopkg/types/validation/installconfig.go
| // Management is the API that will be used for machine management in the cluster. | ||
| // Using this field for control plane machines requires the ClusterAPIControlPlaneInstall feature gate. | ||
| // Using this field for compute machines requires the ClusterAPIComputeInstall feature gate. | ||
| // Supported platforms: aws | ||
| // Default is ClusterAPI. | ||
| // | ||
| // +kubebuilder:default=ClusterAPI | ||
| // +kubebuilder:validation:Enum=ClusterAPI;MachineAPI | ||
| // +optional | ||
| Management MachineManagementAPI `json:"management,omitempty"` |
There was a problem hiding this comment.
Don't default the new field to ClusterAPI.
ClusterAPI is the gated opt-in path introduced here. Making management default to ClusterAPI flips omitted values onto the new behavior instead of preserving today's MachineAPI behavior.
Suggested fix
- // Using this field for control plane machines requires the ClusterAPIControlPlaneInstall feature gate.
- // Using this field for compute machines requires the ClusterAPIComputeInstall feature gate.
+ // Setting this field to ClusterAPI for control plane machines requires the ClusterAPIControlPlaneInstall feature gate.
+ // Setting this field to ClusterAPI for compute machines requires the ClusterAPIComputeInstall feature gate.
// Supported platforms: aws
- // Default is ClusterAPI.
+ // Default is MachineAPI.
//
- // +kubebuilder:default=ClusterAPI
+ // +kubebuilder:default=MachineAPI🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/types/machinepools.go` around lines 132 - 141, The new Management field
is incorrectly defaulting to ClusterAPI; change its kubebuilder default so
omitted values preserve today's MachineAPI behavior by either removing the
+kubebuilder:default=ClusterAPI line or replacing it with
+kubebuilder:default=MachineAPI for the Management MachineManagementAPI
`management` field in pkg/types/machinepools.go; ensure the validation enum
still allows both ClusterAPI and MachineAPI and that any related logic/tests
expecting MachineAPI remain unchanged.
| { | ||
| FeatureGateName: features.FeatureGateClusterAPIComputeInstall, | ||
| Condition: len(c.Compute) > 0 && c.Compute[0].Management == types.ClusterAPI, | ||
| Field: field.NewPath("compute", "management"), |
There was a problem hiding this comment.
Check every compute pool and use an indexed field path.
management is per machine pool, but this only inspects c.Compute[0]. A later compute pool can set ClusterAPI without hitting the gate, and the error is reported as compute.management instead of compute[n].management.
Suggested fix
func validateMachinePoolFeatureGates(c *types.InstallConfig) []featuregates.GatedInstallConfigFeature {
- return []featuregates.GatedInstallConfigFeature{
+ gated := []featuregates.GatedInstallConfigFeature{
{
FeatureGateName: features.FeatureGateDualReplica,
Condition: c.ControlPlane != nil && c.ControlPlane.Fencing != nil,
Field: field.NewPath("platform", "none", "fencingCredentials"),
},
{
FeatureGateName: features.FeatureGateMultiDiskSetup,
Condition: c.ControlPlane != nil && len(c.ControlPlane.DiskSetup) != 0,
Field: field.NewPath("controlPlane", "diskSetup"),
},
{
FeatureGateName: features.FeatureGateMultiDiskSetup,
Condition: func() bool {
computeMachinePool := c.Compute
for _, compute := range computeMachinePool {
if len(compute.DiskSetup) != 0 {
return true
}
}
return false
}(),
Field: field.NewPath("compute", "diskSetup"),
},
{
FeatureGateName: features.FeatureGateOSStreams,
Condition: len(c.OSImageStream) != 0,
Field: field.NewPath("osImageStream"),
},
{
FeatureGateName: features.FeatureGateClusterAPIControlPlaneInstall,
Condition: c.ControlPlane != nil && c.ControlPlane.Management == types.ClusterAPI,
Field: field.NewPath("controlPlane", "management"),
},
- {
- FeatureGateName: features.FeatureGateClusterAPIComputeInstall,
- Condition: len(c.Compute) > 0 && c.Compute[0].Management == types.ClusterAPI,
- Field: field.NewPath("compute", "management"),
- },
}
+
+ for i := range c.Compute {
+ gated = append(gated, featuregates.GatedInstallConfigFeature{
+ FeatureGateName: features.FeatureGateClusterAPIComputeInstall,
+ Condition: c.Compute[i].Management == types.ClusterAPI,
+ Field: field.NewPath("compute").Index(i).Child("management"),
+ })
+ }
+
+ return gated
- }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/types/validation/featuregates.go` around lines 48 - 51, The gate
currently only inspects c.Compute[0] and reports a non-indexed path; update the
FeatureGateClusterAPIComputeInstall check to iterate over all compute pools in
c.Compute, trigger the condition if any pool's Management equals
types.ClusterAPI, and set the Field to an indexed path for the offending pool
(use the loop index to build a field path like "compute[i].management" via
field.NewPath("compute").Index(i).Child("management") or equivalent). Ensure you
reference the existing symbols
FeatureGateName/features.FeatureGateClusterAPIComputeInstall,
Condition/c.Compute, types.ClusterAPI, and field.NewPath("compute",
"management") when making the change.
| if c.ControlPlane != nil { | ||
| gatedFeatures = append(gatedFeatures, defaultsvalidation.GatedFeatures(c)...) | ||
| } | ||
| gatedFeatures = append(gatedFeatures, validateMachinePoolFeatureGates(c)...) |
There was a problem hiding this comment.
Use a gate-specific error message for ClusterAPI management.
These new gates are allowed by DevPreviewNoUpgrade in this PR, but the shared template below still tells users to enable TechPreviewNoUpgrade or CustomNoUpgrade. Rejected management: ClusterAPI configs will point people at the wrong remediation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/types/validation/installconfig.go` at line 1654, The error message for
rejected ClusterAPI management feature-gate configs is using a generic template
that directs users to enable TechPreviewNoUpgrade or CustomNoUpgrade; update the
code path that appends validateMachinePoolFeatureGates (the call to
validateMachinePoolFeatureGates and the template it returns) so that when the
failing gate is the ClusterAPI management gate it emits a gate-specific
remediation message recommending DevPreviewNoUpgrade (or the correct gate)
instead of the shared TechPreview/Custom suggestion; locate and adjust the
message construction inside validateMachinePoolFeatureGates (or the shared
template function it calls) to return the ClusterAPI-specific guidance.
There was a problem hiding this comment.
this is true, but this is an existing issue with the validation. not sure if we want to fix it in this scope
There was a problem hiding this comment.
@patrickdillon Fair point — since the template is shared across all gated features, fixing it cleanly would require per-gate message customization, which is broader than this PR's intent. Would you like me to open a follow-up GitHub issue to track improving the error message for ClusterAPI management feature-gate rejections?
Adds a management field to machinepools to allow toggling day-2 machine management between MachineAPI and ClusterAPI.
This commit adds validation to ensure that the appropriate feature gate is enabled to set the management field on the machine pool. For some reason the machine pool feature gate validation was in the defaults package, so this moves it to the validation package.
patrickdillon
force-pushed
the
mapicapi-ic
branch
from
860ee27 to
8109a03
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@data/data/install.openshift.io_installconfigs.yaml`:
- Around line 146-157: The schema currently sets an unconditional default of
"ClusterAPI" on the management field which is AWS-only and feature-gated; remove
the default entry from the management schema (delete the default: ClusterAPI
line) so the CRD does not auto-materialize an unsupported value, and make the
same removal in the arbiter, compute, and controlPlane management schemas
(ensure enum and type remain intact); retain any platform/feature-aware
defaulting logic in server-side or controller code rather than in the CRD
schema.
- Around line 146-157: The arbiter schema exposes the management field
(arbiter.management) but the description only mentions control-plane and compute
feature gates; either add explicit arbiter semantics to the description (state
whether arbiter supports ClusterAPI and/or MachineAPI, list supported platforms
for arbiter, and identify which feature gate applies for arbiter installation)
or remove the management field from the arbiter schema until arbiter behavior
and gate are defined; ensure the enum (ClusterAPI, MachineAPI) and type: string
remain consistent with the documented arbiter behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d1607282-fa69-4744-83de-a10c482ea042
📒 Files selected for processing (5)
data/data/install.openshift.io_installconfigs.yamlpkg/types/machinepools.gopkg/types/validation/featuregate_test.gopkg/types/validation/featuregates.gopkg/types/validation/installconfig.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/types/validation/installconfig.go
- pkg/types/machinepools.go
| management: | ||
| default: ClusterAPI | ||
| description: |- | ||
| Management is the API that will be used for machine management in the cluster. | ||
| Using this field for control plane machines requires the ClusterAPIControlPlaneInstall feature gate. | ||
| Using this field for compute machines requires the ClusterAPIComputeInstall feature gate. | ||
| Supported platforms: aws | ||
| Default is ClusterAPI. | ||
| enum: | ||
| - ClusterAPI | ||
| - MachineAPI | ||
| type: string |
There was a problem hiding this comment.
Don't make an AWS-only, gated path the unconditional schema default.
These blocks say management is AWS-only and feature-gated, but the schema still defaults it to ClusterAPI. A CRD default cannot express those conditions, so any consumer that honors schema defaults can materialize an unsupported/gated value outside the valid context.
Minimal safe fix
- default: ClusterAPI
description: |-
Management is the API that will be used for machine management in the cluster.
Using this field for control plane machines requires the ClusterAPIControlPlaneInstall feature gate.
Using this field for compute machines requires the ClusterAPIComputeInstall feature gate.
Supported platforms: aws
- Default is ClusterAPI.Apply the same change to the arbiter, compute, and controlPlane schemas, and keep any defaulting in code that can evaluate platform and feature-gate state.
Also applies to: 1718-1729, 3230-3241
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@data/data/install.openshift.io_installconfigs.yaml` around lines 146 - 157,
The schema currently sets an unconditional default of "ClusterAPI" on the
management field which is AWS-only and feature-gated; remove the default entry
from the management schema (delete the default: ClusterAPI line) so the CRD does
not auto-materialize an unsupported value, and make the same removal in the
arbiter, compute, and controlPlane management schemas (ensure enum and type
remain intact); retain any platform/feature-aware defaulting logic in
server-side or controller code rather than in the CRD schema.
Define arbiter semantics before exposing arbiter.management.
This field is published under arbiter, but the description only explains control-plane and compute gating. As written, the public schema does not tell users whether arbiter is supported here or which gate applies. Either document arbiter behavior explicitly or leave this field out of the arbiter schema for now.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@data/data/install.openshift.io_installconfigs.yaml` around lines 146 - 157,
The arbiter schema exposes the management field (arbiter.management) but the
description only mentions control-plane and compute feature gates; either add
explicit arbiter semantics to the description (state whether arbiter supports
ClusterAPI and/or MachineAPI, list supported platforms for arbiter, and identify
which feature gate applies for arbiter installation) or remove the management
field from the arbiter schema until arbiter behavior and gate are defined;
ensure the enum (ClusterAPI, MachineAPI) and type: string remain consistent with
the documented arbiter behavior.
|
@patrickdillon: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. |
|
Changes here LGTM |
3 participants
Adds an install config field to allow users to specify day-2 machine management API. Also adds feature gates to limit usage of this field.
An example install config would look like: