From 9047f71b7099936e032db1198550a8e32d2c516e Mon Sep 17 00:00:00 2001 From: Hemant Kumar Date: Mon, 11 May 2026 17:34:32 -0400 Subject: [PATCH] Improve performance characteristic of selinux metric emission --- .../selinuxwarning/cache/volumecache.go | 76 ++++--- .../selinuxwarning/cache/volumecache_test.go | 194 ++++++++++++++++-- .../volume/selinuxwarning/metrics.go | 8 +- .../selinux_warning_controller_test.go | 8 +- 4 files changed, 227 insertions(+), 59 deletions(-) diff --git a/pkg/controller/volume/selinuxwarning/cache/volumecache.go b/pkg/controller/volume/selinuxwarning/cache/volumecache.go index daedde49b5e..f1869ae4bb7 100644 --- a/pkg/controller/volume/selinuxwarning/cache/volumecache.go +++ b/pkg/controller/volume/selinuxwarning/cache/volumecache.go @@ -48,8 +48,8 @@ type VolumeCache interface { // change their SELinux support dynamically. GetPodsForCSIDriver(driverName string) []cache.ObjectName - // SendConflicts sends all current conflicts to the given channel. - SendConflicts(logger klog.Logger, ch chan<- Conflict) + // GetConflicts returns the current set of active conflicts (both directions). + GetConflicts(logger klog.Logger) []Conflict } // VolumeCache stores all volumes used by Pods and their properties that the controller needs to track, @@ -62,6 +62,8 @@ type volumeCache struct { // Reverse index: maps each pod to the list of volumes it uses. // The index is used during pod deletion. podToVolumes map[cache.ObjectName]sets.Set[v1.UniqueVolumeName] + // Currently active conflicts per volume (both directions, symmetric pairs). + conflicts map[v1.UniqueVolumeName][]Conflict } var _ VolumeCache = &volumeCache{} @@ -72,6 +74,7 @@ func NewVolumeLabelCache(seLinuxTranslator *translator.ControllerSELinuxTranslat seLinuxTranslator: seLinuxTranslator, volumes: make(map[v1.UniqueVolumeName]usedVolume), podToVolumes: make(map[cache.ObjectName]sets.Set[v1.UniqueVolumeName]), + conflicts: make(map[v1.UniqueVolumeName][]Conflict), } } @@ -164,6 +167,7 @@ func (c *volumeCache) AddVolume(logger klog.Logger, volumeName v1.UniqueVolumeNa OtherPod: podKey, OtherPropertyValue: string(changePolicy), }) + } if c.seLinuxTranslator.ConflictsParsed(otherPodInfo.seLinuxParts, podInfo.seLinuxParts) { // Send conflict to both pods @@ -184,6 +188,21 @@ func (c *volumeCache) AddVolume(logger klog.Logger, volumeName v1.UniqueVolumeNa }) } } + // Update the conflict cache for this volume: remove stale conflicts for this pod, then add new ones + volumeConflicts := c.conflicts[volumeName] + updated := make([]Conflict, 0, len(volumeConflicts)) + for _, existing := range volumeConflicts { + if existing.Pod != podKey && existing.OtherPod != podKey { + updated = append(updated, existing) + } + } + updated = append(updated, conflicts...) + if len(updated) == 0 { + delete(c.conflicts, volumeName) + } else { + c.conflicts[volumeName] = updated + } + return conflicts } @@ -205,6 +224,21 @@ func (c *volumeCache) DeletePod(logger klog.Logger, podKey cache.ObjectName) { } } delete(c.podToVolumes, podKey) + + // Remove cached conflicts involving the deleted pod + for volName, volConflicts := range c.conflicts { + updated := make([]Conflict, 0, len(volConflicts)) + for _, existing := range volConflicts { + if existing.Pod != podKey && existing.OtherPod != podKey { + updated = append(updated, existing) + } + } + if len(updated) == 0 { + delete(c.conflicts, volName) + } else { + c.conflicts[volName] = updated + } + } } // registerPodVolume adds volumeName to the pod volume index. @@ -281,42 +315,16 @@ func (c *volumeCache) GetPodsForCSIDriver(driverName string) []cache.ObjectName return pods } -// SendConflicts sends all current conflicts to the given channel. -func (c *volumeCache) SendConflicts(logger klog.Logger, ch chan<- Conflict) { +// GetConflicts returns the current set of active conflicts (both directions, symmetric pairs). +func (c *volumeCache) GetConflicts(logger klog.Logger) []Conflict { c.mutex.RLock() defer c.mutex.RUnlock() logger.V(4).Info("Scraping conflicts") c.dump(logger) - for _, volume := range c.volumes { - // compare pods that use the same volume with each other - for podKey, podInfo := range volume.pods { - for otherPodKey, otherPodInfo := range volume.pods { - if podKey == otherPodKey { - continue - } - // create conflict only for the first pod. The other pod will get the same conflict in its own iteration of `volume.pods` loop. - if podInfo.changePolicy != otherPodInfo.changePolicy { - ch <- Conflict{ - PropertyName: "SELinuxChangePolicy", - EventReason: "SELinuxChangePolicyConflict", - Pod: podKey, - PropertyValue: string(podInfo.changePolicy), - OtherPod: otherPodKey, - OtherPropertyValue: string(otherPodInfo.changePolicy), - } - } - if c.seLinuxTranslator.Conflicts(podInfo.seLinuxLabel, otherPodInfo.seLinuxLabel) { - ch <- Conflict{ - PropertyName: "SELinuxLabel", - EventReason: "SELinuxLabelConflict", - Pod: podKey, - PropertyValue: podInfo.seLinuxLabel, - OtherPod: otherPodKey, - OtherPropertyValue: otherPodInfo.seLinuxLabel, - } - } - } - } + result := make([]Conflict, 0) + for _, volConflicts := range c.conflicts { + result = append(result, volConflicts...) } + return result } diff --git a/pkg/controller/volume/selinuxwarning/cache/volumecache_test.go b/pkg/controller/volume/selinuxwarning/cache/volumecache_test.go index b1121cd2834..766c5a6ebb6 100644 --- a/pkg/controller/volume/selinuxwarning/cache/volumecache_test.go +++ b/pkg/controller/volume/selinuxwarning/cache/volumecache_test.go @@ -147,8 +147,8 @@ func addReverseConflict(conflicts []Conflict) []Conflict { return newConflicts } -// Test AddVolume and SendConflicts together, they both provide []conflict with the same data -func TestVolumeCache_AddVolumeSendConflicts(t *testing.T) { +// Test that AddVolume and GetConflicts return the same []conflict data +func TestVolumeCache_AddVolumeGetConflicts(t *testing.T) { existingPods := []podWithVolume{ { podNamespace: "ns1", @@ -488,18 +488,8 @@ func TestVolumeCache_AddVolumeSendConflicts(t *testing.T) { // Verify reverse index consistency verifyReverseIndexConsistency(t, c) - // Act again: get the conflicts via SendConflicts - ch := make(chan Conflict) - go func() { - c.SendConflicts(logger, ch) - close(ch) - }() - - // Assert - receivedConflicts := []Conflict{} - for c := range ch { - receivedConflicts = append(receivedConflicts, c) - } + // Verify that GetConflicts returns the same conflicts + receivedConflicts := c.GetConflicts(logger) sortConflicts(receivedConflicts) if !reflect.DeepEqual(receivedConflicts, expectedConflicts) { t.Errorf("SendConflicts returned unexpected conflicts: %+v", receivedConflicts) @@ -509,6 +499,182 @@ func TestVolumeCache_AddVolumeSendConflicts(t *testing.T) { } } +// Test that conflicts are tracked per-volume: a pod with conflicts on +// multiple volumes retains all of them after successive AddVolume calls. +func TestVolumeCache_MultiVolumeConflicts(t *testing.T) { + logger, _ := getTestLoggers(t) + seLinuxTranslator := &translator.ControllerSELinuxTranslator{} + c := NewVolumeLabelCache(seLinuxTranslator).(*volumeCache) + + podA := cache.ObjectName{Namespace: "ns", Name: "podA"} + podB := cache.ObjectName{Namespace: "ns", Name: "podB"} + podC := cache.ObjectName{Namespace: "ns", Name: "podC"} + + // podB uses vol1 with label1 + c.AddVolume(logger, "vol1", podB, "system_u:system_r:labelB", v1.SELinuxChangePolicyMountOption, "driver1") + // podC uses vol2 with label2 + c.AddVolume(logger, "vol2", podC, "system_u:system_r:labelC", v1.SELinuxChangePolicyMountOption, "driver1") + + // podA uses vol1 with a different label (conflict with podB) + conflicts1 := c.AddVolume(logger, "vol1", podA, "system_u:system_r:labelA", v1.SELinuxChangePolicyMountOption, "driver1") + if len(conflicts1) == 0 { + t.Fatal("Expected conflicts on vol1 between podA and podB") + } + + // podA also uses vol2 with a different label (conflict with podC) + conflicts2 := c.AddVolume(logger, "vol2", podA, "system_u:system_r:labelA", v1.SELinuxChangePolicyMountOption, "driver1") + if len(conflicts2) == 0 { + t.Fatal("Expected conflicts on vol2 between podA and podC") + } + + // GetConflicts must return conflicts from BOTH volumes + allConflicts := c.GetConflicts(logger) + expectedCount := len(conflicts1) + len(conflicts2) + if len(allConflicts) != expectedCount { + t.Errorf("GetConflicts returned %d conflicts, expected %d (vol1: %d + vol2: %d)", + len(allConflicts), expectedCount, len(conflicts1), len(conflicts2)) + } + + // After deleting podA, all conflicts should be gone + c.DeletePod(logger, podA) + remaining := c.GetConflicts(logger) + if len(remaining) != 0 { + t.Errorf("Expected no conflicts after deleting podA, got %d: %+v", len(remaining), remaining) + } +} + +func TestVolumeCache_DeletePodConflicts(t *testing.T) { + podA := cache.ObjectName{Namespace: "ns", Name: "podA"} + podB := cache.ObjectName{Namespace: "ns", Name: "podB"} + podC := cache.ObjectName{Namespace: "ns", Name: "podC"} + podD := cache.ObjectName{Namespace: "ns", Name: "podD"} + + tests := []struct { + name string + // Pods to add before deletion. + initialPods []podWithVolume + // Pod to delete. + podToDelete cache.ObjectName + // If true, delete the pod a second time to verify idempotency. + deleteTwice bool + // Pod pairs that must still have symmetric conflicts after deletion. + // Each pair [2]cache.ObjectName expects both (A→B) and (B→A) to be present. + expectedSurvivingPairs [][2]cache.ObjectName + }{ + { + name: "delete one of two conflicting pods clears all conflicts", + initialPods: []podWithVolume{ + {podNamespace: "ns", podName: "podA", volumeName: "vol1", label: "system_u:system_r:labelA", changePolicy: v1.SELinuxChangePolicyMountOption}, + {podNamespace: "ns", podName: "podB", volumeName: "vol1", label: "system_u:system_r:labelB", changePolicy: v1.SELinuxChangePolicyMountOption}, + }, + podToDelete: podA, + expectedSurvivingPairs: nil, + }, + { + name: "delete non-conflicting pod preserves existing conflicts", + initialPods: []podWithVolume{ + {podNamespace: "ns", podName: "podA", volumeName: "vol1", label: "system_u:system_r:labelA", changePolicy: v1.SELinuxChangePolicyMountOption}, + {podNamespace: "ns", podName: "podB", volumeName: "vol1", label: "system_u:system_r:labelB", changePolicy: v1.SELinuxChangePolicyMountOption}, + {podNamespace: "ns", podName: "podC", volumeName: "vol2", label: "system_u:system_r:labelC", changePolicy: v1.SELinuxChangePolicyMountOption}, + }, + podToDelete: podC, + expectedSurvivingPairs: [][2]cache.ObjectName{{podA, podB}}, + }, + { + name: "three pods on same volume delete one leaves remaining pair conflict", + initialPods: []podWithVolume{ + {podNamespace: "ns", podName: "podA", volumeName: "vol1", label: "system_u:system_r:labelA", changePolicy: v1.SELinuxChangePolicyMountOption}, + {podNamespace: "ns", podName: "podB", volumeName: "vol1", label: "system_u:system_r:labelB", changePolicy: v1.SELinuxChangePolicyMountOption}, + {podNamespace: "ns", podName: "podC", volumeName: "vol1", label: "system_u:system_r:labelC", changePolicy: v1.SELinuxChangePolicyMountOption}, + }, + podToDelete: podA, + expectedSurvivingPairs: [][2]cache.ObjectName{{podB, podC}}, + }, + { + name: "delete pod with conflicts on multiple volumes", + initialPods: []podWithVolume{ + {podNamespace: "ns", podName: "podB", volumeName: "vol1", label: "system_u:system_r:labelB", changePolicy: v1.SELinuxChangePolicyMountOption}, + {podNamespace: "ns", podName: "podC", volumeName: "vol2", label: "system_u:system_r:labelC", changePolicy: v1.SELinuxChangePolicyMountOption}, + {podNamespace: "ns", podName: "podA", volumeName: "vol1", label: "system_u:system_r:labelA", changePolicy: v1.SELinuxChangePolicyMountOption}, + {podNamespace: "ns", podName: "podA", volumeName: "vol2", label: "system_u:system_r:labelA", changePolicy: v1.SELinuxChangePolicyMountOption}, + }, + podToDelete: podA, + expectedSurvivingPairs: nil, + }, + { + name: "delete pod preserves conflicts on unrelated volumes", + initialPods: []podWithVolume{ + {podNamespace: "ns", podName: "podA", volumeName: "vol1", label: "system_u:system_r:labelA", changePolicy: v1.SELinuxChangePolicyMountOption}, + {podNamespace: "ns", podName: "podB", volumeName: "vol1", label: "system_u:system_r:labelB", changePolicy: v1.SELinuxChangePolicyMountOption}, + {podNamespace: "ns", podName: "podC", volumeName: "vol2", label: "system_u:system_r:labelC", changePolicy: v1.SELinuxChangePolicyMountOption}, + {podNamespace: "ns", podName: "podD", volumeName: "vol2", label: "system_u:system_r:labelD", changePolicy: v1.SELinuxChangePolicyMountOption}, + }, + podToDelete: podA, + expectedSurvivingPairs: [][2]cache.ObjectName{{podC, podD}}, + }, + { + name: "delete pod that was already deleted is a no-op", + initialPods: []podWithVolume{ + {podNamespace: "ns", podName: "podA", volumeName: "vol1", label: "system_u:system_r:labelA", changePolicy: v1.SELinuxChangePolicyMountOption}, + {podNamespace: "ns", podName: "podB", volumeName: "vol1", label: "system_u:system_r:labelB", changePolicy: v1.SELinuxChangePolicyMountOption}, + }, + podToDelete: podA, + deleteTwice: true, + expectedSurvivingPairs: nil, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + logger, _ := getTestLoggers(t) + seLinuxTranslator := &translator.ControllerSELinuxTranslator{} + c := NewVolumeLabelCache(seLinuxTranslator).(*volumeCache) + + for _, pod := range tt.initialPods { + c.AddVolume(logger, pod.volumeName, cache.ObjectName{Namespace: pod.podNamespace, Name: pod.podName}, pod.label, pod.changePolicy, "driver1") + } + + c.DeletePod(logger, tt.podToDelete) + if tt.deleteTwice { + c.DeletePod(logger, tt.podToDelete) + } + + remaining := c.GetConflicts(logger) + + // Deleted pod must not appear in any conflict + for _, conflict := range remaining { + if conflict.Pod == tt.podToDelete || conflict.OtherPod == tt.podToDelete { + t.Errorf("found conflict involving deleted pod %s: %+v", tt.podToDelete, conflict) + } + } + + // 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) + } + + verifyReverseIndexConsistency(t, c) + }) + } +} + func TestVolumeCache_GetPodsForCSIDriver(t *testing.T) { seLinuxTranslator := &translator.ControllerSELinuxTranslator{} c := NewVolumeLabelCache(seLinuxTranslator).(*volumeCache) diff --git a/pkg/controller/volume/selinuxwarning/metrics.go b/pkg/controller/volume/selinuxwarning/metrics.go index d95665c9162..c285bd78db4 100644 --- a/pkg/controller/volume/selinuxwarning/metrics.go +++ b/pkg/controller/volume/selinuxwarning/metrics.go @@ -59,13 +59,7 @@ func (c *collector) DescribeWithStability(ch chan<- *metrics.Desc) { } func (c *collector) CollectWithStability(ch chan<- metrics.Metric) { - conflictCh := make(chan cache.Conflict) - go func() { - c.cache.SendConflicts(c.logger, conflictCh) - close(conflictCh) - }() - - for conflict := range conflictCh { + for _, conflict := range c.cache.GetConflicts(c.logger) { ch <- metrics.NewLazyConstMetric(seLinuxConflictDesc, metrics.GaugeValue, 1.0, diff --git a/pkg/controller/volume/selinuxwarning/selinux_warning_controller_test.go b/pkg/controller/volume/selinuxwarning/selinux_warning_controller_test.go index ccb4c4bfba3..48832947a30 100644 --- a/pkg/controller/volume/selinuxwarning/selinux_warning_controller_test.go +++ b/pkg/controller/volume/selinuxwarning/selinux_warning_controller_test.go @@ -778,12 +778,12 @@ func (f *fakeVolumeCache) GetPodsForCSIDriver(driverName string) []cache.ObjectN return pods } -func (f *fakeVolumeCache) SendConflicts(logger klog.Logger, ch chan<- volumecache.Conflict) { +func (f *fakeVolumeCache) GetConflicts(logger klog.Logger) []volumecache.Conflict { + result := make([]volumecache.Conflict, 0) for _, conflicts := range f.conflictsToSend { - for _, conflict := range conflicts { - ch <- conflict - } + result = append(result, conflicts...) } + return result } func collectEvents(source <-chan string) []string {