OCPBUGS-86044: Fix oc adm policy SCC test race on multi-master clusters#31221
OCPBUGS-86044: Fix oc adm policy SCC test race on multi-master clusters#31221tchap wants to merge 1 commit into
Conversation
Batch user and service account subjects into single oc invocations to eliminate cross-master watch cache races, and poll with Eventually to wait for cache propagation between adds and removes.
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci-robot
added
jira/valid-reference
|
@tchap: This pull request references Jira Issue OCPBUGS-86044, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
The bug has been updated to refer to the pull request using the external bug tracker. 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. |
WalkthroughThis PR updates the SCC policy tests in ChangesSCC Policy CLI Test Async Polling
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@tchap: This pull request references Jira Issue OCPBUGS-86044, which is valid. 3 validation(s) were run on this bug
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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: tchap 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 `@test/extended/cli/admin.go`:
- Around line 225-233: The polling callback passed to Eventually that runs
oc.Run("get").Args("clusterrolebinding/system:openshift:scc:privileged", "-o",
"yaml").Output() must treat a "not found" error as a terminal success: if the
get command returns an error and the error message indicates the
ClusterRoleBinding is missing (e.g. contains "NotFound" or "not found"), return
an empty string and nil so Eventually stops; otherwise propagate the error as
before. Update the anonymous func used in the Eventually call to detect the
missing-binding error and convert it into a successful (empty, nil) response.
🪄 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), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 83551d20-0e8b-46b7-91e3-5f5fe21302d8
📒 Files selected for processing (1)
test/extended/cli/admin.go
openshift-ci
Bot
added
the
ready-for-human-review
|
Scheduling required tests: |
|
@tchap: 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. |
| // 2. the binding does not exists, if we removed all entities from the binding | ||
| o.Expect(out).To(o.ContainSubstring(`clusterrolebindings.rbac.authorization.k8s.io "system:openshift:scc:privileged" not found`)) | ||
| } | ||
| o.Eventually(func() (string, error) { |
There was a problem hiding this comment.
It makes sense using Eventually if we have reasons for it. Maybe we should add documentation about this.
| o.Expect(err).To(o.HaveOccurred()) | ||
| o.Expect(out).To(o.ContainSubstring("error: rolebinding custom found for role view, not other")) | ||
|
|
||
| o.Expect(oc.Run("adm", "policy", "add-scc-to-user").Args("privileged", "fake-user").Execute()).To(o.Succeed()) |
There was a problem hiding this comment.
I think it is better to keep the tests we have.
| o.Expect(out).To(o.ContainSubstring("fake-sa")) | ||
| o.Expect(out).To(o.ContainSubstring("fake-group")) | ||
|
|
||
| o.Expect(oc.Run("adm", "policy", "remove-scc-from-user").Args("privileged", "fake-user").Execute()).To(o.Succeed()) |
There was a problem hiding this comment.
Same. We should keep it, unless this causes the race condition
3 participants
Batch user and service account subjects into single oc invocations to eliminate cross-master watch cache races, and poll with Eventually to wait for cache propagation between adds and removes.
Summary by CodeRabbit
Note: This release contains test improvements only. No user-facing functionality has changed.