Skip to content

Commit

Permalink
Address PR review comments (third pass): return -seconds suffix
Browse files Browse the repository at this point in the history
Signed-off-by: Eugene Glotov <kivagant@gmail.com>
  • Loading branch information
KIVagant committed Dec 17, 2019
1 parent fce26c0 commit 04812a9
Show file tree
Hide file tree
Showing 10 changed files with 97 additions and 95 deletions.
2 changes: 1 addition & 1 deletion charts/linkerd2/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ The following table lists the configurable parameters of the Linkerd2 chart and
| `proxy.trace.collectorSvcAccount` | Service account associated with the Trace collector instance ||
| `proxy.trace.collectorSvcAddr` | Collector Service address for the proxies to send Trace Data ||
| `proxy.uid` | User id under which the proxy runs | `2102` |
| `Proxy.WaitBeforeExitSeconds` | The proxy sidecar will stay alive for at least the given period before receiving SIGTERM signal from Kubernetes but no longer than pod's `terminationGracePeriodSeconds`. | `0` |
| `proxy.waitBeforeExitSeconds` | The proxy sidecar will stay alive for at least the given period before receiving SIGTERM signal from Kubernetes but no longer than pod's `terminationGracePeriodSeconds`. | `0` |
| `proxyInit.ignoreInboundPorts` | Inbound ports the proxy should ignore ||
| `proxyInit.ignoreOutboundPorts` | Outbound ports the proxy should ignore ||
| `proxyInit.image.name` | Docker image for the proxy-init container | `gcr.io/linkerd-io/proxy-init` |
Expand Down
2 changes: 1 addition & 1 deletion charts/linkerd2/values.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ proxy:
# and wait for this duration before letting the proxy process the SIGTERM signal.
# See https://kubernetes.io/docs/concepts/containers/container-lifecycle-hooks/#container-hooks
# for more info on container lifecycle hooks.
WaitBeforeExitSeconds: 0
waitBeforeExitSeconds: 0

