AUTOSCALE-806: Introduce Karpenter API lifecycle object and controller#16
AUTOSCALE-806: Introduce Karpenter API lifecycle object and controller#16maxcao13 wants to merge 3 commits into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
@maxcao13: This pull request references AUTOSCALE-806 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 "5.0.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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: maxcao13 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 |
openshift-ci
Bot
added
the
approved
WalkthroughThis PR introduces the Karpenter custom resource API, CRD, example manifest, and RBAC rules. It changes reconciliation ownership from the operator Deployment to the Karpenter singleton and propagates its log level to the operand. Makefile generation and clean-tree verification are also added. ChangesKarpenter custom resource and reconciliation
Generation and verification tooling
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant KarpenterCR
participant KarpenterController
participant OperandResources
KarpenterCR->>KarpenterController: enqueue singleton change
KarpenterController->>KarpenterController: read spec and derive log level
KarpenterController->>OperandResources: apply owned RBAC and Deployment
KarpenterCR->>OperandResources: provide logLevel configuration
Possibly related PRs
Suggested labels: Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (12 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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.12.2)level=error msg="Running error: context loading failed: failed to load packages: failed to load packages: failed to load with go/packages: err: exit status 1: stderr: go: inconsistent vendoring in :\n\tgithub.com/aws/aws-sdk-go-v2/config@v1.32.17: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/aws/aws-sdk-go-v2/service/ec2@v1.302.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/aws/karpenter-provider-aws@v1.9.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/onsi/ginkgo/v2@v2.27.5: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/onsi/gomega@v1.39.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/openshift/api@v0.0.0-20260107103503-6d35063ca179: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/ ... [truncated 11763 characters] ... tly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tsigs.k8s.io/randfill@v1.0.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tsigs.k8s.io/structured-merge-diff/v6@v6.3.2-0.20260122202528-d9cc6641c482: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tsigs.k8s.io/yaml@v1.6.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/aws/karpenter-provider-aws: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tsigs.k8s.io/karpenter: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\n\tTo ignore the vendor directory, use -mod=readonly or -mod=mod.\n\tTo sync the vendor directory, run:\n\t\tgo mod vendor\n" Comment |
|
/test images checking if the tests in the other PR are just flaking for no reason or if there's an actual problem there. |
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@install/04_rbac.yaml`:
- Around line 73-75: The RBAC rule for the Karpenter CR currently grants write
access that the controller path does not use. Update the permission block for
the Karpenter resource in the RBAC manifest to read-only verbs for the
controller’s read path, and remove status/finalizer write permissions unless the
related reconciliation logic in the Karpenter controller actually performs those
updates in this PR.
In `@pkg/assets/operator/rbac.yaml`:
- Around line 54-56: The generated operator RBAC is too broad for the
controller’s actual usage; reduce the permissions in the mirrored RBAC asset so
the Karpenter singleton is read-only. Update the rule covering
autoscaling.openshift.io resources for karpenters, karpenters/status, and
karpenters/finalizers by removing write verbs and keeping only the read/watch
access needed by the controller code that fetches and watches the object. Ensure
the resulting rbac.yaml stays least-privilege and matches the controller’s
observed access pattern.
In `@pkg/controllers/karpenter/controller.go`:
- Around line 77-83: The controller is fetching the Karpenter singleton without
a namespace, so Reconcile can miss the namespaced CR and skip deployment
updates. Update Controller.Reconcile to use the operator namespace when building
the client.ObjectKey for the c.client.Get call, and make sure the synthetic
request enqueued by the controller also includes that namespace so the same
namespaced singleton is reconciled consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 26e51886-7254-464d-b65e-2ca18b68ca83
⛔ Files ignored due to path filters (1)
pkg/apis/autoscaling/v1alpha1/zz_generated.deepcopy.gois excluded by!**/zz_generated*
📒 Files selected for processing (10)
Makefileexamples/karpenter.yamlinstall/04_rbac.yamlinstall/autoscaling.openshift.io_karpenters.yamlpkg/apis/autoscaling/v1alpha1/doc.gopkg/apis/autoscaling/v1alpha1/karpenter.gopkg/assets/operator/rbac.yamlpkg/controllers/controllers.gopkg/controllers/karpenter/controller.gopkg/operator/operator.go
|
@maxcao13: 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. |
This commit adds a new CRD and API to the autoscaling.openshift.io group. The full GVR is v1alpha1.karpenters.autoscaling.openshift.io. This CRD will be used to manage the lifecycle of Karpenter in standalone OCP. The operator will reconcile on this CR to control Karpenter settings, and scale up and down Karpenter. Refer to openshift/enhancements#2007 for details. Signed-off-by: Max Cao <macao@redhat.com>
Allows the existence of a Karpenter in the openshift-karpenter ns to reconcile Karpenter deployment. This supersedes the previous implementation that the operator automatically created the operand. From now on, the user is required to create the Karpenter object, in order to enable Karpenter in the cluster. Signed-off-by: Max Cao <macao@redhat.com>
Signed-off-by: Max Cao <macao@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
install/04_rbac.yaml (1)
149-151: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low value
nodeclaims/finalizersin the read rule is a no-op grant.A
*/finalizerssubresource is only meaningfully accessed viaupdate/patch;get/list/watchon it grant nothing useful, and finalizer writes are already covered by the write rule at Lines 170-171. Drop it here to keep the read rule minimal.As per path instructions, "RBAC: least privilege; no cluster-admin for workloads."
Suggested read-rule trim
- apiGroups: ["karpenter.sh"] - resources: ["nodepools", "nodepools/status", "nodeclaims", "nodeclaims/status", "nodeclaims/finalizers", "nodeoverlays", "nodeoverlays/status"] + resources: ["nodepools", "nodepools/status", "nodeclaims", "nodeclaims/status", "nodeoverlays", "nodeoverlays/status"] verbs: ["get", "list", "watch"]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@install/04_rbac.yaml` around lines 149 - 151, Remove "nodeclaims/finalizers" from the resources list in the read RBAC rule, while leaving finalizer access covered by the existing write rule.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Makefile`:
- Line 144: Serialize the prerequisites of the Makefile verify target so update
completes before manifest-diff and verify-git-clean run. Adjust the verify
target or its recipe while preserving the existing vet, lint, and test checks,
ensuring parallel make cannot read files during update.
---
Nitpick comments:
In `@install/04_rbac.yaml`:
- Around line 149-151: Remove "nodeclaims/finalizers" from the resources list in
the read RBAC rule, while leaving finalizer access covered by the existing write
rule.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8de356ed-c3af-4a5b-9714-ffd066f1fc08
⛔ Files ignored due to path filters (1)
pkg/apis/autoscaling/v1alpha1/zz_generated.deepcopy.gois excluded by!**/zz_generated*
📒 Files selected for processing (11)
Makefileexamples/karpenter.yamlinstall/04_rbac.yamlinstall/autoscaling.openshift.io_karpenters.yamlpkg/apis/autoscaling/v1alpha1/doc.gopkg/apis/autoscaling/v1alpha1/karpenter.gopkg/assets/karpenter/clusterrole-core.yamlpkg/assets/operator/rbac.yamlpkg/controllers/controllers.gopkg/controllers/karpenter/controller.gopkg/operator/operator.go
✅ Files skipped from review due to trivial changes (3)
- pkg/operator/operator.go
- pkg/assets/karpenter/clusterrole-core.yaml
- examples/karpenter.yaml
🚧 Files skipped from review as they are similar to previous changes (6)
- pkg/apis/autoscaling/v1alpha1/doc.go
- install/autoscaling.openshift.io_karpenters.yaml
- pkg/controllers/controllers.go
- pkg/assets/operator/rbac.yaml
- pkg/apis/autoscaling/v1alpha1/karpenter.go
- pkg/controllers/karpenter/controller.go
|
|
||
| .PHONY: verify | ||
| verify: vet fmt lint manifest-diff verify-deps test ## Run all verification checks. | ||
| verify: vet lint manifest-diff update verify-git-clean test ## Run all verification checks. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '1,240p' MakefileRepository: openshift/karpenter-operator
Length of output: 7594
🏁 Script executed:
rg -n "\bmake\b.*\bverify\b|^\s*verify:" .github MakefileRepository: openshift/karpenter-operator
Length of output: 319
Serialize the verify steps in Makefile:144.
update, manifest-diff, and verify-git-clean are independent, so make -j verify can let update rewrite files while the diff checks read them. Run update before manifest-diff and verify-git-clean, or move them into a sequential recipe to avoid flaky partial-state reads.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Makefile` at line 144, Serialize the prerequisites of the Makefile verify
target so update completes before manifest-diff and verify-git-clean run. Adjust
the verify target or its recipe while preserving the existing vet, lint, and
test checks, ensuring parallel make cannot read files during update.
2 participants
First commit:
This commit adds a new CRD and API to the
autoscaling.openshift.iogroup with the kindKarpenter.The full GVR is
v1alpha1.karpenters.autoscaling.openshift.io.This CRD will be used to manage the lifecycle of
Karpenterin standalone OCP.The operator will reconcile on this CR to control
Karpentersettings, and scale up and downKarpenter.Second commit:
Allows the existence of a
Karpenterin theopenshift-karpenterns to reconcileKarpenterdeployment.This supersedes the previous implementation that the operator automatically created the operand.
From now on, the user is required to create the
Karpenterobject, in order to enableKarpenterin the cluster.For now we only have
logLevelas the only field to control from theKarpenterCR which controls the log level of the Karpenter deployment.Additionally:
Improves the development workflow by adding an
updatemake target.updateis intended to do write operations to fix linting errors automatically.verifynow serves as the full command to first do a read-only check of linting, formatting, etc., then performupdatein the second half.This PR also adds controller-gen to the project to automatically generate CRDs from the contents of
pkg/apisand the kubebuilder annotations.Finally this PR also substitutes a lot of magic numbers and strings with consts and variables.
Summary by CodeRabbit