The Wayback Machine - https://web.archive.org/web/20221220035244/https://github.com/prometheus-operator/kube-prometheus/issues/1500
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

Remove addons/strip-limits.libsonnet addon #1500

Open
paulfantom opened this issue Nov 10, 2021 · 4 comments
Open

Remove addons/strip-limits.libsonnet addon #1500

paulfantom opened this issue Nov 10, 2021 · 4 comments

Comments

@paulfantom
Copy link
Member

paulfantom commented Nov 10, 2021

README.md is still pointing to addons/strip-limits.libsonnet addon as a way to strip containers from set resource limits. However since release-0.8 resource requests are first-level settings and users should use the following form:

values+: {
  <component>+: {
    resources: {
      requests: { SET HERE },
      limits: { SET HERE },
    },
  },
}

README section should be removed and a new doc describing how to set resource requests and limits should be created. This also fits nicely with #1005

Anything else we need to know?:

Issue created as a result of #1498

@andrein
Copy link
Contributor

andrein commented Nov 10, 2021

@paulfantom I did this for all components in my cluster, however I can't find a way to set the resource requests for kube-rbac-proxy.

image

@ArthurSens
Copy link
Member

ArthurSens commented Nov 12, 2021

Good point @andrein, looking at the code it does seem like kube-rbac-proxy is kinda left out.

@paulfantom what do you think about kube-rbac-proxy being embedded into the defaults of the object that use them?

e.g. node exporter could have something like this:

local krp = import './kube-rbac-proxy.libsonnet';

local defaults = {
  local defaults = self,
  // Convention: Top-level fields related to CRDs are public, other fields are hidden
  // If there is no CRD for the component, everything is hidden in defaults.
  name:: 'node-exporter',
  namespace:: error 'must provide namespace',
  version:: error 'must provide version',
  image:: error 'must provide version',
  resources:: {
    requests: { cpu: '102m', memory: '180Mi' },
    limits: { cpu: '250m', memory: '180Mi' },
  },
  .
  .
  .
  .
  kubeRbacProxy:: {
    name: 'kube-rbac-proxy'
    upstream: 'http://127.0.0.1:' + defaults.port + '/',
    secureListenAddress: '[$(IP)]:' + defaults.port,
    resources: {
      requests: { cpu: '100m', memory: '100Mi' },
      limits: { cpu: '150m', memory: '100Mi' },
    }
   .
   .
   .
  }
};

function(params) {
  local ne = self,
  _config:: defaults + params,

  local kubeRbacProxy = krp(ne._config.kubeRbacProxy),
  .
  .
  .
}
@andrein
Copy link
Contributor

andrein commented Nov 12, 2021

I like the idea!

On top of that, assuming that all the kube-rbac-proxies have roughly the same resource usage, I would also like to be able to specify those limits globally in main.libsonnet like so:

{
  values:: {
    common: {
      kubeRbacProxy: {
        resources:: {
          requests: { cpu: '102m', memory: '180Mi' },
          limits: { cpu: '250m', memory: '180Mi' },
        },
      },
    },
  },
}

or, maybe as a separate component like so:

{
  values:: {
    kubeRbacProxy: {
      resources:: {
        requests: { cpu: '102m', memory: '180Mi' },
        limits: { cpu: '250m', memory: '180Mi' },
      },
    },
  }
}

What do you think?

@paulfantom
Copy link
Member Author

paulfantom commented Nov 15, 2021

That would make sense in the past. However, right now I think this would clash with the idea of reusing CRD-defined fields in defaults and reducing additional fields.

We could put the kube-rbac-proxy definition into defaults.containers list though, but this would still be problematic from UX pov.


From what I've seen in most cases people want to remove limits from kube-rbac-proxy container because of info-level CPUThrottlingHigh alert firing. In those cases a solution is not to remove limits (this is considered a bad practice) or increase limits, but to check if this affects operations of the service. If it affects operations - please file a bug report and we can increase those values for all. However if it doesn't affect the operations, consider doing one of the following:

  • silence the alert forever
  • inhibit all info-level alerts
  • change $.values.kubernetesControlPlane.mixin._config.cpuThrottlingPercent to a different value (default is 25).

Long-term solution is being worked in #861.

Because of that I think we should discourage usage of strip-limits.libsonnet addon, describe how to set limits for components via values, and describe why removing limits from kube-rbac-proxy is not a correct path forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment