Repository: linkerd/linkerd2
Pull request #5170 (2020-11-03 02:05:29)

inject: Set LINKERD2_PROXY_CORES from the cpu limit
Per #5165, Kubernetes does not necessarily limit the proxy's access to
cores via `cgroups` when a CPU limit is set. As of #5168, the proxy now
supports a `LINKERD2_PROXY_CORES` environment configuration.

This change modifies the proxy injector to ensure that this environment
is configured from `Values.proxy.cores` Helm value and the
`config.linkerd.io/proxy-cpu-limit` annotation.

<!--  Thanks for sending a pull request!

If you already have a well-structured git commit message, chances are GitHub
set the title and description of this PR to the git commit message subject and
body, respectively. If so, you may delete these instructions and submit your PR.

If this is your first time, please read our contributor guide:
https://github.com/linkerd/linkerd2/blob/main/CONTRIBUTING.md

The title and description of your Pull Request should match the git commit
subject and body, respectively. Git commit messages are structured as follows:

```
Subject

Problem

Solution

Validation

Fixes #[GitHub issue ID]

DCO Sign off
```

Example git commit message:

```
Introduce Pull Request Template

GitHub's community guidelines recommend a pull request template, the repo was
lacking one.

Introduce a `PULL_REQUEST_TEMPLATE.md` file.

Once merged, the
[Community profile checklist](https://github.com/linkerd/linkerd2/community)
should indicate the repo now provides a pull request template.

Fixes #3321

Signed-off-by: Jane Smith <jane.smith@example.com>
```

Note the git commit message subject becomes the pull request title.

For more details around git commits, see the section on Committing in our
contributor guide:
https://github.com/linkerd/linkerd2/blob/main/CONTRIBUTING.md#committing
-->


