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
- 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-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. |
WalkthroughAdds an optional PriorityClassName to PodConfig, updates CRD schemas to expose the field, and propagates the value into NodeAgent DaemonSet and Velero Deployment pod specs with accompanying tests. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to 📒 Files selected for processing (2)
🔇 Additional comments (6)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kaovilai 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
![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 |
|
@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. |
3 participants
Why the changes were made
How to test the changes made