OCPBUGS-85410: Fix performance related issues when selinux metrics are emitted#2668
Open
gnufied wants to merge 3 commits into
Open
OCPBUGS-85410: Fix performance related issues when selinux metrics are emitted#2668gnufied wants to merge 3 commits into
gnufied wants to merge 3 commits into
Conversation
openshift-ci-robot
added
backports/validated-commits
|
@gnufied: This pull request references Jira Issue OCPBUGS-85410, 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. |
|
@gnufied: the contents of this pull request could be automatically validated. The following commits are valid:
Comment |
gnufied
changed the title
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughThe PR refactors the SELinux warning controller's conflict cache from a streaming ChangesSELinux Conflict Cache Refactoring
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: gnufied 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 |
|
@gnufied: This pull request references Jira Issue OCPBUGS-85410, which is valid. 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. |
![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 (2)
pkg/controller/volume/selinuxwarning/cache/volumecache_test.go (1)
680-700: ⚡ Quick winTighten surviving-conflict assertions to reject unexpected extras.
This currently verifies required pairs exist, but when
expectedSurvivingPairsis non-empty it does not fail on additional unexpected conflicts.Suggested patch
// Verify each expected surviving pair exists in both directions for _, pair := range tt.expectedSurvivingPairs { hasForward := false hasReverse := false for _, conflict := range remaining { if conflict.Pod == pair[0] && conflict.OtherPod == pair[1] { hasForward = true } if conflict.Pod == pair[1] && conflict.OtherPod == pair[0] { hasReverse = true } } if !hasForward || !hasReverse { t.Errorf("expected symmetric conflict between %s and %s, got %+v", pair[0], pair[1], remaining) } } - // If no pairs are expected, there should be no conflicts at all - if len(tt.expectedSurvivingPairs) == 0 && len(remaining) != 0 { - t.Errorf("expected no conflicts, got %+v", remaining) - } + expectedConflictCount := len(tt.expectedSurvivingPairs) * 2 // forward + reverse + if len(remaining) != expectedConflictCount { + t.Errorf("expected %d remaining conflicts, got %d: %+v", expectedConflictCount, len(remaining), remaining) + }🤖 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 `@pkg/controller/volume/selinuxwarning/cache/volumecache_test.go` around lines 680 - 700, The test currently only checks that each expected symmetric pair appears in remaining but does not fail on extra unexpected conflicts; update the assertion to require exact equivalence when tt.expectedSurvivingPairs is non-empty by building the full expected set of directional conflicts (for each pair in tt.expectedSurvivingPairs include both [a,b] and [b,a]), then assert that remaining contains exactly that set (compare lengths and that every conflict in remaining matches an entry in the expected set using conflict.Pod and conflict.OtherPod) and fail if any extras or missing entries are found; keep the existing symmetric existence check only for the empty-case branch.pkg/controller/volume/selinuxwarning/cache/volumecache.go (1)
151-189: 💤 Low valueConsider skipping self-comparison in conflict detection loop.
The loop iterates all pods including the one being added (
podKey). While self-comparison produces no conflicts (same policy, same labels), explicitly skipping it saves one iteration and oneConflictsParsedcall perAddVolume.♻️ Proposed optimization
// Emit conflicts for the pod for otherPodKey, otherPodInfo := range volume.pods { + if otherPodKey == podKey { + continue + } if otherPodInfo.changePolicy != changePolicy {🤖 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 `@pkg/controller/volume/selinuxwarning/cache/volumecache.go` around lines 151 - 189, The loop over volume.pods currently compares the pod being added against itself, causing an unnecessary iteration and a redundant call to c.seLinuxTranslator.ConflictsParsed; modify the loop in the function that adds pods (the AddVolume / pod-add block containing volume.pods, podKey, podInfo) to explicitly skip self-comparison by checking if otherPodKey == podKey and continue, so you avoid creating duplicate/confusing conflict entries and avoid the extra ConflictsParsed invocation on the same pod.
🤖 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 `@pkg/controller/volume/selinuxwarning/cache/volumecache_test.go`:
- Line 495: The assertion message is stale: replace the "SendConflicts returned
unexpected conflicts: %+v" text with a message that reflects the current API
under test (e.g. "GetConflicts returned unexpected conflicts: %+v") in the test
that checks receivedConflicts in volumecache_test.go so failures correctly
reference GetConflicts; update the error string used in the t.Errorf call that
reports receivedConflicts.
In `@pkg/controller/volume/selinuxwarning/cache/volumecache.go`:
- Line 301: The code calls slices.Sort(podVolumes) but the slices package is not
imported; add the missing import for the slices package so the call
compiles—prefer importing the standard library "slices" (Go 1.21+), or if the
project targets older Go, import "golang.org/x/exp/slices"; ensure the import is
added alongside other imports so slices.Sort(podVolumes) resolves.
---
Nitpick comments:
In `@pkg/controller/volume/selinuxwarning/cache/volumecache_test.go`:
- Around line 680-700: The test currently only checks that each expected
symmetric pair appears in remaining but does not fail on extra unexpected
conflicts; update the assertion to require exact equivalence when
tt.expectedSurvivingPairs is non-empty by building the full expected set of
directional conflicts (for each pair in tt.expectedSurvivingPairs include both
[a,b] and [b,a]), then assert that remaining contains exactly that set (compare
lengths and that every conflict in remaining matches an entry in the expected
set using conflict.Pod and conflict.OtherPod) and fail if any extras or missing
entries are found; keep the existing symmetric existence check only for the
empty-case branch.
In `@pkg/controller/volume/selinuxwarning/cache/volumecache.go`:
- Around line 151-189: The loop over volume.pods currently compares the pod
being added against itself, causing an unnecessary iteration and a redundant
call to c.seLinuxTranslator.ConflictsParsed; modify the loop in the function
that adds pods (the AddVolume / pod-add block containing volume.pods, podKey,
podInfo) to explicitly skip self-comparison by checking if otherPodKey == podKey
and continue, so you avoid creating duplicate/confusing conflict entries and
avoid the extra ConflictsParsed invocation on the same pod.
🪄 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: 52eb5551-368b-4461-9666-fd519900a29b
📒 Files selected for processing (7)
pkg/controller/volume/selinuxwarning/cache/volumecache.gopkg/controller/volume/selinuxwarning/cache/volumecache_test.gopkg/controller/volume/selinuxwarning/internal/parse/selinux_label.gopkg/controller/volume/selinuxwarning/internal/parse/selinux_label_test.gopkg/controller/volume/selinuxwarning/metrics.gopkg/controller/volume/selinuxwarning/selinux_warning_controller_test.gopkg/controller/volume/selinuxwarning/translator/selinux_translator.go
gnufied
force-pushed
the
manual-backport-selinux-perf-fix-ocp-master
branch
from
f3ff95e to
cfab855
Compare
|
@gnufied: the contents of this pull request could be automatically validated. The following commits are valid:
Comment |
When calling ControllerSELinuxTranslator.Conflicts(), the SELinux label is repeatedly split into []string to detect conflicts. This causes a huge number of allocations when there are many comparisons. This is now made more efficient by pre-parsing the SELinux label and storing it in podInfo as [4]string for fast comparison when needed.
Added podToVolumes reverse index to optimize DeletePod. Currently we simply iterate through all the volumes and remove the pod being deleted from there. This is inefficient and takes longer the longer the volume list becomes. Keeping a map pod -> volumes makes removing a pod fast. We can just jump to the relevant volumes directly and remove the pod from there.
Also prevent duplicate metric emissions
gnufied
force-pushed
the
manual-backport-selinux-perf-fix-ocp-master
branch
from
cfab855 to
1f27b44
Compare
|
@gnufied: the contents of this pull request could be automatically validated. The following commits are valid:
Comment |
|
@gnufied: This pull request references Jira Issue OCPBUGS-85410, 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. |
|
@gnufied: 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes https://redhat.atlassian.net/browse/OCPBUGS-85410
Summary by CodeRabbit