From b18339638aed6707c91a92456a6b29b3512ef35c Mon Sep 17 00:00:00 2001 From: Manuel Grandeit Date: Sat, 6 Dec 2025 21:28:21 +0100 Subject: [PATCH] Fix data race in garbage collector on node.owners field Add ownersLock to protect concurrent access to node.owners between GraphBuilder.processGraphChanges() (writer) and GC worker goroutines reading in blockingDependents() and unblockOwnerReferences() methods. Also fix concurrent reads in the HTTP debug handler (/graph endpoint) for owners, dependents, beingDeleted, deletingDependents, and virtual fields by using their respective thread-safe accessor methods. --- pkg/controller/garbagecollector/dump.go | 14 ++++++------- pkg/controller/garbagecollector/graph.go | 20 +++++++++++++++++-- .../garbagecollector/graph_builder.go | 2 +- pkg/controller/garbagecollector/patch.go | 4 ++-- 4 files changed, 28 insertions(+), 12 deletions(-) diff --git a/pkg/controller/garbagecollector/dump.go b/pkg/controller/garbagecollector/dump.go index 1943a7f1fa3..884255cc413 100644 --- a/pkg/controller/garbagecollector/dump.go +++ b/pkg/controller/garbagecollector/dump.go @@ -148,9 +148,9 @@ func NewDOTVertex(node *node) *dotVertex { gvk: gv.WithKind(node.identity.Kind), namespace: node.identity.Namespace, name: node.identity.Name, - beingDeleted: node.beingDeleted, - deletingDependents: node.deletingDependents, - virtual: node.virtual, + beingDeleted: node.isBeingDeleted(), + deletingDependents: node.isDeletingDependents(), + virtual: !node.isObserved(), } } @@ -195,7 +195,7 @@ func toDOTNodesAndEdges(uidToNode map[types.UID]*node) ([]*dotVertex, []dotEdge) // add the vertices first, then edges. That avoids having to deal with missing refs. for _, node := range uidToNode { // skip adding objects that don't have owner references and aren't referred to. - if len(node.dependents) == 0 && len(node.owners) == 0 { + if node.dependentsLength() == 0 && len(node.getOwners()) == 0 { continue } vertex := NewDOTVertex(node) @@ -204,7 +204,7 @@ func toDOTNodesAndEdges(uidToNode map[types.UID]*node) ([]*dotVertex, []dotEdge) } for _, node := range uidToNode { currVertex := uidToVertex[node.identity.UID] - for _, ownerRef := range node.owners { + for _, ownerRef := range node.getOwners() { currOwnerVertex, ok := uidToVertex[ownerRef.UID] if !ok { currOwnerVertex = NewMissingdotVertex(ownerRef) @@ -253,14 +253,14 @@ func toDOTNodesAndEdgesForObj(uidToNode map[types.UID]*node, uids ...types.UID) interestingNodes[node.identity.UID] = node - for _, ownerRef := range node.owners { + for _, ownerRef := range node.getOwners() { // if we've already inspected this UID, don't add it to be inspected again if _, ok := interestingNodes[ownerRef.UID]; ok { continue } uidsToCheck = append(uidsToCheck, ownerRef.UID) } - for dependent := range node.dependents { + for _, dependent := range node.getDependents() { // if we've already inspected this UID, don't add it to be inspected again if _, ok := interestingNodes[dependent.identity.UID]; ok { continue diff --git a/pkg/controller/garbagecollector/graph.go b/pkg/controller/garbagecollector/graph.go index 77715309729..432e6537c63 100644 --- a/pkg/controller/garbagecollector/graph.go +++ b/pkg/controller/garbagecollector/graph.go @@ -79,7 +79,8 @@ type node struct { virtualLock sync.RWMutex // when processing an Update event, we need to compare the updated // ownerReferences with the owners recorded in the graph. - owners []metav1.OwnerReference + owners []metav1.OwnerReference + ownersLock sync.RWMutex } // clone() must only be called from the single-threaded GraphBuilder.processGraphChanges() @@ -138,6 +139,18 @@ func (n *node) isDeletingDependents() bool { return n.deletingDependents } +func (n *node) setOwners(owners []metav1.OwnerReference) { + n.ownersLock.Lock() + defer n.ownersLock.Unlock() + n.owners = owners +} + +func (n *node) getOwners() []metav1.OwnerReference { + n.ownersLock.RLock() + defer n.ownersLock.RUnlock() + return n.owners +} + func (n *node) addDependent(dependent *node) { n.dependentsLock.Lock() defer n.dependentsLock.Unlock() @@ -179,7 +192,7 @@ func (n *node) blockingDependents() []*node { dependents := n.getDependents() var ret []*node for _, dep := range dependents { - for _, owner := range dep.owners { + for _, owner := range dep.getOwners() { if owner.UID == n.identity.UID && owner.BlockOwnerDeletion != nil && *owner.BlockOwnerDeletion { ret = append(ret, dep) } @@ -217,6 +230,9 @@ func (n *node) String() string { n.dependentsLock.RLock() defer n.dependentsLock.RUnlock() + n.ownersLock.RLock() + defer n.ownersLock.RUnlock() + return fmt.Sprintf("%#v", n) } diff --git a/pkg/controller/garbagecollector/graph_builder.go b/pkg/controller/garbagecollector/graph_builder.go index 7348576e291..893dabb6fe9 100644 --- a/pkg/controller/garbagecollector/graph_builder.go +++ b/pkg/controller/garbagecollector/graph_builder.go @@ -783,7 +783,7 @@ func (gb *GraphBuilder) processGraphChanges(logger klog.Logger) bool { // waiting for the deletion of their dependents. gb.addUnblockedOwnersToDeleteQueue(logger, removed, changed) // update the node itself - existingNode.owners = accessor.GetOwnerReferences() + existingNode.setOwners(accessor.GetOwnerReferences()) // Add the node to its new owners' dependent lists. gb.addDependentToOwners(logger, existingNode, added) // remove the node from the dependent list of node that are no longer in diff --git a/pkg/controller/garbagecollector/patch.go b/pkg/controller/garbagecollector/patch.go index 25d8b208513..2d69d328029 100644 --- a/pkg/controller/garbagecollector/patch.go +++ b/pkg/controller/garbagecollector/patch.go @@ -134,7 +134,7 @@ func (n *node) unblockOwnerReferencesStrategicMergePatch() ([]byte, error) { var dummy metaonly.MetadataOnlyObject var blockingRefs []metav1.OwnerReference falseVar := false - for _, owner := range n.owners { + for _, owner := range n.getOwners() { if owner.BlockOwnerDeletion != nil && *owner.BlockOwnerDeletion { ref := owner ref.BlockOwnerDeletion = &falseVar @@ -157,7 +157,7 @@ func (gc *GarbageCollector) unblockOwnerReferencesJSONMergePatch(n *node) ([]byt expectedObjectMeta.ResourceVersion = accessor.GetResourceVersion() var expectedOwners []metav1.OwnerReference falseVar := false - for _, owner := range n.owners { + for _, owner := range n.getOwners() { owner.BlockOwnerDeletion = &falseVar expectedOwners = append(expectedOwners, owner) }