Skip to content

Commit

Permalink
sidecar: kubelet: don't bother killing pods when non-sidecars are done
Browse files Browse the repository at this point in the history 
This change turns off the ability to completely kill pods when the
non-sidecars are done. This is useful for cronjobs, where the
non-sidecars finish work and exit, this code previously would clean up
the pod and its resources.

This feature was pulled in from kubernetes#75099.

This is a feature that sounds nice in practice, but its not what we
need. It seems to be a bit buggy since the Pod sandbox can
potentially be deleted and recreated during the liftime of the
Pod. That ain't good.

Datadog: **NOT FROM UPSTREAM K8S**. From Lyft: lyft@a31b7fc
  • Loading branch information
@tomwans @bpineau
tomwans authored and bpineau committed Aug 28, 2020
1 parent 8977a65 commit 3509e1d
Show file tree
Hide file tree
Showing 4 changed files with 15 additions and 104 deletions.
6 changes: 3 additions & 3 deletions pkg/kubelet/kuberuntime/kuberuntime_container.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,7 @@ func (m *kubeGenericRuntimeManager) startContainer(podSandboxID string, podSandb
if handlerErr != nil {
m.recordContainerEvent(pod, container, kubeContainerID.ID, v1.EventTypeWarning, events.FailedPostStartHook, msg)
if err := m.killContainer(pod, kubeContainerID, container.Name, "FailedPostStartHook", 0); err != nil {
glog.Errorf("Failed to kill container %q(id=%q) in pod %q: %v, %v",
klog.Errorf("Failed to kill container %q(id=%q) in pod %q: %v, %v",
container.Name, kubeContainerID.String(), format.Pod(pod), ErrPostStartHook, err)
}
return msg, fmt.Errorf("%s: %v", ErrPostStartHook, handlerErr)
Expand Down Expand Up @@ -669,7 +669,7 @@ func (m *kubeGenericRuntimeManager) killContainersWithSyncResult(pod *v1.Pod, ru
containerResults := make(chan *kubecontainer.SyncResult, len(runningPod.Containers))
// non-sidecars first
start := time.Now()
glog.Infof("Pod: %s, killContainersWithSyncResult: killing %d non-sidecars, %s termination period", runningPod.Name, len(nonSidecars), gracePeriodDuration)
klog.Infof("Pod: %s, killContainersWithSyncResult: killing %d non-sidecars, %s termination period", runningPod.Name, len(nonSidecars), gracePeriodDuration)
nonSidecarsWg := sync.WaitGroup{}
nonSidecarsWg.Add(len(nonSidecars))
for _, container := range nonSidecars {
Expand All @@ -691,7 +691,7 @@ func (m *kubeGenericRuntimeManager) killContainersWithSyncResult(pod *v1.Pod, ru
}

// then sidecars
glog.Infof("Pod: %s, killContainersWithSyncResult: killing %d sidecars, %s left", runningPod.Name, len(sidecars), gracePeriodDuration)
klog.Infof("Pod: %s, killContainersWithSyncResult: killing %d sidecars, %s left", runningPod.Name, len(sidecars), gracePeriodDuration)
wg := sync.WaitGroup{}
wg.Add(len(sidecars))
for _, container := range sidecars {
Expand Down
39 changes: 6 additions & 33 deletions pkg/kubelet/kuberuntime/kuberuntime_manager.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -479,7 +479,7 @@ func containerSucceeded(c *v1.Container, podStatus *kubecontainer.PodStatus) boo

func isSidecar(pod *v1.Pod, containerName string) bool {
if pod == nil {
glog.V(5).Infof("isSidecar: pod is nil, so returning false")
klog.V(5).Infof("isSidecar: pod is nil, so returning false")
return false
}
return pod.Annotations[fmt.Sprintf("sidecars.lyft.net/container-lifecycle-%s", containerName)] == "Sidecar"
Expand All @@ -506,21 +506,9 @@ func (m *kubeGenericRuntimeManager) computePodActions(pod *v1.Pod, podStatus *ku
}
}

// Determine if there are any non sidecar containers that are running or need restarting
// if there are none, we can kill the remaining sidecars
sidecarsOnly := true
for _, container := range pod.Spec.Containers {
containerStatus := podStatus.FindContainerStatusByName(container.Name)
if !isSidecar(pod, container.Name) {
if kubecontainer.ShouldContainerBeRestarted(&container, pod, podStatus) || (containerStatus != nil && containerStatus.State == kubecontainer.ContainerStateRunning) {
sidecarsOnly = false
}
}
}

// determine sidecar status
sidecarStatus := status.GetSidecarsStatus(pod)
glog.Infof("Pod: %s, sidecars: %s, status: Present=%v,Ready=%v,ContainerWaiting=%v", format.Pod(pod), sidecarNames, sidecarStatus.SidecarsPresent, sidecarStatus.SidecarsReady, sidecarStatus.ContainersWaiting)
klog.Infof("Pod: %s, sidecars: %s, status: Present=%v,Ready=%v,ContainersWaiting=%v", format.Pod(pod), sidecarNames, sidecarStatus.SidecarsPresent, sidecarStatus.SidecarsReady, sidecarStatus.ContainersWaiting)

// If we need to (re-)create the pod sandbox, everything will need to be
// killed and recreated, and init containers should be purged.
Expand Down Expand Up @@ -613,7 +601,7 @@ func (m *kubeGenericRuntimeManager) computePodActions(pod *v1.Pod, podStatus *ku
// - the non-sidecars have run before (i.e. they are not in a Waiting state) OR
// - the sidecars are ready (we're starting them for the first time)
if !isSidecar(pod, container.Name) && sidecarStatus.SidecarsPresent && sidecarStatus.ContainersWaiting && !sidecarStatus.SidecarsReady {
glog.Infof("Pod: %s, Container: %s, sidecar=%v skipped: Present=%v,Ready=%v,ContainerWaiting=%v", format.Pod(pod), container.Name, isSidecar(pod, container.Name), sidecarStatus.SidecarsPresent, sidecarStatus.SidecarsReady, sidecarStatus.ContainersWaiting)
klog.Infof("Pod: %s, Container: %s, sidecar=%v skipped: Present=%v,Ready=%v,ContainerWaiting=%v", format.Pod(pod), container.Name, isSidecar(pod, container.Name), sidecarStatus.SidecarsPresent, sidecarStatus.SidecarsReady, sidecarStatus.ContainersWaiting)
continue
}

Expand All @@ -632,10 +620,6 @@ func (m *kubeGenericRuntimeManager) computePodActions(pod *v1.Pod, podStatus *ku
// If container does not exist, or is not running, check whether we
// need to restart it.
if containerStatus == nil || containerStatus.State != kubecontainer.ContainerStateRunning {
if sidecarsOnly && isSidecar(pod, container.Name) {
glog.Infof("Pod: %s: %s: is sidecar, sidecars only, so not restarting", format.Pod(pod), container.Name)
continue
}
if kubecontainer.ShouldContainerBeRestarted(&container, pod, podStatus) {
message := fmt.Sprintf("%s: Container %+v is dead, but RestartPolicy says that we should restart it.", pod.Name, container)
klog.V(3).Infof(message)
Expand All @@ -662,17 +646,6 @@ func (m *kubeGenericRuntimeManager) computePodActions(pod *v1.Pod, podStatus *ku
// Restart regardless of the restart policy because the container
// spec changed.
restart = true
} else if sidecarsOnly && isSidecar(pod, container.Name) {
// in this case, the container is a sidecar, but no
// non-sidecars are ever going to run again. we don't need
// the sidecars, so we kill it as well
reason = "Non-sidecars have completed. Container will be killed."
// we are not planning to restart this container.
restart = false
// keepCount set to avoid killing pod right away - we should only
// kill pod once all containers have exited and we get back into this
// loop.
keepCount += 1
} else if liveness, found := m.livenessManager.Get(containerStatus.ID); found && liveness == proberesults.Failure {
// If the container failed the liveness probe, we should kill it.
message = fmt.Sprintf("Container %s failed liveness probe", container.Name)
Expand Down Expand Up @@ -702,7 +675,7 @@ func (m *kubeGenericRuntimeManager) computePodActions(pod *v1.Pod, podStatus *ku
}

if keepCount == 0 && len(changes.ContainersToStart) == 0 {
glog.Infof("Pod: %s: KillPod=true", format.Pod(pod))
klog.Infof("Pod: %s: KillPod=true", format.Pod(pod))
changes.KillPod = true
}

Expand Down Expand Up @@ -951,10 +924,10 @@ func (m *kubeGenericRuntimeManager) KillPod(pod *v1.Pod, runningPod kubecontaine
// grace period and also the sidecar status.
if pod == nil {
for _, container := range runningPod.Containers {
glog.Infof("Pod: %s, KillPod: pod nil, trying to restore from container %s", runningPod.Name, container.ID)
klog.Infof("Pod: %s, KillPod: pod nil, trying to restore from container %s", runningPod.Name, container.ID)
podSpec, _, err := m.restoreSpecsFromContainerLabels(container.ID)
if err != nil {
glog.Errorf("Pod: %s, KillPod: couldn't restore: %s", runningPod.Name, container.ID)
klog.Errorf("Pod: %s, KillPod: couldn't restore: %s", runningPod.Name, container.ID)
continue
}
pod = podSpec
Expand Down
63 changes: 1 addition & 62 deletions pkg/kubelet/kuberuntime/kuberuntime_manager_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1336,72 +1336,11 @@ func TestComputePodActionsWithSidecar(t *testing.T) {
mutateStatusFn func(*kubecontainer.PodStatus)
actions podActions
}{
"Kill sidecars if all non-sidecars are terminated": {
mutatePodFn: func(pod *v1.Pod) {
pod.Spec.RestartPolicy = v1.RestartPolicyOnFailure
},
mutateStatusFn: func(status *kubecontainer.PodStatus) {
for i := range status.ContainerStatuses {
if i == 1 {
continue
}
status.ContainerStatuses[i].State = kubecontainer.ContainerStateExited
status.ContainerStatuses[i].ExitCode = 0
}
},
actions: podActions{
SandboxID: baseStatus.SandboxStatuses[0].Id,
ContainersToKill: getKillMap(basePod, baseStatus, []int{1}),
ContainersToStart: []int{},
},
},
"Kill pod if all sidecars and non-sidecars are terminated": {
mutatePodFn: func(pod *v1.Pod) {
pod.Spec.RestartPolicy = v1.RestartPolicyOnFailure
},
mutateStatusFn: func(status *kubecontainer.PodStatus) {
for i := range status.ContainerStatuses {
status.ContainerStatuses[i].State = kubecontainer.ContainerStateExited
if i == 1 {
status.ContainerStatuses[i].ExitCode = 1
} else {
status.ContainerStatuses[i].ExitCode = 0
}
}
},
actions: podActions{
KillPod: true,
SandboxID: baseStatus.SandboxStatuses[0].Id,
ContainersToKill: getKillMap(basePod, baseStatus, []int{}),
ContainersToStart: []int{},
},
},
"Don't restart sidecars if all non-sidecars are terminated": {
mutatePodFn: func(pod *v1.Pod) {
pod.Spec.RestartPolicy = v1.RestartPolicyOnFailure
},
mutateStatusFn: func(status *kubecontainer.PodStatus) {
for i := range status.ContainerStatuses {
status.ContainerStatuses[i].State = kubecontainer.ContainerStateExited
if i == 1 {
status.ContainerStatuses[i].ExitCode = 1
} else {
status.ContainerStatuses[i].ExitCode = 0
}
}
},
actions: podActions{
KillPod: true,
SandboxID: baseStatus.SandboxStatuses[0].Id,
ContainersToKill: getKillMap(basePod, baseStatus, []int{}),
ContainersToStart: []int{},
},
},
"Start sidecar containers before non-sidecars when creating a new pod": {
mutateStatusFn: func(status *kubecontainer.PodStatus) {
// No container or sandbox exists.
status.SandboxStatuses = []*runtimeapi.PodSandboxStatus{}
status.ContainerStatuses = []*kubecontainer.ContainerStatus{}
status.ContainerStatuses = []*kubecontainer.Status{}
},
actions: podActions{
KillPod: true,
Expand Down
11 changes: 5 additions & 6 deletions pkg/kubelet/status/status_manager.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ import (
"time"

clientset "k8s.io/client-go/kubernetes"
"k8s.io/klog/glog"

v1 "k8s.io/api/core/v1"
apiequality "k8s.io/apimachinery/pkg/api/equality"
Expand Down Expand Up @@ -755,11 +754,11 @@ type SidecarsStatus struct {
// GetSidecarsStatus returns the SidecarsStatus for the given pod
func GetSidecarsStatus(pod *v1.Pod) SidecarsStatus {
if pod == nil {
glog.Infof("Pod was nil, returning empty sidecar status")
klog.Infof("Pod was nil, returning empty sidecar status")
return SidecarsStatus{}
}
if pod.Spec.Containers == nil || pod.Status.ContainerStatuses == nil {
glog.Infof("Pod Containers or Container status was nil, returning empty sidecar status")
klog.Infof("Pod Containers or Container status was nil, returning empty sidecar status")
return SidecarsStatus{}
}
sidecarsStatus := SidecarsStatus{SidecarsPresent: false, SidecarsReady: true, ContainersWaiting: false}
Expand All @@ -769,14 +768,14 @@ func GetSidecarsStatus(pod *v1.Pod) SidecarsStatus {
if pod.Annotations[fmt.Sprintf("sidecars.lyft.net/container-lifecycle-%s", container.Name)] == "Sidecar" {
sidecarsStatus.SidecarsPresent = true
if !status.Ready {
glog.Infof("Pod %s: %s: sidecar not ready", format.Pod(pod), container.Name)
klog.Infof("Pod %s: %s: sidecar not ready", format.Pod(pod), container.Name)
sidecarsStatus.SidecarsReady = false
} else {
glog.Infof("Pod %s: %s: sidecar is ready", format.Pod(pod), container.Name)
klog.Infof("Pod %s: %s: sidecar is ready", format.Pod(pod), container.Name)
}
} else if status.State.Waiting != nil {
// check if non-sidecars have started
glog.Infof("Pod: %s: %s: non-sidecar waiting", format.Pod(pod), container.Name)
klog.Infof("Pod: %s: %s: non-sidecar waiting", format.Pod(pod), container.Name)
sidecarsStatus.ContainersWaiting = true
}
}
Expand Down

0 comments on commit 3509e1d

Please sign in to comment.