Consider pod image in duplicates strategy

This increases the specificity of the RemoveDuplicates strategy by removing pods which not only have the same
owner, but who also must have the same list of container images. This also adds a parameter, `ExcludeOwnerKinds`
to the RemoveDuplicates strategy which accepts a list of Kinds. If a pod has any of these Kinds as an owner, that
pod is not considered for eviction.

pkg/api/types.go

@@ -50,6 +50,7 @@ type StrategyParameters struct {
 	NodeAffinityType                  []string
 	PodsHavingTooManyRestarts         *PodsHavingTooManyRestarts
 	MaxPodLifeTimeSeconds             *uint
+	RemoveDuplicates                  *RemoveDuplicates
 }
 
 type Percentage float64
@@ -65,3 +66,7 @@ type PodsHavingTooManyRestarts struct {
 	PodRestartThreshold     int32
 	IncludingInitContainers bool
 }
+
+type RemoveDuplicates struct {
+	ExcludeOwnerKinds []string
+}

pkg/api/v1alpha1/types.go

@@ -50,6 +50,7 @@ type StrategyParameters struct {
 	NodeAffinityType                  []string                           `json:"nodeAffinityType,omitempty"`
 	PodsHavingTooManyRestarts         *PodsHavingTooManyRestarts         `json:"podsHavingTooManyRestarts,omitempty"`
 	MaxPodLifeTimeSeconds             *uint                              `json:"maxPodLifeTimeSeconds,omitempty"`
+	RemoveDuplicates                  *RemoveDuplicates                  `json:"removeDuplicates,omitempty"`
 }
 
 type Percentage float64
@@ -65,3 +66,7 @@ type PodsHavingTooManyRestarts struct {
 	PodRestartThreshold     int32 `json:"podRestartThreshold,omitempty"`
 	IncludingInitContainers bool  `json:"includingInitContainers,omitempty"`
 }
+
+type RemoveDuplicates struct {
+	ExcludeOwnerKinds []string `json:"excludeOwnerKinds,omitempty"`
+}

pkg/descheduler/strategies/duplicates.go

