Consider pod image in duplicates strategy

README.md

@@ -66,15 +66,21 @@ Replication Controller (RC), Deployment, or Job running on the same node. If the
 those duplicate pods are evicted for better spreading of pods in a cluster. This issue could happen
 if some nodes went down due to whatever reasons, and pods on them were moved to other nodes leading to
 more than one pod associated with a RS or RC, for example, running on the same node. Once the failed nodes
-are ready again, this strategy could be enabled to evict those duplicate pods. Currently, there are no
-parameters associated with this strategy. To disable this strategy, the policy should look like:
+are ready again, this strategy could be enabled to evict those duplicate pods.
+
+It provides one optional parameter, `ExcludeOwnerKinds`, which is a list of OwnerRef `Kind`s. If a pod
+has any of these `Kind`s listed as an `OwnerRef`, that pod will not be considered for eviction.
 
 ```
 apiVersion: "descheduler/v1alpha1"
 kind: "DeschedulerPolicy"
 strategies:
   "RemoveDuplicates":
-     enabled: false
+     enabled: true
+     params:
+       removeDuplicates:
+         excludeOwnerKinds:
+         - "ReplicaSet"
 ```
 
 ### LowNodeUtilization

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
 }