From 8b9d815f27720a0f18b36a694739960606c28c9a 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 3b534f1536f..d09dc704a4b 100644 --- a/pkg/kubelet/kuberuntime/kuberuntime_container.go +++ b/pkg/kubelet/kuberuntime/kuberuntime_container.go @@ -803,7 +803,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 86976ac2b54..a52904efe9a 100644 --- a/pkg/kubelet/kuberuntime/kuberuntime_manager.go +++ b/pkg/kubelet/kuberuntime/kuberuntime_manager.go @@ -828,6 +828,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 { @@ -857,7 +859,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 @@ -875,7 +877,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} @@ -898,7 +900,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 { @@ -1025,7 +1027,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 @@ -1269,7 +1271,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 b994ba30c8c..0563f539815 100644 --- a/pkg/kubelet/kuberuntime/kuberuntime_manager_test.go +++ b/pkg/kubelet/kuberuntime/kuberuntime_manager_test.go @@ -49,6 +49,7 @@ import ( kubecontainer "k8s.io/kubernetes/pkg/kubelet/container" containertest "k8s.io/kubernetes/pkg/kubelet/container/testing" proberesults "k8s.io/kubernetes/pkg/kubelet/prober/results" + kubelettypes "k8s.io/kubernetes/pkg/kubelet/types" ) var ( @@ -1426,6 +1427,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 { @@ -1436,12 +1451,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 } @@ -2039,12 +2057,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 7f7fc5b799b..2bec6a9f785 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 +}