Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CPU limit behaviour for istio-proxy changes from 1.6+ #28465

Closed
Stono opened this issue Oct 31, 2020 · 9 comments
Closed

CPU limit behaviour for istio-proxy changes from 1.6+ #28465

Stono opened this issue Oct 31, 2020 · 9 comments
Labels
area/environments area/perf and scalability lifecycle/automatically-closed Indicates a PR or issue that has been closed automatically. lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while

Comments

@Stono
Copy link
Contributor

Stono commented Oct 31, 2020

Bug description
There are numerous known issues with overzealous docker CPU throttling, for certain critical components - such as istio-proxy, we do not set CPU limits, only CPU requests.

On 1.5 you could achieve that with:

  proxy:
    resources:
      requests:
        cpu: 30m
        memory: 65Mi
      limits:
        memory: 512Mi

Which mapped into the injection-template:

          "resources": {
            "limits": {
              "memory": "512Mi"
            },
            "requests": {
              "cpu": "30m",
              "memory": "65Mi"
            }
          },

However, on 1.6, the same configuration in istiooperator.yaml, the cpu when omitted is defaulted to 2000m

          "resources": {
            "limits": {
              "cpu": "2000m",
              "memory": "512Mi"
            },
            "requests": {
              "cpu": "30m",
              "memory": "65Mi"
            }
          },

This has led to some unwanted throttling on core services such as ingress-nginix, and caused a spike iin container_cpu_cfs_throttled_seconds_total cardinality in prometheus (almost doubled, due to now including istio-proxy)

It's worth noting that for 1.6 we're using istioctl manifest generate, whereas on 1.5 we were using the helm template.

[ ] Docs
[x] Installation
[ ] Networking
[x] Performance and Scalability
[ ] Extensions and Telemetry
[ ] Security
[ ] Test and Release
[x] User Experience
[ ] Developer Infrastructure

Expected behavior
It to work like 1.5, or if the behaviour has changed for a good reason, clearly explaining why in the upgrade notes for 1.6

Steps to reproduce the bug

Version (include the output of istioctl version --remote and kubectl version --short and helm version if you used Helm)
1.6.13

How was Istio installed?
Helm

Environment where bug was observed (cloud vendor, OS, etc)

@howardjohn
Copy link
Member

Is this just an (undocumented) defaulting change or is it also not possible to unset the limit?

@Stono
Copy link
Contributor Author

Stono commented Oct 31, 2020

@howardjohn I can't seem to unset it, so it could be both an intentional but undocumented change but then also an issue that I can't remove it

@howardjohn
Copy link
Member

1.5 chart:

        resources:
          {{ if or (isset .ObjectMeta.Annotations `sidecar.istio.io/proxyCPU`) (isset .ObjectMeta.Annotations `sidecar.istio.io/proxyMemory`) -}}
          requests:
            {{ if (isset .ObjectMeta.Annotations `sidecar.istio.io/proxyCPU`) -}}
            cpu: "{{ index .ObjectMeta.Annotations `sidecar.istio.io/proxyCPU` }}"
            {{ end}}
            {{ if (isset .ObjectMeta.Annotations `sidecar.istio.io/proxyMemory`) -}}
            memory: "{{ index .ObjectMeta.Annotations `sidecar.istio.io/proxyMemory` }}"
            {{ end }}
        {{ else -}}
      {{- if .Values.global.proxy.resources }}
          {{ toYaml .Values.global.proxy.resources | indent 4 }}
      {{- end }}

Seems like it would set it if you didn't have an annotation, and we didn't support setting limit via annotation

$ helm template . -f /tmp/values.yml | k grep ConfigMap/istio-sidecar-injector | toJson | jq .data.values -r | jq '.global.proxy.resources'
{
  "limits": {
    "cpu": "2000m",
    "memory": "512Mi"
  },
  "requests": {
    "cpu": "30m",
    "memory": "65Mi"
  }
}

Suggest the CPU is set there as well

How did you actually get this below?

Which mapped into the injection-template:

      "resources": {
        "limits": {
          "memory": "512Mi"
        },
        "requests": {
          "cpu": "30m",
          "memory": "65Mi"
        }
      },

@messiahUA
Copy link

Stumbled upon a similar issue. In 1.6 it was possible to set proxy.resources.limits to null and it removed limits. After upgrade to 1.7 it stopped working in spite of it is unset in istiooperators.install.istio.io as expected.

@istio-policy-bot istio-policy-bot added the lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while label Feb 1, 2021
@istio-policy-bot
Copy link

🚧 This issue or pull request has been closed due to not having had activity from an Istio team member since 2020-11-02. If you feel this issue or pull request deserves attention, please reopen the issue. Please see this wiki page for more information. Thank you for your contributions.

Created by the issue and PR lifecycle manager.

@istio-policy-bot istio-policy-bot added the lifecycle/automatically-closed Indicates a PR or issue that has been closed automatically. label Feb 16, 2021
@Mohid-A
Copy link

Mohid-A commented Jul 23, 2022

Stumbled upon a similar issue. In 1.6 it was possible to set proxy.resources.limits to null and it removed limits. After upgrade to 1.7 it stopped working in spite of it is unset in istiooperators.install.istio.io as expected.

@messiahUA I am facing the same issue, setting the limit to "null" does not work with version 1.11.3. Did you find any resolution to this?

@messiahUA
Copy link

Stumbled upon a similar issue. In 1.6 it was possible to set proxy.resources.limits to null and it removed limits. After upgrade to 1.7 it stopped working in spite of it is unset in istiooperators.install.istio.io as expected.

@messiahUA I am facing the same issue, setting the limit to "null" does not work with version 1.11.3. Did you find any resolution to this?

unfortunately, no

@awalther52
Copy link

Just tried setting cpu limit to null with version 1.18.0 and it worked, the cpu limit was removed.

@teceP
Copy link

teceP commented Mar 27, 2024

@awalther52 how you do this?
When i set limits.cpu: null, the cpu limit still uses the default 2000m
When i set limits.cpu: "null" and initially deploy Deployments, I get following error:
admission webhook "namespace.sidecar-injector.istio.io" denied the request: failed to run injection template: failed applying injection overlay: unmarshal patched pod: quantities must match the regular expression '^([+-]?[0-9.]+)([eEinumkKMGTP]*[-+]?[0-9]*)$'

Of course, because "null" is not parsable with the given regex.

When i set the limits.cpu to something regex parsable, then deploy the Deployment, then set limits.cpu: "null", then redeploy the Pod, it works without the error. But this is not the optimal solution for us.
Do you have any hint for me?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/environments area/perf and scalability lifecycle/automatically-closed Indicates a PR or issue that has been closed automatically. lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while
Projects
None yet
Development

No branches or pull requests

8 participants