diff --git a/pkg/controller/garbagecollector/BUILD b/pkg/controller/garbagecollector/BUILD index 3a534936465..792ed2e1a9a 100644 --- a/pkg/controller/garbagecollector/BUILD +++ b/pkg/controller/garbagecollector/BUILD @@ -83,6 +83,7 @@ go_test( "//vendor/github.com/stretchr/testify/assert:go_default_library", "//vendor/gonum.org/v1/gonum/graph:go_default_library", "//vendor/gonum.org/v1/gonum/graph/simple:go_default_library", + "//vendor/k8s.io/utils/pointer:go_default_library", ], ) diff --git a/pkg/controller/garbagecollector/garbagecollector.go b/pkg/controller/garbagecollector/garbagecollector.go index 872520fc359..341bb921237 100644 --- a/pkg/controller/garbagecollector/garbagecollector.go +++ b/pkg/controller/garbagecollector/garbagecollector.go @@ -72,7 +72,7 @@ type GarbageCollector struct { attemptToOrphan workqueue.RateLimitingInterface dependencyGraphBuilder *GraphBuilder // GC caches the owners that do not exist according to the API server. - absentOwnerCache *UIDCache + absentOwnerCache *ReferenceCache workerLock sync.RWMutex } @@ -94,7 +94,7 @@ func NewGarbageCollector( attemptToDelete := workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "garbage_collector_attempt_to_delete") attemptToOrphan := workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "garbage_collector_attempt_to_orphan") - absentOwnerCache := NewUIDCache(500) + absentOwnerCache := NewReferenceCache(500) gc := &GarbageCollector{ metadataClient: metadataClient, restMapper: mapper, @@ -338,10 +338,20 @@ func (gc *GarbageCollector) attemptToDeleteWorker() bool { // returns its latest state. func (gc *GarbageCollector) isDangling(reference metav1.OwnerReference, item *node) ( dangling bool, owner *metav1.PartialObjectMetadata, err error) { - if gc.absentOwnerCache.Has(reference.UID) { + + // check for recorded absent cluster-scoped parent + absentOwnerCacheKey := objectReference{OwnerReference: ownerReferenceCoordinates(reference)} + if gc.absentOwnerCache.Has(absentOwnerCacheKey) { klog.V(5).Infof("according to the absentOwnerCache, object %s's owner %s/%s, %s does not exist", item.identity.UID, reference.APIVersion, reference.Kind, reference.Name) return true, nil, nil } + // check for recorded absent namespaced parent + absentOwnerCacheKey.Namespace = item.identity.Namespace + if gc.absentOwnerCache.Has(absentOwnerCacheKey) { + klog.V(5).Infof("according to the absentOwnerCache, object %s's owner %s/%s, %s does not exist in namespace %s", item.identity.UID, reference.APIVersion, reference.Kind, reference.Name, item.identity.Namespace) + return true, nil, nil + } + // TODO: we need to verify the reference resource is supported by the // system. If it's not a valid resource, the garbage collector should i) // ignore the reference when decide if the object should be deleted, and @@ -352,6 +362,9 @@ func (gc *GarbageCollector) isDangling(reference metav1.OwnerReference, item *no if err != nil { return false, nil, err } + if !namespaced { + absentOwnerCacheKey.Namespace = "" + } // TODO: It's only necessary to talk to the API server if the owner node // is a "virtual" node. The local graph could lag behind the real @@ -359,7 +372,7 @@ func (gc *GarbageCollector) isDangling(reference metav1.OwnerReference, item *no owner, err = gc.metadataClient.Resource(resource).Namespace(resourceDefaultNamespace(namespaced, item.identity.Namespace)).Get(context.TODO(), reference.Name, metav1.GetOptions{}) switch { case errors.IsNotFound(err): - gc.absentOwnerCache.Add(reference.UID) + gc.absentOwnerCache.Add(absentOwnerCacheKey) klog.V(5).Infof("object %s's owner %s/%s, %s is not found", item.identity.UID, reference.APIVersion, reference.Kind, reference.Name) return true, nil, nil case err != nil: @@ -368,7 +381,7 @@ func (gc *GarbageCollector) isDangling(reference metav1.OwnerReference, item *no if owner.GetUID() != reference.UID { klog.V(5).Infof("object %s's owner %s/%s, %s is not found, UID mismatch", item.identity.UID, reference.APIVersion, reference.Kind, reference.Name) - gc.absentOwnerCache.Add(reference.UID) + gc.absentOwnerCache.Add(absentOwnerCacheKey) return true, nil, nil } return false, owner, nil diff --git a/pkg/controller/garbagecollector/garbagecollector_test.go b/pkg/controller/garbagecollector/garbagecollector_test.go index f98704d9953..caeb0910bd4 100644 --- a/pkg/controller/garbagecollector/garbagecollector_test.go +++ b/pkg/controller/garbagecollector/garbagecollector_test.go @@ -29,6 +29,7 @@ import ( "github.com/stretchr/testify/assert" _ "k8s.io/kubernetes/pkg/apis/core/install" + "k8s.io/utils/pointer" v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/meta" @@ -398,7 +399,7 @@ func TestProcessEvent(t *testing.T) { uidToNode: make(map[types.UID]*node), }, attemptToDelete: workqueue.NewRateLimitingQueue(workqueue.DefaultControllerRateLimiter()), - absentOwnerCache: NewUIDCache(2), + absentOwnerCache: NewReferenceCache(2), } for i := 0; i < len(scenario.events); i++ { dependencyGraphBuilder.graphChanges.Add(&scenario.events[i]) @@ -459,13 +460,14 @@ func podToGCNode(pod *v1.Pod) *node { } } -func TestAbsentUIDCache(t *testing.T) { +func TestAbsentOwnerCache(t *testing.T) { rc1Pod1 := getPod("rc1Pod1", []metav1.OwnerReference{ { Kind: "ReplicationController", Name: "rc1", UID: "1", APIVersion: "v1", + Controller: pointer.BoolPtr(true), }, }) rc1Pod2 := getPod("rc1Pod2", []metav1.OwnerReference{ @@ -474,6 +476,7 @@ func TestAbsentUIDCache(t *testing.T) { Name: "rc1", UID: "1", APIVersion: "v1", + Controller: pointer.BoolPtr(false), }, }) rc2Pod1 := getPod("rc2Pod1", []metav1.OwnerReference{ @@ -528,7 +531,7 @@ func TestAbsentUIDCache(t *testing.T) { defer srv.Close() gc := setupGC(t, clientConfig) defer close(gc.stop) - gc.absentOwnerCache = NewUIDCache(2) + gc.absentOwnerCache = NewReferenceCache(2) gc.attemptToDeleteItem(podToGCNode(rc1Pod1)) gc.attemptToDeleteItem(podToGCNode(rc2Pod1)) // rc1 should already be in the cache, no request should be sent. rc1 should be promoted in the UIDCache @@ -536,13 +539,13 @@ func TestAbsentUIDCache(t *testing.T) { // after this call, rc2 should be evicted from the UIDCache gc.attemptToDeleteItem(podToGCNode(rc3Pod1)) // check cache - if !gc.absentOwnerCache.Has(types.UID("1")) { + if !gc.absentOwnerCache.Has(objectReference{Namespace: "ns1", OwnerReference: metav1.OwnerReference{Kind: "ReplicationController", Name: "rc1", UID: "1", APIVersion: "v1"}}) { t.Errorf("expected rc1 to be in the cache") } - if gc.absentOwnerCache.Has(types.UID("2")) { + if gc.absentOwnerCache.Has(objectReference{Namespace: "ns1", OwnerReference: metav1.OwnerReference{Kind: "ReplicationController", Name: "rc2", UID: "2", APIVersion: "v1"}}) { t.Errorf("expected rc2 to not exist in the cache") } - if !gc.absentOwnerCache.Has(types.UID("3")) { + if !gc.absentOwnerCache.Has(objectReference{Namespace: "ns1", OwnerReference: metav1.OwnerReference{Kind: "ReplicationController", Name: "rc3", UID: "3", APIVersion: "v1"}}) { t.Errorf("expected rc3 to be in the cache") } // check the request sent to the server diff --git a/pkg/controller/garbagecollector/graph.go b/pkg/controller/garbagecollector/graph.go index 2d19a150dd6..252a56e98a9 100644 --- a/pkg/controller/garbagecollector/graph.go +++ b/pkg/controller/garbagecollector/graph.go @@ -148,6 +148,17 @@ func (n *node) blockingDependents() []*node { return ret } +// ownerReferenceCoordinates returns an owner reference containing only the coordinate fields +// from the input reference (uid, name, kind, apiVersion) +func ownerReferenceCoordinates(ref metav1.OwnerReference) metav1.OwnerReference { + return metav1.OwnerReference{ + UID: ref.UID, + Name: ref.Name, + Kind: ref.Kind, + APIVersion: ref.APIVersion, + } +} + // String renders node as a string using fmt. Acquires a read lock to ensure the // reflective dump of dependents doesn't race with any concurrent writes. func (n *node) String() string { diff --git a/pkg/controller/garbagecollector/graph_builder.go b/pkg/controller/garbagecollector/graph_builder.go index 7aba0de1604..734eaea8bd7 100644 --- a/pkg/controller/garbagecollector/graph_builder.go +++ b/pkg/controller/garbagecollector/graph_builder.go @@ -104,7 +104,7 @@ type GraphBuilder struct { attemptToOrphan workqueue.RateLimitingInterface // GraphBuilder and GC share the absentOwnerCache. Objects that are known to // be non-existent are added to the cached. - absentOwnerCache *UIDCache + absentOwnerCache *ReferenceCache sharedInformers informerfactory.InformerFactory ignoredResources map[schema.GroupResource]struct{} } @@ -327,8 +327,10 @@ func DefaultIgnoredResources() map[schema.GroupResource]struct{} { // enqueueVirtualDeleteEvent is used to add a virtual delete event to be processed for virtual nodes // once it is determined they do not have backing objects in storage func (gb *GraphBuilder) enqueueVirtualDeleteEvent(ref objectReference) { + gv, _ := schema.ParseGroupVersion(ref.APIVersion) gb.graphChanges.Add(&event{ eventType: deleteEvent, + gvk: gv.WithKind(ref.Kind), obj: &metaonly.MetadataOnlyObject{ TypeMeta: metav1.TypeMeta{APIVersion: ref.APIVersion, Kind: ref.Kind}, ObjectMeta: metav1.ObjectMeta{Namespace: ref.Namespace, UID: ref.UID, Name: ref.Name}, @@ -348,7 +350,7 @@ func (gb *GraphBuilder) addDependentToOwners(n *node, owners []metav1.OwnerRefer // exist in the graph yet. ownerNode = &node{ identity: objectReference{ - OwnerReference: owner, + OwnerReference: ownerReferenceCoordinates(owner), Namespace: n.identity.Namespace, }, dependents: make(map[*node]struct{}), @@ -603,7 +605,15 @@ func (gb *GraphBuilder) processGraphChanges() bool { existingNode.dependentsLock.RLock() defer existingNode.dependentsLock.RUnlock() if len(existingNode.dependents) > 0 { - gb.absentOwnerCache.Add(accessor.GetUID()) + gb.absentOwnerCache.Add(objectReference{ + OwnerReference: metav1.OwnerReference{ + APIVersion: event.gvk.GroupVersion().String(), + Kind: event.gvk.Kind, + Name: accessor.GetName(), + UID: accessor.GetUID(), + }, + Namespace: accessor.GetNamespace(), + }) } for dep := range existingNode.dependents { gb.attemptToDelete.Add(dep) diff --git a/pkg/controller/garbagecollector/uid_cache.go b/pkg/controller/garbagecollector/uid_cache.go index 3ad40c32b0f..6206ac80738 100644 --- a/pkg/controller/garbagecollector/uid_cache.go +++ b/pkg/controller/garbagecollector/uid_cache.go @@ -20,33 +20,32 @@ import ( "sync" "github.com/golang/groupcache/lru" - "k8s.io/apimachinery/pkg/types" ) -// UIDCache is an LRU cache for uid. -type UIDCache struct { +// ReferenceCache is an LRU cache for uid. +type ReferenceCache struct { mutex sync.Mutex cache *lru.Cache } -// NewUIDCache returns a UIDCache. -func NewUIDCache(maxCacheEntries int) *UIDCache { - return &UIDCache{ +// NewReferenceCache returns a ReferenceCache. +func NewReferenceCache(maxCacheEntries int) *ReferenceCache { + return &ReferenceCache{ cache: lru.New(maxCacheEntries), } } // Add adds a uid to the cache. -func (c *UIDCache) Add(uid types.UID) { +func (c *ReferenceCache) Add(reference objectReference) { c.mutex.Lock() defer c.mutex.Unlock() - c.cache.Add(uid, nil) + c.cache.Add(reference, nil) } // Has returns if a uid is in the cache. -func (c *UIDCache) Has(uid types.UID) bool { +func (c *ReferenceCache) Has(reference objectReference) bool { c.mutex.Lock() defer c.mutex.Unlock() - _, found := c.cache.Get(uid) + _, found := c.cache.Get(reference) return found }