Consider pod image in duplicates strategy

pkg/descheduler/strategies/duplicates.go

@@ -41,39 +41,45 @@ func RemoveDuplicatePods(
 	for _, node := range nodes {
 		klog.V(1).Infof("Processing node: %#v", node.Name)
 		dpm := listDuplicatePodsOnANode(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(pods[i], node); err != nil {
-						break
-					}
-				}
+		for _, pod := range dpm {
+			if _, err := podEvictor.EvictPod(pod, node); err != nil {
+				break
 			}
 		}
 	}
 }
 
-//type creator string
-type duplicatePodsMap map[string][]*v1.Pod
-
 // listDuplicatePodsOnANode lists duplicate pods on a given node.
-func listDuplicatePodsOnANode(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
+// Then adds it to a map, keyed on the pod's namespace/name. We use this instead of a simple list to make
+// sure a pod isn't added multiple times (for multiple matching container images) which would lead to multiple
+// attemps to delete the same pod, and false errors from that.
+func listDuplicatePodsOnANode(client clientset.Interface, node *v1.Node, evictLocalStoragePods bool) map[string]*v1.Pod {
 	pods, err := podutil.ListEvictablePodsOnNode(client, node, evictLocalStoragePods)
 	if err != nil {
 		return nil
 	}
 
-	dpm := duplicatePodsMap{}
+	duplicatePods := map[string]*v1.Pod{}
+	dpm := map[string][]*v1.Pod{}
 	// Ignoring the error here as in the ListDuplicatePodsOnNode function we call ListEvictablePodsOnNode which checks for error.
 	for _, pod := range pods {
 		ownerRefList := podutil.OwnerRef(pod)
 		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}, "/")
+				dpm[s] = append(dpm[s], pod)
+				// If we've already found a pod with the same owner string, above, we check if this pod has already been
+				// marked as a duplicate in dupPodsMap (possibly for one of its other containers). If not, we add it
+				key := pod.ObjectMeta.Namespace + "/" + pod.ObjectMeta.Name
+				if len(dpm[s]) > 1 && duplicatePods[key] == nil {
+					duplicatePods[key] = pod
+				}
+			}
 		}
 	}
-	return dpm
+	return duplicatePods
 }