OADP-8446: add namesapce resource policy for oadp#2304
Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
Signed-off-by: Wesley Hayutin <weshayutin@gmail.com>
weshayutin
changed the title
|
@weshayutin: This pull request references OADP-8446 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. |
openshift-ci-robot
added
the
jira/valid-reference
WalkthroughThe operator now ensures an ChangesResourceQuota support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant DPAReconcile
participant ReconcileResourceQuota
participant KubernetesAPI
DPAReconcile->>ReconcileResourceQuota: reconcile DPA namespace quota
ReconcileResourceQuota->>KubernetesAPI: get oadp-resource-quota
KubernetesAPI-->>ReconcileResourceQuota: existing quota or NotFound
ReconcileResourceQuota->>KubernetesAPI: create default ResourceQuota when missing
KubernetesAPI-->>ReconcileResourceQuota: created or AlreadyExists
Suggested reviewers: 🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: weshayutin 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@internal/controller/resourcequota_test.go`:
- Around line 101-134: Add a ResourceQuota watch or ownership configuration in
SetupWithManager so deleting the quota enqueues the related DPA reconciliation
and restores defaults without a direct ReconcileResourceQuota call. Ensure the
watch maps the deleted ResourceQuota to the correct DataProtectionApplication.
🪄 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: a9c35a98-fb3b-4da3-9373-69f992e4fe5b
📒 Files selected for processing (10)
bundle/manifests/oadp-operator.clusterserviceversion.yamlbundle/manifests/oadp.openshift.io_dataprotectionapplications.yamlconfig/crd/bases/oadp.openshift.io_dataprotectionapplications.yamlconfig/quota/resource_quota.yamlconfig/rbac/role.yamldocs/config/resource_quota.mddocs/design/2026-07-16-resourcequota-design.mdinternal/controller/dataprotectionapplication_controller.gointernal/controller/resourcequota.gointernal/controller/resourcequota_test.go
| t.Run("recreates defaults after admin deletes ResourceQuota", func(t *testing.T) { | ||
| fakeClient, err := getFakeClientFromObjects(dpa) | ||
| require.NoError(t, err) | ||
|
|
||
| r := &DataProtectionApplicationReconciler{ | ||
| Client: fakeClient, | ||
| Scheme: fakeClient.Scheme(), | ||
| Context: context.Background(), | ||
| dpa: dpa, | ||
| } | ||
|
|
||
| _, err = r.ReconcileResourceQuota(logr.Discard()) | ||
| require.NoError(t, err) | ||
|
|
||
| existing := &corev1.ResourceQuota{} | ||
| err = fakeClient.Get(context.Background(), types.NamespacedName{ | ||
| Name: oadpResourceQuotaName, | ||
| Namespace: ns, | ||
| }, existing) | ||
| require.NoError(t, err) | ||
| require.NoError(t, fakeClient.Delete(context.Background(), existing)) | ||
|
|
||
| cont, err := r.ReconcileResourceQuota(logr.Discard()) | ||
| require.NoError(t, err) | ||
| assert.True(t, cont) | ||
|
|
||
| got := &corev1.ResourceQuota{} | ||
| err = fakeClient.Get(context.Background(), types.NamespacedName{ | ||
| Name: oadpResourceQuotaName, | ||
| Namespace: ns, | ||
| }, got) | ||
| require.NoError(t, err) | ||
| assert.True(t, resource.MustParse("200").Equal(got.Spec.Hard[corev1.ResourcePods])) | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
rg -n --type=go -C4 \
'Owns\(&corev1\.ResourceQuota|Watches\([^)]*ResourceQuota|ResourceQuota\{\}' \
.Repository: openshift/oadp-operator
Length of output: 3824
🏁 Script executed:
#!/bin/bash
set -eu
printf '\n== resourcequota.go ==\n'
cat -n internal/controller/resourcequota.go
printf '\n== controller setup references ==\n'
rg -n --type=go -C3 'SetupWithManager|Owns\(|Watches\(|For\(|ResourceQuota|DataProtectionApplication' internal/controllerRepository: openshift/oadp-operator
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -eu
printf '\n== all references to ReconcileResourceQuota ==\n'
rg -n --type=go -C3 'ReconcileResourceQuota' .
printf '\n== controller registration/setup files ==\n'
rg -n --type=go -C3 'SetupWithManager|For\(|Owns\(|Watches\(' internalRepository: openshift/oadp-operator
Length of output: 7650
🏁 Script executed:
#!/bin/bash
set -eu
rg -n --type=go -C4 'SetupWithManager|Owns\(|Watches\(|For\(' internalRepository: openshift/oadp-operator
Length of output: 4141
Add a ResourceQuota watch if deletion should self-heal. This test only exercises a direct ReconcileResourceQuota call; SetupWithManager does not watch or own corev1.ResourceQuota, so deleting the quota will not enqueue a DPA reconcile on its own.
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 106-106: : # github.com/openshift/oadp-operator/api/v1alpha1
api/v1alpha1/cloudstorage_types.go:106:25: cannot use &CloudStorage{} (value of type *CloudStorage) as "k8s.io/apimachinery/pkg/runtime".Object value in argument to SchemeBuilder.Register: *CloudStorage does not implement "k8s.io/apimachinery/pkg/runtime".Object (missing method DeepCopyObject)
api/v1alpha1/cloudstorage_types.go:106:42: cannot use &CloudStorageList{} (value of type *CloudStorageList) as "k8s.io/apimachinery/pkg/runtime".Object value in argument to SchemeBuilder.Register: *CloudStorageList does not implement "k8s.io/apimachinery/pkg/runtime".Object (missing method DeepCopyObject)
api/v1alpha1/dataprotectionapplication_types.go:512:9: (*in).DeepCopyInto undefined (type *LoadConcurrency has no field or method DeepCopyInto)
api/v1alpha1/dataprotectionapplication_types.go:521:11: (*in).DeepCopyInto undefined (type *LoadAffinity has no field or method DeepCopyInto)
api/v1alpha1/dataprotectionapplication_types.go:1054:25: cannot use &DataProtectionApplication{} (value of type *DataProtectionApplication) as "k8s.io/apimachinery/pkg/runtime".Object value in argument to SchemeBuilder.Register: *DataProtectionApplication does not implement "k8s.io/apimachinery/pkg/runtime".Object (missing method DeepCopyObject)
api/v1alpha1/dataprotectionapplication_types.go:1054:55: cannot use &DataProtectionApplicationList{} (value of type *DataProtectionApplicationList) as "k8s.io/apimachinery/pkg/runtime".Object value in argument to SchemeBuilder.Register: *DataProtectionApplicationList does not implement "k8s.io/apimachinery/pkg/runtime".Object (missing method DeepCopyObject)
api/v1alpha1/dataprotectiontest_types.go:215:25: cannot use &DataProtectionTest{} (value of type *DataProtectionTest) as "k8s.io/apimachinery/pkg/runtime".Object value in argument to SchemeBuilder.Register: *DataProtectionTest does not implement "k8s.io/apimachinery/pkg/runtime".Object (missing method DeepCopyObject)
api/v1alpha1/dataprotectiontest_types.go:215:48: cannot use &DataProtectionTestList{} (value of type *DataProtectionTestList) as "k8s.io/apimachinery/pkg/runtime".Object value in argument to SchemeBuilder.Register: *DataProtectionTestList does not implement "k8s.io/apimachinery/pkg/runtime".Object (missing method DeepCopyObject)
(typecheck)
🤖 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 `@internal/controller/resourcequota_test.go` around lines 101 - 134, Add a
ResourceQuota watch or ownership configuration in SetupWithManager so deleting
the quota enqueues the related DPA reconciliation and restores defaults without
a direct ReconcileResourceQuota call. Ensure the watch maps the deleted
ResourceQuota to the correct DataProtectionApplication.
|
/retest |
|
@weshayutin: 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. |
weshayutin
added
the
do-not-merge/hold
3 participants
Summary by CodeRabbit
New Features
Documentation
Bug Fixes