Merge pull request #79148 from RobertKrawitz/mirror-pod-delete-loop-fix

Issue 79147: Do not delete an incorrect pod when replacing a mirror pod

pkg/kubelet/kubelet.go

@@ -1621,11 +1621,13 @@ func (kl *Kubelet) syncPod(o syncPodOptions) error {
 			if mirrorPod.DeletionTimestamp != nil || !kl.podManager.IsMirrorPodOf(mirrorPod, pod) {
 				// The mirror pod is semantically different from the static pod. Remove
 				// it. The mirror pod will get recreated later.
-				klog.Warningf("Deleting mirror pod %q because it is outdated", format.Pod(mirrorPod))
-				if err := kl.podManager.DeleteMirrorPod(podFullName); err != nil {
+				klog.Infof("Trying to delete pod %s %v", podFullName, mirrorPod.ObjectMeta.UID)
+				var err error
+				deleted, err = kl.podManager.DeleteMirrorPod(podFullName, &mirrorPod.ObjectMeta.UID)
+				if deleted {
+					klog.Warningf("Deleted mirror pod %q because it is outdated", format.Pod(mirrorPod))
+				} else if err != nil {
 					klog.Errorf("Failed deleting mirror pod %q: %v", format.Pod(mirrorPod), err)
-				} else {
-					deleted = true
 				}
 			}
 		}

pkg/kubelet/pod/mirror_client.go

@@ -20,6 +20,7 @@ import (
 	"k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/types"
 	clientset "k8s.io/client-go/kubernetes"
 	"k8s.io/klog"
 	kubecontainer "k8s.io/kubernetes/pkg/kubelet/container"
@@ -35,7 +36,7 @@ type MirrorClient interface {
 	CreateMirrorPod(pod *v1.Pod) error
 	// DeleteMirrorPod deletes the mirror pod with the given full name from
 	// the API server or returns an error.
-	DeleteMirrorPod(podFullName string) error
+	DeleteMirrorPod(podFullName string, uid *types.UID) (bool, error)
 }
 
 // basicMirrorClient is a functional MirrorClient.  Mirror pods are stored in
@@ -73,21 +74,35 @@ func (mc *basicMirrorClient) CreateMirrorPod(pod *v1.Pod) error {
 	return err
 }
 
-func (mc *basicMirrorClient) DeleteMirrorPod(podFullName string) error {
+// DeleteMirrorPod deletes a mirror pod.
+// It takes the full name of the pod and optionally a UID.  If the UID
+// is non-nil, the pod is deleted only if its UID matches the supplied UID.
+// It returns whether the pod was actually deleted, and any error returned
+// while parsing the name of the pod.
+// Non-existence of the pod or UID mismatch is not treated as an error; the
+// routine simply returns false in that case.
+func (mc *basicMirrorClient) DeleteMirrorPod(podFullName string, uid *types.UID) (bool, error) {
 	if mc.apiserverClient == nil {
-		return nil
+		return false, nil
 	}
 	name, namespace, err := kubecontainer.ParsePodFullName(podFullName)
 	if err != nil {
 		klog.Errorf("Failed to parse a pod full name %q", podFullName)
-		return err
+		return false, err
 	}
-	klog.V(2).Infof("Deleting a mirror pod %q", podFullName)
-	// TODO(random-liu): Delete the mirror pod with uid precondition in mirror pod manager
-	if err := mc.apiserverClient.CoreV1().Pods(namespace).Delete(name, metav1.NewDeleteOptions(0)); err != nil && !errors.IsNotFound(err) {
-		klog.Errorf("Failed deleting a mirror pod %q: %v", podFullName, err)
+	klog.V(2).Infof("Deleting a mirror pod %q (uid %#v)", podFullName, uid)
+	var GracePeriodSeconds int64
+	GracePeriodSeconds = 0
+	if err := mc.apiserverClient.CoreV1().Pods(namespace).Delete(name, &metav1.DeleteOptions{GracePeriodSeconds: &GracePeriodSeconds, Preconditions: &metav1.Preconditions{UID: uid}}); err != nil {
+		// Unfortunately, there's no generic error for failing a precondition
+		if !(errors.IsNotFound(err) || errors.IsConflict(err)) {
+			// We should return the error here, but historically this routine does
+			// not return an error unless it can't parse the pod name
+			klog.Errorf("Failed deleting a mirror pod %q: %v", podFullName, err)
+		}
+		return false, nil
 	}
-	return nil
+	return true, nil
 }
 
 // IsStaticPod returns true if the pod is a static pod.

pkg/kubelet/pod/pod_manager.go

@@ -338,7 +338,7 @@ func (pm *basicManager) getOrphanedMirrorPodNames() []string {
 func (pm *basicManager) DeleteOrphanedMirrorPods() {
 	podFullNames := pm.getOrphanedMirrorPodNames()
 	for _, podFullName := range podFullNames {
-		pm.MirrorClient.DeleteMirrorPod(podFullName)
+		pm.MirrorClient.DeleteMirrorPod(podFullName, nil)
 	}
 }
 

pkg/kubelet/pod/testing/fake_mirror_client.go

@@ -20,6 +20,7 @@ import (
 	"sync"
 
 	"k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/sets"
 	cp "k8s.io/kubernetes/pkg/kubelet/checkpoint"
 	"k8s.io/kubernetes/pkg/kubelet/checkpointmanager"
@@ -52,12 +53,13 @@ func (fmc *FakeMirrorClient) CreateMirrorPod(pod *v1.Pod) error {
 	return nil
 }
 
-func (fmc *FakeMirrorClient) DeleteMirrorPod(podFullName string) error {
+// TODO (Robert Krawitz): Implement UID checking
+func (fmc *FakeMirrorClient) DeleteMirrorPod(podFullName string, _ *types.UID) (bool, error) {
 	fmc.mirrorPodLock.Lock()
 	defer fmc.mirrorPodLock.Unlock()
 	fmc.mirrorPods.Delete(podFullName)
 	fmc.deleteCounts[podFullName]++
-	return nil
+	return true, nil
 }
 
 func (fmc *FakeMirrorClient) HasPod(podFullName string) bool {

pkg/kubelet/pod/testing/mock_manager.go

@@ -48,17 +48,17 @@ func (_m *MockManager) CreateMirrorPod(_a0 *v1.Pod) error {
 }
 
 // DeleteMirrorPod provides a mock function with given fields: podFullName
-func (_m *MockManager) DeleteMirrorPod(podFullName string) error {
+func (_m *MockManager) DeleteMirrorPod(podFullName string, _ *types.UID) (bool, error) {
 	ret := _m.Called(podFullName)
 
 	var r0 error
 	if rf, ok := ret.Get(0).(func(string) error); ok {
-		r0 = rf(podFullName)
+		return true, rf(podFullName)
 	} else {
 		r0 = ret.Error(0)
 	}
 
-	return r0
+	return false, r0
 }
 
 // DeleteOrphanedMirrorPods provides a mock function with given fields: