From cae56bea0a7192dfae234ef30c4ac19a6249dd7f Mon Sep 17 00:00:00 2001
From: Jordan Liggitt <liggitt@google.com>
Date: Fri, 26 Jun 2020 23:51:24 -0400
Subject: [PATCH] Replace virtual node with observed node if identity differs

If the graph contains a virtual node (because some child object referenced it in an OwnerRef),
and a real informer event is observed for that uid at different coordinates,
we want to fix the coordinates of the node in the graph to match the actual coordinates.

The safe way to do this is to clone the node, replace the identity in the clone,
then replace the node with the clone.

Modifying the identity directly is not safe because it is accessed lock-free from many code paths.

Replacing the node in the graph from processGraphChanges is safe because it is the only graph writer.
---
 pkg/controller/garbagecollector/graph.go      | 19 +++++++++++++++++++
 .../garbagecollector/graph_builder.go         |  8 ++++++++
 2 files changed, 27 insertions(+)

diff --git a/pkg/controller/garbagecollector/graph.go b/pkg/controller/garbagecollector/graph.go
index 252a56e98a9..d98b83b7d54 100644
--- a/pkg/controller/garbagecollector/graph.go
+++ b/pkg/controller/garbagecollector/graph.go
@@ -61,6 +61,25 @@ type node struct {
 	owners []metav1.OwnerReference
 }
 
+// clone() must only be called from the single-threaded GraphBuilder.processGraphChanges()
+func (n *node) clone() *node {
+	c := &node{
+		identity:           n.identity,
+		dependents:         make(map[*node]struct{}, len(n.dependents)),
+		deletingDependents: n.deletingDependents,
+		beingDeleted:       n.beingDeleted,
+		virtual:            n.virtual,
+		owners:             make([]metav1.OwnerReference, 0, len(n.owners)),
+	}
+	for dep := range n.dependents {
+		c.dependents[dep] = struct{}{}
+	}
+	for _, owner := range n.owners {
+		c.owners = append(c.owners, owner)
+	}
+	return c
+}
+
 // An object is on a one way trip to its final deletion if it starts being
 // deleted, so we only provide a function to set beingDeleted to true.
 func (n *node) markBeingDeleted() {
diff --git a/pkg/controller/garbagecollector/graph_builder.go b/pkg/controller/garbagecollector/graph_builder.go
index ddf4037abc9..1de1685f165 100644
--- a/pkg/controller/garbagecollector/graph_builder.go
+++ b/pkg/controller/garbagecollector/graph_builder.go
@@ -567,6 +567,14 @@ func (gb *GraphBuilder) processGraphChanges() bool {
 		// this marks the node as having been observed via an informer event
 		// 1. this depends on graphChanges only containing add/update events from the actual informer
 		// 2. this allows things tracking virtual nodes' existence to stop polling and rely on informer events
+		observedIdentity := identityFromEvent(event, accessor)
+		if observedIdentity != existingNode.identity {
+			// make a copy (so we don't modify the existing node in place), store the observed identity, and replace the virtual node
+			klog.V(2).Infof("replacing virtual node %s with observed node %s", existingNode.identity, observedIdentity)
+			existingNode = existingNode.clone()
+			existingNode.identity = observedIdentity
+			gb.uidToNode.Write(existingNode)
+		}
 		existingNode.markObserved()
 	}
 	switch {