diff --git a/pkg/controller/controller_utils.go b/pkg/controller/controller_utils.go index 4ae1e1369e1..ff73903d0a5 100644 --- a/pkg/controller/controller_utils.go +++ b/pkg/controller/controller_utils.go @@ -36,6 +36,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/rand" + utilruntime "k8s.io/apimachinery/pkg/util/runtime" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/strategicpatch" "k8s.io/apimachinery/pkg/util/wait" @@ -87,12 +88,9 @@ const ( // PodNodeNameKeyIndex is the name of the index used by PodInformer to index pods by their node name. PodNodeNameKeyIndex = "spec.nodeName" - // OrphanPodIndexKey is used to index all Orphan pods to this key - OrphanPodIndexKey = "_ORPHAN_POD" - - // podControllerUIDIndex is the name for the Pod store's index function, - // which is to index by pods's controllerUID. - PodControllerUIDIndex = "podControllerUID" + // PodControllerIndex is the name for the Pod store's index function, + // which indexes by the key returned from PodControllerIndexKey. + PodControllerIndex = "podController" ) var UpdateTaintBackoff = wait.Backoff{ @@ -1083,35 +1081,74 @@ func AddPodNodeNameIndexer(podInformer cache.SharedIndexInformer) error { }) } -// OrphanPodIndexKeyForNamespace returns the orphan pod index key for a specific namespace. -func OrphanPodIndexKeyForNamespace(namespace string) string { - return OrphanPodIndexKey + "/" + namespace +// PodControllerIndexKey returns the index key to locate pods with the specified controller ownerReference. +// If ownerReference is nil, the returned key locates pods in the namespace without a controller ownerReference. +func PodControllerIndexKey(namespace string, ownerReference *metav1.OwnerReference) string { + if ownerReference == nil { + return namespace + } + return namespace + "/" + ownerReference.Kind + "/" + ownerReference.Name + "/" + string(ownerReference.UID) } -// AddPodControllerUIDIndexer adds an indexer for Pod's controllerRef.UID to the given PodInformer. +// AddPodControllerIndexer adds an indexer for Pod's controllerRef.UID to the given PodInformer. // This indexer is used to efficiently look up pods by their ControllerRef.UID -func AddPodControllerUIDIndexer(podInformer cache.SharedIndexInformer) error { - if _, exists := podInformer.GetIndexer().GetIndexers()[PodControllerUIDIndex]; exists { +func AddPodControllerIndexer(podInformer cache.SharedIndexInformer) error { + if _, exists := podInformer.GetIndexer().GetIndexers()[PodControllerIndex]; exists { // indexer already exists, do nothing return nil } return podInformer.AddIndexers(cache.Indexers{ - PodControllerUIDIndex: func(obj interface{}) ([]string, error) { + PodControllerIndex: func(obj interface{}) ([]string, error) { pod, ok := obj.(*v1.Pod) if !ok { return nil, nil } - // Get the ControllerRef of the Pod to check if it's managed by a controller - if ref := metav1.GetControllerOf(pod); ref != nil { - return []string{string(ref.UID)}, nil - } - // If the Pod has no controller (i.e., it's orphaned), index it with the OrphanPodIndexKeyForNamespace - // This helps identify orphan pods for reconciliation and adoption by controllers - return []string{OrphanPodIndexKeyForNamespace(pod.Namespace)}, nil + // Get the ControllerRef of the Pod to check if it's managed by a controller. + // Index with a non-nil controller (indicating an owned pod) or a nil controller (indicating an orphan pod). + return []string{PodControllerIndexKey(pod.Namespace, metav1.GetControllerOf(pod))}, nil }, }) } +// FilterPodsByOwner gets the Pods managed by an owner or orphan Pods in the owner's namespace +func FilterPodsByOwner(podIndexer cache.Indexer, owner *metav1.ObjectMeta, ownerKind string, includeOrphanedPods bool) ([]*v1.Pod, error) { + result := []*v1.Pod{} + + if len(owner.Namespace) == 0 { + return nil, fmt.Errorf("no owner namespace provided") + } + if len(owner.Name) == 0 { + return nil, fmt.Errorf("no owner name provided") + } + if len(owner.UID) == 0 { + return nil, fmt.Errorf("no owner uid provided") + } + if len(ownerKind) == 0 { + return nil, fmt.Errorf("no owner kind provided") + } + // Always include the owner key, which identifies Pods that are controlled by the owner + keys := []string{PodControllerIndexKey(owner.Namespace, &metav1.OwnerReference{Name: owner.Name, Kind: ownerKind, UID: owner.UID})} + if includeOrphanedPods { + // Optionally include the unowned key, which identifies orphaned Pods in the owner's namespace and might be adopted by the owner later + keys = append(keys, PodControllerIndexKey(owner.Namespace, nil)) + } + for _, key := range keys { + pods, err := podIndexer.ByIndex(PodControllerIndex, key) + if err != nil { + return nil, err + } + for _, obj := range pods { + pod, ok := obj.(*v1.Pod) + if !ok { + utilruntime.HandleError(fmt.Errorf("unexpected object type in pod indexer: %v", obj)) + continue + } + result = append(result, pod) + } + } + return result, nil +} + // PodKey returns a key unique to the given pod within a cluster. // It's used so we consistently use the same key scheme in this module. // It does exactly what cache.MetaNamespaceKeyFunc would have done diff --git a/pkg/controller/controller_utils_test.go b/pkg/controller/controller_utils_test.go index e24add9a555..7cf06a48a5c 100644 --- a/pkg/controller/controller_utils_test.go +++ b/pkg/controller/controller_utils_test.go @@ -39,6 +39,7 @@ import ( "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/uuid" utilfeature "k8s.io/apiserver/pkg/util/feature" + "k8s.io/client-go/informers" clientset "k8s.io/client-go/kubernetes" "k8s.io/client-go/kubernetes/fake" clientscheme "k8s.io/client-go/kubernetes/scheme" @@ -1359,3 +1360,164 @@ func TestAddOrUpdateTaintOnNode(t *testing.T) { test.name, test.requestCount, test.nodeHandler.RequestCount) } } + +func TestFilterPodsByOwner(t *testing.T) { + newPod := func(name, ns string, owner *metav1.OwnerReference) *v1.Pod { + pod := &v1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: ns, + }, + } + if owner != nil { + pod.OwnerReferences = append(pod.OwnerReferences, *owner) + } + return pod + } + + ownerKind := "OwnerKind" + ownerName := "ownerName" + cases := map[string]struct { + owner *metav1.ObjectMeta + ownedOnly bool + allPods []*v1.Pod + wantPodsKeys sets.Set[string] + }{ + "multiple Pods, some are owned by the owner": { + owner: &metav1.ObjectMeta{ + Namespace: "ns1", + Name: ownerName, + UID: "abc", + }, + allPods: []*v1.Pod{ + newPod("a", "ns1", &metav1.OwnerReference{ + UID: "abc", + Kind: ownerKind, + Name: ownerName, + Controller: ptr.To(true), + }), + newPod("b", "ns1", &metav1.OwnerReference{ + UID: "def", + Kind: ownerKind, + Name: ownerName, + Controller: ptr.To(true), + }), + newPod("c", "ns1", &metav1.OwnerReference{ + UID: "abc", + Kind: ownerKind, + Name: ownerName, + Controller: ptr.To(true), + }), + }, + wantPodsKeys: sets.New("ns1/a", "ns1/c"), + }, + "orphan Pods in multiple namespaces": { + owner: &metav1.ObjectMeta{ + Namespace: "ns1", + Name: ownerName, + UID: "abc", + }, + allPods: []*v1.Pod{ + newPod("a", "ns1", nil), + newPod("b", "ns2", nil), + }, + wantPodsKeys: sets.New("ns1/a"), + }, + "owned Pods and orphan Pods in the owner's namespace": { + owner: &metav1.ObjectMeta{ + Namespace: "ns1", + Name: ownerName, + UID: "abc", + }, + allPods: []*v1.Pod{ + newPod("a", "ns1", nil), + newPod("b", "ns2", nil), + newPod("c", "ns1", &metav1.OwnerReference{ + UID: "abc", + Kind: ownerKind, + Name: ownerName, + Controller: ptr.To(true), + }), + }, + wantPodsKeys: sets.New("ns1/a", "ns1/c"), + }, + "exclude orphan pods, pods in mismatched ns,uid,kind,name,controller": { + owner: &metav1.ObjectMeta{ + Namespace: "ns1", + Name: ownerName, + UID: "abc", + }, + allPods: []*v1.Pod{ + newPod("a", "ns1", nil), + newPod("other-ns-orphan", "ns2", nil), + newPod("other-ns-owned", "ns2", &metav1.OwnerReference{ + UID: "abc", + Kind: ownerKind, + Name: ownerName, + Controller: ptr.To(true), + }), + newPod("c", "ns1", &metav1.OwnerReference{ + UID: "abc", + Kind: ownerKind, + Name: ownerName, + Controller: ptr.To(true), + }), + newPod("other-uid", "ns1", &metav1.OwnerReference{ + UID: "other-uid", + Kind: ownerKind, + Name: ownerName, + Controller: ptr.To(true), + }), + newPod("other-kind", "ns1", &metav1.OwnerReference{ + UID: "abc", + Kind: "OtherKind", + Name: ownerName, + Controller: ptr.To(true), + }), + newPod("other-name", "ns1", &metav1.OwnerReference{ + UID: "abc", + Kind: ownerKind, + Name: "otherName", + Controller: ptr.To(true), + }), + newPod("non-controller", "ns1", &metav1.OwnerReference{ + UID: "abc", + Kind: ownerKind, + Name: ownerName, + Controller: ptr.To(false), + }), + }, + ownedOnly: true, + wantPodsKeys: sets.New("ns1/c"), + }, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + fakeClient := fake.NewSimpleClientset() + sharedInformers := informers.NewSharedInformerFactory(fakeClient, 0) + podInformer := sharedInformers.Core().V1().Pods() + + err := AddPodControllerIndexer(podInformer.Informer()) + if err != nil { + t.Fatalf("failed to register indexer: %v", err) + } + podIndexer := podInformer.Informer().GetIndexer() + for _, pod := range tc.allPods { + if err := podIndexer.Add(pod); err != nil { + t.Fatalf("failed adding Pod to indexer: %v", err) + } + } + gotPods, err := FilterPodsByOwner(podIndexer, tc.owner, ownerKind, !tc.ownedOnly) + if err != nil { + t.Fatal(err) + } + gotPodKeys := sets.New[string]() + for _, pod := range gotPods { + gotPodKeys.Insert(pod.Namespace + "/" + pod.Name) + } + if diff := cmp.Diff(tc.wantPodsKeys, gotPodKeys); diff != "" { + t.Errorf("unexpected pods returned, diff=%s", diff) + } + }) + } +} diff --git a/pkg/controller/daemon/daemon_controller.go b/pkg/controller/daemon/daemon_controller.go index a7d419fca99..3041f5ab93b 100644 --- a/pkg/controller/daemon/daemon_controller.go +++ b/pkg/controller/daemon/daemon_controller.go @@ -217,7 +217,7 @@ func NewDaemonSetsController( dsc.podLister = podInformer.Lister() dsc.podStoreSynced = podInformer.Informer().HasSynced controller.AddPodNodeNameIndexer(podInformer.Informer()) - controller.AddPodControllerUIDIndexer(podInformer.Informer()) + controller.AddPodControllerIndexer(podInformer.Informer()) dsc.podIndexer = podInformer.Informer().GetIndexer() nodeInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{ @@ -689,30 +689,6 @@ func (dsc *DaemonSetsController) updateNode(logger klog.Logger, old, cur interfa dsc.nodeUpdateQueue.Add(curNode.Name) } -// getPodsFromCache returns the Pods that a given DS should manage. -func (dsc *DaemonSetsController) getDaemonPodsFromCache(ds *apps.DaemonSet) ([]*v1.Pod, error) { - // Iterate over two keys: - // The UID of the Daemonset, which identifies Pods that are controlled by the Daemonset. - // The OrphanPodIndexKey, which helps identify orphaned Pods that are not currently managed by any controller, - // but may be adopted later on if they have matching labels with the Daemonset. - podsForDS := []*v1.Pod{} - for _, key := range []string{string(ds.UID), controller.OrphanPodIndexKeyForNamespace(ds.Namespace)} { - podObjs, err := dsc.podIndexer.ByIndex(controller.PodControllerUIDIndex, key) - if err != nil { - return nil, err - } - for _, obj := range podObjs { - pod, ok := obj.(*v1.Pod) - if !ok { - utilruntime.HandleError(fmt.Errorf("unexpected object type in pod indexer: %v", obj)) - continue - } - podsForDS = append(podsForDS, pod) - } - } - return podsForDS, nil -} - // getDaemonPods returns daemon pods owned by the given ds. // This also reconciles ControllerRef by adopting/orphaning. // Note that returned Pods are pointers to objects in the cache. @@ -723,7 +699,7 @@ func (dsc *DaemonSetsController) getDaemonPods(ctx context.Context, ds *apps.Dae return nil, err } // List all pods indexed to DS UID and Orphan pods - pods, err := dsc.getDaemonPodsFromCache(ds) + pods, err := controller.FilterPodsByOwner(dsc.podIndexer, &ds.ObjectMeta, "DaemonSet", true) if err != nil { return nil, err }