add NoExecute taint manager #1945

Garrybest · 2022-06-01T13:48:02Z

Signed-off-by: Garrybest garrybest@foxmail.com

What type of PR is this?
/kind feature

What this PR does / why we need it:
Add taint manager for NoExecute manager.

Which issue(s) this PR fixes:
Fixes part of #1762

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

`karmada-controller-manager`: Introduced `taint manager` to cluster controller to evict workloads from clusters with `no-execute` taint.

RainbowMango · 2022-06-02T02:11:12Z

/assign
But I'm afraid don't have enough time to look at it.

Garrybest · 2022-06-02T02:32:07Z

Thanks @RainbowMango. The taint manager in k/k is really complicated. I have tried my best to keep our taint manager concise. Let me invite more reviewers to check.

/cc @lynnsong @XiShanYongYe-Chang @huone1

dddddai

Cool!

pkg/controllers/cluster/taint_manager.go

RainbowMango

It's really enjoyable to look at code like this.

Thanks @Garrybest and sorry for let this sit.

pkg/controllers/cluster/taint_manager.go

pkg/util/binding.go

Garrybest · 2022-06-13T10:04:33Z

The e2e error does not make any sense. Maybe we need a retest.

RainbowMango · 2022-06-13T10:15:11Z

Right.

Echo here for @XiShanYongYe-Chang to check.
https://github.com/karmada-io/karmada/runs/6858581736?check_suite_focus=true

[namespace auto-provision] namespace auto-provision testing joining new cluster
namespace should be propagated to new created clusters[needCreateCluster]
/home/runner/work/karmada/karmada/test/e2e/namespace_test.go:131
------------------------------
• [FAILED] [105.817 seconds]
[namespace auto-provision] namespace auto-provision testing
/home/runner/work/karmada/karmada/test/e2e/namespace_test.go:24
joining new cluster [BeforeEach][needCreateCluster]
/home/runner/work/karmada/karmada/test/e2e/namespace_test.go:84
    namespace should be propagated to new created clusters
/home/runner/work/karmada/karmada/test/e2e/namespace_test.go:131
Begin Captured StdOut/StdErr Output >>
    E0613 09:56:15.873001   42505 unjoin.go:277] Failed to delete cluster object. cluster name: member-e2e-blx, error: Delete "https://172.18.0.5:5443/apis/cluster.karmada.io/v1alpha1/clusters/member-e2e-blx": dial tcp 172.18.0.5:5443: connect: connection refused
    E0613 09:56:15.873109   42505 unjoin.go:166] Failed to delete cluster object. cluster name: member-e2e-blx, error: Delete "https://172.18.0.5:5443/apis/cluster.karmada.io/v1alpha1/clusters/member-e2e-blx": dial tcp 172.18.0.5:5443: connect: connection refused

XiShanYongYe-Chang · 2022-06-13T11:57:17Z

Logs are lost. Access to karmada-apiserver is refused based on existing error information:

Delete "https://172.18.0.5:5443/apis/cluster.karmada.io/v1alpha1/clusters/member-e2e-blx": dial tcp 172.18.0.5:5443: connect: connection refused
Get \"[https://172.18.0.5:5443/api/v1/namespaces/karmada-cluster\](https://172.18.0.5:5443/api/v1/namespaces/karmada-cluster/)": dial tcp 172.18.0.5:5443: connect: connection refused
Delete "https://172.18.0.5:5443/api/v1/namespaces/karmadatest-zzk": dial tcp 172.18.0.5:5443: connect: connection refused

Garrybest · 2022-06-14T03:31:07Z

No big deal, maybe just some occasional errors.

dddddai · 2022-06-17T02:14:44Z

Looks pretty good!

Garrybest · 2022-07-06T02:35:06Z

Could we move forward?

RainbowMango · 2022-07-06T02:55:49Z

