Add `label-with-exclude-from-external-lbs` CLI argument to enable graceful removal/addition from external load balancers #419

amorey · 2021-08-04T07:31:55Z

Previously, kured issued the system reboot command without first removing nodes from any connected external load balancers (ELBs).

This behavior caused downtime on restart because ELBs send traffic to kube-proxy pods running on nodes until the ELB health checks fail or the node is de-registered explicitly.

This patch solves the problem by adding a command line argument (label-with-exclude-from-external-lbs) that, when enabled, adds a "node.kubernetes.io/exclude-from-external-load-balancers" label to nodes undergoing kured reboot. This label tells the Kubernetes control plane to de-register the affected node from any connected ELBs. The node label is removed after restart which causes the control plane to re-register the node with the ELBs.

Close #358

kingdonb · 2021-08-06T19:58:19Z

I released the e2e checks that were waiting for approval. Thanks for re-submitting this!

evrardjp · 2021-08-25T15:11:28Z

I will review asap.

evrardjp

Little detail: I think this should be opt-in.

cmd/kured/main.go

amorey · 2021-08-25T16:01:09Z

Little detail: I think this should be opt-in.

What would you suggest calling the command line argument?

evrardjp · 2021-08-25T16:22:46Z

Little detail: I think this should be opt-in.

What would you suggest calling the command line argument?

I have no strong opinion :)
Maybe "--label-with-exclude-from-external-lb" ?

evrardjp · 2021-08-25T16:35:40Z

I think it would be wise to clarify this (future) CLI flag's purpose in our documentation + include it (commented out) in our manifest.

amorey · 2021-08-25T20:43:44Z

I rebased with upstream/main, added a label-with-exclude-from-external-lbs CLI option and updated the documentation, manifests and helm charts. I didn't bump the helm chart version number because I bumped it in #430 and didn't want to cause a conflict. I also updated the title and description of this PR to reflect the current commit message.

evrardjp · 2021-08-26T16:48:50Z

I should have been clearer and mention no change in the helm chart, as the helm chart is trailing the release. Sorry for that. The manifest and docs are to be included in the release however.

amorey · 2021-08-27T08:08:52Z

I should have been clearer and mention no change in the helm chart, as the helm chart is trailing the release. Sorry for that. The manifest and docs are to be included in the release however.

Ok, fixed.

evrardjp · 2021-08-27T10:51:29Z

cmd/kured/main.go

@@ -492,6 +552,9 @@ func rebootAsRequired(nodeID string, rebootCommand []string, sentinelCommand []s
 		log.Fatal(err)
 	}

+	// Remove ExcludeFromELBs label immediately to allow ELB registration
+	disableExcludeFromELBs(client, nodeID)


Is there a reason you only guard the enable, not the disable? I would expect no change to the labels by default if the option is disabled. (hence, adding a guard here too).

I was thinking about the case of a user who disables the option during a reboot after the label has been added. In order to avoid leaving the rebooted node off of the external load balancers when the node is rebooted, disable() has to run on startup. On the other hand, there's no harm in running disable() when there's no label present since it won't do anything.

It does an api call, so it doesn't do nothing ;)

However, it made me realise that an error in disableExcludeFromELBs (e.g. one that the API can throw) is not used in any way... Why do we return the error if it's to do nothing with it? How should we behave if the API call fails?

Currently the returned error is being used in the unit tests.

In terms of what to do if the API call fails, I think it makes sense to make API failures in enableExcludeFromELBs() fatal since success is a must for users who have enabled --label-with-exclude-from-external-lbs.

On the other hand, I don't think API errors in disableExcludeFromELBs() necessarily need to be fatal but it looks like similar operations such as annotation deletion failures are treated fatally elsewhere in the code.

Should we make the API failures fatal in enableExcludeFromELBs() and disableExcludeFromELBs()?

What would you like to do here? Do you want to guard the disable and make API errors fatal?

evrardjp · 2021-08-27T10:52:13Z

Looks good overall, just a small final nit.

amorey · 2021-09-03T10:40:53Z

Are there any changes that need to be made to the PR?

evrardjp

I think I would like to discuss the guard and the last bit of the code.

evrardjp · 2021-09-27T16:34:21Z

cmd/kured/main.go

