NO-JIRA: derive cluster quota from namespace configmaps - #31476
NO-JIRA: derive cluster quota from namespace configmaps#31476not-stbenjam wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
@not-stbenjam: This pull request explicitly references no 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
openshift-ci
Bot
added
ready-for-human-review
|
Hi @not-stbenjam. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: not-stbenjam 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 |
WalkthroughThe cluster quota test now observes existing ConfigMaps in both projects before creating the quota. Usage assertions add newly created ConfigMaps to this observed baseline instead of using fixed counts or Istio-specific recovery logic. ChangesCluster quota test
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/extended/quota/clusterquota.go (1)
79-106: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftRead the active quota used count before adding headroom.
The ConfigMap baseline is taken by
ConfigMaps(ns).List()beforecqis created and before the namespaces are labeled into the selector. If a cluster controller creates a ConfigMap after that list but before the quota controller syncs,quota.Status.Total.Used[corev1.ResourceConfigMaps]will include it whilecq.Spec.Quota.Hardstill equalsinitialConfigMapCount + 2. That can make the legitimate creates at the expected counts fail with Forbidden and fail the e2e immediately.Use the quota-controller-observed baseline (for example,
Status.Total.Used[corev1.ResourceConfigMaps]afterwaitForQuotaStatusconfirms the quota is active) before adding the test’s two planned ConfigMap creations.🤖 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 `@test/extended/quota/clusterquota.go` around lines 79 - 106, Use the quota-controller-observed ConfigMap usage as the baseline instead of the pre-creation namespace list. After waitForQuotaStatus confirms the cluster quota is active, read cq.Status.Total.Used[corev1.ResourceConfigMaps], then add headroom for the two planned creations when setting cq.Spec.Quota.Hard; retain the existing initial list only if needed for logging or setup.
🤖 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.
Outside diff comments:
In `@test/extended/quota/clusterquota.go`:
- Around line 79-106: Use the quota-controller-observed ConfigMap usage as the
baseline instead of the pre-creation namespace list. After waitForQuotaStatus
confirms the cluster quota is active, read
cq.Status.Total.Used[corev1.ResourceConfigMaps], then add headroom for the two
planned creations when setting cq.Spec.Quota.Hard; retain the existing initial
list only if needed for logging or setup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: d99302b9-c78a-4abd-b678-f400df4cc200
📒 Files selected for processing (1)
test/extended/quota/clusterquota.go
2 participants
The ClusterResourceQuota conformance test reserves room for two known namespace ConfigMaps, then has a one-name fallback for
istio-ca-root-cert. Recent clusters can have a different third controller-owned ConfigMap in each test namespace; the quota is already full at 6/6, the fallback does not recognize the baseline, and the test fails before exercising its intended quota behavior.This change creates the two namespaces first, waits for the mandatory CA ConfigMaps, observes the complete ConfigMap baseline in each namespace, and then creates the ClusterResourceQuota with room for exactly the test's two ConfigMaps. The enforcement assertions remain unchanged: one creation in each namespace succeeds and a second creation is forbidden.
Prow evidence:
Corpus impact: 20 exact failed test executions were found across the retained study corpus; 16 already recovered on a twin attempt. The four unrecovered child runs all had independent blocking failures, so the conservative whole-job rescue estimate is 0 even though this removes the flaky assertion and retry pressure.
Validation:
GOTOOLCHAIN=auto GOMAXPROCS=2 go test -mod=vendor -vet=off -p=2 -run '^$' ./test/extended/quotagit diff --checkNo RHCOS or OS-layer evidence was examined.
Summary by CodeRabbit