CNTRLPLANE-3160: Drop AutoNodeKarpenter feature gate and promote EC2NodeClass to v1#8166
CNTRLPLANE-3160: Drop AutoNodeKarpenter feature gate and promote EC2NodeClass to v1#8166enxebre wants to merge 6 commits intoopenshift:mainfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
Skipping CI for Draft Pull Request. |
|
@enxebre: This pull request references CNTRLPLANE-3160 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 epic 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. |
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: enxebre The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/label tide/merge-method-squash |
9b013f2 to
ca02ee4
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8166 +/- ##
==========================================
+ Coverage 32.17% 33.89% +1.71%
==========================================
Files 766 768 +2
Lines 91968 93161 +1193
==========================================
+ Hits 29592 31575 +1983
+ Misses 59843 58927 -916
- Partials 2533 2659 +126
🚀 New features to boost your workflow:
|
|
/test e2e-aws |
Test Resultse2e-aws
|
ca02ee4 to
a8d9bd3
Compare
|
/test e2e-aws |
1 similar comment
|
/test e2e-aws |
|
/assign @jkyros @JoelSpeed |
jparrill
left a comment
There was a problem hiding this comment.
Dropped some comments, mostly to learn. 🙏
| // +openshift:enable:FeatureGate=AutoNodeKarpenter | ||
| // +optional | ||
| AutoNode *AutoNode `json:"autoNode,omitempty"` | ||
| AutoNode AutoNode `json:"autoNode,omitzero"` |
There was a problem hiding this comment.
Just to know more, is this change expected by promoting a featureGate or this is due to the new api linter?
There was a problem hiding this comment.
is expected that any API follow best practices, specially if it's GA. The linter enforces those best practices.
| // +optional | ||
| // +unionMember | ||
| AWS *KarpenterAWSConfig `json:"aws,omitempty"` | ||
| AWS KarpenterAWSConfig `json:"aws,omitzero"` |
| text: 'arrayofstruct: OpenStackPlatformSpec.Subnets is an array of structs, but the struct has no required fields. At least one field should be marked as required to prevent ambiguous YAML configurations' | ||
| - linters: | ||
| - kubeapilinter | ||
| path: karpenter/v1beta1/karpenter_types.go |
There was a problem hiding this comment.
Should we add v1 to this path?
There was a problem hiding this comment.
the exception is not actually needed, the struct already has // +kubebuilder:validation:MinProperties=1
There was a problem hiding this comment.
this triggered an offline discussion for me with Joel. This would still hit the linter on a new field or a field change. "Not having a specific requirement but at least one" is not an ideal choice when the struct belongs to a slice. The reasoning is that with all fields optional, a user could accidentally write two separate items thinking they're writing one (or the other way around). I think we'll accepted that here.
There was a problem hiding this comment.
To put the slice thing into an example
- foo: some
- bar: thing
vs
- foo: some
bar: thing
Is a very subtle difference that you might glance over, but has a very different meaning to the API.
This has caught people out in security contexts before with pretty nasty results
| @@ -0,0 +1,261 @@ | |||
| apiVersion: apiextensions.k8s.io/v1 | |||
There was a problem hiding this comment.
It's there any docs to know how to add more testCases to this suite/framework?
There was a problem hiding this comment.
| // +groupName=karpenter.hypershift.openshift.io | ||
| // +k8s:openapi-gen=true | ||
| package v1beta1 | ||
| package v1 |
There was a problem hiding this comment.
By doing this as a rename, rather than creating a duplicate, anyone currently vendoring HyperShift cannot safely have an intermediate step where they update their HyperShift dependency, are able to compile, and then move over to the v1 version of this API.
We would normally expect to create a v1 dir alongside the v1alpha1 (or beta in this case)
A secondary benefit of that is then we get a full diff of the API being promoted and can give it a more thorough API review
| text: 'arrayofstruct: OpenStackPlatformSpec.Subnets is an array of structs, but the struct has no required fields. At least one field should be marked as required to prevent ambiguous YAML configurations' | ||
| - linters: | ||
| - kubeapilinter | ||
| path: karpenter/v1beta1/karpenter_types.go |
There was a problem hiding this comment.
To put the slice thing into an example
- foo: some
- bar: thing
vs
- foo: some
bar: thing
Is a very subtle difference that you might glance over, but has a very different meaning to the API.
This has caught people out in security contexts before with pretty nasty results
| if hostedCluster.Spec.AutoNode == nil || | ||
| hostedCluster.Spec.AutoNode.Provisioner.Karpenter == nil || | ||
| hostedCluster.Spec.AutoNode.Provisioner.Karpenter.AWS == nil { | ||
| if hostedCluster.Spec.AutoNode.Provisioner.Karpenter.AWS == (hyperv1.KarpenterAWSConfig{}) { |
There was a problem hiding this comment.
comparing to an empty struct doesn't seems right. Should we check hostedCluster.Spec.AutoNode.Provisioner.Karpenter.Platform == AWS instead?
There was a problem hiding this comment.
Agree, use a discriminator field if it exists
Remove the AutoNodeKarpenter feature gate to promote the autoNode field to stable/GA. The autoNode field on HostedCluster and HostedControlPlane spec/status is no longer gated behind TechPreviewNoUpgrade. Updates CLI and test code to use value types instead of pointers for the now-ungated AutoNode, KarpenterConfig, and KarpenterAWSConfig structs. Removes the TECH_PREVIEW_NO_UPGRADE skip from the karpenter e2e test. No runtime code changes are needed because all controllers use IsKarpenterEnabled() which checks spec values, not the feature gate. Ref: CNTRLPLANE-3160
Replace api/karpenter/v1beta1/ with api/karpenter/v1/ to signal API stability as part of the AutoNode GA promotion. The types, structs, and validation markers are unchanged - only the API version is bumped. All source files importing api/karpenter/v1beta1 are updated to api/karpenter/v1. The CRD will now have v1 as the served and storage version. Since the karpenter-operator controls the CRD lifecycle (not the hypershift install), there is no multi-version serving concern. Also fixes a godoc comment on the Conditions field and removes the now-stale v1beta1 lint suppression from .golangci.yml. Ref: CNTRLPLANE-3160
Add two new envtest test suites: - stable.hostedclusters.karpenter.testsuite.yaml: 6 tests covering HostedCluster autoNode CEL validation (provisioner union, platform union, enum validation, roleARN validation) - stable.openshiftec2nodeclasses.testsuite.yaml: 18 tests covering OpenshiftEC2NodeClass v1 CEL validation (subnet/SG selectors, tags, block device mappings, version, capacity reservations, metadata) The karpenter EC2NodeClass tests are self-contained under karpenter-operator/controllers/karpenter/assets/ with their own zz_generated.crd-manifests/ directory. The Makefile karpenter-api target copies the CRD there, and suite_test.go loads both asset dirs. Ref: CNTRLPLANE-3160
Regenerate all auto-generated artifacts after dropping the AutoNodeKarpenter feature gate and promoting OpenshiftEC2NodeClass to v1: - CRD manifests: autoNode fields merged into AAA_ungated.yaml, AutoNodeKarpenter.yaml deleted - Featuregate payload manifests updated - Client code regenerated for karpenter v1 - Vendor directory synced - API docs regenerated Ref: CNTRLPLANE-3160
Address PR review feedback: check the platform discriminator field rather than comparing against an empty struct to determine if Karpenter AWS is configured. Ref: CNTRLPLANE-3160
df67072 to
dc1cc61
Compare
|
/test e2e-aws |
|
/test e2e-aws |
Add karpenter-operator/controllers/karpenter/assets/tests/** to the path filters so that changes to the karpenter envtest suites trigger the envtest workflows. Also fixes the path for the crds/ directory rename. Ref: CNTRLPLANE-3160
|
PR needs rebase. 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. |
|
@enxebre: The following test failed, say
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. |
|
I have all the evidence needed. Here is the analysis: Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe job failed during the Prow git clone phase — before any CI step could execute. When Prow attempted to merge PR #8166 (SHA Root CauseThe PR branch
The base commit on Recommendations
Evidence
|
What this PR does / why we need it:
Promotes the AutoNode/Karpenter feature to stable/GA by:
AutoNodeKarpenterfeature gate — theautoNodefield on HostedCluster and HostedControlPlane is no longer gated behind TechPreviewNoUpgradeNo runtime/controller code changes — all controllers use
IsKarpenterEnabled()which checks spec values, not the feature gate.Which issue(s) this PR fixes:
Fixes https://issues.redhat.com/browse/CNTRLPLANE-3160
Special notes for your reviewer:
karpenter-operator/controllers/karpenter/assets/with their ownzz_generated.crd-manifests/andtests/directories. The only integration point is adding a second path toLoadTestSuiteSpecsintest/envtest/suite_test.go.cmd/install/install.go) is unaffected — the karpenter CRD is not placed in the hypershift-operator install assets.Checklist:
🤖 Generated with Claude Code via
/jira:solve [CNTRLPLANE-3160](https://redhat.atlassian.net/browse/CNTRLPLANE-3160)