add support to override not ready taint to be ignored by the CA #19247

thejosephstevens · 2022-03-29T04:43:03Z

Please ensure your pull request adheres to the following guidelines:

For first time contributors, read Submitting a pull request
All code is covered by unit and/or runtime tests where feasible.
All commits contain a well written commit description including a title,
description and a Fixes: #XXX line if the commit addresses a particular
GitHub issue.
All commits are signed off. See the section Developer’s Certificate of Origin
Provide a title or release-note blurb suitable for the release notes.
Thanks for contributing!

As described in this issue, if you scale a node pool down to zero, agent-not-ready taints can prevent the cluster autoscaler from scaling the node pool up from zero nodes because it thinks nothing will be able to run on the node (or at least the pending app pods won't be able to).

This PR adds the ability to pass a prefix that will be prepended to the agent-not-ready taint in order for it to be ignored by the cluster autoscaler when generating sanitized node templates in order to make scaling decisions. More specifically, this allows scaling node pools up from zero in GKE, where you can not customize the command line arguments of the cluster autoscaler, and where a multi-zone node pool is made up of multiple node pools under the hood, making it very easy to end up with a "node pool" with no nodes in it if you scale below 1 node per zone.

Fixes: #19241

Add config flag to add a prefix to AgentNotReadyNodeTaint value in order to enable the taint being ignored by cluster autoscaler.

sayboras

Small questions as per below, but LGTM in general, thanks.

sayboras · 2022-03-30T08:42:33Z

pkg/option/config.go

@@ -621,6 +622,14 @@ const (
 	// K8sNamespaceName is the name of the K8sNamespace option
 	K8sNamespaceName = "k8s-namespace"

+	// AgentNotReadyNodeTaintPrefixDefault hold the default value of
+	// AgentNotReadyNodeTaintPrefix
+	AgentNotReadyNodeTaintPrefixDefault = ""


is this default param used anywhere ?

I actually meant to use that since it didn't look like viper.GetString will return a default empty-string value. I'm a bit confused though as I'm trying to figure out the behaviors. For example, k8s-namespace doesn't appear to have a default value set anywhere except in bugtool. I'll start work on an edit to use that default if the config value isn't set, but if you happen to know if there's a standard way to set those defaults I'd welcome a pointer.

sayboras · 2022-03-30T08:53:13Z

pkg/option/config.go

@@ -2999,6 +3020,7 @@ func (c *DaemonConfig) Populate() {
 	c.HTTP403Message = viper.GetString(HTTP403Message)
 	c.DisableEnvoyVersionCheck = viper.GetBool(DisableEnvoyVersionCheck)
 	c.K8sNamespace = viper.GetString(K8sNamespaceName)
+	c.AgentNotReadyNodeTaintPrefix = viper.GetString(AgentNotReadyNodeTaintPrefixName)


as this flag is hidden, I reckon you need to use helm extraConfig param to set value, right ?

I think that's right from reading, but I'm not actually sure whether that's desirable behavior. I'm still working on a test of this on my own infra to verify, so I may make changes based on how that goes (was building images on top of the 1.11.2 tag to test with).

thanks, appreciate if you can share your testing later on.

aanm · 2022-04-11T12:10:02Z

let's wait until #19074 and one other PR is not open yet is merged first.

aanm · 2022-04-27T11:29:33Z

@thejosephstevens the PR #19074 was merged, can you rebase and squash this PR against master? Thank you

bmcustodio

Thanks for the PR @thejosephstevens! I have a few comments:

As it currently stands, users will have to resort to extraConfig in Helm to specify agent-not-ready-taint-prefix, because a corresponding Helm value is currently not being added. It would be great if you could add this (I suggest agentNotReadyTaintPrefix so it matched the name of the option).
Also, the choice being given to users seems to be between the old taint key (agent.cilium.io/not-ready) and <custom-prefix>/cilium-agent-not-ready. While this is fine and works, I wonder if we should instead give users full control over the taint key (i.e., insteadl of agent-not-ready-taint-prefix, we'd support something like agent-not-ready-taint-key). It seems to me the work is essentially done, so this shouldn't imply a big change.
There is no documentation being added/updated as part of the PR. I suggest adding the Helm value and document it there, as that should be picked up by helm-docs, and at least making a mention to the new property and the reason for introducing it in this page.
I also believe that we should update CI to exercise the new code path. This should amount to updating this line (and the corresponding ones in the -1.10 and -1.11 files), and setting the new option somewhere around here.

thejosephstevens · 2022-04-27T18:51:36Z

Thanks for the PR @thejosephstevens! I have a few comments:

As it currently stands, users will have to resort to extraConfig in Helm to specify agent-not-ready-taint-prefix, because a corresponding Helm value is currently not being added. It would be great if you could add this (I suggest agentNotReadyTaintPrefix so it matched the name of the option).

Also, the choice being given to users seems to be between the old taint key (agent.cilium.io/not-ready) and <custom-prefix>/cilium-agent-not-ready. While this is fine and works, I wonder if we should instead give users full control over the taint key (i.e., insteadl of agent-not-ready-taint-prefix, we'd support something like agent-not-ready-taint-key). It seems to me the work is essentially done, so this shouldn't imply a big change.

There is no documentation being added/updated as part of the PR. I suggest adding the Helm value and document it there, as that should be picked up by helm-docs, and at least making a mention to the new property and the reason for introducing it in this page.

I also believe that we should update CI to exercise the new code path. This should amount to updating this line (and the corresponding ones in the -1.10 and -1.11 files), and setting the new option somewhere around here.

Thanks for the review @bmcustodio ! We're working through the suggestions (they look pretty straightforward - I expect we'll have something ready later today). Just a note, @XiaoxiHuang21 is taking over lead on this PR. Feel free to tag me, but you'll likely get quicker responses tagging her directly.

sayboras · 2022-06-02T04:02:21Z

I have forced pushed your commit here after resolving conflicts with master, as well as minor formatting changes to make CI happy. Thanks for your support.

sayboras · 2022-06-02T04:02:37Z

/test

Job 'Cilium-PR-K8s-1.22-kernel-5.4' failed:

Click to show.

Test Name

K8sUpdates Tests upgrade and downgrade from a Cilium stable image to master

Failure Output

FAIL: Unable to download helm chart v1.11 from GitHub

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.22-kernel-5.4 so I can create one.

Job 'Cilium-PR-K8s-GKE' failed:

Click to show.

Test Name

K8sUpdates Tests upgrade and downgrade from a Cilium stable image to master

Failure Output

FAIL: Timed out after 33.828s.

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-GKE so I can create one.

Job 'Cilium-PR-K8s-1.16-kernel-4.9' failed:

Click to show.

Test Name

K8sUpdates Tests upgrade and downgrade from a Cilium stable image to master

Failure Output

FAIL: Timed out after 30.001s.

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.16-kernel-4.9 so I can create one.

sayboras · 2022-06-02T09:43:06Z

/test

Job 'Cilium-PR-K8s-1.22-kernel-5.4' failed:

Click to show.

Test Name

K8sUpdates Tests upgrade and downgrade from a Cilium stable image to master

Failure Output

FAIL: Timed out after 30.001s.

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.22-kernel-5.4 so I can create one.

Job 'Cilium-PR-K8s-GKE' failed:

Click to show.

Test Name

K8sUpdates Tests upgrade and downgrade from a Cilium stable image to master

Failure Output

FAIL: Timed out after 30.000s.

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-GKE so I can create one.

sayboras · 2022-06-06T08:37:46Z

/test

Job 'Cilium-PR-K8s-1.22-kernel-5.4' failed:

Click to show.

Test Name

K8sUpdates Tests upgrade and downgrade from a Cilium stable image to master

Failure Output

FAIL: Timed out after 30.057s.

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.22-kernel-5.4 so I can create one.

If you scale a node pool down to zero nodes the Cluster Autoscaler won't add new nodes because pending pods don't tolerate the cilium agent-not-ready taint. If you're managing your own CA you can ignore the cilium taint with a command line flag on the CA. However if you're running on managed k8s (e.g. GKE), you may not have access to configure the CA, and will need to use a custom taint prefix to be ignored by the autoscaler when scaling up from zero nodes in a pool. This commit adds config to add a prefix to the agent-not-ready taint so it can be ignored by the cluster-autoscaler. Fixes: cilium#19241 Co-authored-by: Xiaoxi Huang <Xiaoxi@ascend.io> Signed-off-by: Joseph Stevens <thejosephstevens@gmail.com>

sayboras · 2022-06-06T10:31:19Z

Forced pushed after rebase with master and fixed upgrade tests.

sayboras · 2022-06-06T10:36:30Z

/test

sayboras · 2022-06-06T12:21:01Z

/test-runtime

Job 'Cilium-PR-K8s-1.23-kernel-net-next' failed:

Click to show.

Test Name

K8sEgressGatewayTest tunnel vxlan with endpointRoutes enabled no egress gw policy connectivity works

Failure Output

FAIL: Expected command: kubectl exec -n kube-system log-gatherer-68qd2 -- curl --path-as-is -s -D /dev/stderr --fail --connect-timeout 5 --max-time 20 http://192.168.56.11:20080 -w "time-> DNS: '%{time_namelookup}(%{remote_ip})', Connect: '%{time_connect}',Transfer '%{time_starttransfer}', total '%{time_total}'"

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.23-kernel-net-next so I can create one.

sayboras · 2022-06-06T13:20:49Z

/test-1.23-net-next

sayboras · 2022-06-06T23:49:28Z

reviews are in, required CI tests are passed. I am marking this ready to merge, please let me know if you think otherwise.

thejosephstevens · 2022-06-07T22:27:03Z

Thanks so much for taking this across the line @sayboras !!!

maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Mar 29, 2022

thejosephstevens marked this pull request as ready for review March 29, 2022 21:22

thejosephstevens requested a review from a team March 29, 2022 21:22

thejosephstevens requested a review from a team as a code owner March 29, 2022 21:22

thejosephstevens requested review from a team, jibi, michi-covalent and sayboras March 29, 2022 21:22

thejosephstevens force-pushed the support-ca-ignore-taint-prefix branch from a3de006 to 4a02e27 Compare March 30, 2022 00:33

sayboras added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Mar 30, 2022

maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Mar 30, 2022

sayboras changed the title ~~add support to override agent not ready taint to be ignored by the cl…~~ add support to override not ready taint to be ignored by the CA Mar 30, 2022

sayboras approved these changes Mar 30, 2022

View reviewed changes

sayboras added the area/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers. label Mar 30, 2022

aanm self-assigned this Apr 11, 2022

aanm added the dont-merge/waiting-for-upstream Only merge once upstream library or kernel features have landed label Apr 11, 2022

This comment was marked as resolved.

Sign in to view

maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Apr 26, 2022

thejosephstevens requested a review from a team as a code owner April 27, 2022 00:02

thejosephstevens requested a review from nebril April 27, 2022 00:02

This comment was marked as resolved.

Sign in to view

sayboras requested a review from bmcustodio April 27, 2022 02:02

aanm removed the dont-merge/waiting-for-upstream Only merge once upstream library or kernel features have landed label Apr 27, 2022

bmcustodio requested changes Apr 27, 2022

View reviewed changes

thejosephstevens requested review from a team as code owners April 27, 2022 21:52

maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Jun 1, 2022

XiaoxiHuang21 force-pushed the support-ca-ignore-taint-prefix branch from d4213cd to ee59e5f Compare June 1, 2022 18:59

maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Jun 1, 2022

sayboras force-pushed the support-ca-ignore-taint-prefix branch 2 times, most recently from 682ed53 to f53e1c1 Compare June 2, 2022 03:50

sayboras requested a review from christarazi June 2, 2022 04:18

sayboras force-pushed the support-ca-ignore-taint-prefix branch from f53e1c1 to df893fe Compare June 2, 2022 09:37

sayboras force-pushed the support-ca-ignore-taint-prefix branch from df893fe to 162316c Compare June 6, 2022 08:21

sayboras force-pushed the support-ca-ignore-taint-prefix branch from 162316c to c2a455a Compare June 6, 2022 10:28

jibi removed their request for review June 6, 2022 13:27

sayboras added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jun 6, 2022

joamaki merged commit c1c9716 into cilium:master Jun 7, 2022

bmcustodio added needs-backport/1.11 labels Jun 10, 2022

joestringer mentioned this pull request Jun 13, 2022

[v1.11] add support to override not ready taint to be ignored by the CA #20183

Merged

joestringer added backport-done/1.11 The backport for Cilium 1.11.x for this PR is done. and removed needs-backport/1.11 labels Jun 13, 2022

joestringer mentioned this pull request Jun 13, 2022

Prepare for release v1.11.6 #20190

Merged

aanm mentioned this pull request Jun 22, 2022

Prepare for release v1.12.0-rc3 #20279

Merged

add support to override not ready taint to be ignored by the CA #19247

add support to override not ready taint to be ignored by the CA #19247

Uh oh!

Conversation

thejosephstevens commented Mar 29, 2022 • edited by sayboras Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sayboras left a comment

Choose a reason for hiding this comment

Uh oh!

sayboras Mar 30, 2022

Choose a reason for hiding this comment

Uh oh!

thejosephstevens Mar 30, 2022

Choose a reason for hiding this comment

Uh oh!

sayboras Mar 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thejosephstevens Mar 30, 2022

Choose a reason for hiding this comment

Uh oh!

sayboras Mar 31, 2022

Choose a reason for hiding this comment

Uh oh!

aanm commented Apr 11, 2022

Uh oh!

This comment was marked as resolved.

This comment was marked as resolved.

aanm commented Apr 27, 2022

Uh oh!

bmcustodio left a comment

Choose a reason for hiding this comment

Uh oh!

thejosephstevens commented Apr 27, 2022

Uh oh!

sayboras commented Jun 2, 2022

Uh oh!

sayboras commented Jun 2, 2022 • edited by maintainer-s-little-helper bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Name

Failure Output

Test Name

Failure Output

Test Name

Failure Output

Uh oh!

sayboras commented Jun 2, 2022 • edited by maintainer-s-little-helper bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Name

Failure Output

Test Name

Failure Output

Uh oh!

sayboras commented Jun 6, 2022 • edited by maintainer-s-little-helper bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Name

Failure Output

Uh oh!

sayboras commented Jun 6, 2022

Uh oh!

sayboras commented Jun 6, 2022

Uh oh!

sayboras commented Jun 6, 2022 • edited by maintainer-s-little-helper bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Name

Failure Output

Uh oh!

sayboras commented Jun 6, 2022

Uh oh!

sayboras commented Jun 6, 2022

Uh oh!

thejosephstevens commented Jun 7, 2022

Uh oh!

Uh oh!

thejosephstevens commented Mar 29, 2022 •

edited by sayboras

Loading

sayboras Mar 30, 2022 •

edited

Loading

sayboras commented Jun 2, 2022 •

edited by maintainer-s-little-helper bot

Loading

sayboras commented Jun 2, 2022 •

edited by maintainer-s-little-helper bot

Loading

sayboras commented Jun 6, 2022 •

edited by maintainer-s-little-helper bot

Loading

sayboras commented Jun 6, 2022 •

edited by maintainer-s-little-helper bot

Loading