Pothulapati: Hmmm, Why not use `values.Global.Proxy.Resources.CPU.Limit` for the same? should make this easy as if we use the same field to set the new env in the template, all the injection (from annotation) and install flow (i.e with `--proxy-cpu-limit)` would work right away. WDYT?

 With this route all of them have to be updated to take into account this new field.
alpeb: @Pothulapati I guess that's because you wouldn't be able to implement directly in the template what `toWholeCPUCores` does...
Pothulapati: @alpeb Ahh, That makes sense then! I'm also guessing it would be really hard to convert k8s.quantity into a whole number in Helm.

With this new variable, we should be overwriting this variable wherever `.values.Global.Proxy.Resources.CPU.Limit` is being to make inject and install overriding work.
alpeb: > With this new variable, we should be overwriting this variable wherever `.values.Global.Proxy.Resources.CPU.Limit` is being to make inject and install overriding work.

Right, for instance `linkerd install --proxy-cpu-limit` is not affecting this new env var, not for control-plane pods nor for pods injected by the injector.

This also made me realize that for the helm install workflows, control-plane pods won't get this env var even if `global.Proxy.Resources.CPU.Limit` is explicitly set. So we end up with the same problem for CP proxies when the node has more cores than that Limit...

One not so straightforward way around that would be to expose in `values.yaml` separate `.proxy.core` entries for each CP component with anchors pointing to `global.Proxy.Resources.CPU.Limit` but then the proxy would have to deal with non-integer values. Actually if the proxy could deal with k8's Quantities directly, we could feed the env var the Limit directly like @Pothulapati suggested and all these issues would go away without additional `values.yaml` entries...
olix0r: > With this new variable, we should be overwriting this variable wherever .values.Global.Proxy.Resources.CPU.Limit is being to make inject and install overriding work.

It's my preference that we not delay the edge release on this (and that we not make further changes between the edge and stable release, unless critical). I think it's sufficient to document this behavior, as it's only necessary when configuring via Helm and kubelet is not enforcing QoS.

The one thing we should confirm, though, is that the correct values are set when the `--proxy-cpu-limit` flag is set at install/upgrade-time.

Patch:
diff --git a/charts/linkerd2/README.md b/charts/linkerd2/README.md
index 2e2e8f145..14bbd69ff 100644
--- a/charts/linkerd2/README.md
+++ b/charts/linkerd2/README.md
@@ -115,6 +115,7 @@ their default values.
 | `global.linkerdVersion`                     | Control plane version                                                                                                                                                                 | latest version                       |
 | `global.namespace`                          | Control plane namespace                                                                                                                                                               | `linkerd`                            |
 | `global.prometheusUrl`     | URL of external prometheus instance to perform queries, used by the `public-api`                                                                                                                    |                               |
+| `global.proxy.cores`                        | The number of proxy threads to be allocated for each proxy. Must be a whole number, and should be kept in sync with `global.proxy.resources.cpu.limit`, if set.                       |                                      |
 | `global.proxy.enableExternalProfiles`       | Enable service profiles for non-Kubernetes services                                                                                                                                   | `false`                              |
 | `global.proxy.image.name`                   | Docker image for the proxy                                                                                                                                                            | `ghcr.io/linkerd/proxy`            |
 | `global.proxy.image.pullPolicy`             | Pull policy for the proxy container Docker image                                                                                                                                      | `IfNotPresent`                       |
diff --git a/charts/linkerd2/values.yaml b/charts/linkerd2/values.yaml
index 7eeb7bb7a..c7b606ee7 100644
--- a/charts/linkerd2/values.yaml
+++ b/charts/linkerd2/values.yaml
@@ -66,6 +66,10 @@ global:
       control: 4190
       inbound: 4143
       outbound: 4140
+    # The `cpu.limit` and `cores` should be kept in sync. The value of `cores`
+    # must be an integer and should typically be set by rounding up from the
+    # limit. E.g. if cpu.limit is '1500m', cores should be 2.
+    cores: 0
     resources:
       cpu:
         limit: ""
diff --git a/charts/partials/templates/_proxy.tpl b/charts/partials/templates/_proxy.tpl
index 36dc5c571..0600e114b 100644
--- a/charts/partials/templates/_proxy.tpl
+++ b/charts/partials/templates/_proxy.tpl
@@ -1,5 +1,9 @@
 {{ define "partials.proxy" -}}
 env:
+{{- if .Values.global.proxy.cores }}
+- name: LINKERD2_PROXY_CORES
+  value: {{.Values.global.proxy.cores | quote}}
+{{- end }}
 {{ if .Values.global.proxy.requireIdentityOnInboundPorts -}}
 - name: LINKERD2_PROXY_INBOUND_PORTS_REQUIRE_IDENTITY
   value: {{.Values.global.proxy.requireIdentityOnInboundPorts | quote}}
diff --git a/cli/cmd/options.go b/cli/cmd/options.go
index 1495c69c0..57dd94d26 100644
--- a/cli/cmd/options.go
+++ b/cli/cmd/options.go
@@ -12,6 +12,7 @@ import (
 
 	"github.com/linkerd/linkerd2/cli/flag"
 	l5dcharts "github.com/linkerd/linkerd2/pkg/charts/linkerd2"
+	"github.com/linkerd/linkerd2/pkg/inject"
 	"github.com/linkerd/linkerd2/pkg/issuercerts"
 	"github.com/linkerd/linkerd2/pkg/k8s"
 	consts "github.com/linkerd/linkerd2/pkg/k8s"
@@ -67,7 +68,9 @@ func makeInstallUpgradeFlags(defaults *l5dcharts.Values) ([]flag.Flag, *pflag.Fl
 					values.ControllerReplicas = haValues.ControllerReplicas
 					values.Global.Proxy.Resources.CPU.Request = haValues.Global.Proxy.Resources.CPU.Request
 					values.Global.Proxy.Resources.Memory.Request = haValues.Global.Proxy.Resources.Memory.Request
-					values.Global.Proxy.Resources.CPU.Limit = haValues.Global.Proxy.Resources.CPU.Limit
+					// NOTE: CPU Limits are not currently set by default HA charts.
+					//values.Global.Proxy.Cores = haValues.Global.Proxy.Cores
+					//values.Global.Proxy.Resources.CPU.Limit = haValues.Global.Proxy.Resources.CPU.Limit
 					values.Global.Proxy.Resources.Memory.Limit = haValues.Global.Proxy.Resources.Memory.Limit
 				}
 				return nil
@@ -402,6 +405,15 @@ func makeProxyFlags(defaults *l5dcharts.Values) ([]flag.Flag, *pflag.FlagSet) {
 
 		flag.NewStringFlag(proxyFlags, "proxy-cpu-limit", defaults.Global.Proxy.Resources.CPU.Limit, "Maximum amount of CPU units that the proxy sidecar can use",
 			func(values *l5dcharts.Values, value string) error {
+				q, err := k8sResource.ParseQuantity(value)
+				if err != nil {
+					return err
+				}
+				c, err := inject.ToWholeCPUCores(q)
+				if err != nil {
+					return err
+				}
+				values.Global.Proxy.Cores = c
 				values.Global.Proxy.Resources.CPU.Limit = value
 				return nil
 			}),
diff --git a/pkg/charts/linkerd2/values.go b/pkg/charts/linkerd2/values.go
index 34e3992e9..15306b1c2 100644
--- a/pkg/charts/linkerd2/values.go
+++ b/pkg/charts/linkerd2/values.go
@@ -112,7 +112,9 @@ type (
 
 	// Proxy contains the fields to set the proxy sidecar container
 	Proxy struct {
-		Capabilities                  *Capabilities    `json:"capabilities"`
+		Capabilities *Capabilities `json:"capabilities"`
+		// This should match .Resources.CPU.Limit, but must be a whole number
+		Cores                         int64            `json:"cores,omitempty"`
 		DisableIdentity               bool             `json:"disableIdentity"`
 		DisableTap                    bool             `json:"disableTap"`
 		EnableExternalProfiles        bool             `json:"enableExternalProfiles"`
diff --git a/pkg/inject/inject.go b/pkg/inject/inject.go
index 62357d6ea..fc228ddb5 100644
--- a/pkg/inject/inject.go
+++ b/pkg/inject/inject.go
@@ -805,11 +805,17 @@ func (conf *ResourceConfig) applyAnnotationOverrides(values *l5dcharts.Values) {
 	}
 
 	if override, ok := annotations[k8s.ProxyCPULimitAnnotation]; ok {
-		_, err := k8sResource.ParseQuantity(override)
+		q, err := k8sResource.ParseQuantity(override)
 		if err != nil {
 			log.Warnf("%s (%s)", err, k8s.ProxyCPULimitAnnotation)
 		} else {
 			values.Global.Proxy.Resources.CPU.Limit = override
+
+			n, err := ToWholeCPUCores(q)
+			if err != nil {
+				log.Warnf("%s (%s)", err, k8s.ProxyCPULimitAnnotation)
+			}
+			values.Global.Proxy.Cores = n
 		}
 	}
 
@@ -985,3 +991,12 @@ func ToPortRanges(portRanges []string) []*config.PortRange {
 	}
 	return ports
 }
+
+// ToWholeCPUCores coerces a k8s resource value to a whole integer value, rounding up.
+func ToWholeCPUCores(q k8sResource.Quantity) (int64, error) {
+	q.RoundUp(0)
+	if n, ok := q.AsInt64(); ok {
+		return n, nil
+	}
+	return 0, fmt.Errorf("Could not parse cores: %s", q.String())
+}


Test patch:
diff --git a/cli/cmd/testdata/inject_emojivoto_deployment_config_overrides.golden.yml b/cli/cmd/testdata/inject_emojivoto_deployment_config_overrides.golden.yml
index 1b5ea2395..7089caf89 100644
--- a/cli/cmd/testdata/inject_emojivoto_deployment_config_overrides.golden.yml
+++ b/cli/cmd/testdata/inject_emojivoto_deployment_config_overrides.golden.yml
@@ -45,6 +45,8 @@ spec:
         - containerPort: 80
           name: http
       - env:
+        - name: LINKERD2_PROXY_CORES
+          value: "1"
         - name: LINKERD2_PROXY_LOG
           value: warn,linkerd=info
         - name: LINKERD2_PROXY_LOG_FORMAT
diff --git a/pkg/inject/inject_test.go b/pkg/inject/inject_test.go
index 8eb69340b..7f437da1e 100644
--- a/pkg/inject/inject_test.go
+++ b/pkg/inject/inject_test.go
@@ -9,6 +9,7 @@ import (
 	"github.com/linkerd/linkerd2/pkg/version"
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
+	k8sResource "k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"sigs.k8s.io/yaml"
 )
@@ -73,6 +74,7 @@ func TestGetOverriddenValues(t *testing.T) {
 			expected: func() *l5dcharts.Values {
 				values, _ := l5dcharts.NewValues(false)
 
+				values.Global.Proxy.Cores = 2
 				values.Global.Proxy.DisableIdentity = true
 				values.Global.Proxy.Image.Name = "ghcr.io/linkerd/proxy"
 				values.Global.Proxy.Image.PullPolicy = pullPolicy
@@ -160,6 +162,7 @@ func TestGetOverriddenValues(t *testing.T) {
 			expected: func() *l5dcharts.Values {
 				values, _ := l5dcharts.NewValues(false)
 
+				values.Global.Proxy.Cores = 2
 				values.Global.Proxy.DisableIdentity = true
 				values.Global.Proxy.Image.Name = "ghcr.io/linkerd/proxy"
 				values.Global.Proxy.Image.PullPolicy = pullPolicy
@@ -288,3 +291,27 @@ func TestGetOverriddenValues(t *testing.T) {
 		})
 	}
 }
+
+func TestWholeCPUCores(t *testing.T) {
+	for _, c := range []struct {
+		v string
+		n int
+	}{
+		{v: "1", n: 1},
+		{v: "1m", n: 1},
+		{v: "1000m", n: 1},
+		{v: "1001m", n: 2},
+	} {
+		q, err := k8sResource.ParseQuantity(c.v)
+		if err != nil {
+			t.Fatal(err)
+		}
+		n, err := ToWholeCPUCores(q)
+		if err != nil {
+			t.Fatal(err)
+		}
+		if n != int64(c.n) {
+			t.Fatalf("Unexpected value: %v != %v", n, c.n)
+		}
+	}
+}