@@ -492,6 +552,9 @@ func rebootAsRequired(nodeID string, rebootCommand []string, sentinelCommand []s
 		log.Fatal(err)
 	}

+	// Remove ExcludeFromELBs label immediately to allow ELB registration
+	disableExcludeFromELBs(client, nodeID)


It does an api call, so it doesn't do nothing ;)

However, it made me realise that an error in disableExcludeFromELBs (e.g. one that the API can throw) is not used in any way... Why do we return the error if it's to do nothing with it? How should we behave if the API call fails?

evrardjp · 2021-11-24T15:44:20Z

I don't want to be a blocker for progress. If we feel it's a good feature, then let's move ahead... please let's just solve git conflicts and human ones ;)

amorey · 2021-11-24T18:49:17Z

I don't want to be a blocker for progress. If we feel it's a good feature, then let's move ahead... please let's just solve git conflicts and human ones ;)

Sounds good to me! I rebased with main, fixed the conflicts and am waiting for feedback on the changes you requested above.

…ceful removal/addition from external load balancers Previously, kured issued the system reboot command without first removing nodes from any connected external load balancers (ELBs). This behavior caused downtime on restart because ELBs send traffic to kube-proxy pods running on nodes until the ELB health checks fail or the node is de-registered explicitly. This patch solves the problem by adding a command line argument (`label-with-exclude-from-external-lbs`) that, when enabled, adds a "node.kubernetes.io/exclude-from-external-load-balancers" label to nodes undergoing kured reboot. This label tells the Kubernetes control plane to de-register the affected node from any connected ELBs. The node label is removed after restart which causes the control plane to re-register the node with the ELBs. Close kubereboot#358

amorey · 2021-12-02T08:29:11Z

#419 (review)

I fenced in disableExcludeFromELBs() and modified the code to fail if the API returns an error. Is there anything left to do before this can be merged?

jackfrancis · 2021-12-06T21:56:37Z

cmd/kured/main.go

@@ -493,6 +553,12 @@ func rebootAsRequired(nodeID string, rebootCommand []string, sentinelCommand []s
 		log.Fatal(err)
 	}

+	if labelWithELBX {


Don't we want to move this remove node label logic down into the the part where we know we're holding the lock? Similar to where we do if annotateNodes && !rebootRequired and then delete node annotations?

The disableExcludeFromELBs() method can be run safely with or without the lock. Are there any particular cases you're thinking of where you would want the method to run only if the lock is present?

I'm just thinking that if we do this when we're not holding the lock, that means we're doing it in every iteration. And that means that we're calling the apiserver for the node object every time. On a cluster with hundreds of thousands of nodes, with a small kured interval (e.g., 1 min), that means we're going to potentially be sending many apiserver requests every second.

If I understand correctly, the only reason we want to disable this label is to remove it after a successful reboot. In every scenario that fits that description, we should have the lock (because we only ever release the lock inside that block).

Does that make sense?

cc @dholbach @ckotzbauer for thoughts as well

I agree with @jackfrancis, that we should avoid requesting the apiserver to excessive. The logic should be safe when only called with a lock being present.

github-actions · 2022-02-07T01:46:53Z

This PR was automatically considered stale due to lack of activity. Please refresh it and/or join our slack channels to highlight it, before it automatically closes (in 7 days).

github-actions · 2022-04-09T02:00:50Z

This PR was automatically considered stale due to lack of activity. Please refresh it and/or join our slack channels to highlight it, before it automatically closes (in 7 days).

amorey · 2022-04-09T05:53:27Z

/remove no-pr-activity

github-actions · 2022-06-09T02:11:22Z

This PR was automatically considered stale due to lack of activity. Please refresh it and/or join our slack channels to highlight it, before it automatically closes (in 7 days).

amorey mentioned this pull request Aug 4, 2021

Add support for excluding/including nodes from external load balancers before/after reboot #378

Closed

evrardjp requested changes Aug 25, 2021

View reviewed changes

cmd/kured/main.go Outdated Show resolved Hide resolved

cmd/kured/main.go Outdated Show resolved Hide resolved

amorey force-pushed the exclude-from-elbs branch 2 times, most recently from d57c317 to fff8cdf Compare August 25, 2021 20:37

amorey changed the title ~~Add support for graceful removal/addition from external load balancers~~ Add label-with-exclude-from-external-lbs CLI argument to enable graceful removal/addition from external load balancers Aug 25, 2021

amorey mentioned this pull request Aug 26, 2021

Add reboot-delay CLI argument to docs, helm charts and manifests #430

Merged

amorey force-pushed the exclude-from-elbs branch from fff8cdf to a9cdeb4 Compare August 26, 2021 15:37

amorey force-pushed the exclude-from-elbs branch from a9cdeb4 to e5272c0 Compare August 27, 2021 08:08

evrardjp requested changes Aug 27, 2021

View reviewed changes

ckotzbauer approved these changes Sep 15, 2021

View reviewed changes

evrardjp reviewed Sep 27, 2021

View reviewed changes

amorey force-pushed the exclude-from-elbs branch from e5272c0 to d49d805 Compare November 24, 2021 18:45

amorey force-pushed the exclude-from-elbs branch from d49d805 to 14c898c Compare December 2, 2021 08:26

jackfrancis reviewed Dec 6, 2021

View reviewed changes

github-actions bot added the no-pr-activity label Feb 7, 2022

ckotzbauer added keep This won't be closed by the stale bot. and removed no-pr-activity labels Feb 7, 2022

github-actions bot added the no-pr-activity label Apr 9, 2022

ckotzbauer removed the no-pr-activity label Apr 9, 2022

github-actions bot added the no-pr-activity label Jun 9, 2022

github-actions bot closed this Jun 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `label-with-exclude-from-external-lbs` CLI argument to enable graceful removal/addition from external load balancers #419

Add `label-with-exclude-from-external-lbs` CLI argument to enable graceful removal/addition from external load balancers #419

amorey commented Aug 4, 2021 •

edited

kingdonb commented Aug 6, 2021

evrardjp commented Aug 25, 2021

evrardjp left a comment

amorey commented Aug 25, 2021

evrardjp commented Aug 25, 2021

evrardjp commented Aug 25, 2021 •

edited

amorey commented Aug 25, 2021 •

edited

evrardjp commented Aug 26, 2021

amorey commented Aug 27, 2021

evrardjp Aug 27, 2021

amorey Aug 27, 2021

evrardjp Sep 27, 2021

amorey Sep 28, 2021 •

edited

amorey Nov 24, 2021

evrardjp commented Aug 27, 2021

amorey commented Sep 3, 2021

evrardjp left a comment

evrardjp Sep 27, 2021

evrardjp commented Nov 24, 2021

amorey commented Nov 24, 2021

amorey commented Dec 2, 2021

jackfrancis Dec 6, 2021

amorey Dec 7, 2021

jackfrancis Dec 7, 2021

ckotzbauer Dec 8, 2021

github-actions bot commented Feb 7, 2022

github-actions bot commented Apr 9, 2022

amorey commented Apr 9, 2022

github-actions bot commented Jun 9, 2022

Add label-with-exclude-from-external-lbs CLI argument to enable graceful removal/addition from external load balancers #419

Add label-with-exclude-from-external-lbs CLI argument to enable graceful removal/addition from external load balancers #419

Conversation

amorey commented Aug 4, 2021 • edited

kingdonb commented Aug 6, 2021

evrardjp commented Aug 25, 2021

evrardjp left a comment

Choose a reason for hiding this comment

amorey commented Aug 25, 2021

evrardjp commented Aug 25, 2021

evrardjp commented Aug 25, 2021 • edited

amorey commented Aug 25, 2021 • edited

evrardjp commented Aug 26, 2021

amorey commented Aug 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amorey Sep 28, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evrardjp commented Aug 27, 2021

amorey commented Sep 3, 2021

evrardjp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evrardjp commented Nov 24, 2021

amorey commented Nov 24, 2021

amorey commented Dec 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 7, 2022

github-actions bot commented Apr 9, 2022

amorey commented Apr 9, 2022

github-actions bot commented Jun 9, 2022

Add `label-with-exclude-from-external-lbs` CLI argument to enable graceful removal/addition from external load balancers #419

Add `label-with-exclude-from-external-lbs` CLI argument to enable graceful removal/addition from external load balancers #419

amorey commented Aug 4, 2021 •

edited

evrardjp commented Aug 25, 2021 •

edited

amorey commented Aug 25, 2021 •

edited

amorey Sep 28, 2021 •

edited