Skip to content

Commit

Permalink
Avoid SidecarContainers code path for non-sidecar pods
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
gjkim42 authored and SergeyKanzhelev committed Sep 6, 2024
1 parent f67b922 commit 8469207
Show file tree
Hide file tree
Showing 4 changed files with 46 additions and 12 deletions.
2 changes: 1 addition & 1 deletion pkg/kubelet/kuberuntime/kuberuntime_container.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
12 changes: 7 additions & 5 deletions pkg/kubelet/kuberuntime/kuberuntime_manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand All @@ -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}
Expand All @@ -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 {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand Down
33 changes: 27 additions & 6 deletions pkg/kubelet/kuberuntime/kuberuntime_manager_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand Down Expand Up @@ -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 {
Expand All @@ -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
}
Expand Down Expand Up @@ -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
}
Expand Down
11 changes: 11 additions & 0 deletions pkg/kubelet/types/pod_update.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

0 comments on commit 8469207

Please sign in to comment.