From 84692077280112bc568d987a2b2931e0d0638bad Mon Sep 17 00:00:00 2001 From: Gunju Kim Date: Fri, 6 Sep 2024 01:50:06 +0900 Subject: [PATCH] Avoid SidecarContainers code path for non-sidecar pods This fixes a regression in the SidecarContainers feature by minimizing the impact of the new code path. Use the old code path for pods without restartable init containers, and apply the new code path only to pods with restartable init containers. --- .../kuberuntime/kuberuntime_container.go | 2 +- .../kuberuntime/kuberuntime_manager.go | 12 ++++--- .../kuberuntime/kuberuntime_manager_test.go | 33 +++++++++++++++---- pkg/kubelet/types/pod_update.go | 11 +++++++ 4 files changed, 46 insertions(+), 12 deletions(-) diff --git a/pkg/kubelet/kuberuntime/kuberuntime_container.go b/pkg/kubelet/kuberuntime/kuberuntime_container.go index b049540d0d338..825ae7acf31a8 100644 --- a/pkg/kubelet/kuberuntime/kuberuntime_container.go +++ b/pkg/kubelet/kuberuntime/kuberuntime_container.go @@ -832,7 +832,7 @@ func (m *kubeGenericRuntimeManager) killContainersWithSyncResult(ctx context.Con wg.Add(len(runningPod.Containers)) var termOrdering *terminationOrdering // we only care about container termination ordering if the sidecars feature is enabled - if utilfeature.DefaultFeatureGate.Enabled(features.SidecarContainers) { + if utilfeature.DefaultFeatureGate.Enabled(features.SidecarContainers) && types.HasRestartableInitContainer(pod) { var runningContainerNames []string for _, container := range runningPod.Containers { runningContainerNames = append(runningContainerNames, container.Name) diff --git a/pkg/kubelet/kuberuntime/kuberuntime_manager.go b/pkg/kubelet/kuberuntime/kuberuntime_manager.go index 2c7a8af44f404..f25b4e328212e 100644 --- a/pkg/kubelet/kuberuntime/kuberuntime_manager.go +++ b/pkg/kubelet/kuberuntime/kuberuntime_manager.go @@ -833,6 +833,8 @@ func (m *kubeGenericRuntimeManager) computePodActions(ctx context.Context, pod * ContainersToKill: make(map[kubecontainer.ContainerID]containerToKillInfo), } + handleRestartableInitContainers := utilfeature.DefaultFeatureGate.Enabled(features.SidecarContainers) && types.HasRestartableInitContainer(pod) + // If we need to (re-)create the pod sandbox, everything will need to be // killed and recreated, and init containers should be purged. if createPodSandbox { @@ -862,7 +864,7 @@ func (m *kubeGenericRuntimeManager) computePodActions(ctx context.Context, pod * // is done and there is no container to start. if len(containersToStart) == 0 { hasInitialized := false - if !utilfeature.DefaultFeatureGate.Enabled(features.SidecarContainers) { + if !handleRestartableInitContainers { _, _, hasInitialized = findNextInitContainerToRun(pod, podStatus) } else { // If there is any regular container, it means all init containers have @@ -880,7 +882,7 @@ func (m *kubeGenericRuntimeManager) computePodActions(ctx context.Context, pod * // state. if len(pod.Spec.InitContainers) != 0 { // Pod has init containers, return the first one. - if !utilfeature.DefaultFeatureGate.Enabled(features.SidecarContainers) { + if !handleRestartableInitContainers { changes.NextInitContainerToStart = &pod.Spec.InitContainers[0] } else { changes.InitContainersToStart = []int{0} @@ -903,7 +905,7 @@ func (m *kubeGenericRuntimeManager) computePodActions(ctx context.Context, pod * } // Check initialization progress. - if !utilfeature.DefaultFeatureGate.Enabled(features.SidecarContainers) { + if !handleRestartableInitContainers { initLastStatus, next, done := findNextInitContainerToRun(pod, podStatus) if !done { if next != nil { @@ -1027,7 +1029,7 @@ func (m *kubeGenericRuntimeManager) computePodActions(ctx context.Context, pod * if keepCount == 0 && len(changes.ContainersToStart) == 0 { changes.KillPod = true - if utilfeature.DefaultFeatureGate.Enabled(features.SidecarContainers) { + if handleRestartableInitContainers { // To prevent the restartable init containers to keep pod alive, we should // not restart them. changes.InitContainersToStart = nil @@ -1285,7 +1287,7 @@ func (m *kubeGenericRuntimeManager) SyncPod(ctx context.Context, pod *v1.Pod, po start(ctx, "ephemeral container", metrics.EphemeralContainer, ephemeralContainerStartSpec(&pod.Spec.EphemeralContainers[idx])) } - if !utilfeature.DefaultFeatureGate.Enabled(features.SidecarContainers) { + if !types.HasRestartableInitContainer(pod) { // Step 6: start the init container. if container := podContainerChanges.NextInitContainerToStart; container != nil { // Start the next init container. diff --git a/pkg/kubelet/kuberuntime/kuberuntime_manager_test.go b/pkg/kubelet/kuberuntime/kuberuntime_manager_test.go index 732a6f7f1c881..c61c11b717c8f 100644 --- a/pkg/kubelet/kuberuntime/kuberuntime_manager_test.go +++ b/pkg/kubelet/kuberuntime/kuberuntime_manager_test.go @@ -51,6 +51,7 @@ import ( containertest "k8s.io/kubernetes/pkg/kubelet/container/testing" imagetypes "k8s.io/kubernetes/pkg/kubelet/images" proberesults "k8s.io/kubernetes/pkg/kubelet/prober/results" + kubelettypes "k8s.io/kubernetes/pkg/kubelet/types" ) var ( @@ -1428,6 +1429,20 @@ func testComputePodActionsWithInitContainers(t *testing.T, sidecarContainersEnab ContainersToKill: getKillMapWithInitContainers(basePod, baseStatus, []int{}), }, }, + "an init container is in the created state due to an unknown error when starting container; restart it": { + mutatePodFn: func(pod *v1.Pod) { pod.Spec.RestartPolicy = v1.RestartPolicyAlways }, + mutateStatusFn: func(status *kubecontainer.PodStatus) { + status.ContainerStatuses[2].State = kubecontainer.ContainerStateCreated + }, + actions: podActions{ + KillPod: false, + SandboxID: baseStatus.SandboxStatuses[0].Id, + NextInitContainerToStart: &basePod.Spec.InitContainers[2], + InitContainersToStart: []int{2}, + ContainersToStart: []int{}, + ContainersToKill: getKillMapWithInitContainers(basePod, baseStatus, []int{}), + }, + }, } { pod, status := makeBasePodAndStatusWithInitContainers() if test.mutatePodFn != nil { @@ -1438,12 +1453,15 @@ func testComputePodActionsWithInitContainers(t *testing.T, sidecarContainersEnab } ctx := context.Background() actions := m.computePodActions(ctx, pod, status) - if !sidecarContainersEnabled { - // If sidecar containers are disabled, we should not see any + handleRestartableInitContainers := sidecarContainersEnabled && kubelettypes.HasRestartableInitContainer(pod) + if !handleRestartableInitContainers { + // If sidecar containers are disabled or the pod does not have any + // restartable init container, we should not see any // InitContainersToStart in the actions. test.actions.InitContainersToStart = nil } else { - // If sidecar containers are enabled, we should not see any + // If sidecar containers are enabled and the pod has any + // restartable init container, we should not see any // NextInitContainerToStart in the actions. test.actions.NextInitContainerToStart = nil } @@ -2041,12 +2059,15 @@ func testComputePodActionsWithInitAndEphemeralContainers(t *testing.T, sidecarCo } ctx := context.Background() actions := m.computePodActions(ctx, pod, status) - if !sidecarContainersEnabled { - // If sidecar containers are disabled, we should not see any + handleRestartableInitContainers := sidecarContainersEnabled && kubelettypes.HasRestartableInitContainer(pod) + if !handleRestartableInitContainers { + // If sidecar containers are disabled or the pod does not have any + // restartable init container, we should not see any // InitContainersToStart in the actions. test.actions.InitContainersToStart = nil } else { - // If sidecar containers are enabled, we should not see any + // If sidecar containers are enabled and the pod has any + // restartable init container, we should not see any // NextInitContainerToStart in the actions. test.actions.NextInitContainerToStart = nil } diff --git a/pkg/kubelet/types/pod_update.go b/pkg/kubelet/types/pod_update.go index 7f7fc5b799bcb..2bec6a9f7854a 100644 --- a/pkg/kubelet/types/pod_update.go +++ b/pkg/kubelet/types/pod_update.go @@ -202,3 +202,14 @@ func IsRestartableInitContainer(initContainer *v1.Container) bool { return *initContainer.RestartPolicy == v1.ContainerRestartPolicyAlways } + +// HasRestartableInitContainer returns true if the pod has any restartable init +// container +func HasRestartableInitContainer(pod *v1.Pod) bool { + for _, container := range pod.Spec.InitContainers { + if IsRestartableInitContainer(&container) { + return true + } + } + return false +}