util: tools to drain and uncordon cluster by Arvindthiru · Pull Request #1060 · Azure/fleet

Arvindthiru · 2025-02-28T01:20:19Z

Description of your changes

Adding new tools to drain and uncordon cluster

Fixes #

I have:

Run make reviewable to ensure this PR is ready for review.

How has this code been tested

Special notes for your reviewer

If drain fails we automatically uncordon cluster

ryanzhang-oss · 2025-03-13T19:53:42Z

+		return nil, err
+	}
+
+	hubClient, err := client.New(restConfig, client.Options{Scheme: scheme})


does this client use cache?

Currently it doesn't use cache https://github.com/kubernetes-sigs/controller-runtime/blob/main/pkg/client/client.go#L82-L89

michaelawyu · 2025-03-18T18:16:14Z

Hi Arvind! Below are some of the points I've been thinking about since our earlier discussion; hopefully they can be of use/relevance to you:

michaelawyu · 2025-03-18T18:39:17Z

Hi Arvind! Below are some of the points I've been thinking about since our earlier discussion; hopefully they can be of use/relevance to you:

a) The kubectl drain command functions similar to what we have in this PR right now, which first marks a node as unschedulable (cordoned) then attempts to evict all pods (if applicable -> certain types of pods will be exempted). The tool is not fully consistent; there still leaves a (albeit quite short) time window when a pod can be assigned to a node during the draining process -> this can happen if the draining runs when the scheduler is about to finish scheduling a pod. In this regard, however, K8s has an advantage over us: in K8s we have first the pods, then the scheduling decisions (whereas in Fleet, bindings are provisioned after the scheduling is completed); so if strong consistency is desired (i.e., must drain everything applicable), kubectl can just watch for unassigned pods in addition to assigned pods, and wait until scheduling is complete on these unassigned pods (successful or not, either way's OK).

michaelawyu · 2025-03-18T19:04:41Z

b) as discussed earlier in our sync, if we would like to implement stronger consistency in the draining tool, options would include

use the NoExecute semantics -> this is not yet implemented
set up the rollout controller to reject bindings associated with a unschedulable cluster -> the concerns are mostly that *) this incurs coupling/logic leaking in our code + *) since we have multiple rollout controller options now it would not feel right to make this case a part of our rollout contract
set up the scheduler to be able to ACK cordoning/draining attempts, so that we know for sure no bindings would be created any more -> this option also incurs concerns regarding coupling/logic leaking

There might not be a very easily approachable solution here 😞

michaelawyu · 2025-03-18T19:06:42Z

c) With that being said, it might make more sense for us to evaluate how important the stronger consistency is to our users, esp. considering that kubectl does not give the same guarantee either (though in theory it can implement this much more easily).

michaelawyu · 2025-03-18T19:14:08Z

d) A side note is that, kubectl drain/cordon marks a cluster as unschedulable by setting its node spec (there is a schedulable boolean value) -> it does not use taints (consequently no toleration can be set up). Of course this might not necessarily apply to our case

kaito-pr-agent · 2025-04-04T04:50:09Z

Persistent review updated to latest commit 7fa0c3f

kaito-pr-agent · 2025-04-04T05:21:22Z

Persistent review updated to latest commit 729c015

kaito-pr-agent · 2025-04-04T05:26:54Z

Persistent review updated to latest commit e657966

kaito-pr-agent · 2025-04-04T05:34:28Z

Persistent review updated to latest commit d7eebb8

michaelawyu

Added a number of comments, PTAL

kaito-pr-agent · 2025-04-07T19:15:43Z

Persistent review updated to latest commit fc6ae91

kaito-pr-agent · 2025-04-07T19:37:36Z

Persistent review updated to latest commit 7c34d23

kaito-pr-agent · 2025-04-09T00:19:27Z

Persistent review updated to latest commit c715acf

kaito-pr-agent · 2025-04-09T00:51:53Z

Persistent review updated to latest commit e4717e6

kaito-pr-agent · 2025-04-09T04:52:20Z

Persistent review updated to latest commit eb20eee

kaito-pr-agent · 2025-04-09T04:57:35Z

Persistent review updated to latest commit a369776

kaito-pr-agent · 2025-04-09T05:03:05Z

Persistent review updated to latest commit f00d3ad

michaelawyu

Hi Arvind! Apologies, I just need a little bit more time to review the code for one more time. Will add approval or more comments later if you do not mind.

michaelawyu · 2025-04-09T19:29:45Z

