AUTOSCALE-491: OpenshiftNodePool CRD for AutoNode #7481
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@maxcao13: This pull request references AUTOSCALE-491 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. |
|
/test e2e-aws-autonode |
WalkthroughAdds an OpenshiftNodePool CRD and full client/informer/lister/applyconfiguration scaffolding, a NodePoolReconciler controller (with tests), CRD assets, and related wiring; exports a GlobalPS label constant and updates operatorpkg and Kubernetes-related module versions. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes ✨ Finishing touches
Comment |
openshift-ci
bot
added
the
area/api
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: maxcao13 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 |
openshift-ci
bot
added
area/control-plane-operator
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@karpenter-operator/controllers/nodepool/nodepool_controller.go`:
- Around line 143-149: The error message in the Patch call refers to
openshiftEC2NodeClass but the code is removing a finalizer from
openshiftNodePool; update the fmt.Errorf message in the error return of the
r.guestClient.Patch call to reference openshiftNodePool (or a generic
"OpenshiftNodePool") so it correctly identifies the resource being patched; the
change should be made where controllerutil.ContainsFinalizer(openshiftNodePool,
finalizer) then controllerutil.RemoveFinalizer(openshiftNodePool, finalizer) and
the subsequent r.guestClient.Patch(...) error is returned.
- Around line 82-88: getHCP currently returns a generic fmt.Errorf when no
HostedControlPlane is found, so apierrors.IsNotFound(err) in the reconciler will
never match; change getHCP to return a Kubernetes NotFound error (use
apierrors.NewNotFound with schema.GroupResource{Group:
"hypershift.openshift.io", Resource: "hostedcontrolplanes"} and the HCP name)
instead of fmt.Errorf, and add the necessary imports
(k8s.io/apimachinery/pkg/api/errors and k8s.io/apimachinery/pkg/runtime/schema)
so the existing IsNotFound check in the caller works as intended.
🧹 Nitpick comments (3)
karpenter-operator/controllers/nodepool/nodepool_controller_test.go (1)
268-292: Potential test flakiness withmetav1.Now()in condition comparison.The test uses
metav1.Now()forLastTransitionTimein bothnodePoolStatus(line 275) andexpectedStatus(line 286). Since these are evaluated at different times during test setup, the timestamps will differ slightly, which could cause intermittent failures if the test ever comparesLastTransitionTimedirectly.The current assertions (lines 330-335) avoid comparing
LastTransitionTimedirectly, which mitigates this. However, for clarity and to prevent future regressions, consider using a fixed timestamp:🔧 Suggested improvement
+ fixedTime := metav1.Now() { name: "When NodePool status has conditions they should be mirrored to OpenshiftNodePool", nodePoolStatus: karpenterv1.NodePoolStatus{ Conditions: []status.Condition{ { Type: string(karpenterv1.ConditionTypeValidationSucceeded), Status: metav1.ConditionTrue, - LastTransitionTime: metav1.Now(), + LastTransitionTime: fixedTime, Reason: "Valid", Message: "NodePool is valid", }, }, }, expectedStatus: hyperkarpenterv1.OpenshiftNodePoolStatus{ Conditions: []status.Condition{ { Type: string(karpenterv1.ConditionTypeValidationSucceeded), Status: metav1.ConditionTrue, - LastTransitionTime: metav1.Now(), + LastTransitionTime: fixedTime, Reason: "Valid", Message: "NodePool is valid", }, }, }, },karpenter-operator/controllers/nodepool/nodepool_controller.go (2)
174-206: Loggingopwhen it may be uninitialized.When
onlyCreateis true,opis never assigned, so line 203 logs a zero-valueOperationResult. Consider logging only whenopis meaningful, or restructuring the log statement.Proposed fix
- log.Info("Reconciled CRDs", "op", op) + if !onlyCreate { + log.Info("Reconciled CRDs", "op", op) + } else { + log.Info("Reconciled CRDs") + }
331-341: Consider adding context if multiple HCPs could exist.The function returns the first HCP found without warning if multiple exist. If this is expected to be a single-HCP namespace, this is fine; otherwise, consider logging a warning or returning an error for ambiguous cases.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (14)
api/go.sumis excluded by!**/*.sumapi/vendor/github.com/awslabs/operatorpkg/status/condition_set.gois excluded by!**/vendor/**api/vendor/github.com/awslabs/operatorpkg/status/controller.gois excluded by!**/vendor/**api/vendor/github.com/awslabs/operatorpkg/status/unstructured_adapter.gois excluded by!**/vendor/**api/vendor/modules.txtis excluded by!**/vendor/**go.sumis excluded by!**/*.sumvendor/github.com/awslabs/operatorpkg/status/condition_set.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/awslabs/operatorpkg/status/controller.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/awslabs/operatorpkg/status/unstructured_adapter.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/karpenter/v1beta1/karpenter_types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/karpenter/v1beta1/openshiftnodepool_types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/karpenter/v1beta1/register.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/karpenter/v1beta1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**vendor/modules.txtis excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (30)
api/go.modapi/karpenter/v1beta1/karpenter_types.goapi/karpenter/v1beta1/openshiftnodepool_types.goapi/karpenter/v1beta1/register.goapi/karpenter/v1beta1/zz_generated.deepcopy.goclient/applyconfiguration/karpenter/v1beta1/openshiftnodepool.goclient/applyconfiguration/karpenter/v1beta1/openshiftnodepoolspec.goclient/applyconfiguration/karpenter/v1beta1/openshiftnodepoolstatus.goclient/applyconfiguration/utils.goclient/clientset/clientset/typed/karpenter/v1beta1/fake/fake_karpenter_client.goclient/clientset/clientset/typed/karpenter/v1beta1/fake/fake_openshiftnodepool.goclient/clientset/clientset/typed/karpenter/v1beta1/generated_expansion.goclient/clientset/clientset/typed/karpenter/v1beta1/karpenter_client.goclient/clientset/clientset/typed/karpenter/v1beta1/openshiftnodepool.goclient/informers/externalversions/generic.goclient/informers/externalversions/karpenter/v1beta1/interface.goclient/informers/externalversions/karpenter/v1beta1/openshiftnodepool.goclient/listers/karpenter/v1beta1/expansion_generated.goclient/listers/karpenter/v1beta1/openshiftnodepool.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/globalps/globalps.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/globalps/globalps_test.gogo.modkarpenter-operator/controllers/karpenter/assets/karpenter.hypershift.openshift.io_openshiftec2nodeclasses.yamlkarpenter-operator/controllers/karpenter/assets/karpenter.hypershift.openshift.io_openshiftnodepools.yamlkarpenter-operator/controllers/karpenter/karpenter_controller.gokarpenter-operator/controllers/nodeclass/ec2_nodeclass_controller.gokarpenter-operator/controllers/nodepool/nodepool_controller.gokarpenter-operator/controllers/nodepool/nodepool_controller_test.gokarpenter-operator/main.gotest/e2e/assets/karpenter-nodepool.yaml
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
client/clientset/clientset/typed/karpenter/v1beta1/generated_expansion.goapi/karpenter/v1beta1/karpenter_types.goclient/informers/externalversions/generic.goclient/listers/karpenter/v1beta1/expansion_generated.gokarpenter-operator/controllers/karpenter/karpenter_controller.goclient/clientset/clientset/typed/karpenter/v1beta1/karpenter_client.goclient/informers/externalversions/karpenter/v1beta1/interface.goclient/applyconfiguration/utils.gokarpenter-operator/controllers/nodeclass/ec2_nodeclass_controller.gogo.modclient/applyconfiguration/karpenter/v1beta1/openshiftnodepoolstatus.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/globalps/globalps_test.goclient/listers/karpenter/v1beta1/openshiftnodepool.gokarpenter-operator/controllers/nodepool/nodepool_controller_test.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/globalps/globalps.goapi/karpenter/v1beta1/zz_generated.deepcopy.goclient/clientset/clientset/typed/karpenter/v1beta1/fake/fake_openshiftnodepool.goclient/clientset/clientset/typed/karpenter/v1beta1/fake/fake_karpenter_client.gokarpenter-operator/controllers/karpenter/assets/karpenter.hypershift.openshift.io_openshiftec2nodeclasses.yamlclient/clientset/clientset/typed/karpenter/v1beta1/openshiftnodepool.goclient/applyconfiguration/karpenter/v1beta1/openshiftnodepool.goclient/applyconfiguration/karpenter/v1beta1/openshiftnodepoolspec.goclient/informers/externalversions/karpenter/v1beta1/openshiftnodepool.gokarpenter-operator/main.goapi/karpenter/v1beta1/openshiftnodepool_types.gokarpenter-operator/controllers/nodepool/nodepool_controller.goapi/karpenter/v1beta1/register.goapi/go.modkarpenter-operator/controllers/karpenter/assets/karpenter.hypershift.openshift.io_openshiftnodepools.yamltest/e2e/assets/karpenter-nodepool.yaml
🧬 Code graph analysis (14)
client/informers/externalversions/generic.go (1)
api/auditlogpersistence/v1alpha1/register.go (1)
SchemeGroupVersion(12-12)
karpenter-operator/controllers/karpenter/karpenter_controller.go (1)
api/karpenter/v1beta1/openshiftnodepool_types.go (1)
OpenshiftNodePoolList(66-70)
client/clientset/clientset/typed/karpenter/v1beta1/karpenter_client.go (1)
client/clientset/clientset/typed/karpenter/v1beta1/openshiftnodepool.go (2)
OpenshiftNodePoolsGetter(34-36)OpenshiftNodePoolInterface(39-54)
client/informers/externalversions/karpenter/v1beta1/interface.go (1)
client/informers/externalversions/karpenter/v1beta1/openshiftnodepool.go (1)
OpenshiftNodePoolInformer(36-39)
client/applyconfiguration/karpenter/v1beta1/openshiftnodepoolstatus.go (2)
test/e2e/util/eventually.go (2)
Conditions(435-506)Condition(415-420)api/karpenter/v1beta1/openshiftnodepool_types.go (1)
OpenshiftNodePoolStatus(13-20)
control-plane-operator/hostedclusterconfigoperator/controllers/globalps/globalps_test.go (1)
control-plane-operator/hostedclusterconfigoperator/controllers/globalps/globalps.go (1)
GlobalPSLabelKey(30-30)
client/listers/karpenter/v1beta1/openshiftnodepool.go (3)
api/karpenter/v1beta1/openshiftnodepool_types.go (1)
OpenshiftNodePool(55-62)client/listers/karpenter/v1beta1/expansion_generated.go (2)
OpenshiftNodePoolListerExpansion(30-30)OpenshiftNodePoolNamespaceListerExpansion(34-34)api/karpenter/v1beta1/register.go (1)
Resource(18-20)
karpenter-operator/controllers/nodepool/nodepool_controller_test.go (2)
api/karpenter/v1beta1/openshiftnodepool_types.go (3)
OpenshiftNodePoolSpec(23-40)OpenshiftNodePool(55-62)OpenshiftNodePoolStatus(13-20)control-plane-operator/hostedclusterconfigoperator/controllers/globalps/globalps.go (1)
GlobalPSLabelKey(30-30)
api/karpenter/v1beta1/zz_generated.deepcopy.go (1)
api/karpenter/v1beta1/openshiftnodepool_types.go (4)
OpenshiftNodePool(55-62)OpenshiftNodePoolList(66-70)OpenshiftNodePoolSpec(23-40)OpenshiftNodePoolStatus(13-20)
client/clientset/clientset/typed/karpenter/v1beta1/fake/fake_karpenter_client.go (1)
client/clientset/clientset/typed/karpenter/v1beta1/openshiftnodepool.go (1)
OpenshiftNodePoolInterface(39-54)
client/applyconfiguration/karpenter/v1beta1/openshiftnodepoolspec.go (1)
api/karpenter/v1beta1/openshiftnodepool_types.go (1)
OpenshiftNodePoolSpec(23-40)
client/informers/externalversions/karpenter/v1beta1/openshiftnodepool.go (4)
client/listers/karpenter/v1beta1/openshiftnodepool.go (2)
OpenshiftNodePoolLister(29-36)NewOpenshiftNodePoolLister(44-46)client/informers/externalversions/karpenter/v1beta1/interface.go (1)
Interface(25-30)api/karpenter/v1beta1/openshiftnodepool_types.go (1)
OpenshiftNodePool(55-62)client/applyconfiguration/karpenter/v1beta1/openshiftnodepool.go (1)
OpenshiftNodePool(37-44)
karpenter-operator/main.go (1)
karpenter-operator/controllers/nodepool/nodepool_controller.go (1)
NodePoolReconciler(45-51)
api/karpenter/v1beta1/register.go (2)
api/karpenter/v1beta1/openshiftnodepool_types.go (2)
OpenshiftNodePool(55-62)OpenshiftNodePoolList(66-70)client/applyconfiguration/karpenter/v1beta1/openshiftnodepool.go (1)
OpenshiftNodePool(37-44)
🪛 golangci-lint (2.5.0)
karpenter-operator/controllers/nodepool/nodepool_controller_test.go
[error] 228-228: : # github.com/openshift/hypershift/sync-global-pullsecret [github.com/openshift/hypershift/sync-global-pullsecret.test]
sync-global-pullsecret/sync-global-pullsecret_test.go:228:23: undefined: MockdbusConn
sync-global-pullsecret/sync-global-pullsecret_test.go:234:26: undefined: MockdbusConn
sync-global-pullsecret/sync-global-pullsecret_test.go:247:26: undefined: MockdbusConn
sync-global-pullsecret/sync-global-pullsecret_test.go:257:26: undefined: MockdbusConn
sync-global-pullsecret/sync-global-pullsecret_test.go:270:26: undefined: MockdbusConn
sync-global-pullsecret/sync-global-pullsecret_test.go:283:26: undefined: MockdbusConn
sync-global-pullsecret/sync-global-pullsecret_test.go:296:26: undefined: MockdbusConn
sync-global-pullsecret/sync-global-pullsecret_test.go:309:26: undefined: MockdbusConn
sync-global-pullsecret/sync-global-pullsecret_test.go:327:12: undefined: NewMockdbusConn
(typecheck)
client/applyconfiguration/karpenter/v1beta1/openshiftnodepool.go
[error] 228-228: : # github.com/openshift/hypershift/sync-global-pullsecret [github.com/openshift/hypershift/sync-global-pullsecret.test]
sync-global-pullsecret/sync-global-pullsecret_test.go:228:23: undefined: MockdbusConn
sync-global-pullsecret/sync-global-pullsecret_test.go:234:26: undefined: MockdbusConn
sync-global-pullsecret/sync-global-pullsecret_test.go:247:26: undefined: MockdbusConn
sync-global-pullsecret/sync-global-pullsecret_test.go:257:26: undefined: MockdbusConn
sync-global-pullsecret/sync-global-pullsecret_test.go:270:26: undefined: MockdbusConn
sync-global-pullsecret/sync-global-pullsecret_test.go:283:26: undefined: MockdbusConn
sync-global-pullsecret/sync-global-pullsecret_test.go:296:26: undefined: MockdbusConn
sync-global-pullsecret/sync-global-pullsecret_test.go:309:26: undefined: MockdbusConn
sync-global-pullsecret/sync-global-pullsecret_test.go:327:12: undefined: NewMockdbusConn
(typecheck)
karpenter-operator/controllers/nodepool/nodepool_controller.go
[error] 228-228: : # github.com/openshift/hypershift/sync-global-pullsecret [github.com/openshift/hypershift/sync-global-pullsecret.test]
sync-global-pullsecret/sync-global-pullsecret_test.go:228:23: undefined: MockdbusConn
sync-global-pullsecret/sync-global-pullsecret_test.go:234:26: undefined: MockdbusConn
sync-global-pullsecret/sync-global-pullsecret_test.go:247:26: undefined: MockdbusConn
sync-global-pullsecret/sync-global-pullsecret_test.go:257:26: undefined: MockdbusConn
sync-global-pullsecret/sync-global-pullsecret_test.go:270:26: undefined: MockdbusConn
sync-global-pullsecret/sync-global-pullsecret_test.go:283:26: undefined: MockdbusConn
sync-global-pullsecret/sync-global-pullsecret_test.go:296:26: undefined: MockdbusConn
sync-global-pullsecret/sync-global-pullsecret_test.go:309:26: undefined: MockdbusConn
sync-global-pullsecret/sync-global-pullsecret_test.go:327:12: undefined: NewMockdbusConn
(typecheck)
🔇 Additional comments (37)
karpenter-operator/controllers/karpenter/assets/karpenter.hypershift.openshift.io_openshiftec2nodeclasses.yaml (1)
47-49: LGTM!Good fix correcting the spec description to reference the correct resource type (
OpenshiftEC2NodeClass) instead of the previously inconsistent reference.control-plane-operator/hostedclusterconfigoperator/controllers/globalps/globalps.go (3)
30-30: LGTM!The constant is appropriately exported with a clear, descriptive name following Go naming conventions. The value remains unchanged.
252-255: LGTM!Consistent usage of the exported constant for node label management.
293-295: LGTM!The nodeSelector correctly uses the exported constant, ensuring DaemonSet pods only schedule on nodes labeled by
labelNodesForGlobalPullSecret.control-plane-operator/hostedclusterconfigoperator/controllers/globalps/globalps_test.go (1)
584-586: LGTM!The test correctly uses the exported
GlobalPSLabelKeyconstant to verify node labeling behavior, maintaining consistency with the implementation.api/karpenter/v1beta1/karpenter_types.go (1)
62-64: LGTM!The documentation comment now correctly references
OpenshiftEC2NodeClass, matching the type it describes.client/clientset/clientset/typed/karpenter/v1beta1/generated_expansion.go (1)
21-22: LGTM!Generated expansion interface correctly follows the existing pattern for the new
OpenshiftNodePoolresource.api/go.mod (2)
80-81: Transitive dependency updates.The
k8s.io/cloud-providerandk8s.io/csi-translation-libupdates to v0.32.3 appear to be indirect dependency alignment. Ensure these are compatible with the primary k8s.io dependencies at v0.34.2.
7-11: Manual verification required for new direct dependencies in api/go.mod.Verify the following before approving:
operatorpkgis actually imported and used for status/conditions handling in OpenshiftNodePool typeskarpenteris actually imported and used for NodePool spec embedding- Both are required as direct dependencies (not already provided transitively)
- Karpenter pseudo-version
v1.2.1-0.20250212185021-45f73ec7a790is a valid commitclient/listers/karpenter/v1beta1/expansion_generated.go (1)
27-34: LGTM!Generated lister expansion interfaces correctly follow the existing pattern, providing extension points for both
OpenshiftNodePoolListerandOpenshiftNodePoolNamespaceLister.client/clientset/clientset/typed/karpenter/v1beta1/openshiftnodepool.go (1)
1-73: LGTM!This is correctly generated client-gen code following standard Kubernetes client-go patterns for the new OpenshiftNodePool resource.
api/karpenter/v1beta1/register.go (1)
32-33: LGTM!The new OpenshiftNodePool types are correctly registered following the same pattern as the existing OpenshiftEC2NodeClass types.
karpenter-operator/main.go (1)
133-138: LGTM!The NodePoolReconciler is correctly wired following the same pattern as existing reconcilers (EC2NodeClassReconciler). Error handling is consistent with the established conventions.
client/informers/externalversions/generic.go (1)
83-84: LGTM!The new OpenshiftNodePools informer case correctly follows the existing pattern for openshiftec2nodeclasses.
karpenter-operator/controllers/nodeclass/ec2_nodeclass_controller.go (1)
71-83: LGTM!Good improvements:
- Explicit controller naming with
Named("ec2nodeclass")improves clarity and distinguishes this controller from the new NodePoolReconciler.- Using
GetName()on the CRD objects instead of hardcoded strings is more maintainable and reduces the risk of name mismatches.client/clientset/clientset/typed/karpenter/v1beta1/fake/fake_karpenter_client.go (1)
34-36: LGTM!The generated fake client method follows the established pattern and correctly implements the
OpenshiftNodePoolsGetterinterface.client/informers/externalversions/karpenter/v1beta1/interface.go (1)
28-29: LGTM!The generated informer interface extension and implementation follow the established patterns.
Also applies to: 47-51
client/clientset/clientset/typed/karpenter/v1beta1/fake/fake_openshiftnodepool.go (1)
1-52: LGTM!The generated fake client implementation for
OpenshiftNodePoolfollows the standardclient-gopatterns with proper resource/kind registration and type factory functions.client/clientset/clientset/typed/karpenter/v1beta1/karpenter_client.go (1)
31-31: LGTM!The generated client properly extends the interface with
OpenshiftNodePoolsGetterand implements the accessor method following the established pattern.Also applies to: 43-45
karpenter-operator/controllers/karpenter/karpenter_controller.go (1)
181-201: LGTM - Correctly updated to use OpenshiftNodePoolList.The deletion flow properly handles the new
OpenshiftNodePoolresources. TheOpenshiftNodePoolcontroller explicitly deletes the underlying KarpenterNodePoolduring reconciliation (nodepool_controller.go lines 133-141), and the existingNodeClaimwait (lines 203-211) ensures all dependent resources are cleaned up before removing the finalizer.client/applyconfiguration/karpenter/v1beta1/openshiftnodepoolspec.go (1)
1-69: LGTM!This is properly generated apply-configuration code that correctly mirrors the
OpenshiftNodePoolSpectype fromapi/karpenter/v1beta1/openshiftnodepool_types.go. The fluent builder pattern is consistent with other apply configurations in this codebase.karpenter-operator/controllers/nodepool/nodepool_controller_test.go (1)
27-241: LGTM!The test cases for
TestReconcileNodePoolare comprehensive and well-structured:
- Validates spec mirroring with GlobalPullSecret label injection
- Tests NodeClassRef translation from
OpenshiftEC2NodeClasstoEC2NodeClass- Verifies OwnerReferences are correctly set
Good coverage of the core reconciliation logic.
client/applyconfiguration/utils.go (1)
368-373: LGTM!The new switch cases correctly map the
OpenshiftNodePool,OpenshiftNodePoolSpec, andOpenshiftNodePoolStatuskinds to their respective apply configuration types, following the same pattern as other karpenter types in this generated file.test/e2e/assets/karpenter-nodepool.yaml (1)
1-22: LGTM!The E2E test asset correctly migrates to the new
OpenshiftNodePoolAPI:
- Uses the new
karpenter.hypershift.openshift.io/v1beta1API version- References
OpenshiftEC2NodeClasswhich will be translated toEC2NodeClassby the controller- The removal of the
hypershift.openshift.io/nodepool-globalps-enabledlabel is correct since the controller automatically injects it during reconciliation (as verified by the unit tests)karpenter-operator/controllers/karpenter/assets/karpenter.hypershift.openshift.io_openshiftnodepools.yaml (2)
1-474: LGTM overall - CRD is well-structured.The CRD is comprehensive with:
- Appropriate validation rules using CEL expressions
- Immutability constraints on
nodeClassRef.groupandnodeClassRef.kindto prevent drift- Well-defined disruption budgets with schedule/duration validation
- Proper status subresource enabled
The schema aligns with the Go types defined in
api/karpenter/v1beta1/openshiftnodepool_types.go.
24-26: Remove or fix theNodesprinter column—status.resources.nodesis not a valid Kubernetes resource key.The
Resourcesfield is a standardcorev1.ResourceList, which uses Kubernetes resource names likecpu,memory, andpods. The keynodesis non-standard and is never populated, so theNodescolumn will always be empty. Either remove this column or change the jsonPath to reference a valid resource or a different field.Likely an incorrect or invalid review comment.
client/applyconfiguration/karpenter/v1beta1/openshiftnodepoolstatus.go (1)
1-54: LGTM!Generated apply-configuration code follows standard patterns and correctly mirrors the
OpenshiftNodePoolStatustype definition.client/listers/karpenter/v1beta1/openshiftnodepool.go (1)
1-69: LGTM!Generated lister code follows standard Kubernetes client patterns.
api/karpenter/v1beta1/zz_generated.deepcopy.go (1)
264-379: LGTM!Generated deepcopy implementations correctly handle all field types including maps, slices, and pointers.
client/informers/externalversions/karpenter/v1beta1/openshiftnodepool.go (1)
1-101: LGTM!Generated informer code follows standard Kubernetes informer patterns with both context-aware and legacy list/watch functions.
api/karpenter/v1beta1/openshiftnodepool_types.go (1)
1-70: Verify generated client code aligns with cluster scope.The type definitions are clean and properly leverage Karpenter types. However, the CRD is marked as
scope=Cluster(line 44), but the generated lister and informer code include namespace parameters—for example,OpenshiftNodePools(namespace string)in the lister interface andclient.KarpenterV1beta1().OpenshiftNodePools(namespace)in the informer. For cluster-scoped resources, generated client code typically omits namespace handling.Note that OpenshiftEC2NodeClass, also marked
scope=Clusterin the same package, exhibits the same namespace parameter pattern. This suggests either the code generation was not run after setting the scope markers, or the lister-gen tool in use does not respect thescope=Clusterannotation. Confirm whether code generation needs to be re-run or clarify if passing empty strings for namespace at runtime is the intended behavior.karpenter-operator/controllers/nodepool/nodepool_controller.go (5)
1-43: LGTM!Imports, constants, and CRD variable declarations follow standard patterns for controller-runtime based operators.
45-76: LGTM!The reconciler setup correctly initializes both management and guest clients, applies CRDs and VAP on startup, and watches both
OpenshiftNodePoolandNodePoolresources for bidirectional synchronization.
208-233: LGTM!The field mapping from
OpenshiftNodePooltoNodePoolis clear, and the automatic injection of the GlobalPullSecret label and NodeClass reference translation are well-documented.
235-252: LGTM!Status mirroring from
NodePoolback toOpenshiftNodePoolwith conditional patching based onreflect.DeepEqualis a clean approach.
254-329: LGTM!The ValidatingAdmissionPolicy correctly blocks direct manipulation of
NodePoolresources by users while allowing the controller (running assystem:hosted-cluster-config) to manage them throughOpenshiftNodePool.client/applyconfiguration/karpenter/v1beta1/openshiftnodepool.go (1)
1-241: LGTM!This is correctly generated apply-configuration code following standard Kubernetes client-go patterns. The fluent builder methods and lazy initialization are consistent with other generated apply configurations.
Note: The static analysis hint about
MockdbusConnat line 228 references a different file (sync-global-pullsecret_test.go) and is unrelated to this file.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
openshift-merge-robot
added
the
needs-rebase
This commit introduces the OpenshiftNodePool CRD. The CRD acts as the de-facto interface for cluster-users interacting with AutoNode NodePools. Similar to OpenshiftEC2NodeClass, the creation of a ONP will render a Karpenter NodePool, protected with a VAP. The ValidatingAdmissionPolicy in the guest cluster will be constantly reconciled to mitigate deletion race conditions. This allows us as a cluster service provider to: - set enforcable/immutable fields and restrictions on NodePools - hide fields that cluster-users should not touch - add OpenShift-specific fields as a user interface Signed-off-by: Max Cao <macao@redhat.com>
maxcao13
force-pushed
the
openshift-nodepool
branch
from
06e664a to
aaf0c18
Compare
openshift-merge-robot
removed
the
needs-rebase
openshift-ci
bot
added
the
area/karpenter-operator
|
/test e2e-aws-autonode |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
karpenter-operator/controllers/nodeclass/ec2_nodeclass_controller.go (1)
157-162: Bug: Finalizer check is on wrong object.Line 157 checks
hcpfor the finalizer, but line 159 adds the finalizer toopenshiftEC2NodeClass. This will cause the finalizer to be added on every reconcile ifhcpdoesn't contain this finalizer, or never added ifhcpalready has it.🐛 Proposed fix
- if !controllerutil.ContainsFinalizer(hcp, finalizer) { + if !controllerutil.ContainsFinalizer(openshiftEC2NodeClass, finalizer) { original := openshiftEC2NodeClass.DeepCopy() controllerutil.AddFinalizer(openshiftEC2NodeClass, finalizer) if err := r.guestClient.Patch(ctx, openshiftEC2NodeClass, client.MergeFromWithOptions(original, client.MergeFromWithOptimisticLock{})); err != nil { return ctrl.Result{}, fmt.Errorf("failed to add finalizer to OpenshiftEC2NodeClass: %w", err) } }
🤖 Fix all issues with AI agents
In `@api/go.mod`:
- Line 7: The go.mod entry for the karpenter dependency is using a
pseudo-version; update the dependency for module github.com/aws/karpenter from
the pseudo-version to the latest stable release (e.g., v1.8.2) in go.mod and run
go get/Go toolchain to pin and tidy the module versions (or, if a stable release
cannot be used, add a short documented rationale in the repository (e.g., in
DEPENDENCY.md or go.mod comment) explaining why the pseudo-version is required),
ensuring the change references the module name github.com/aws/karpenter and that
you run go mod tidy to verify the dependency graph.
🧹 Nitpick comments (2)
karpenter-operator/controllers/nodepool/nodepool_controller.go (1)
198-203: Log message may show empty operation result.When
onlyCreateis true (lines 184-189), theopvariable remains unset (zero value), so the log at line 203 will showop="". Consider moving the log inside the else branch or handling both paths.Suggested fix
} else { op, err = r.CreateOrUpdate(ctx, r.guestClient, crd, func() error { return nil }) if err != nil { errs = append(errs, err) } - + log.Info("Reconciled CRD", "name", crd.Name, "op", op) } } if err := utilerrors.NewAggregate(errs); err != nil { return fmt.Errorf("failed to reconcile CRDs: %w", err) } - log.Info("Reconciled CRDs", "op", op) return nilkarpenter-operator/controllers/nodepool/nodepool_controller_test.go (1)
303-305: Unused fake client.
fakeManagementClientis created but never used in this test. Consider removing it to clean up the test setup.🧹 Proposed cleanup
g := NewWithT(t) - fakeManagementClient := fake.NewClientBuilder(). - WithScheme(scheme). - Build() - fakeGuestClient := fake.NewClientBuilder(). WithScheme(scheme). WithObjects(openshiftNodePool). WithStatusSubresource(openshiftNodePool). Build() r := &NodePoolReconciler{ - managementClient: fakeManagementClient, guestClient: fakeGuestClient, Namespace: "namespace", }
|
|
||
| require ( | ||
| github.com/aws/karpenter-provider-aws v1.0.8 | ||
| github.com/awslabs/operatorpkg v0.0.0-20250320000002-b05af0f15c68 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check available releases for operatorpkg and karpenter
echo "=== Checking operatorpkg releases ==="
gh api repos/awslabs/operatorpkg/releases --jq '.[0:10] | .[] | {tag_name, published_at, prerelease}' 2>/dev/null || echo "gh cli not available or repo access issue"
echo -e "\n=== Checking karpenter releases ==="
gh api repos/kubernetes-sigs/karpenter/releases --jq '.[0:10] | .[] | {tag_name, published_at, prerelease}' 2>/dev/null || echo "gh cli not available or repo access issue"
echo -e "\n=== Fallback: Check via web for recent releases ==="
echo "operatorpkg repo: https://github.com/awslabs/operatorpkg/releases"
echo "karpenter repo: https://github.com/kubernetes-sigs/karpenter/releases"Repository: openshift/hypershift
Length of output: 1117
🌐 Web query:
operatorpkg awslabs stable releases v1.0
💡 Result:
There is no awslabs/operatorpkg v1.0 stable release. The GitHub repo shows the project is pre-1.0 (uses 0.x / development versions) and releases/tags reflect 0.* or pseudo module versions rather than a 1.0 stable tag. [1][2]
Sources:
- awslabs/operatorpkg (GitHub repository, README / versioning). [1]
- Libraries.io release listing for github.com/awslabs/operatorpkg (shows releases/tags; no v1.0). [2]
🌐 Web query:
kubernetes-sigs karpenter stable releases latest version
💡 Result:
The latest stable Karpenter release is v1.8.2 — released Jan 15, 2026. [1][2]
Sources:
- GitHub releases (kubernetes-sigs/karpenter) — v1.8.2 (Jan 15, 2026). [1]
- Karpenter documentation / compatibility & upgrade pages. [2]
Consider upgrading karpenter to a stable release.
karpenter is now a direct dependency using a pseudo-version (v1.2.1-0.20250212185021-45f73ec7a790 from Feb 12, 2025), but stable releases are available—most recently v1.8.2 (Jan 15, 2026). Using a pseudo-version for direct dependencies complicates supply chain auditing and dependency tracking. If a stable release meets the feature requirements, upgrade to it; if not, document why the pseudo-version is necessary.
operatorpkg uses a pseudo-version by necessity—this project has no stable releases and only pre-release versions, so this is acceptable.
🤖 Prompt for AI Agents
In `@api/go.mod` at line 7, The go.mod entry for the karpenter dependency is using
a pseudo-version; update the dependency for module github.com/aws/karpenter from
the pseudo-version to the latest stable release (e.g., v1.8.2) in go.mod and run
go get/Go toolchain to pin and tidy the module versions (or, if a stable release
cannot be used, add a short documented rationale in the repository (e.g., in
DEPENDENCY.md or go.mod comment) explaining why the pseudo-version is required),
ensuring the change references the module name github.com/aws/karpenter and that
you run go mod tidy to verify the dependency graph.
|
/retest |
|
/retest |
|
@maxcao13: The following tests 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. |
|
Test failures look like its suffering the same fate as #7478 because of disk space reasons on the node: |
|
pending openshift/enhancements#1924 |
|
can we make sure this passes the tech-preview tests before others? |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
|
Seems like this has been decided on a "wont-do" in our rosa sync meeting. I will close this now. |
4 participants
What this PR does / why we need it:
This commit introduces the OpenshiftNodePool CRD.
The CRD acts as the de-facto interface for cluster-users interacting with AutoNode NodePools. Similar to OpenshiftEC2NodeClass, the creation of a ONP will render a Karpenter NodePool, protected with a VAP. The ValidatingAdmissionPolicy in the guest cluster will be constantly reconciled to mitigate deletion race conditions.
This allows us as a cluster service provider to:
Which issue(s) this PR fixes:
Fixes AUTOSCALE-491
Special notes for your reviewer:
As of now, there is nothing that we need to override or hide. We are simply just reflecting the OpenshiftNodePool spec to the NodePool, and the NodePool status back to the ONP.
Checklist: