Use correct tolerations value when deploying cilium-operator and cilium-etcd-operator via helm

[michaelpetrov]

Use correct tolerations value when deploying cilium-operator via helm. Without this the global tolerations value is used and the operator runs on all nodes including ones with taints.

Use correct tolerations value when deploying cilium-operator via helm.

Signed-off-by: Michael Petrov petrov.michael@gmail.com

[kaworu]

Hi @michaelpetrov and thanks for the PR. Patch LGTM.

Without this the global tolerations value is used and the operator runs on all nodes including ones with taints.

Can you elaborate on this because I see no difference between the global tolerations and the operator's ones by default:

$ yq '.tolerations,.operator.tolerations' install/kubernetes/cilium/values.yaml
[
  {
    "operator": "Exists"
  }
]
[
  {
    "operator": "Exists"
  }
]

[michaelpetrov]

@kaworu on our clusters we have cpu and gpu nodes in two node pools, and the gpu nodes have a taint to avoid scheduling non-essential work on them (because they are far more expensive nodes). cilium-operator gets consistently scheduled onto the gpu nodes in our case and there is no way to override it in Helm. If we override the default global tolerations to not use the gpu nodes then we also lose the cilium agent on them since the setting is shared between the two components.

To be honest, I think the top level .Values.tolerations is a pretty bad idea in a multi-service chart where you want just the main agent and node-init everywhere and auxiliary services only on some nodes. Ideally it can be removed in favor of something closer to .Values.agent.tolerations but that would conflict with the way agent settings are currently done.

[maintainer-s-little-helper]

Commit b3747d1ec0b048d7346d86e8ef9fafb1102f67c8 does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

[kaworu]

@michaelpetrov thanks for the insight, all you wrote make perfect sense.

In our current Helm charts we have many top-level items that are about the agent. For example, .Values.image, .Values.affinity, .Values.tolerations (as you correctly pointed out), etc.

[nathanjsweet]

Good catch. Thanks for the PR!

[michaelpetrov]

In our current Helm charts we have many top-level items that are about the agent. For example, .Values.image, .Values.affinity, .Values.tolerations (as you correctly pointed out), etc.

I'm actually even slightly worried about this diff because some existing chart installations might depend on .Values.tolerations to schedule things onto nodes that has various taints - so this change is not strictly backwards compatible. For our use case at OpenAI I do really want this separation though. Not sure what's the best way to improve things here.

[kaworu]

I'm actually even slightly worried about this diff because some existing chart installations might depend on .Values.tolerations to schedule things onto nodes that has various taints - so this change is not strictly backwards compatible. For our use case at OpenAI I do really want this separation though. Not sure what's the best way to improve things here.

@michaelpetrov that is a valid concern and why the release note is minor, but granted that might not be enough safety net. I'll defer to @seanmwinn about the potential breakage.

To keep some level of backward compatibility, could we could use Helm's default function? e.g.

 {{- with (.Values.etcd.tolerations | default .Values.tolerations) }}

[pchaigno]

test-me-please

[pchaigno]

@michaelpetrov Could you please rebase your PR on latest master? We've fixed a couple of flakes in the past few days and CI should have a better time after rebasing.

[aanm]

test-me-please

[aanm]

Failures are unrelated, merging