Two changes needed:

• Can you please edit values.yaml.tmpl instead? Then, do a make -C install/kubernetes to propagate your changes
• The operator does need some tolerations, so that it can be scheduled in early-stage clusters. Can you please add, for a start, tolerations for node.cilium.io/agent-not-ready, {{.Values.agentNotReadyTaintKey}}, and node.kubernetes.io/not-ready

Thank you so much for your review!! I am working on it.

• The operator does need some tolerations, so that it can be scheduled in early-stage clusters. Can you please add, for a start, tolerations for node.cilium.io/agent-not-ready, {{.Values.agentNotReadyTaintKey}}, and node.kubernetes.io/not-ready

@squeed in my approach I think in /install/kubernetes/cilium/values.yaml.tmpl
adding tolerations for node.kubernetes.io/not-ready

  - key: node.kubernetes.io/not-ready
    effect: NoSchedule   
    operator: Exists

and for node.cilium.io/agent-not-ready

tolerations:
  - key: node.cilium.io/agent-not-ready
    effect: NoSchedule   
    operator: Exists     

and for agentNotReadyTaintKey: "node.cilium.io/agent-not-ready" I am not sure how to go about it as agentNotReadyTaintKey prevents any cillium operations to be performed on any tainted nodes and adding the above tolerations for node.cilium.io/agent-not-ready will give it some tolerations. Can you guide me on this issue. Thank you

@jignyasamishra basically, the problem is that the operator's set of tolerations is over-broad. So, we need to give it a decent set of default tolerations, but allow users to override those defaults.

Helm makes these sorts of changes possible, but it does take a bit of doing. So, you need to make two changes:

1: in values.yaml.tmpl: make the "default" value invalid

operator:
  #-- Tolerations for cilium-operator.
  # @default a reasonable set of tolerations to allow for running on the control plane
  tolerations: ~ 

Then, in cilium-operator/deployment.yaml:

tolerations:
{{- if (not (kindIs "invalid" .Values.operator.tolerations )) }}
  {{- toYaml . | trim | nindent 8 }}
{{- else }}
  - key: "node-role.kubernetes.io/control-plane"
    operator: Exists
  - key: "node-role.kubernetes.io/master" #deprecated
    operator: Exists
  - key: node.kubernetes.io/not-ready
    operator: Exists
  {{- with (coalesce .Values.agentNotReadyTaintKey "node.cilium.io/agent-not-ready" }}
  - key: {{ . }}
    operator: Exists
  {{- end }}

Something like that. Make sense?

@jignyasamishra basically, the problem is that the operator's set of tolerations is over-broad. So, we need to give it a decent set of default tolerations, but allow users to override those defaults.

Helm makes these sorts of changes possible, but it does take a bit of doing. So, you need to make two changes:

1: in values.yaml.tmpl: make the "default" value invalid

operator:
  #-- Tolerations for cilium-operator.
  # @default a reasonable set of tolerations to allow for running on the control plane
  tolerations: ~ 

Then, in cilium-operator/deployment.yaml:

tolerations:
{{- if (not (kindIs "invalid" .Values.operator.tolerations )) }}
  {{- toYaml . | trim | nindent 8 }}
{{- else }}
  - key: "node-role.kubernetes.io/control-plane"
    operator: Exists
  - key: "node-role.kubernetes.io/master" #deprecated
    operator: Exists
  - key: node.kubernetes.io/not-ready
    operator: Exists
  {{- with (coalesce .Values.agentNotReadyTaintKey "node.cilium.io/agent-not-ready" }}
  - key: {{ . }}
    operator: Exists
  {{- end }}

Something like that. Make sense?

@squeed why is this a bug that Cilium has to fix? The default tolerations are "- Exists". Users can still override those in case the feel appropriated. I'm only saying this because it's easier to know what tolerations are and that you need to modify them than not knowing what they are and not understanding why Cilium operator is not being scheduled to a particular node because of taints. This is the primarily reason why we only have "Exists" as the default toleration.

@aanm in this case, adding a reasonable set of tolerations can be an advantage, since the operator is a Deployment. In other words, if there is a maintenance issue on one of the control-plane nodes, it will be rescheduled. This is not relevant for DaemonSets since they shouldn't be de-scheduled anyways.

It really stinks to have the operator not running because the node has gone missing. The node will be tainted accordingly, but the operator never rescheduled.

Whatever we do, we should basically match the tolerations in #28947, since they are the same.

@aanm in this case, adding a reasonable set of tolerations can be an advantage, since the operator is a Deployment. In other words, if there is a maintenance issue on one of the control-plane nodes, it will be rescheduled. This is not relevant for DaemonSets since they shouldn't be de-scheduled anyways.

@squeed I understand your point but at the same time this does not happen on environments where you are trying out Cilium for the first time. My point is that if you know that your control-plane nodes are being maintained then you will also know how to change the tolerations of an existing deployment.

First-time users will not know this and if they are running on an environment that has some node taints that is not tolerated by Cilium Operator they will not be able to try out Cilium.

The helm chart should provide some sane defaults that allows users to run on 99% of their environments. If their environment is special then they should change the helm values accordingly.

It really stinks to have the operator not running because the node has gone missing. The node will be tainted accordingly, but the operator never rescheduled.

Whatever we do, we should basically match the tolerations in #28947, since they are the same.

The helm chart should provide some sane defaults that allows users to run on 99% of their environments. If their environment is special then they should change the helm values accordingly.

Agreed; that's why I propose it provide a reasonable set of tolerations. Tolerating all possible taints goes counter to general expectations, especially for something that isn't part of an individual node's networking.

In other words, people generally expect kubectl cordon to work, and I don't see any reason why we shouldn't support that by default.

This pull request has been automatically marked as stale because it
has not had recent activity. It will be closed if no further activity
occurs. Thank you for your contributions.

This pull request has not seen any activity since it was marked stale.
Closing.

@squeed can I pick this up again? I just ran into this because a node couldn't be drained because of the cilium-operator being rescheduled right away. I'll happily add a set of sane default tolerations.