@@ -18,9 +18,13 @@ package strategies
 
 import (
 	"context"
+	"reflect"
+	"sort"
 	"strings"
 
 	v1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/util/sets"
 	clientset "k8s.io/client-go/kubernetes"
 	"k8s.io/klog"
 
@@ -31,7 +35,8 @@ import (
 
 // RemoveDuplicatePods removes the duplicate pods on node. This strategy evicts all duplicate pods on node.
 // A pod is said to be a duplicate of other if both of them are from same creator, kind and are within the same
-// namespace. As of now, this strategy won't evict daemonsets, mirror pods, critical pods and pods with local storages.
+// namespace, and have at least one container with the same image.
+// As of now, this strategy won't evict daemonsets, mirror pods, critical pods and pods with local storages.
 func RemoveDuplicatePods(
 	ctx context.Context,
 	client clientset.Interface,
@@ -42,41 +47,83 @@ func RemoveDuplicatePods(
 ) {
 	for _, node := range nodes {
 		klog.V(1).Infof("Processing node: %#v", node.Name)
-		dpm := listDuplicatePodsOnANode(ctx, client, node, evictLocalStoragePods)
-		for creator, pods := range dpm {
-			if len(pods) > 1 {
-				klog.V(1).Infof("%#v", creator)
-				// i = 0 does not evict the first pod
-				for i := 1; i < len(pods); i++ {
-					if _, err := podEvictor.EvictPod(ctx, pods[i], node); err != nil {
-						klog.Errorf("Error evicting pod: (%#v)", err)
-						break
-					}
-				}
+		duplicatePods := listDuplicatePodsOnANode(ctx, client, node, strategy, evictLocalStoragePods)
+		for _, pod := range duplicatePods {
+			if _, err := podEvictor.EvictPod(ctx, pod, node); err != nil {
+				klog.Errorf("Error evicting pod: (%#v)", err)
+				break
 			}
 		}
 	}
 }
 
-//type creator string
-type duplicatePodsMap map[string][]*v1.Pod
-
 // listDuplicatePodsOnANode lists duplicate pods on a given node.
-func listDuplicatePodsOnANode(ctx context.Context, client clientset.Interface, node *v1.Node, evictLocalStoragePods bool) duplicatePodsMap {
+// It checks for pods which have the same owner and have at least 1 container with the same image spec
+func listDuplicatePodsOnANode(ctx context.Context, client clientset.Interface, node *v1.Node, strategy api.DeschedulerStrategy, evictLocalStoragePods bool) []*v1.Pod {
 	pods, err := podutil.ListEvictablePodsOnNode(ctx, client, node, evictLocalStoragePods)
 	if err != nil {
 		return nil
 	}
 
-	dpm := duplicatePodsMap{}
-	// Ignoring the error here as in the ListDuplicatePodsOnNode function we call ListEvictablePodsOnNode which checks for error.
+	duplicatePods := make([]*v1.Pod, 0, len(pods))
+	// Each pod has a list of owners and a list of containers, and each container has 1 image spec.
+	// For each pod, we go through all the OwnerRef/Image mappings and represent them as a "key" string.
+	// All of those mappings together makes a list of "key" strings that essentially represent that pod's uniqueness.
+	// This list of keys representing a single pod is then sorted alphabetically.
+	// If any other pod has a list that matches that pod's list, those pods are undeniably duplicates for the following reasons:
+	//   - The 2 pods have the exact same ownerrefs
+	//   - The 2 pods have the exact same container images
+	//
+	// duplicateKeysMap maps the first Namespace/Kind/Name/Image in a pod's list to a 2D-slice of all the other lists where that is the first key
+	// (Since we sort each pod's list, we only need to key the map on the first entry in each list. Any pod that doesn't have
+	// the same first entry is clearly not a duplicate. This makes lookup quick and minimizes storage needed).
+	// If any of the existing lists for that first key matches the current pod's list, the current pod is a duplicate.
+	// If not, then we add this pod's list to the list of lists for that key.
+	duplicateKeysMap := map[string][][]string{}
 	for _, pod := range pods {
 		ownerRefList := podutil.OwnerRef(pod)
+		if hasExcludedOwnerRefKind(ownerRefList, strategy) {
+			continue
+		}
+		podContainerKeys := make([]string, 0, len(ownerRefList)*len(pod.Spec.Containers))
 		for _, ownerRef := range ownerRefList {
-			// Namespace/Kind/Name should be unique for the cluster.
-			s := strings.Join([]string{pod.ObjectMeta.Namespace, ownerRef.Kind, ownerRef.Name}, "/")
-			dpm[s] = append(dpm[s], pod)
+			for _, container := range pod.Spec.Containers {
+				// Namespace/Kind/Name should be unique for the cluster.
+				// We also consider the image, as 2 pods could have the same owner but serve different purposes
+				// So any non-unique Namespace/Kind/Name/Image pattern is a duplicate pod.
+				s := strings.Join([]string{pod.ObjectMeta.Namespace, ownerRef.Kind, ownerRef.Name, container.Image}, "/")
+				podContainerKeys = append(podContainerKeys, s)
+			}
+		}
+		sort.Strings(podContainerKeys)
+
+		// If there have been any other pods with the same first "key", look through all the lists to see if any match
+		if existing, ok := duplicateKeysMap[podContainerKeys[0]]; ok {
+			for _, keys := range existing {
+				if reflect.DeepEqual(keys, podContainerKeys) {
+					duplicatePods = append(duplicatePods, pod)
+					break
+				}
+				// Found no matches, add this list of keys to the list of lists that have the same first key
+				duplicateKeysMap[podContainerKeys[0]] = append(duplicateKeysMap[podContainerKeys[0]], podContainerKeys)
+			}
+		} else {
+			// This is the first pod we've seen that has this first "key" entry
+			duplicateKeysMap[podContainerKeys[0]] = [][]string{podContainerKeys}
+		}
+	}
+	return duplicatePods
+}
+
+func hasExcludedOwnerRefKind(ownerRefs []metav1.OwnerReference, strategy api.DeschedulerStrategy) bool {
+	if strategy.Params.RemoveDuplicates == nil {
+		return false
+	}
+	exclude := sets.NewString(strategy.Params.RemoveDuplicates.ExcludeOwnerKinds...)
+	for _, owner := range ownerRefs {
+		if exclude.Has(owner.Kind) {
+			return true
 		}
 	}
-	return dpm
+	return false
 }