# proxy-init configuration
proxyInit:
Expand Down
4 changes: 2 additions & 2 deletions charts/partials/templates/_proxy.tpl
Original file line number Diff line number Diff line change
Expand Up @@ -110,14 +110,14 @@ securityContext:
readOnlyRootFilesystem: true
runAsUser: {{.Values.proxy.uid}}
terminationMessagePolicy: FallbackToLogsOnError
{{- if .Values.Proxy.WaitBeforeExitSeconds }}
{{- if .Values.proxy.waitBeforeExitSeconds }}
lifecycle:
preStop:
exec:
command:
- /bin/bash
- -c
- sleep {{.Values.Proxy.WaitBeforeExitSeconds}}
- sleep {{.Values.proxy.waitBeforeExitSeconds}}
{{- end }}
{{- if or (not .Values.proxy.disableIdentity) (.Values.proxy.saMountPath) }}
volumeMounts:
Expand Down
16 changes: 9 additions & 7 deletions cli/cmd/inject.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,6 @@ import (
"strconv"
"strings"

"github.com/docker/docker/api/types/time"

jsonpatch "github.com/evanphx/json-patch"
cfg "github.com/linkerd/linkerd2/controller/gen/config"
"github.com/linkerd/linkerd2/controller/gen/public"
Expand Down Expand Up @@ -102,10 +100,10 @@ sub-folders, or coming from stdin.`,
&manualOption, "manual", manualOption,
"Include the proxy sidecar container spec in the YAML output (the auto-injector won't pick it up, so config annotations aren't supported) (default false)",
)
flags.DurationVar(
&options.waitBeforeExit, "wait-before-exit", options.waitBeforeExit,
flags.UintVar(
&options.waitBeforeExitSeconds, "wait-before-exit-seconds", options.waitBeforeExitSeconds,
"The period during which the proxy sidecar must stay alive while its pod is terminating. "+
"Must be smaller than terminationGracePeriodSeconds for the pod (default 0). E.g.: 2m, 10s, 2m3s",
"Must be smaller than terminationGracePeriodSeconds for the pod (default 0)",
)
flags.BoolVar(
&options.disableIdentity, "disable-identity", options.disableIdentity,
Expand Down Expand Up @@ -446,11 +444,15 @@ func (options *proxyConfigOptions) overrideConfigs(configs *cfg.All, overrideAnn
if options.traceCollectorSvcAccount != "" {
overrideAnnotations[k8s.ProxyTraceCollectorSvcAccountAnnotation] = options.traceCollectorSvcAccount
}
if options.waitBeforeExit != 0 {
overrideAnnotations[k8s.ProxyWaitBeforeExitAnnotation] = time.DurationToSecondsString(options.waitBeforeExit) + "s"
if options.waitBeforeExitSeconds != 0 {
overrideAnnotations[k8s.ProxyWaitBeforeExitAnnotation] = uintToString(options.waitBeforeExitSeconds)
}
}

func uintToString(v uint) string {
return strconv.FormatUint(uint64(v), 10)
}

func toPort(p uint) *cfg.Port {
return &cfg.Port{Port: uint32(p)}
}
Expand Down
2 changes: 1 addition & 1 deletion cli/cmd/install.go
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,7 @@ func newInstallOptionsWithDefaults() (*installOptions, error) {
proxyCPULimit: defaults.Proxy.Resources.CPU.Limit,
proxyMemoryLimit: defaults.Proxy.Resources.Memory.Limit,
enableExternalProfiles: defaults.Proxy.EnableExternalProfiles,
waitBeforeExit: time.Duration(defaults.Proxy.WaitBeforeExitSeconds) * time.Second,
waitBeforeExitSeconds: defaults.Proxy.WaitBeforeExitSeconds,
},
identityOptions: &installIdentityOptions{
trustDomain: defaults.Identity.TrustDomain,
Expand Down
2 changes: 1 addition & 1 deletion cli/cmd/root.go
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ type proxyConfigOptions struct {
enableExternalProfiles bool
traceCollector string
traceCollectorSvcAccount string
waitBeforeExit time.Duration
waitBeforeExitSeconds uint
// ignoreCluster is not validated by validate().
ignoreCluster bool
disableIdentity bool
Expand Down
2 changes: 1 addition & 1 deletion pkg/charts/linkerd2/values.go
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ type (
Resources *Resources `json:"resources"`
Trace *Trace `json:"trace"`
UID int64 `json:"uid"`
WaitBeforeExitSeconds int32 `json:"waitBeforeExitSeconds"`
WaitBeforeExitSeconds uint `json:"waitBeforeExitSeconds"`
}

// ProxyInit contains the fields to set the proxy-init container
Expand Down
29 changes: 13 additions & 16 deletions pkg/inject/inject.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,6 @@ import (
"encoding/json"
"errors"
"fmt"
"regexp"
"sort"
"strconv"
"strings"
"time"

"github.com/linkerd/linkerd2/controller/gen/config"
"github.com/linkerd/linkerd2/pkg/charts"
l5dcharts "github.com/linkerd/linkerd2/pkg/charts/linkerd2"
Expand All @@ -24,7 +18,11 @@ import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/helm/pkg/chartutil"
"regexp"
"sigs.k8s.io/yaml"
"sort"
"strconv"
"strings"
)

const (
Expand Down Expand Up @@ -452,6 +450,7 @@ func (conf *ResourceConfig) complete(template *corev1.PodTemplateSpec) {

// injectPodSpec adds linkerd sidecars to the provided PodSpec.
func (conf *ResourceConfig) injectPodSpec(values *patch) {
waitBeforeExitSeconds, _ := conf.proxyWaitBeforeExitSeconds()
values.Proxy = &l5dcharts.Proxy{
Component: conf.pod.labels[k8s.ProxyDeploymentLabel],
EnableExternalProfiles: conf.enableExternalProfiles(),
Expand All @@ -470,7 +469,7 @@ func (conf *ResourceConfig) injectPodSpec(values *patch) {
},
UID: conf.proxyUID(),
Resources: conf.proxyResourceRequirements(),
WaitBeforeExitSeconds: int32(conf.proxyWaitBeforeExit() / time.Second),
WaitBeforeExitSeconds: waitBeforeExitSeconds,
}

if v := conf.pod.meta.Annotations[k8s.ProxyEnableDebugAnnotation]; v != "" {
Expand Down Expand Up @@ -759,20 +758,18 @@ func (conf *ResourceConfig) tapDisabled() bool {
return false
}

func (conf *ResourceConfig) proxyWaitBeforeExit() time.Duration {
func (conf *ResourceConfig) proxyWaitBeforeExitSeconds() (uint, error) {
if override := conf.getOverride(k8s.ProxyWaitBeforeExitAnnotation); override != "" {
waitBeforeExit, err := time.ParseDuration(override)

// Try to parse plain integer as value in seconds if it is not formatted as Duration
if err != nil {
waitInSeconds, _ := strconv.ParseUint(override, 10, 32)
waitBeforeExit = time.Duration(waitInSeconds) * time.Second
waitBeforeExitSeconds, err := strconv.ParseUint(override, 10, 32)
if nil != err {
log.Warnf("unrecognized value used for the %s annotation, unsigned int is expected: %s",
k8s.ProxyWaitBeforeExitAnnotation, override)
}

return waitBeforeExit
return uint(waitBeforeExitSeconds), err
}

return 0
return 0, nil
}

func (conf *ResourceConfig) proxyResourceRequirements() *l5dcharts.Resources {
Expand Down
131 changes: 67 additions & 64 deletions pkg/inject/inject_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package inject
import (
"reflect"
"testing"
"time"

"github.com/linkerd/linkerd2/controller/gen/config"
l5dcharts "github.com/linkerd/linkerd2/pkg/charts/linkerd2"
Expand All @@ -16,24 +15,24 @@ import (
)

type expectedProxyConfigs struct {
identityContext *config.IdentityContext
image string
imagePullPolicy string
proxyVersion string
controlPort int32
inboundPort int32
adminPort int32
outboundPort int32
proxyWaitBeforeExit time.Duration
logLevel string
resourceRequirements *l5dcharts.Resources
proxyUID int64
initImage string
initImagePullPolicy string
initVersion string
inboundSkipPorts string
outboundSkipPorts string
trace *l5dcharts.Trace
identityContext *config.IdentityContext
image string
imagePullPolicy string
proxyVersion string
controlPort int32
inboundPort int32
adminPort int32
outboundPort int32
proxyWaitBeforeExitSeconds uint
logLevel string
resourceRequirements *l5dcharts.Resources
proxyUID int64
initImage string
initImagePullPolicy string
initVersion string
inboundSkipPorts string
outboundSkipPorts string
trace *l5dcharts.Trace
}

func TestConfigAccessors(t *testing.T) {
Expand Down Expand Up @@ -109,22 +108,22 @@ func TestConfigAccessors(t *testing.T) {
k8s.ProxyVersionOverrideAnnotation: proxyVersionOverride,
k8s.ProxyTraceCollectorSvcAddrAnnotation: "oc-collector.tracing:55678",
k8s.ProxyTraceCollectorSvcAccountAnnotation: "default",
k8s.ProxyWaitBeforeExitAnnotation: "2m3s",
k8s.ProxyWaitBeforeExitAnnotation: "123",
},
},
Spec: corev1.PodSpec{},
},
},
expected: expectedProxyConfigs{
image: "gcr.io/linkerd-io/proxy",
imagePullPolicy: "Always",
proxyVersion: proxyVersionOverride,
controlPort: int32(4000),
inboundPort: int32(5000),
adminPort: int32(5001),
outboundPort: int32(5002),
proxyWaitBeforeExit: time.Duration(123) * time.Second,
logLevel: "debug,linkerd2_proxy=debug",
image: "gcr.io/linkerd-io/proxy",
imagePullPolicy: "Always",
proxyVersion: proxyVersionOverride,
controlPort: int32(4000),
inboundPort: int32(5000),
adminPort: int32(5001),
outboundPort: int32(5002),
proxyWaitBeforeExitSeconds: 123,
logLevel: "debug,linkerd2_proxy=debug",
resourceRequirements: &l5dcharts.Resources{
CPU: l5dcharts.Constraints{
Limit: "1500m",
Expand Down Expand Up @@ -155,16 +154,16 @@ func TestConfigAccessors(t *testing.T) {
},
},
expected: expectedProxyConfigs{
identityContext: &config.IdentityContext{},
image: "gcr.io/linkerd-io/proxy",
imagePullPolicy: "IfNotPresent",
proxyVersion: proxyVersion,
controlPort: int32(9000),
inboundPort: int32(6000),
adminPort: int32(6001),
outboundPort: int32(6002),
proxyWaitBeforeExit: time.Duration(0),
logLevel: "info,linkerd2_proxy=debug",
identityContext: &config.IdentityContext{},
image: "gcr.io/linkerd-io/proxy",
imagePullPolicy: "IfNotPresent",
proxyVersion: proxyVersion,
controlPort: int32(9000),
inboundPort: int32(6000),
adminPort: int32(6001),
outboundPort: int32(6002),
proxyWaitBeforeExitSeconds: 0,
logLevel: "info,linkerd2_proxy=debug",
resourceRequirements: &l5dcharts.Resources{
CPU: l5dcharts.Constraints{
Limit: "1",
Expand Down Expand Up @@ -205,23 +204,23 @@ func TestConfigAccessors(t *testing.T) {
k8s.ProxyVersionOverrideAnnotation: proxyVersionOverride,
k8s.ProxyTraceCollectorSvcAddrAnnotation: "oc-collector.tracing:55678",
k8s.ProxyTraceCollectorSvcAccountAnnotation: "default",
k8s.ProxyWaitBeforeExitAnnotation: "2m3s",
k8s.ProxyWaitBeforeExitAnnotation: "123",
},
spec: appsv1.DeploymentSpec{
Template: corev1.PodTemplateSpec{
Spec: corev1.PodSpec{},
},
},
expected: expectedProxyConfigs{
image: "gcr.io/linkerd-io/proxy",
imagePullPolicy: "Always",
proxyVersion: proxyVersionOverride,
controlPort: int32(4000),
inboundPort: int32(5000),
adminPort: int32(5001),
outboundPort: int32(5002),
proxyWaitBeforeExit: time.Duration(123) * time.Second,
logLevel: "debug,linkerd2_proxy=debug",
image: "gcr.io/linkerd-io/proxy",
imagePullPolicy: "Always",
proxyVersion: proxyVersionOverride,
controlPort: int32(4000),
inboundPort: int32(5000),
adminPort: int32(5001),
outboundPort: int32(5002),
proxyWaitBeforeExitSeconds: 123,
logLevel: "debug,linkerd2_proxy=debug",
resourceRequirements: &l5dcharts.Resources{
CPU: l5dcharts.Constraints{
Limit: "1500m",
Expand All @@ -244,9 +243,9 @@ func TestConfigAccessors(t *testing.T) {
},
},
},
{id: "wait before exit annotation is integer",
{id: "use not a uint value for ProxyWaitBeforeExitAnnotation annotation",
nsAnnotations: map[string]string{
k8s.ProxyWaitBeforeExitAnnotation: "222",
k8s.ProxyWaitBeforeExitAnnotation: "-111",
},
spec: appsv1.DeploymentSpec{
Template: corev1.PodTemplateSpec{
Expand All @@ -255,16 +254,16 @@ func TestConfigAccessors(t *testing.T) {
},
},
expected: expectedProxyConfigs{
identityContext: &config.IdentityContext{},
image: "gcr.io/linkerd-io/proxy",
imagePullPolicy: "IfNotPresent",
proxyVersion: proxyVersion,
controlPort: int32(9000),
inboundPort: int32(6000),
adminPort: int32(6001),
outboundPort: int32(6002),
proxyWaitBeforeExit: time.Duration(222) * time.Second,
logLevel: "info,linkerd2_proxy=debug",
identityContext: &config.IdentityContext{},
image: "gcr.io/linkerd-io/proxy",
imagePullPolicy: "IfNotPresent",
proxyVersion: proxyVersion,
controlPort: int32(9000),
inboundPort: int32(6000),
adminPort: int32(6001),
outboundPort: int32(6002),
proxyWaitBeforeExitSeconds: 0,
logLevel: "info,linkerd2_proxy=debug",
resourceRequirements: &l5dcharts.Resources{
CPU: l5dcharts.Constraints{
Limit: "1",
Expand Down Expand Up @@ -361,9 +360,13 @@ func TestConfigAccessors(t *testing.T) {
}
})

t.Run("proxyWaitBeforeExit", func(t *testing.T) {
expected := testCase.expected.proxyWaitBeforeExit
if actual := resourceConfig.proxyWaitBeforeExit(); expected != actual {
t.Run("proxyWaitBeforeExitSeconds", func(t *testing.T) {
expected := testCase.expected.proxyWaitBeforeExitSeconds
actual, err := resourceConfig.proxyWaitBeforeExitSeconds()
if nil != err {
// negative test cases above cover the situation
}
if expected != actual {
t.Errorf("Expected: %v Actual: %v", expected, actual)
}
})
Expand Down
2 changes: 1 addition & 1 deletion pkg/k8s/labels.go
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ const (
// ProxyWaitBeforeExitAnnotation makes the proxy container to wait for the given period before exiting
// after the Pod entered the Terminating state. Must be smaller than terminationGracePeriodSeconds
// configured for the Pod
ProxyWaitBeforeExitAnnotation = ProxyConfigAnnotationsPrefixAlpha + "/proxy-wait-before-exit"
ProxyWaitBeforeExitAnnotation = ProxyConfigAnnotationsPrefixAlpha + "/proxy-wait-before-exit-seconds"

// ProxyTraceCollectorSvcAccountAnnotation is used to specify the service account
// associated with the trace collector. It is used to create the service's
Expand Down

0 comments on commit 04812a9

Please sign in to comment.