feat: add per-component priorityClassName to ArgoCD CRD (RFE-8802)#1189
Conversation
|
[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 |
|
Hi @aradhanay621. Thanks for your PR. I'm waiting for a redhat-developer member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis change adds an optional ChangesPriorityClass schema exposure
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
🤖 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 `@bundle/manifests/argoproj.io_argocds.yaml`:
- Around line 253-257: The CRD schema now accepts priorityClassName fields in
multiple component configurations, but the corresponding Go API struct fields
are missing from api/v1beta1/argocd_types.go and the reconciliation logic to
apply these values to pod specs is missing from
controllers/argocd/deployment.go. Add the priorityClassName fields to the
relevant Go struct definitions in the API types, and then update the controller
logic in the pod/deployment creation methods to read these priorityClassName
values from the CR and apply them to the generated pod specs (by setting the
PriorityClassName field on the PodSpec). Apply these changes to all locations
mentioned in the line ranges.
In `@config/crd/bases/argoproj.io_argocds.yaml`:
- Around line 242-246: The priorityClassName schema field is exposed in the CRD
but lacks corresponding API struct fields and reconciliation logic in the
operator to actually apply it to PodSpec.PriorityClassName, causing the field to
be silently ignored at runtime. Either remove all priorityClassName schema
additions (including those at lines 1244-1248, 2461-2465, 4258-4262, 8290-8294,
8509-8513, 9164-9168, 13724-13728, 18621-18625, 20437-20441, 26245-26249,
30110-30114) from this CRD definition, or ensure the corresponding API struct
field and controller reconciliation logic are implemented in the
argoproj-labs/argocd-operator to actually set and apply the priorityClassName
value before merging this schema change.
🪄 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 YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: beff128e-bd41-4675-8abd-f5ac758d50fc
📒 Files selected for processing (2)
bundle/manifests/argoproj.io_argocds.yamlconfig/crd/bases/argoproj.io_argocds.yaml
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
argoproj-labs/argocd-operator(manual)
| priorityClassName: | ||
| description: |- | ||
| PriorityClassName is the name of the PriorityClass resource to assign to this component's | ||
| pod. The PriorityClass must already exist in the cluster. | ||
| type: string |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Schema exposes priorityClassName, but runtime contract is incomplete.
These CRD fields are now accepted, but the linked codebase findings show no matching Go API fields in api/v1beta1/argocd_types.go and no reconciliation logic applying PriorityClassName to pod specs (for example in controllers/argocd/deployment.go). This makes the feature a silent no-op at runtime.
Please land the corresponding API struct fields (v1alpha1/v1beta1 as applicable) and controller wiring before release so accepted CR input is actually enforced on workloads.
As per path instructions, “Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.”
Also applies to: 1255-1259, 2472-2476, 4269-4273, 8301-8305, 8520-8524, 9175-9179, 13735-13739, 18632-18636, 20448-20452, 26256-26260, 30121-30125
🤖 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 `@bundle/manifests/argoproj.io_argocds.yaml` around lines 253 - 257, The CRD
schema now accepts priorityClassName fields in multiple component
configurations, but the corresponding Go API struct fields are missing from
api/v1beta1/argocd_types.go and the reconciliation logic to apply these values
to pod specs is missing from controllers/argocd/deployment.go. Add the
priorityClassName fields to the relevant Go struct definitions in the API types,
and then update the controller logic in the pod/deployment creation methods to
read these priorityClassName values from the CR and apply them to the generated
pod specs (by setting the PriorityClassName field on the PodSpec). Apply these
changes to all locations mentioned in the line ranges.
Sources: Path instructions, Linked repositories
| priorityClassName: | ||
| description: |- | ||
| PriorityClassName is the name of the PriorityClass resource to assign to this component's | ||
| pod. The PriorityClass must already exist in the cluster. | ||
| type: string |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Schema/controller contract is currently broken for priorityClassName.
These schema additions expose priorityClassName, but the linked argoproj-labs/argocd-operator findings show no corresponding API struct fields and no reconciliation logic applying PodSpec.PriorityClassName. In that state, users can set the field in CRs and it will be silently ignored at runtime. Please ship this schema change together with (or gated behind) the API/controller implementation to avoid a no-op user-facing feature.
As per path instructions, this focuses on a major cross-layer integration issue and avoids minor/nitpick feedback.
Also applies to: 1244-1248, 2461-2465, 4258-4262, 8290-8294, 8509-8513, 9164-9168, 13724-13728, 18621-18625, 20437-20441, 26245-26249, 30110-30114
🤖 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 `@config/crd/bases/argoproj.io_argocds.yaml` around lines 242 - 246, The
priorityClassName schema field is exposed in the CRD but lacks corresponding API
struct fields and reconciliation logic in the operator to actually apply it to
PodSpec.PriorityClassName, causing the field to be silently ignored at runtime.
Either remove all priorityClassName schema additions (including those at lines
1244-1248, 2461-2465, 4258-4262, 8290-8294, 8509-8513, 9164-9168, 13724-13728,
18621-18625, 20437-20441, 26245-26249, 30110-30114) from this CRD definition, or
ensure the corresponding API struct field and controller reconciliation logic
are implemented in the argoproj-labs/argocd-operator to actually set and apply
the priorityClassName value before merging this schema change.
Sources: Path instructions, Linked repositories
Expose priorityClassName in the ArgoCD Custom Resource for each core component so cluster administrators can control pod scheduling priority and prevent eviction during resource contention. The following components now support spec.<component>.priorityClassName: - spec.controller.priorityClassName (Application Controller StatefulSet) - spec.server.priorityClassName (ArgoCD Server Deployment) - spec.repo.priorityClassName (Repo Server Deployment) - spec.redis.priorityClassName (Redis Deployment) - spec.applicationSet.priorityClassName (ApplicationSet Controller) - spec.sso.dex.priorityClassName (Dex Server Deployment) CRD schemas updated for both v1alpha1 and v1beta1 API versions in config/crd/bases/ and bundle/manifests/. Upstream argocd-operator changes (type definitions, controllers, unit tests, and conversion functions) are tracked separately in: argoproj-labs/argocd-operator#2125 Resolves: RFE-8802 Co-authored-by: Cursor <cursoragent@cursor.com>
aradhanay621
force-pushed
the
feat/per-component-priorityClassName-rfe-8802
branch
from
4322bd3 to
37f927f
Compare
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
1 participant
Summary
Exposes
priorityClassNameper core ArgoCD component in theArgoCDCustom Resource, resolving RFE-8802.Without this field, ArgoCD pods use the cluster's global default PriorityClass and are at risk of eviction during resource contention. This change gives cluster administrators fine-grained control over scheduling priority per component.
New fields
spec.controller.priorityClassNamespec.server.priorityClassNamespec.repo.priorityClassNamespec.redis.priorityClassNamespec.applicationSet.priorityClassNamespec.sso.dex.priorityClassNameChanges
config/crd/bases/argoproj.io_argocds.yaml— CRD base schema updated forv1alpha1andv1beta1bundle/manifests/argoproj.io_argocds.yaml— OLM bundle manifest updated forv1alpha1andv1beta1Example usage
Upstream dependency
The controller-side implementation (Go type definitions, reconcilers, conversion functions between
v1alpha1↔v1beta1, and unit tests) is tracked upstream in argoproj-labs/argocd-operator#2125. Thego.moddependency bump will follow once that upstream change is merged and tagged.Resolves: RFE-8802