From cc6de915cb9000a6ef6db6467737f252e4aa2588 Mon Sep 17 00:00:00 2001 From: liuzhenwei Date: Wed, 24 May 2023 22:36:46 +0800 Subject: [PATCH] Sidecar terminator ignore the exit code of the sidecar container Signed-off-by: liuzhenwei add ut Signed-off-by: liuzhenwei add crr event handler ut Signed-off-by: liuzhenwei fix crr status Signed-off-by: liuzhenwei fix, support kubelet and crr controller report pod status Signed-off-by: liuzhenwei --- .../kill_container_action.go | 8 + .../sidecar_terminator_controller.go | 145 ++++++++++- .../sidecar_terminator_controller_test.go | 227 ++++++++++++++---- .../sidecar_terminator_pod_event_handler.go | 7 +- ...decar_terminator_pod_event_handler_test.go | 60 ++++- 5 files changed, 382 insertions(+), 65 deletions(-) diff --git a/pkg/controller/sidecarterminator/kill_container_action.go b/pkg/controller/sidecarterminator/kill_container_action.go index 94d096b2d5..04ad71a17c 100644 --- a/pkg/controller/sidecarterminator/kill_container_action.go +++ b/pkg/controller/sidecarterminator/kill_container_action.go @@ -98,3 +98,11 @@ func filterUncompletedSidecars(pod *corev1.Pod, sidecars sets.String) sets.Strin func getCRRName(pod *corev1.Pod) string { return fmt.Sprintf("sidecar-termination-%v", pod.UID) } + +func getJobName(pod *corev1.Pod) string { + ref := metav1.GetControllerOf(pod) + if ref == nil { + return "" + } + return ref.Name +} diff --git a/pkg/controller/sidecarterminator/sidecar_terminator_controller.go b/pkg/controller/sidecarterminator/sidecar_terminator_controller.go index 8cb4270620..001773ce08 100644 --- a/pkg/controller/sidecarterminator/sidecar_terminator_controller.go +++ b/pkg/controller/sidecarterminator/sidecar_terminator_controller.go @@ -22,22 +22,28 @@ import ( "strings" "time" - appsv1alpha1 "github.com/openkruise/kruise/apis/apps/v1alpha1" - "github.com/openkruise/kruise/pkg/features" - utilclient "github.com/openkruise/kruise/pkg/util/client" - utilfeature "github.com/openkruise/kruise/pkg/util/feature" - "github.com/openkruise/kruise/pkg/util/ratelimiter" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/clock" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/client-go/tools/record" + "k8s.io/client-go/util/retry" "k8s.io/klog/v2" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller" "sigs.k8s.io/controller-runtime/pkg/manager" "sigs.k8s.io/controller-runtime/pkg/reconcile" "sigs.k8s.io/controller-runtime/pkg/source" + + "k8s.io/apimachinery/pkg/types" + + appsv1alpha1 "github.com/openkruise/kruise/apis/apps/v1alpha1" + "github.com/openkruise/kruise/pkg/features" + utilclient "github.com/openkruise/kruise/pkg/util/client" + utilfeature "github.com/openkruise/kruise/pkg/util/feature" + "github.com/openkruise/kruise/pkg/util/ratelimiter" ) func init() { @@ -70,6 +76,7 @@ func newReconciler(mgr manager.Manager) reconcile.Reconciler { Client: cli, recorder: recorder, scheme: mgr.GetScheme(), + clock: clock.RealClock{}, } } @@ -99,6 +106,7 @@ type ReconcileSidecarTerminator struct { client.Client recorder record.EventRecorder scheme *runtime.Scheme + clock clock.Clock } // Reconcile get the pod whose sidecar containers should be stopped, and stop them. @@ -129,8 +137,8 @@ func (r *ReconcileSidecarTerminator) doReconcile(pod *corev1.Pod) (reconcile.Res return reconcile.Result{}, nil } - if containersCompleted(pod, getSidecar(pod)) { - klog.V(3).Infof("SidecarTerminator -- all sidecars of pod(%v/%v) have been completed, no need to process", pod.Namespace, pod.Name) + if containersSucceeded(pod, getSidecar(pod)) { + klog.V(3).Infof("SidecarTerminator -- all sidecars of pod(%v/%v) have been succeeded, no need to process", pod.Namespace, pod.Name) return reconcile.Result{}, nil } @@ -139,7 +147,8 @@ func (r *ReconcileSidecarTerminator) doReconcile(pod *corev1.Pod) (reconcile.Res return reconcile.Result{}, nil } - sidecarNeedToExecuteKillContainer, sidecarNeedToExecuteInPlaceUpdate, err := r.groupSidecars(pod) + sidecarNeedToExecuteKillContainer, sidecarNeedToExecuteInPlaceUpdate, sidecarNeedToSyncStatus, err := r.groupSidecars(pod) + if err != nil { return reconcile.Result{}, err } @@ -152,23 +161,126 @@ func (r *ReconcileSidecarTerminator) doReconcile(pod *corev1.Pod) (reconcile.Res return reconcile.Result{}, err } + if sidecarNeedToSyncStatus.Len() > 0 { + if err := r.syncSidecarStatus(pod, sidecarNeedToSyncStatus); err != nil { + return reconcile.Result{}, err + } + } + return reconcile.Result{}, nil } -func (r *ReconcileSidecarTerminator) groupSidecars(pod *corev1.Pod) (sets.String, sets.String, error) { +func (r *ReconcileSidecarTerminator) syncSidecarStatus(pod *corev1.Pod, sidecars sets.String) error { + // + if deduceWhetherTheJobIsCompletedFromThePod(pod) { + return nil + } + + var changed bool + newSidecarStatus := make(map[string]corev1.ContainerStatus) + for i := range pod.Spec.Containers { + status := &pod.Status.ContainerStatuses[i] + if !sidecars.Has(status.Name) { + continue + } + changed = true + // skip sync status of sidecar container if job has completed. + // the real status that reported by kubelet will be store into the state of sidecar container. + // the pod is repeatedly processed by job controller until to job reaches completed phase. because kubelet and the logic bellow will report different status of the container. + if status.State.Terminated != nil && status.State.Terminated.ExitCode != int32(0) { + klog.V(3).Infof("SidecarTerminator -- ignore the non-zero exitCode of the sidecar container %s/%s", pod.Name, status.Name) + newStatus := *status.DeepCopy() + newStatus.Ready = false + newStatus.Started = &newStatus.Ready + newStatus.State = corev1.ContainerState{ + Terminated: &corev1.ContainerStateTerminated{ + ExitCode: int32(0), + Reason: "Completed", + StartedAt: status.State.Terminated.StartedAt, + FinishedAt: status.State.Terminated.FinishedAt, + ContainerID: status.ContainerID, + }, + } + newSidecarStatus[status.Name] = newStatus + + } else if status.State.Terminated == nil && status.State.Running != nil { + klog.V(3).Infof("SidecarTerminator -- sync the status of the sidecar container %s/%s,crr has reached the completed phase", pod.Name, status.Name) + newStatus := *status.DeepCopy() + newStatus.Ready = false + newStatus.Started = &newStatus.Ready + newStatus.State = corev1.ContainerState{ + Terminated: &corev1.ContainerStateTerminated{ + ExitCode: int32(0), + Reason: "Completed", + StartedAt: status.State.Running.StartedAt, + FinishedAt: metav1.NewTime(r.clock.Now()), + ContainerID: status.ContainerID, + }, + } + newSidecarStatus[status.Name] = newStatus + } + + } + var err error + if changed { + err = retry.RetryOnConflict(retry.DefaultBackoff, func() error { + latestPod := &corev1.Pod{} + if err = r.Get(context.TODO(), types.NamespacedName{Namespace: pod.Namespace, Name: pod.Name}, latestPod); err != nil { + return err + } + for i := range latestPod.Spec.Containers { + for name, status := range newSidecarStatus { + if latestPod.Status.ContainerStatuses[i].Name == name { + latestPod.Status.ContainerStatuses[i] = status + } + } + } + + // we should let job reaches completed phase, if main container has already reached succeeded phase, ignore exitCode of sidecar container. + if getSidecar(latestPod).Len() == len(newSidecarStatus) { + if containersSucceeded(latestPod, getMain(latestPod)) { + latestPod.Status.Phase = corev1.PodSucceeded + for i, condition := range latestPod.Status.Conditions { + if condition.Type == corev1.PodReady || condition.Type == corev1.ContainersReady { + latestPod.Status.Conditions[i].Reason = "PodCompleted" + latestPod.Status.Conditions[i].Status = corev1.ConditionTrue + } + } + } else { + latestPod.Status.Phase = corev1.PodFailed + for i, condition := range latestPod.Status.Conditions { + if condition.Type == corev1.PodReady || condition.Type == corev1.ContainersReady { + latestPod.Status.Conditions[i].Reason = "PodFailed" + latestPod.Status.Conditions[i].Status = corev1.ConditionFalse + } + } + } + + } + + return r.Status().Update(context.TODO(), latestPod) + }) + } + + return err +} + +func (r *ReconcileSidecarTerminator) groupSidecars(pod *corev1.Pod) (sets.String, sets.String, sets.String, error) { runningOnVK, err := IsPodRunningOnVirtualKubelet(pod, r.Client) if err != nil { - return nil, nil, client.IgnoreNotFound(err) + return nil, nil, nil, client.IgnoreNotFound(err) } inPlaceUpdate := sets.NewString() killContainer := sets.NewString() + syncStatusContainer := sets.NewString() for i := range pod.Spec.Containers { container := &pod.Spec.Containers[i] for j := range container.Env { if !runningOnVK && container.Env[j].Name == appsv1alpha1.KruiseTerminateSidecarEnv && strings.EqualFold(container.Env[j].Value, "true") { killContainer.Insert(container.Name) + syncStatusContainer.Insert(container.Name) break } if container.Env[j].Name == appsv1alpha1.KruiseTerminateSidecarWithImageEnv && @@ -177,7 +289,7 @@ func (r *ReconcileSidecarTerminator) groupSidecars(pod *corev1.Pod) (sets.String } } } - return killContainer, inPlaceUpdate, nil + return killContainer, inPlaceUpdate, syncStatusContainer, nil } func containersCompleted(pod *corev1.Pod, containers sets.String) bool { @@ -208,3 +320,14 @@ func containersSucceeded(pod *corev1.Pod, containers sets.String) bool { } return true } + +func deduceWhetherTheJobIsCompletedFromThePod(pod *corev1.Pod) bool { + mainContainers := getMain(pod) + if containersCompleted(pod, mainContainers) && containersSucceeded(pod, mainContainers) { + return pod.Status.Phase == corev1.PodSucceeded + } + if containersCompleted(pod, mainContainers) && !containersSucceeded(pod, mainContainers) { + return pod.Status.Phase == corev1.PodFailed + } + return false +} diff --git a/pkg/controller/sidecarterminator/sidecar_terminator_controller_test.go b/pkg/controller/sidecarterminator/sidecar_terminator_controller_test.go index 63c5d1c4e7..b531b26246 100644 --- a/pkg/controller/sidecarterminator/sidecar_terminator_controller_test.go +++ b/pkg/controller/sidecarterminator/sidecar_terminator_controller_test.go @@ -18,19 +18,22 @@ package sidecarterminator import ( "context" - "encoding/json" - "reflect" "testing" - appsv1alpha1 "github.com/openkruise/kruise/apis/apps/v1alpha1" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/clock" "k8s.io/client-go/tools/record" + "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" "sigs.k8s.io/controller-runtime/pkg/reconcile" + + batchv1 "k8s.io/api/batch/v1" + + appsv1alpha1 "github.com/openkruise/kruise/apis/apps/v1alpha1" ) const ( @@ -38,6 +41,8 @@ const ( ) var ( + lTrue = true + stime = metav1.Now() scheme *runtime.Scheme succeededMainContainerStatus = corev1.ContainerStatus{ Name: "main", @@ -73,12 +78,56 @@ var ( }, } + failedSidecarContainerStatus = corev1.ContainerStatus{ + Name: "sidecar", + State: corev1.ContainerState{ + Terminated: &corev1.ContainerStateTerminated{ + ExitCode: int32(137), + }, + }, + } uncompletedSidecarContainerStatus = corev1.ContainerStatus{ Name: "sidecar", State: corev1.ContainerState{ Terminated: nil, }, } + runningSidecarContainerStatus = corev1.ContainerStatus{ + Name: "sidecar", + State: corev1.ContainerState{ + Running: &corev1.ContainerStateRunning{ + StartedAt: metav1.Now(), + }, + }, + } + jobDemo = &batchv1.Job{ + TypeMeta: metav1.TypeMeta{ + APIVersion: batchv1.SchemeGroupVersion.String(), + Kind: "Job", + }, + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test-sidecar-terminator", + UID: "8707667788", + Name: "job01", + ResourceVersion: "", + }, + Spec: batchv1.JobSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + RestartPolicy: corev1.RestartPolicyNever, + Containers: []corev1.Container{ + { + Name: "main", + Image: "main-container:latest", + }, + }, + }, + }, + }, + Status: batchv1.JobStatus{ + StartTime: &stime, + }, + } podDemo = &corev1.Pod{ TypeMeta: metav1.TypeMeta{ @@ -89,6 +138,9 @@ var ( Namespace: "test-sidecar-terminator", UID: "87076677", Name: "job-generate-rand-str", + OwnerReferences: []metav1.OwnerReference{ + {UID: jobDemo.UID, Name: jobDemo.Name, Controller: &lTrue, APIVersion: jobDemo.APIVersion, Kind: jobDemo.Kind}, + }, }, Spec: corev1.PodSpec{ NodeName: "normal-node", @@ -176,6 +228,7 @@ func init() { scheme = runtime.NewScheme() _ = corev1.AddToScheme(scheme) _ = appsv1alpha1.AddToScheme(scheme) + _ = batchv1.AddToScheme(scheme) } func sidecarContainerFactory(name string, strategy string) corev1.Container { @@ -197,81 +250,138 @@ func sidecarContainerFactory(name string, strategy string) corev1.Container { func TestKruiseDaemonStrategy(t *testing.T) { cases := []struct { - name string - getIn func() *corev1.Pod - getCRR func() *appsv1alpha1.ContainerRecreateRequest + name string + getIn func() *corev1.Pod + expectedPod func() *corev1.Pod }{ { name: "normal pod with sidecar, restartPolicy=Never, main containers have not been completed", getIn: func() *corev1.Pod { podIn := podDemo.DeepCopy() podIn.Status.ContainerStatuses[0] = uncompletedMainContainerStatus + podIn.Status.ContainerStatuses[1] = runningSidecarContainerStatus + return podIn + }, + expectedPod: func() *corev1.Pod { + return podDemo.DeepCopy() + }, + }, + { + name: "normal pod with sidecar, restartPolicy=Never, main containers failed and sidecar running,job completed", + getIn: func() *corev1.Pod { + podIn := podDemo.DeepCopy() + podIn.Status.ContainerStatuses[0] = failedMainContainerStatus + podIn.Status.ContainerStatuses[1] = runningSidecarContainerStatus return podIn }, - getCRR: func() *appsv1alpha1.ContainerRecreateRequest { - return nil + expectedPod: func() *corev1.Pod { + pod := podDemo.DeepCopy() + pod.Status.Phase = corev1.PodFailed + return pod }, }, { - name: "normal pod with sidecar, restartPolicy=Never, main containers failed", + name: "normal pod with sidecar, restartPolicy=Never, main containers failed and sidecar running,job running", getIn: func() *corev1.Pod { podIn := podDemo.DeepCopy() podIn.Status.ContainerStatuses[0] = failedMainContainerStatus + podIn.Status.ContainerStatuses[1] = runningSidecarContainerStatus return podIn }, - getCRR: func() *appsv1alpha1.ContainerRecreateRequest { - return crrDemo.DeepCopy() + expectedPod: func() *corev1.Pod { + pod := podDemo.DeepCopy() + pod.Status.Phase = corev1.PodFailed + return pod }, }, { - name: "normal pod with sidecar, restartPolicy=Never, main containers succeeded", + name: "normal pod with sidecar, restartPolicy=Never, main containers succeeded and sidecar failed,job running", getIn: func() *corev1.Pod { podIn := podDemo.DeepCopy() podIn.Status.ContainerStatuses[0] = succeededMainContainerStatus + podIn.Status.ContainerStatuses[1] = failedSidecarContainerStatus return podIn }, - getCRR: func() *appsv1alpha1.ContainerRecreateRequest { - return crrDemo.DeepCopy() + expectedPod: func() *corev1.Pod { + pod := podDemo.DeepCopy() + pod.Status.Phase = corev1.PodSucceeded + return pod }, }, { - name: "normal pod with sidecar, restartPolicy=OnFailure, main containers have not been completed", + name: "normal pod with sidecar, restartPolicy=Never, main containers failed and sidecar failed,job running", + getIn: func() *corev1.Pod { + podIn := podDemo.DeepCopy() + podIn.Status.ContainerStatuses[0] = failedMainContainerStatus + podIn.Status.ContainerStatuses[1] = failedSidecarContainerStatus + return podIn + }, + expectedPod: func() *corev1.Pod { + pod := podDemo.DeepCopy() + pod.Status.Phase = corev1.PodFailed + return pod + }, + }, + { + name: "normal pod with sidecar, restartPolicy=Never, main containers succeeded and sidecar running,job running", + getIn: func() *corev1.Pod { + podIn := podDemo.DeepCopy() + podIn.Status.ContainerStatuses[0] = succeededMainContainerStatus + podIn.Status.ContainerStatuses[1] = runningSidecarContainerStatus + return podIn + }, + expectedPod: func() *corev1.Pod { + pod := podDemo.DeepCopy() + pod.Status.Phase = corev1.PodSucceeded + return pod + }, + }, + { + name: "normal pod with sidecar, restartPolicy=OnFailure, main containers have not been completed and sidecar running", getIn: func() *corev1.Pod { podIn := podDemo.DeepCopy() podIn.Spec.RestartPolicy = corev1.RestartPolicyOnFailure podIn.Status.ContainerStatuses[0] = uncompletedMainContainerStatus + podIn.Status.ContainerStatuses[1] = runningSidecarContainerStatus return podIn }, - getCRR: func() *appsv1alpha1.ContainerRecreateRequest { - return nil + expectedPod: func() *corev1.Pod { + pod := podDemo.DeepCopy() + return pod }, }, { - name: "normal pod with sidecar, restartPolicy=OnFailure, main containers failed", + name: "normal pod with sidecar, restartPolicy=OnFailure, main containers failed and sidecar succeeded", getIn: func() *corev1.Pod { podIn := podDemo.DeepCopy() podIn.Spec.RestartPolicy = corev1.RestartPolicyOnFailure podIn.Status.ContainerStatuses[0] = failedMainContainerStatus + podIn.Status.ContainerStatuses[1] = completedSidecarContainerStatus return podIn }, - getCRR: func() *appsv1alpha1.ContainerRecreateRequest { - return nil + expectedPod: func() *corev1.Pod { + pod := podDemo.DeepCopy() + return pod }, }, { - name: "normal pod with sidecar, restartPolicy=OnFailure, main containers succeeded", + name: "normal pod with sidecar, restartPolicy=OnFailure, main containers succeeded and sidecar succeeded,job completed", getIn: func() *corev1.Pod { podIn := podDemo.DeepCopy() podIn.Spec.RestartPolicy = corev1.RestartPolicyOnFailure + podIn.Status.Phase = corev1.PodSucceeded podIn.Status.ContainerStatuses[0] = succeededMainContainerStatus + podIn.Status.ContainerStatuses[1] = completedSidecarContainerStatus return podIn }, - getCRR: func() *appsv1alpha1.ContainerRecreateRequest { - return crrDemo.DeepCopy() + expectedPod: func() *corev1.Pod { + pod := podDemo.DeepCopy() + pod.Status.Phase = corev1.PodSucceeded + return pod }, }, { - name: "normal pod with sidecar, restartPolicy=OnFailure, 2 succeeded main containers, 2 sidecars", + name: "normal pod with sidecar, restartPolicy=OnFailure, 2 succeeded main containers, 2 sidecars uncompleted", getIn: func() *corev1.Pod { podIn := podDemo.DeepCopy() podIn.Spec.Containers = []corev1.Container{ @@ -289,12 +399,35 @@ func TestKruiseDaemonStrategy(t *testing.T) { } return podIn }, - getCRR: func() *appsv1alpha1.ContainerRecreateRequest { - crr := crrDemo.DeepCopy() - crr.Spec.Containers = []appsv1alpha1.ContainerRecreateRequestContainer{ - {Name: "sidecar-1"}, {Name: "sidecar-2"}, + expectedPod: func() *corev1.Pod { + pod := podDemo.DeepCopy() + pod.Status.Phase = corev1.PodRunning + return pod + }, + }, + { + name: "normal pod with sidecar, restartPolicy=OnFailure, 2 succeeded main containers, 2 sidecars running", + getIn: func() *corev1.Pod { + podIn := podDemo.DeepCopy() + podIn.Spec.Containers = []corev1.Container{ + mainContainerFactory("main-1"), + mainContainerFactory("main-2"), + sidecarContainerFactory("sidecar-1", "true"), + sidecarContainerFactory("sidecar-2", "true"), + } + podIn.Spec.RestartPolicy = corev1.RestartPolicyOnFailure + podIn.Status.ContainerStatuses = []corev1.ContainerStatus{ + rename(succeededMainContainerStatus.DeepCopy(), "main-1"), + rename(succeededMainContainerStatus.DeepCopy(), "main-2"), + rename(runningSidecarContainerStatus.DeepCopy(), "sidecar-1"), + rename(runningSidecarContainerStatus.DeepCopy(), "sidecar-2"), } - return crr + return podIn + }, + expectedPod: func() *corev1.Pod { + pod := podDemo.DeepCopy() + pod.Status.Phase = corev1.PodSucceeded + return pod }, }, { @@ -316,12 +449,9 @@ func TestKruiseDaemonStrategy(t *testing.T) { } return podIn }, - getCRR: func() *appsv1alpha1.ContainerRecreateRequest { - crr := crrDemo.DeepCopy() - crr.Spec.Containers = []appsv1alpha1.ContainerRecreateRequestContainer{ - {Name: "sidecar-2"}, - } - return crr + expectedPod: func() *corev1.Pod { + pod := podDemo.DeepCopy() + return pod }, }, { @@ -343,8 +473,9 @@ func TestKruiseDaemonStrategy(t *testing.T) { } return podIn }, - getCRR: func() *appsv1alpha1.ContainerRecreateRequest { - return nil + expectedPod: func() *corev1.Pod { + pod := podDemo.DeepCopy() + return pod }, }, } @@ -357,6 +488,7 @@ func TestKruiseDaemonStrategy(t *testing.T) { r := ReconcileSidecarTerminator{ Client: fakeClient, recorder: fakeRecord, + clock: clock.RealClock{}, } _, err := r.Reconcile(context.Background(), reconcile.Request{ @@ -369,19 +501,16 @@ func TestKruiseDaemonStrategy(t *testing.T) { t.Fatalf("Failed to reconcile, error: %v", err) } - realCRR := &appsv1alpha1.ContainerRecreateRequest{} - err = fakeClient.Get(context.TODO(), types.NamespacedName{ - Name: getCRRName(podDemo), - Namespace: podDemo.Namespace, - }, realCRR) - expectCRR := cs.getCRR() - - realBy, _ := json.Marshal(realCRR) - expectBy, _ := json.Marshal(expectCRR) - - if !(expectCRR == nil && errors.IsNotFound(err) || reflect.DeepEqual(realBy, expectBy)) { - t.Fatal("Get unexpected CRR") + pod := &corev1.Pod{} + err = fakeClient.Get(context.TODO(), client.ObjectKey{Namespace: podDemo.Namespace, Name: podDemo.Name}, pod) + if err != nil { + t.Fatalf("Get pod error: %v", err) } + expectPod := cs.expectedPod() + if pod.Status.Phase != expectPod.Status.Phase { + t.Fatalf("Get an expected pod phase : expectd=%s,got=%s", expectPod.Status.Phase, pod.Status.Phase) + } + }) } } diff --git a/pkg/controller/sidecarterminator/sidecar_terminator_pod_event_handler.go b/pkg/controller/sidecarterminator/sidecar_terminator_pod_event_handler.go index 32789898cd..1ceafdd96b 100644 --- a/pkg/controller/sidecarterminator/sidecar_terminator_pod_event_handler.go +++ b/pkg/controller/sidecarterminator/sidecar_terminator_pod_event_handler.go @@ -19,7 +19,6 @@ package sidecarterminator import ( "strings" - appsv1alpha1 "github.com/openkruise/kruise/apis/apps/v1alpha1" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" @@ -28,6 +27,8 @@ import ( "sigs.k8s.io/controller-runtime/pkg/event" "sigs.k8s.io/controller-runtime/pkg/handler" "sigs.k8s.io/controller-runtime/pkg/reconcile" + + appsv1alpha1 "github.com/openkruise/kruise/apis/apps/v1alpha1" ) var _ handler.EventHandler = &enqueueRequestForPod{} @@ -74,12 +75,12 @@ func (p *enqueueRequestForPod) handlePodUpdate(q workqueue.RateLimitingInterface func isInterestingPod(pod *corev1.Pod) bool { if pod.DeletionTimestamp != nil || - pod.Status.Phase != corev1.PodRunning || + pod.Status.Phase == corev1.PodPending || pod.Spec.RestartPolicy == corev1.RestartPolicyAlways { return false } - if containersCompleted(pod, getSidecar(pod)) { + if pod.Status.Phase != corev1.PodRunning && containersSucceeded(pod, getSidecar(pod)) { return false } diff --git a/pkg/controller/sidecarterminator/sidecar_terminator_pod_event_handler_test.go b/pkg/controller/sidecarterminator/sidecar_terminator_pod_event_handler_test.go index 5002db990f..47cb085987 100644 --- a/pkg/controller/sidecarterminator/sidecar_terminator_pod_event_handler_test.go +++ b/pkg/controller/sidecarterminator/sidecar_terminator_pod_event_handler_test.go @@ -120,7 +120,7 @@ func TestEnqueueRequestForPodUpdate(t *testing.T) { } return newPod }, - expectLen: 0, + expectLen: 1, }, { name: "Pod, main container completed -> completed, sidecar container completed", @@ -140,8 +140,50 @@ func TestEnqueueRequestForPodUpdate(t *testing.T) { } return newPod }, + expectLen: 1, + }, + { + name: "Pod, main container completed -> completed, sidecar container completed and pod has reached succeeded phase", + getOldPod: func() *corev1.Pod { + oldPod := oldPodDemo.DeepCopy() + oldPod.Status.ContainerStatuses = []corev1.ContainerStatus{ + succeededMainContainerStatus, + completedSidecarContainerStatus, + } + return oldPod + }, + getNewPod: func() *corev1.Pod { + newPod := newPodDemo.DeepCopy() + newPod.Status.ContainerStatuses = []corev1.ContainerStatus{ + succeededMainContainerStatus, + completedSidecarContainerStatus, + } + newPod.Status.Phase = corev1.PodSucceeded + return newPod + }, expectLen: 0, }, + { + name: "Pod, main container completed -> completed, sidecar container failed and pod has reached succeeded phase", + getOldPod: func() *corev1.Pod { + oldPod := oldPodDemo.DeepCopy() + oldPod.Status.ContainerStatuses = []corev1.ContainerStatus{ + succeededMainContainerStatus, + completedSidecarContainerStatus, + } + return oldPod + }, + getNewPod: func() *corev1.Pod { + newPod := newPodDemo.DeepCopy() + newPod.Status.ContainerStatuses = []corev1.ContainerStatus{ + succeededMainContainerStatus, + failedSidecarContainerStatus, + } + newPod.Status.Phase = corev1.PodSucceeded + return newPod + }, + expectLen: 1, + }, { name: "Pod, main container completed -> uncompleted, sidecar container completed", getOldPod: func() *corev1.Pod { @@ -260,17 +302,31 @@ func TestEnqueueRequestForPodCreate(t *testing.T) { expectLen: 1, }, { - name: "Pod, main container completed, sidecar container completed", + name: "Pod, main container completed, sidecar container completed and pod has reached succeeded phase", getPod: func() *corev1.Pod { newPod := demoPod.DeepCopy() newPod.Status.ContainerStatuses = []corev1.ContainerStatus{ succeededMainContainerStatus, completedSidecarContainerStatus, } + newPod.Status.Phase = corev1.PodSucceeded return newPod }, expectLen: 0, }, + { + name: "Pod, main container completed, sidecar container failed and pod has reached succeeded phase", + getPod: func() *corev1.Pod { + newPod := demoPod.DeepCopy() + newPod.Status.ContainerStatuses = []corev1.ContainerStatus{ + succeededMainContainerStatus, + failedSidecarContainerStatus, + } + newPod.Status.Phase = corev1.PodSucceeded + return newPod + }, + expectLen: 1, + }, { name: "Pod, main container uncompleted, sidecar container completed", getPod: func() *corev1.Pod {