I just talked to @XiShanYongYe-Chang about this, he will work on the issue I mentioned above(#1945 (comment)).

I don't want the replica/cluster to be removed directly from binding.spec.clusters that will lead to the replicas being removed immediately.

So, can we just hold for a while? I guess we can include this in the next release(v1.3).

Garrybest · 2022-07-06T05:10:49Z

OK.

RainbowMango · 2022-07-13T09:47:38Z

cmd/controller-manager/app/controllermanager.go

+		ClusterTaintEvictionRetryFrequency: 10 * time.Second,
+		ConcurrentReconciles:               3,
+	}
+	if err := taintManager.SetupWithManager(mgr); err != nil {


The NoExecuteTaintManager is essentially a controller, do you think we should reserve the capacity of disabling it?

I think NoExecuteTaintManager is a part of cluster controller.

Kubernetes takes TaintManager as a part of NodeLifecycleController. It provides an option --enable-taint-manager in NodeLifecycleController to enable taint manager.

So what about adding an option --enable-taint-manager as well?

I agree. the --enable-taint-manager defaults to true. But, I think the feature is part of Failover, I suggest checking both the feature gate and the flags.
By the way, the feature gate will be removed eventually.

But, I think the feature is part of Failover.

Actually, I think this feature is independent. We implement NoExecute taint manager, so another effect of taint is introduced into karmada. Users may taint the cluster by themselves manually. Meanwhile, Failover does depend on this feature, right?

Oh, the feature gate should be added in #1781. Let me make a change here.

Actually, I think this feature is independent. We implement NoExecute taint manager, so another effect of taint is introduced into karmada. Users may taint the cluster by themselves manually. Meanwhile, Failover does depend on this feature, right?

After the eviction, the scheduler would select another cluster to fill the slot(if possible), which is exactly the scenario of Failover. For the case users taint the cluster, I think it's a special scenario that users initiate the failover by themselves.

By the way, how do think about the failover feature? What are the use cases?

Oh, the feature gate should be added in #1781. Let me make a change here.

I'm ok with it. The feature gate means the feature is just introduced, maybe not mature enough to default enable, right?

By the way, how do think about the failover feature? What are the use cases?

Cluster has something wrong, ready condition becomes not ready or unknown.

Cluster controller tries to add NoExecute taint after 5 minutes, enhance cluster lifecycle management: add taints for the clusters which are unhealthy for a period of time #1781.

Taint manager removes scheduling result of failed cluster in rb.spec.clsuters, add NoExecute taint manager #1945. It's a little bit like what descheduler does.

Scheduler tries to scale up these evicted replicas.

I'm ok with it. The feature gate means the feature is just introduced, maybe not mature enough to default enable, right?

Not exactly, we just try to move Failover from scheduler to controller-manager.

Not exactly, we just try to move Failover from scheduler to controller-manager.

I see the plan at #1762. I think removing failover from the scheduler means relieving some responsibilities from the scheduler and letting the scheduler focus on select cluster for workload according to PropgationPolicy. Are we on the same page?

Anyway, let's have a talk at next week's meeting, are you available?

Cool. No problem.

RainbowMango

Generally looks good to me.

Even though I hope this issue could be solved before this PR, but at the meantime, I don't want this to be delied so much, so I suggest moving this forward now since we have a lot of things that addressed by #1945 to do.

Does that makes sense to you? @Garrybest

pkg/controllers/cluster/taint_manager.go

RainbowMango · 2022-07-14T08:09:40Z

cmd/controller-manager/app/controllermanager.go

+		ClusterTaintEvictionRetryFrequency: 10 * time.Second,
+		ConcurrentReconciles:               3,
+	}
+	if err := taintManager.SetupWithManager(mgr); err != nil {


I agree. the --enable-taint-manager defaults to true. But, I think the feature is part of Failover, I suggest checking both the feature gate and the flags.
By the way, the feature gate will be removed eventually.

Garrybest · 2022-07-14T13:52:08Z

Thanks, I will add a new option and let us move forward.

cmd/controller-manager/app/controllermanager.go

pkg/controllers/cluster/taint_manager.go

RainbowMango · 2022-07-15T02:16:47Z

Please rebase your branch as we are fixing the fake testing recently.

Signed-off-by: Garrybest <garrybest@foxmail.com>

cmd/controller-manager/app/controllermanager.go

Signed-off-by: Garrybest <garrybest@foxmail.com>

XiShanYongYe-Chang

Looks good to me.

RainbowMango

/lgtm
/approve

/hold
for
no release notes?

karmada-bot · 2022-07-18T06:12:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RainbowMango

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [RainbowMango]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

RainbowMango · 2022-07-18T08:25:51Z

/hold cancel
PS: I updated the release notes to the PR description.

Garrybest · 2022-07-18T09:17:26Z

OK.

add NoExecute Taints add NoExecute Taints karmada-io#1945 Signed-off-by: Garrybest <garrybest@foxmail.com>

karmada-bot added the kind/feature Categorizes issue or PR as related to a new feature. label Jun 1, 2022

karmada-bot requested review from lonelyCZ, pigletfly, qianjun1993 and RainbowMango June 1, 2022 13:48

Garrybest mentioned this pull request Jun 1, 2022

TODO List: Enhance Cluster Lifecycle Management #1762

Closed

4 tasks

karmada-bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jun 1, 2022

Garrybest force-pushed the pr_taint branch from 196a824 to d60cb85 Compare June 1, 2022 15:20

karmada-bot assigned RainbowMango Jun 2, 2022

karmada-bot requested review from huone1, lynnsong and XiShanYongYe-Chang June 2, 2022 02:32

dddddai reviewed Jun 2, 2022

View reviewed changes

Garrybest force-pushed the pr_taint branch from d60cb85 to fe7f55e Compare June 2, 2022 04:04

RainbowMango reviewed Jun 10, 2022

View reviewed changes

Garrybest force-pushed the pr_taint branch from fe7f55e to 7f105ca Compare June 13, 2022 06:59

Garrybest changed the title ~~add NoExecute taint manager~~ [WIP] add NoExecute taint manager Jun 13, 2022

karmada-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 13, 2022

Garrybest force-pushed the pr_taint branch from 7f105ca to c25601c Compare June 13, 2022 09:17

Garrybest changed the title ~~[WIP] add NoExecute taint manager~~ add NoExecute taint manager Jun 13, 2022

karmada-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 13, 2022

RainbowMango mentioned this pull request Jun 16, 2022

Add cluster success threshold #1884

Merged

RainbowMango reviewed Jul 13, 2022

View reviewed changes

RainbowMango reviewed Jul 14, 2022

View reviewed changes

RainbowMango reviewed Jul 15, 2022

View reviewed changes

cmd/controller-manager/app/controllermanager.go Outdated Show resolved Hide resolved

pkg/controllers/cluster/taint_manager.go Outdated Show resolved Hide resolved

add taint manager

1d2028b

Signed-off-by: Garrybest <garrybest@foxmail.com>

Garrybest force-pushed the pr_taint branch 2 times, most recently from cdceb6f to c2b3c85 Compare July 15, 2022 03:28

RainbowMango reviewed Jul 15, 2022

View reviewed changes

cmd/controller-manager/app/controllermanager.go Show resolved Hide resolved

add option and feature gate for taint manager

e39c584

Signed-off-by: Garrybest <garrybest@foxmail.com>

Garrybest force-pushed the pr_taint branch from c2b3c85 to e39c584 Compare July 15, 2022 08:29

Garrybest requested a review from RainbowMango July 16, 2022 14:28

XiShanYongYe-Chang reviewed Jul 18, 2022

View reviewed changes

RainbowMango reviewed Jul 18, 2022

View reviewed changes

karmada-bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Jul 18, 2022

karmada-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 18, 2022

karmada-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 18, 2022

karmada-bot merged commit d626fcc into karmada-io:master Jul 18, 2022

Garrybest deleted the pr_taint branch July 18, 2022 09:17

This was referenced Jul 25, 2022

supprot NoExecute taint #878

Closed

Added TaintEffectNoExecute support to the scheduled TaintToleration p… #905

Closed

Garrybest pushed a commit to Garrybest/karmada that referenced this pull request Aug 25, 2022

Merge branch 'feat-taint' into 'develop' (merge request !131)

1b5c2f0

add NoExecute Taints add NoExecute Taints karmada-io#1945 Signed-off-by: Garrybest <garrybest@foxmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add NoExecute taint manager #1945

add NoExecute taint manager #1945

Garrybest commented Jun 1, 2022 •

edited by RainbowMango

Loading

RainbowMango commented Jun 2, 2022

Garrybest commented Jun 2, 2022

dddddai left a comment

RainbowMango left a comment

Garrybest commented Jun 13, 2022

RainbowMango commented Jun 13, 2022

XiShanYongYe-Chang commented Jun 13, 2022 •

edited

Loading

Garrybest commented Jun 14, 2022

dddddai commented Jun 17, 2022

Garrybest commented Jul 6, 2022

RainbowMango commented Jul 6, 2022

Garrybest commented Jul 6, 2022

RainbowMango Jul 13, 2022

Garrybest Jul 13, 2022

RainbowMango Jul 14, 2022

Garrybest Jul 14, 2022

Garrybest Jul 14, 2022

RainbowMango Jul 15, 2022

RainbowMango Jul 15, 2022

Garrybest Jul 15, 2022

RainbowMango Jul 15, 2022

Garrybest Jul 15, 2022

RainbowMango left a comment

RainbowMango Jul 14, 2022

Garrybest commented Jul 14, 2022

RainbowMango commented Jul 15, 2022

XiShanYongYe-Chang left a comment

RainbowMango left a comment

karmada-bot commented Jul 18, 2022

RainbowMango commented Jul 18, 2022

Garrybest commented Jul 18, 2022

add NoExecute taint manager #1945

add NoExecute taint manager #1945

Conversation

Garrybest commented Jun 1, 2022 • edited by RainbowMango Loading

RainbowMango commented Jun 2, 2022

Garrybest commented Jun 2, 2022

dddddai left a comment

Choose a reason for hiding this comment

RainbowMango left a comment

Choose a reason for hiding this comment

Garrybest commented Jun 13, 2022

RainbowMango commented Jun 13, 2022

XiShanYongYe-Chang commented Jun 13, 2022 • edited Loading

Garrybest commented Jun 14, 2022

dddddai commented Jun 17, 2022

Garrybest commented Jul 6, 2022

RainbowMango commented Jul 6, 2022

Garrybest commented Jul 6, 2022

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RainbowMango left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Garrybest commented Jul 14, 2022

RainbowMango commented Jul 15, 2022

XiShanYongYe-Chang left a comment

Choose a reason for hiding this comment

RainbowMango left a comment

Choose a reason for hiding this comment

karmada-bot commented Jul 18, 2022

RainbowMango commented Jul 18, 2022

Garrybest commented Jul 18, 2022

Garrybest commented Jun 1, 2022 •

edited by RainbowMango

Loading

XiShanYongYe-Chang commented Jun 13, 2022 •

edited

Loading