Set cluster-autoscaler.kubernetes.io/safe-to-evict: "true" on sidecar by default (was EmptyDir in istio sidecar prevents cluster autoscaler from evicting a node)

kyessenov commented 4 years ago

Per https://github.com/kubernetes/autoscaler/blob/master/cluster-autoscaler/FAQ.md#what-types-of-pods-can-prevent-ca-from-removing-a-node, presence of local storage emptyDir prevents the cluster autoscaler from evicting nodes with injected pods.

/cc @howardjohn @duderino

taisph commented 4 years ago

What's the recommended way to apply the eviction annotation to istio 1.3.8 sidecars?

philandstuff commented 4 years ago

A different workaround from using the annotation: call cluster autoscaler with --skip-nodes-with-local-storage=false. This causes the cluster autoscaler to ignore HostPath and EmptyDir for determining which pods are evictable. (You should check if this is appropriate for your workloads before blindly applying it.)

This is IMO preferable because there may be other reasons that a particular istio-injected pod should be considered unsafe to evict, but injecting the annotation would cause the cluster autoscaler to ignore all those criteria.

ericbuehl commented 4 years ago

Is there a recommended strategy for making sure the pods in istio-system themselves do not prevent scaling down as they do not have the safe-to-evict annotation?

seeruk commented 4 years ago

One thing to note here is that on GKE you can't configure CA with --skip-nodes-with-local-storage=false. Your only option is to spam the safe-to-evict annotation everywhere, which is quite unfortunate.

howardjohn commented 4 years ago

Maybe we can just inject the annotation if the user doesn't have emptyDir in their pod spec. Although technically it could cause issues if a later mutating webhook adds an emptyDir

seeruk commented 4 years ago

As CA isn't configurable on GKE I opted for developing a mutating admission controller that flips how this behaviour works in a way. If you have a safe-to-evict annotation already present (with any value) it leaves it alone, otherwise it adds the safe-to-evict=true annotation (excluding kube-system and kube-public namespaces). This way we don't need to spam the annotations anywhere, and the default behaviour is to allow eviction (in our cluster we completely avoid persistent state, so this default makes far more sense).

Unfortunately, it's closed-source right now, but it was pretty simple to develop. After we've battle-tested it a bit more I'll see if I can open source it because it might help sidestep some issues like this one.

nrjpoddar commented 4 years ago

Is this still an issue even if you use "memory" emptyDir? We have two emptyDirs usage in proxy, one is memory and the other is not? What is "istio-data" volume used for?

thesuperzapper commented 4 years ago

Just wanted to put a solution for those who come across this Issue on Google.

If you are installing the istio with the istio helm chart, you can, use the sidecarInjectorWebhook.injectedAnnotations value to inject the safe-to-evict annotation at the same time as the istio-sidecar.

Add this to your `values.yaml`:

sidecarInjectorWebhook:
  enabled: true
  injectedAnnotations:
    cluster-autoscaler.kubernetes.io/safe-to-evict: "true"

rmoesbergen commented 4 years ago

Is this still an issue even if you use "memory" emptyDir? We have two emptyDirs usage in proxy, one is memory and the other is not? What is "istio-data" volume used for?

I checked the source of autoscaler, and the presence of emptyDir (no matter which backend), will count as 'local storage' and prevent eviction.

pc-rshetty commented 3 years ago

Just wanted to put a solution for those who come across this Issue on Google.

If you are installing the istio with the istio helm chart, you can, use the sidecarInjectorWebhook.injectedAnnotations value to inject the safe-to-evict annotation at the same time as the istio-sidecar.

Add this to your values.yaml:
sidecarInjectorWebhook:
  enabled: true
  injectedAnnotations:
    cluster-autoscaler.kubernetes.io/safe-to-evict: "true"

Do you know whats the equivalent on istio operator file when installing with istioctl I tried this but validation fails

i tried this config

meshConfig:
    sidecarInjectorWebhook:
      injectedAnnotations:
        cluster-autoscaler.kubernetes.io/safe-to-evict: "true"

but get error on upgrade (istio 1.6) 2020-12-30T02:56:28.351494Z info Error: failed to generate Istio configs from file [/tmp/qa-v1-istio-operator-values.yaml], error: validation errors (use --force to override): failed to unmarshall mesh config: unknown field "sidecarInjectorWebhook" in v1alpha1.MeshConfig

pc-rshetty commented 3 years ago

ok works when i specify under values

  values:
    sidecarInjectorWebhook:
      injectedAnnotations:
        cluster-autoscaler.kubernetes.io/safe-to-evict: "true"

ldemailly commented 3 years ago

Can this be the default?

ldemailly commented 3 years ago

John kindly pointed out what I missed; the annotation would evict any pod; while what we want is have the sidecar not prevent eviction but also not change the logic for the main/app container: if that one does have a volume, that should drive the eviction policy (not the sidecar')

Will file an issue with autoscaler and link here

ldemailly commented 3 years ago

closing this one as https://github.com/kubernetes/autoscaler/issues/3947 is really the only safe option outside of duplicating the logic

fauzan-n commented 3 years ago

is injectedAnnotations available in istio version 1.2.x ?

thesuperzapper commented 1 year ago

For those watching, it looks like cluster-autoscaler 1.27.0 implemented the cluster-autoscaler.kubernetes.io/safe-to-evict-local-volumes annotation, you can read about it in the FAQ.

It's a little clunky to use in its current form because we have to list all the local volumes by name, which means that depending on your istio setup (and possibly version) the following annotation may or may not work for you:

sidecarInjectorWebhook:
  enabled: true
  injectedAnnotations:
    cluster-autoscaler.kubernetes.io/safe-to-evict-local-volumes: "workload-socket,workload-certs,istio-envoy,istio-data"

Some things to note:

you should confirm this list of volumes is comprehensive by looking at what volumes your pods end up with after injection
setting this annotation with injectedAnnotations will make it impossible for specific apps on your cluster to set their own values, because any value they set would be replaced by the sidecar injector
if your pods have other local-storage volumes, they will not be evictable unless you have also included them in this list

If cluster-autoscaler eventually supports using a wildcard like "*", this might be an alternative that effectively does the same thing as setting the overall cluster-autoscaler config skip-nodes-with-local-storage to false.

Also, it looks like GKE (who was the main cause of these complaints due to not exposing their cluster-autoscaler configs, has made skip-nodes-with-local-storage false by default since GKE on 1.22.

istio-policy-bot commented 1 year ago

🚧 This issue or pull request has been closed due to not having had activity from an Istio team member since 2023-05-19. 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 / istio