Merge pull request #135647 from grandeit/fix-gc-data-race-135621

Fix data race in garbage collector on node.owners field

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

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)
 }
 

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

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)
 	}