+			}
+		}
+		executedCondition := eviction.GetCondition(string(placementv1beta1.PlacementEvictionConditionTypeExecuted))
+		if executedCondition == nil || executedCondition.Status == metav1.ConditionFalse {


Hi Arvind! It's not a blocker per se but I still believe that we should reply on API semantics for validations in this regard.

michaelawyu · 2025-04-09T19:30:35Z

+			}
+		}
+		executedCondition := eviction.GetCondition(string(placementv1beta1.PlacementEvictionConditionTypeExecuted))
+		if executedCondition == nil || executedCondition.Status == metav1.ConditionFalse {


Plus, even with the function you mentioned, unknown Valid condition can still pass the check, so are unknown Executed conditions.

michaelawyu

Added a few other nits; otherwise LGTM.

kaito-pr-agent · 2025-04-10T00:42:07Z

Persistent review updated to latest commit 1885458

michaelawyu

LGTM ;)

ryanzhang-oss reviewed Feb 28, 2025

View reviewed changes

Arvindthiru marked this pull request as ready for review March 4, 2025 21:08

ryanzhang-oss reviewed Mar 4, 2025

View reviewed changes

Comment thread tools/README.md Outdated

Comment thread tools/cordon-cluster/main/main.go Outdated

ryanzhang-oss reviewed Mar 5, 2025

View reviewed changes

Arvindthiru marked this pull request as draft March 7, 2025 22:29

Arvindthiru changed the title ~~util: simple tool to cordon cluster~~ util: tools to drain and uncordon cluster Mar 11, 2025

Arvindthiru marked this pull request as ready for review March 12, 2025 00:29

Arvindthiru force-pushed the cordon-tool branch from b46d854 to 221a497 Compare March 12, 2025 02:25

Arvindthiru commented Mar 12, 2025

View reviewed changes

Comment thread tools/draincluster/main.go Outdated

ryanzhang-oss reviewed Mar 13, 2025

View reviewed changes

Arvind Thirumurugan added 15 commits March 31, 2025 14:03

simple tool to cordon cluster

bf57014

minor changes

62c1328

add retry with backoff

4c2f90d

list CRPs instead of CRBs

62607c0

wait for evictions to reach terminal state

8b8ec57

minor fix

5279951

minor change

de42202

add uncordon file, readme

8d7e4da

minor changes

3a0d293

minor fix

6f047a0

refactor

cb575db

minor fix

41145c0

add drain retry

88ba286

minor fix

dd0a3e2

restructure files

6bda9cc

Arvind Thirumurugan added 2 commits March 31, 2025 18:03

doc update

6470834

refactor code

7fa0c3f

minor changes

729c015

minor changes

e657966

change struct names

d7eebb8

michaelawyu reviewed Apr 4, 2025

View reviewed changes

address comments

7c34d23

Arvindthiru force-pushed the cordon-tool branch from fc6ae91 to 7c34d23 Compare April 7, 2025 19:36

address comments

c715acf

make reviewable

e4717e6

minor fixes

eb20eee

minor change

a369776

minor change

f00d3ad

michaelawyu reviewed Apr 9, 2025

View reviewed changes

Comment thread tools/draincluster/drain/drain.go Outdated

Comment thread tools/draincluster/drain/drain.go Outdated

Comment thread tools/draincluster/main.go

address comments

1885458

michaelawyu approved these changes Apr 10, 2025

View reviewed changes

Arvindthiru merged commit d246699 into Azure:main Apr 10, 2025
17 checks passed

Conversation

Arvindthiru commented Feb 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of your changes

How has this code been tested

Special notes for your reviewer

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ryanzhang-oss Mar 13, 2025

Choose a reason for hiding this comment

Uh oh!

Arvindthiru Mar 28, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

michaelawyu commented Mar 18, 2025

Uh oh!

michaelawyu commented Mar 18, 2025

Uh oh!

michaelawyu commented Mar 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michaelawyu commented Mar 18, 2025

Uh oh!

michaelawyu commented Mar 18, 2025

Uh oh!

kaito-pr-agent Bot commented Apr 4, 2025

Uh oh!

kaito-pr-agent Bot commented Apr 4, 2025

Uh oh!

kaito-pr-agent Bot commented Apr 4, 2025

Uh oh!

kaito-pr-agent Bot commented Apr 4, 2025

Uh oh!

michaelawyu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kaito-pr-agent Bot commented Apr 7, 2025

Uh oh!

kaito-pr-agent Bot commented Apr 7, 2025

Uh oh!

kaito-pr-agent Bot commented Apr 9, 2025

Uh oh!

kaito-pr-agent Bot commented Apr 9, 2025

Uh oh!

kaito-pr-agent Bot commented Apr 9, 2025

Arvindthiru commented Feb 28, 2025 •

edited

Loading

michaelawyu commented Mar 18, 2025 •

edited

Loading

michaelawyu left a comment •

edited

Loading