OADP-2785: Add PriorityClassName support to PodConfig in DataProtectionApplication#2010
OADP-2785: Add PriorityClassName support to PodConfig in DataProtectionApplication#2010kaovilai wants to merge 2 commits intoopenshift:oadp-devfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@kaovilai: This pull request references OADP-2785 which is a valid jira issue. 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds an optional Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
bot
added
the
approved
![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
🧹 Nitpick comments (3)
internal/controller/nodeagent.go (2)
362-367: Mirror the safer pattern used for VeleroPrefer computing the string with guards and passing it without an IIFE for readability and consistency.
Apply this diff:
- install.WithPriorityClassName(func() string { - if dpa.Spec.Configuration.NodeAgent != nil && dpa.Spec.Configuration.NodeAgent.PodConfig != nil { - return dpa.Spec.Configuration.NodeAgent.PodConfig.PriorityClassName - } - return "" - }()), + func() install.Option { + pc := "" + if dpa.Spec.Configuration != nil && + dpa.Spec.Configuration.NodeAgent != nil && + dpa.Spec.Configuration.NodeAgent.PodConfig != nil { + pc = dpa.Spec.Configuration.NodeAgent.PodConfig.PriorityClassName + } + return install.WithPriorityClassName(pc) + }(),
303-307: Nit: event message says “deployment” for a DaemonSetUse “daemonset” to avoid confusion in events.
- fmt.Sprintf("performed %s on NodeAgent deployment %s/%s", op, ds.Namespace, ds.Name), + fmt.Sprintf("performed %s on NodeAgent daemonset %s/%s", op, ds.Namespace, ds.Name),internal/controller/nodeagent_test.go (1)
571-573: PriorityClassName assignment OK; consider simplifyingYou can set PriorityClassName unconditionally (zero-value is “”) to reduce branching. Behavior is identical.
- if len(options.priorityClassName) > 0 { - testBuiltNodeAgentDaemonSet.Spec.Template.Spec.PriorityClassName = options.priorityClassName - } + testBuiltNodeAgentDaemonSet.Spec.Template.Spec.PriorityClassName = options.priorityClassName
📜 Review details
Configuration used: CodeRabbit 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 selected for processing (7)
api/v1alpha1/dataprotectionapplication_types.go(1 hunks)bundle/manifests/oadp.openshift.io_dataprotectionapplications.yaml(3 hunks)config/crd/bases/oadp.openshift.io_dataprotectionapplications.yaml(3 hunks)internal/controller/nodeagent.go(1 hunks)internal/controller/nodeagent_test.go(3 hunks)internal/controller/velero.go(1 hunks)internal/controller/velero_test.go(3 hunks)
🔇 Additional comments (7)
api/v1alpha1/dataprotectionapplication_types.go (1)
365-367: API change is correctOptional PriorityClassName on PodConfig with the expected json tag aligns with the CRD.
config/crd/bases/oadp.openshift.io_dataprotectionapplications.yaml (1)
497-499: Bundle is in sync with base CRDVerification confirms priorityClassName is present at all three locations in the bundle (497–499, 854–856, 1374–1376) with consistent field definitions. The change has been properly propagated.
internal/controller/nodeagent_test.go (1)
258-259: New test option field looks goodThe addition aligns with PodSpec’s string field. No issues.
bundle/manifests/oadp.openshift.io_dataprotectionapplications.yaml (1)
497-500: CRD schema additions are correct and consistentAll verification checks passed:
- API type has correct json tag:
api/v1alpha1/dataprotectionapplication_types.go:367- Base and bundle CRDs are in sync with 3 priorityClassName occurrences each (nodeAgent, restic, velero)
- Velero controller correctly applies the field from
dpa.Spec.Configuration.Velero.PodConfig.PriorityClassName(velero.go:204-209)- NodeAgent controller correctly applies the field from
dpa.Spec.Configuration.NodeAgent.PodConfig.PriorityClassName(nodeagent.go:362-367)- Restic field exists in CRD for backwards compatibility despite being deprecated in favor of nodeAgent
internal/controller/velero_test.go (3)
634-634: LGTM! Clean addition of the test option field.The
priorityClassNamefield addition follows the existing naming conventions and patterns in the test helper struct.
774-777: LGTM! Correct implementation of PriorityClassName assignment.The conditional logic properly checks for a non-empty string before setting the field on the deployment's pod template spec, following the same pattern as other optional fields in this helper function.
1038-1061: LGTM! Comprehensive test coverage for PriorityClassName feature.The test case properly validates that a PriorityClassName specified in the DPA's PodConfig is correctly propagated to the Velero deployment. The test follows established patterns and uses realistic test data.
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
2 similar comments
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
1 similar comment
|
/retest |
openshift-ci
bot
added
the
needs-rebase
- Introduced PriorityClassName field in PodConfig struct to allow specification of pod priority. - Updated CRD and related manifests to include PriorityClassName description. - Modified NodeAgent and Velero deployment builders to utilize PriorityClassName from DataProtectionApplication spec. - Enhanced tests to validate PriorityClassName functionality in NodeAgent and Velero deployments.
openshift-ci
bot
removed
the
needs-rebase
There was a problem hiding this comment.
Pull request overview
Adds support for configuring Kubernetes priorityClassName on pods created by the OADP operator via spec.configuration.*.podConfig.priorityClassName in the DataProtectionApplication API, wiring it through to the Velero Deployment and NodeAgent DaemonSet, and updating generated CRDs/manifests and unit tests accordingly.
Changes:
- Extend
PodConfigAPI type withPriorityClassName. - Plumb
PriorityClassNameinto Velero Deployment and NodeAgent DaemonSet builders viainstall.WithPriorityClassName(...). - Update CRD/bundle manifests and add/extend tests to validate the behavior.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
api/v1alpha1/dataprotectionapplication_types.go |
Adds PriorityClassName to PodConfig (API surface). |
internal/controller/velero.go |
Applies PodConfig.PriorityClassName to the Velero Deployment build. |
internal/controller/nodeagent.go |
Applies PodConfig.PriorityClassName to the NodeAgent DaemonSet build. |
internal/controller/velero_test.go |
Adds test coverage for Velero Deployment priorityClassName behavior. |
internal/controller/nodeagent_test.go |
Adds test coverage for NodeAgent DaemonSet priorityClassName behavior. |
config/crd/bases/oadp.openshift.io_dataprotectionapplications.yaml |
Updates the generated CRD schema to include priorityClassName. |
bundle/manifests/oadp.openshift.io_dataprotectionapplications.yaml |
Updates the shipped bundle CRD schema to include priorityClassName. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@kaovilai: 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. |
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kaovilai, sseago, weshayutin 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 |
Why the changes were made
How to test the changes made