diff --git a/pkg/controller/sidecarterminator/sidecar_terminator_controller.go b/pkg/controller/sidecarterminator/sidecar_terminator_controller.go index 251baa8b34..cc293daaaa 100644 --- a/pkg/controller/sidecarterminator/sidecar_terminator_controller.go +++ b/pkg/controller/sidecarterminator/sidecar_terminator_controller.go @@ -22,23 +22,27 @@ import ( "strings" "time" - appsv1alpha1 "github.com/openkruise/kruise/apis/apps/v1alpha1" - "github.com/openkruise/kruise/pkg/features" - "github.com/openkruise/kruise/pkg/util" - 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/klog/v2" + podutil "k8s.io/kubernetes/pkg/api/v1/pod" "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" + + appsv1alpha1 "github.com/openkruise/kruise/apis/apps/v1alpha1" + "github.com/openkruise/kruise/pkg/features" + "github.com/openkruise/kruise/pkg/util" + 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() { @@ -46,7 +50,8 @@ func init() { } var ( - concurrentReconciles = 3 + concurrentReconciles = 3 + SidecarTerminated corev1.PodConditionType = "SidecarTerminated" ) /** @@ -71,6 +76,7 @@ func newReconciler(mgr manager.Manager) reconcile.Reconciler { Client: cli, recorder: recorder, scheme: mgr.GetScheme(), + clock: clock.RealClock{}, } } @@ -101,6 +107,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. @@ -131,8 +138,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 } @@ -141,7 +148,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 } @@ -150,6 +158,10 @@ func (r *ReconcileSidecarTerminator) doReconcile(pod *corev1.Pod) (reconcile.Res return reconcile.Result{}, err } + if err := r.terminateJobPod(pod, sidecarNeedToSyncStatus); err != nil { + return reconcile.Result{}, err + } + if err := r.executeKillContainerAction(pod, sidecarNeedToExecuteKillContainer); err != nil { return reconcile.Result{}, err } @@ -157,20 +169,77 @@ func (r *ReconcileSidecarTerminator) doReconcile(pod *corev1.Pod) (reconcile.Res return reconcile.Result{}, nil } -func (r *ReconcileSidecarTerminator) groupSidecars(pod *corev1.Pod) (sets.String, sets.String, error) { +// terminateJobPod terminate the job pod and skip the state of the sidecar containers +// This method should only be called after the executeKillContainerAction is called +func (r *ReconcileSidecarTerminator) terminateJobPod(pod *corev1.Pod, sidecars sets.String) error { + if sidecars.Len() == 0 { + return nil + } + if isJobPodCompleted(pod) { + klog.V(3).Infof("the job pod is completed -- pod (%s/%s) ,no need to process", pod.Namespace, pod.Name) + return nil + } + + // after the pod is terminated by the sidecar terminator, kubelet will kill the containers that are not in the terminal phase + // 1. sidecar container terminate with non-zero exit code + // 2. sidecar container is not in a terminal phase (still running or waiting) + if containersSucceeded(pod, sidecars) { + return nil + } + + klog.V(3).Infof("all of the main containers are completed, will terminate the job pod %s/%s", pod.Namespace, pod.Name) + // terminate the pod, ignore the status of the sidecar containers. + // kubelet will not override pod phase after we updated (in kubelet,pods are not allowed to transition out of terminal phases). + if containersSucceeded(pod, getMain(pod)) { + pod.Status.Phase = corev1.PodSucceeded + for i, condition := range pod.Status.Conditions { + if condition.Type == corev1.PodReady || condition.Type == corev1.ContainersReady { + pod.Status.Conditions[i].Reason = "PodCompleted" + pod.Status.Conditions[i].Status = corev1.ConditionTrue + } + } + } else { + pod.Status.Phase = corev1.PodFailed + for i, condition := range pod.Status.Conditions { + if condition.Type == corev1.PodReady || condition.Type == corev1.ContainersReady { + pod.Status.Conditions[i].Reason = "PodFailed" + pod.Status.Conditions[i].Status = corev1.ConditionFalse + } + } + } + + // condition + _, condition := podutil.GetPodCondition(&pod.Status, SidecarTerminated) + if condition == nil { + pod.Status.Conditions = append(pod.Status.Conditions, corev1.PodCondition{ + Type: SidecarTerminated, + Status: corev1.ConditionTrue, + LastTransitionTime: metav1.Now(), + Message: "Terminated by Sidecar Terminator", + }) + } else { + condition.LastTransitionTime = metav1.Now() + } + klog.V(3).Infof("terminate the job pod %s/%s phase=%s", pod.Namespace, pod.Name, pod.Status.Phase) + return r.Status().Update(context.TODO(), pod) +} + +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 && @@ -179,7 +248,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 { @@ -210,3 +279,14 @@ func containersSucceeded(pod *corev1.Pod, containers sets.String) bool { } return true } + +func isJobPodCompleted(pod *corev1.Pod) bool { + mainContainers := getMain(pod) + if !containersCompleted(pod, mainContainers) { + return false + } + if containersSucceeded(pod, mainContainers) { + return pod.Status.Phase == corev1.PodSucceeded + } + return pod.Status.Phase == corev1.PodFailed +} diff --git a/pkg/controller/sidecarterminator/sidecar_terminator_controller_test.go b/pkg/controller/sidecarterminator/sidecar_terminator_controller_test.go index ad62205649..58970b5215 100644 --- a/pkg/controller/sidecarterminator/sidecar_terminator_controller_test.go +++ b/pkg/controller/sidecarterminator/sidecar_terminator_controller_test.go @@ -24,15 +24,18 @@ import ( utilruntime "k8s.io/apimachinery/pkg/util/runtime" - 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" + + appsv1alpha1 "github.com/openkruise/kruise/apis/apps/v1alpha1" ) const ( @@ -75,12 +78,28 @@ 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(), + }, + }, + } podDemo = &corev1.Pod{ TypeMeta: metav1.TypeMeta{ @@ -199,81 +218,166 @@ 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 + getCRR func() *appsv1alpha1.ContainerRecreateRequest + 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 }, getCRR: func() *appsv1alpha1.ContainerRecreateRequest { return nil }, + expectedPod: func() *corev1.Pod { + return podDemo.DeepCopy() + }, }, { - name: "normal pod with sidecar, restartPolicy=Never, main containers failed", + name: "normal pod with sidecar, restartPolicy=Never, main containers failed and sidecar 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 failed and sidecar 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 and sidecar failed", + getIn: func() *corev1.Pod { + podIn := podDemo.DeepCopy() + podIn.Status.ContainerStatuses[0] = succeededMainContainerStatus + podIn.Status.ContainerStatuses[1] = failedSidecarContainerStatus + return podIn + }, + getCRR: func() *appsv1alpha1.ContainerRecreateRequest { + return nil + }, + expectedPod: func() *corev1.Pod { + pod := podDemo.DeepCopy() + pod.Status.Phase = corev1.PodSucceeded + return pod + }, + }, + { + name: "normal pod with sidecar, restartPolicy=Never, main containers failed and sidecar failed", + getIn: func() *corev1.Pod { + podIn := podDemo.DeepCopy() + podIn.Status.ContainerStatuses[0] = failedMainContainerStatus + podIn.Status.ContainerStatuses[1] = failedSidecarContainerStatus + 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 succeeded", + name: "normal pod with sidecar, restartPolicy=Never, main containers succeeded and sidecar running", getIn: func() *corev1.Pod { podIn := podDemo.DeepCopy() podIn.Status.ContainerStatuses[0] = succeededMainContainerStatus + 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.PodSucceeded + return pod + }, }, { - name: "normal pod with sidecar, restartPolicy=OnFailure, main containers have not been completed", + 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", 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() + return nil + }, + 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{ @@ -298,6 +402,43 @@ func TestKruiseDaemonStrategy(t *testing.T) { } return crr }, + 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 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 podIn + }, + getCRR: func() *appsv1alpha1.ContainerRecreateRequest { + crr := crrDemo.DeepCopy() + crr.Spec.Containers = []appsv1alpha1.ContainerRecreateRequestContainer{ + {Name: "sidecar-1"}, {Name: "sidecar-2"}, + } + return crr + }, + 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 but 1 completed", @@ -325,6 +466,11 @@ func TestKruiseDaemonStrategy(t *testing.T) { } return crr }, + expectedPod: func() *corev1.Pod { + pod := podDemo.DeepCopy() + pod.Status.Phase = corev1.PodSucceeded + return pod + }, }, { name: "normal pod with sidecar, restartPolicy=OnFailure, 2 main containers but 1 uncompleted, 2 sidecars but 1 completed", @@ -348,6 +494,10 @@ func TestKruiseDaemonStrategy(t *testing.T) { getCRR: func() *appsv1alpha1.ContainerRecreateRequest { return nil }, + expectedPod: func() *corev1.Pod { + pod := podDemo.DeepCopy() + return pod + }, }, } @@ -359,6 +509,7 @@ func TestKruiseDaemonStrategy(t *testing.T) { r := ReconcileSidecarTerminator{ Client: fakeClient, recorder: fakeRecord, + clock: clock.RealClock{}, } _, err := r.Reconcile(context.Background(), reconcile.Request{ @@ -384,6 +535,17 @@ func TestKruiseDaemonStrategy(t *testing.T) { 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 { diff --git a/pkg/webhook/containerrecreaterequest/mutating/crr_mutating_handler.go b/pkg/webhook/containerrecreaterequest/mutating/crr_mutating_handler.go index 2985d88e93..58db07e77a 100644 --- a/pkg/webhook/containerrecreaterequest/mutating/crr_mutating_handler.go +++ b/pkg/webhook/containerrecreaterequest/mutating/crr_mutating_handler.go @@ -23,10 +23,6 @@ import ( "net/http" "reflect" - appsv1alpha1 "github.com/openkruise/kruise/apis/apps/v1alpha1" - "github.com/openkruise/kruise/pkg/features" - "github.com/openkruise/kruise/pkg/util" - utilfeature "github.com/openkruise/kruise/pkg/util/feature" admissionv1 "k8s.io/api/admission/v1" v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" @@ -37,6 +33,12 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/runtime/inject" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + + appsv1alpha1 "github.com/openkruise/kruise/apis/apps/v1alpha1" + "github.com/openkruise/kruise/pkg/controller/sidecarterminator" + "github.com/openkruise/kruise/pkg/features" + "github.com/openkruise/kruise/pkg/util" + utilfeature "github.com/openkruise/kruise/pkg/util/feature" ) const ( @@ -123,7 +125,7 @@ func (h *ContainerRecreateRequestHandler) Handle(ctx context.Context, req admiss } return admission.Errored(http.StatusInternalServerError, fmt.Errorf("failed to find Pod %s: %v", obj.Spec.PodName, err)) } - if !kubecontroller.IsPodActive(pod) { + if !kubecontroller.IsPodActive(pod) && !isTerminatedBySidecarTerminator(pod) { return admission.Errored(http.StatusBadRequest, fmt.Errorf("not allowed to recreate containers in an inactive Pod")) } else if pod.Spec.NodeName == "" { return admission.Errored(http.StatusBadRequest, fmt.Errorf("not allowed to recreate containers in a pending Pod")) @@ -144,6 +146,15 @@ func (h *ContainerRecreateRequestHandler) Handle(ctx context.Context, req admiss return admission.PatchResponseFromRaw(req.AdmissionRequest.Object.Raw, marshalled) } +func isTerminatedBySidecarTerminator(pod *v1.Pod) bool { + for _, c := range pod.Status.Conditions { + if c.Type == sidecarterminator.SidecarTerminated { + return true + } + } + return false +} + func injectPodIntoContainerRecreateRequest(obj *appsv1alpha1.ContainerRecreateRequest, pod *v1.Pod) error { obj.Labels[appsv1alpha1.ContainerRecreateRequestNodeNameKey] = pod.Spec.NodeName obj.Labels[appsv1alpha1.ContainerRecreateRequestPodUIDKey] = string(pod.UID) diff --git a/pkg/webhook/util/util.go b/pkg/webhook/util/util.go index c4a3f4f7a9..427ddb57a4 100644 --- a/pkg/webhook/util/util.go +++ b/pkg/webhook/util/util.go @@ -20,12 +20,13 @@ import ( "os" "strconv" - "github.com/openkruise/kruise/pkg/util" "k8s.io/klog/v2" + + "github.com/openkruise/kruise/pkg/util" ) func GetHost() string { - return os.Getenv("WEBHOOK_HOST") + return os.Getenv("$1ST") } func GetNamespace() string { diff --git a/test/e2e/apps/sidecarterminator.go b/test/e2e/apps/sidecarterminator.go index d49a57b752..92d3b632d3 100644 --- a/test/e2e/apps/sidecarterminator.go +++ b/test/e2e/apps/sidecarterminator.go @@ -7,15 +7,16 @@ import ( "github.com/onsi/ginkgo" "github.com/onsi/gomega" - appsv1alpha1 "github.com/openkruise/kruise/apis/apps/v1alpha1" - kruiseclientset "github.com/openkruise/kruise/pkg/client/clientset/versioned" - "github.com/openkruise/kruise/test/e2e/framework" batchv1 "k8s.io/api/batch/v1" v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/rand" clientset "k8s.io/client-go/kubernetes" "k8s.io/utils/pointer" + + appsv1alpha1 "github.com/openkruise/kruise/apis/apps/v1alpha1" + kruiseclientset "github.com/openkruise/kruise/pkg/client/clientset/versioned" + "github.com/openkruise/kruise/test/e2e/framework" ) var _ = SIGDescribe("SidecarTerminator", func() { @@ -51,6 +52,18 @@ var _ = SIGDescribe("SidecarTerminator", func() { }, }, } + sidecarContainerNeverStop := v1.Container{ + Name: "sidecar", + Image: "busybox:latest", + ImagePullPolicy: v1.PullIfNotPresent, + Command: []string{"/bin/sh", "-c", "sleep 10000"}, + Env: []v1.EnvVar{ + { + Name: appsv1alpha1.KruiseTerminateSidecarEnv, + Value: "true", + }, + }, + } cases := []struct { name string @@ -90,6 +103,39 @@ var _ = SIGDescribe("SidecarTerminator", func() { return false }, }, + { + name: "native job, restartPolicy=Never, main succeeded, sidecar failed", + createJob: func(str string) metav1.Object { + job := &batchv1.Job{ + ObjectMeta: metav1.ObjectMeta{Namespace: ns, Name: "job-" + str}, + Spec: batchv1.JobSpec{ + Template: v1.PodTemplateSpec{ + Spec: v1.PodSpec{ + Containers: []v1.Container{ + *mainContainer.DeepCopy(), + *sidecarContainerNeverStop.DeepCopy(), + }, + RestartPolicy: v1.RestartPolicyNever, + }, + }, + }, + } + job, err := c.BatchV1().Jobs(ns).Create(context.TODO(), job, metav1.CreateOptions{}) + gomega.Expect(err).NotTo(gomega.HaveOccurred()) + return job + }, + checkStatus: func(object metav1.Object) bool { + job, err := c.BatchV1().Jobs(object.GetNamespace()). + Get(context.TODO(), object.GetName(), metav1.GetOptions{}) + gomega.Expect(err).NotTo(gomega.HaveOccurred()) + for _, cond := range job.Status.Conditions { + if cond.Type == batchv1.JobComplete { + return true + } + } + return false + }, + }, { name: "native job, restartPolicy=OnFailure, main failed", createJob: func(str string) metav1.Object {