kube-prometheus icon indicating copy to clipboard operation
kube-prometheus copied to clipboard

Remove addons/strip-limits.libsonnet addon

Open paulfantom opened this issue 4 years ago • 4 comments

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 https://github.com/prometheus-operator/kube-prometheus/issues/1005

Anything else we need to know?:

Issue created as a result of https://github.com/prometheus-operator/kube-prometheus/pull/1498

paulfantom avatar Nov 10 '21 18:11 paulfantom

@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

andrein avatar Nov 10 '21 20:11 andrein

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),
  .
  .
  .
}

ArthurSens avatar Nov 12 '21 00:11 ArthurSens

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?

andrein avatar Nov 12 '21 06:11 andrein

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 https://github.com/prometheus-operator/kube-prometheus/issues/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.

paulfantom avatar Nov 15 '21 09:11 paulfantom