From 18794c15e9aaa6226a0384c5d8bca891173407b1 Mon Sep 17 00:00:00 2001 From: Mike Cutalo Date: Wed, 11 Oct 2023 14:02:24 -0700 Subject: [PATCH 1/2] min sidecar graceperoid --- .../kuberuntime/kuberuntime_container.go | 15 ++++++++--- pkg/kubelet/kuberuntime/labels.go | 25 +++++++++++++++++-- 2 files changed, 34 insertions(+), 6 deletions(-) diff --git a/pkg/kubelet/kuberuntime/kuberuntime_container.go b/pkg/kubelet/kuberuntime/kuberuntime_container.go index 2e534587c2de8..07c6bf599862b 100644 --- a/pkg/kubelet/kuberuntime/kuberuntime_container.go +++ b/pkg/kubelet/kuberuntime/kuberuntime_container.go @@ -314,7 +314,7 @@ func (m *kubeGenericRuntimeManager) generateContainerConfig(container *v1.Contai command, args := kubecontainer.ExpandContainerCommandAndArgs(container, opts.Envs) logDir := BuildContainerLogsDirectory(pod.Namespace, pod.Name, pod.UID, container.Name) - err = m.osInterface.MkdirAll(logDir, 0755) + err = m.osInterface.MkdirAll(logDir, 0o755) if err != nil { return nil, cleanupAction, fmt.Errorf("create container log directory for container %s failed: %v", container.Name, err) } @@ -413,7 +413,7 @@ func (m *kubeGenericRuntimeManager) makeMounts(opts *kubecontainer.RunContainerO // open(2) to create the file, so the final mode used is "mode & // ~umask". But we want to make sure the specified mode is used // in the file no matter what the umask is. - if err := m.osInterface.Chmod(containerLogPath, 0666); err != nil { + if err := m.osInterface.Chmod(containerLogPath, 0o666); err != nil { utilruntime.HandleError(fmt.Errorf("unable to set termination-log file permissions %q: %v", containerLogPath, err)) } @@ -782,9 +782,16 @@ func (m *kubeGenericRuntimeManager) killContainersWithSyncResult(pod *v1.Pod, ru } nonSidecarsWg.Wait() + // If non-sidecar containers eat up all of the gracePerioidDuration + // We still want to add a bit of time to give sidecars some chance to gracefully shutdown + // The default value is 10 seconds + // To orveride the default specify the annotation with a duration + // sidecars.lyft.net/sidecar-min-graceperoid=[time in seconds] + minimumGracePeriod := getSidecarMinGraceperoid(pod.Annotations) + gracePeriodDuration = gracePeriodDuration - time.Since(start) - if gracePeriodDuration < 0 { - gracePeriodDuration = 0 + if gracePeriodDuration < minimumGracePeriod { + gracePeriodDuration = minimumGracePeriod } // then sidecars diff --git a/pkg/kubelet/kuberuntime/labels.go b/pkg/kubelet/kuberuntime/labels.go index 8c8a57c5577a7..ce8598d648668 100644 --- a/pkg/kubelet/kuberuntime/labels.go +++ b/pkg/kubelet/kuberuntime/labels.go @@ -20,6 +20,7 @@ import ( "encoding/json" "runtime" "strconv" + "time" v1 "k8s.io/api/core/v1" kubetypes "k8s.io/apimachinery/pkg/types" @@ -47,8 +48,9 @@ const ( // xref: https://github.com/kubernetes/kubernetes/pull/99576/commits/42fb66073214eed6fe43fa8b1586f396e30e73e3#r635392090 // Currently, ContainerD on Windows does not yet fully support HostProcess containers // but will pass annotations to hcsshim which does have support. - windowsHostProcessContainer = "microsoft.com/hostprocess-container" - containerSidecarLabel = "com.lyft.sidecars.container-lifecycle" + windowsHostProcessContainer = "microsoft.com/hostprocess-container" + containerSidecarLabel = "com.lyft.sidecars.container-lifecycle" + sidecarMinGraceperoidAnnotation = "sidecars.lyft.net/sidecar-min-graceperoid" ) type labeledPodSandboxInfo struct { @@ -326,3 +328,22 @@ func getJSONObjectFromLabel(labels map[string]string, label string, value interf // If the label is not found, return not found. return false, nil } + +// getSidecarMinGraceperoid returns if set the minimum graceperoid for sidecars +// in the event of a parse failure, or the annotation is not present, +// return a default value. +func getSidecarMinGraceperoid(annotations map[string]string) time.Duration { + const defaultGraceperoid = 10 * time.Second + + if min, ok := annotations[sidecarMinGraceperoidAnnotation]; ok { + minGraceperoid, err := strconv.Atoi(min) + if err != nil { + // if for whatever reason this is not parseable + // return a default value + return defaultGraceperoid + } + return time.Duration(minGraceperoid) * time.Second + } + + return defaultGraceperoid +} From dc961295a075b38530c95dbd5d0d676bb53a29a7 Mon Sep 17 00:00:00 2001 From: Mike Cutalo Date: Thu, 12 Oct 2023 14:07:07 -0700 Subject: [PATCH 2/2] tests --- .../kuberuntime/kuberuntime_container.go | 2 +- pkg/kubelet/kuberuntime/labels.go | 3 +- pkg/kubelet/kuberuntime/labels_test.go | 35 +++++++++++++++++-- 3 files changed, 35 insertions(+), 5 deletions(-) diff --git a/pkg/kubelet/kuberuntime/kuberuntime_container.go b/pkg/kubelet/kuberuntime/kuberuntime_container.go index 07c6bf599862b..09e93884e10f1 100644 --- a/pkg/kubelet/kuberuntime/kuberuntime_container.go +++ b/pkg/kubelet/kuberuntime/kuberuntime_container.go @@ -783,7 +783,7 @@ func (m *kubeGenericRuntimeManager) killContainersWithSyncResult(pod *v1.Pod, ru nonSidecarsWg.Wait() // If non-sidecar containers eat up all of the gracePerioidDuration - // We still want to add a bit of time to give sidecars some chance to gracefully shutdown + // We still want to add a bit of time to give sidecars a chance to gracefully shutdown // The default value is 10 seconds // To orveride the default specify the annotation with a duration // sidecars.lyft.net/sidecar-min-graceperoid=[time in seconds] diff --git a/pkg/kubelet/kuberuntime/labels.go b/pkg/kubelet/kuberuntime/labels.go index ce8598d648668..1e6718833df7c 100644 --- a/pkg/kubelet/kuberuntime/labels.go +++ b/pkg/kubelet/kuberuntime/labels.go @@ -330,8 +330,7 @@ func getJSONObjectFromLabel(labels map[string]string, label string, value interf } // getSidecarMinGraceperoid returns if set the minimum graceperoid for sidecars -// in the event of a parse failure, or the annotation is not present, -// return a default value. +// In the event of a parse failure, or the annotation is not present, return a default value. func getSidecarMinGraceperoid(annotations map[string]string) time.Duration { const defaultGraceperoid = 10 * time.Second diff --git a/pkg/kubelet/kuberuntime/labels_test.go b/pkg/kubelet/kuberuntime/labels_test.go index 26b95d3311387..17ff90b9283d0 100644 --- a/pkg/kubelet/kuberuntime/labels_test.go +++ b/pkg/kubelet/kuberuntime/labels_test.go @@ -19,8 +19,9 @@ package kuberuntime import ( "reflect" "testing" + "time" - "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/intstr" kubecontainer "k8s.io/kubernetes/pkg/kubelet/container" @@ -64,7 +65,7 @@ func TestContainerLabels(t *testing.T) { }, } - var tests = []struct { + tests := []struct { description string expected *labeledContainerInfo }{ @@ -237,3 +238,33 @@ func TestPodAnnotations(t *testing.T) { t.Errorf("expected %v, got %v", expected, podSandboxInfo) } } + +func TestGetSidecarMinGraceperoid(t *testing.T) { + tests := []struct { + id string + annotations map[string]string + expect time.Duration + }{ + { + id: "default value", + annotations: map[string]string{}, + expect: time.Duration(10) * time.Second, + }, + { + id: "overrided", + annotations: map[string]string{ + sidecarMinGraceperoidAnnotation: "100", + }, + expect: time.Duration(100) * time.Second, + }, + } + + for _, test := range tests { + t.Run(test.id, func(t *testing.T) { + actual := getSidecarMinGraceperoid(test.annotations) + if test.expect != actual { + t.Errorf("expected: %v, got %v", test.expect, actual) + } + }) + } +}