fix a corner case that re-schedule be skipped in case of the cluster becomes not fit. #2912

jwcesign · 2022-12-07T07:40:20Z

Signed-off-by: jwcesign jiangwei115@huawei.com

What type of PR is this?
/kind bug

What this PR does / why we need it:

Fix resource hangover in member cluster when propagating missmatch

Which issue(s) this PR fixes:
Fixes #2906

Special notes for your reviewer:
none

Does this PR introduce a user-facing change?:

`karmada-scheduler`: Fixed a corner case that re-schedule be skipped in case of the cluster becomes not fit.

jwcesign · 2022-12-07T07:43:25Z

cc @XiShanYongYe-Chang

XiShanYongYe-Chang · 2022-12-07T07:47:02Z

Thanks~
/assign

codecov-commenter · 2022-12-07T07:53:04Z

Codecov Report

Merging #2912 (24bc5a0) into master (69bd4ec) will increase coverage by 0.65%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #2912      +/-   ##
==========================================
+ Coverage   37.87%   38.52%   +0.65%     
==========================================
  Files         201      205       +4     
  Lines       18494    18815     +321     
==========================================
+ Hits         7004     7249     +245     
- Misses      11081    11137      +56     
- Partials      409      429      +20

Flag	Coverage Δ
unittests	`38.52% <0.00%> (+0.65%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/scheduler/core/generic_scheduler.go	`0.00% <0.00%> (ø)`
pkg/scheduler/event_handler.go	`27.27% <0.00%> (-0.70%)`	⬇️
...ceinterpreter/configurableinterpreter/luavm/lua.go	`57.14% <0.00%> (-0.12%)`	⬇️
pkg/util/rbac.go	`100.00% <0.00%> (ø)`
pkg/search/proxy/store/util.go	`93.36% <0.00%> (ø)`
pkg/karmadactl/interpret/execute.go	`63.63% <0.00%> (ø)`
pkg/karmadactl/interpret/interpret.go	`56.00% <0.00%> (ø)`
...einterpreter/configurableinterpreter/luavm/kube.go	`67.39% <0.00%> (ø)`
pkg/karmadactl/interpret/check.go	`91.17% <0.00%> (ø)`
pkg/util/names/names.go	`96.55% <0.00%> (+0.44%)`	⬆️
... and 1 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

RainbowMango · 2022-12-07T08:11:04Z

/retitle fix a corner case that re-schedule be skipped in case of the cluster becomes not fit.

jwcesign · 2022-12-07T09:04:30Z

cc @chaunceyjiang , Please take a look, I've heard that this question has other context.

pkg/scheduler/core/generic_scheduler.go

test/e2e/propagationpolicy_test.go

pkg/scheduler/core/generic_scheduler.go

test/e2e/rescheduling_test.go

test/e2e/framework/cluster.go

test/e2e/rescheduling_test.go

pkg/scheduler/event_handler.go

chaunceyjiang · 2022-12-09T02:22:11Z

Oh, I missed this pr.

cc @chaunceyjiang , Please take a look, I've heard that this question has other context.

yes ,refer to #2261

pkg/scheduler/event_handler.go

pkg/scheduler/core/generic_scheduler.go

XiShanYongYe-Chang

Just some typo.
Other parts LTGM

test/e2e/framework/cluster.go

test/e2e/rescheduling_test.go

XiShanYongYe-Chang · 2022-12-12T07:55:30Z

/lgtm
/assign @Garrybest

pkg/scheduler/event_handler.go

Garrybest · 2022-12-12T11:31:25Z

pkg/scheduler/scheduler.go

@@ -476,13 +478,14 @@ func (s *Scheduler) scheduleResourceBinding(resourceBinding *workv1alpha2.Resour
 	}

 	scheduleResult, err := s.Algorithm.Schedule(context.TODO(), &placement, &resourceBinding.Spec, &core.ScheduleAlgorithmOption{EnableEmptyWorkloadPropagation: s.enableEmptyWorkloadPropagation})
-	if err != nil {
+	if err != nil && reflect.TypeOf(err) != reflect.TypeOf(&framework.FitError{}) {


I'm afraid we have a situation here. When a cluster is not ready for a while, the cluster will be added a NoSchedule taint. And before, a workload is running on this cluster and happens to scale up(may be triggered by users or hpa-controller), however, no cluster is fit due to the taint. I guess the previous replicas will be evicted here because now fit error is skipped and result will be patched to RB.

According to

karmada/pkg/scheduler/framework/plugins/tainttoleration/taint_toleration.go

Line 42 in 37cca1c

if bindingSpec.TargetContains(cluster.Name) {

, the previous replicas will not be evicted.

Oh, I see. I didn't realize taint plugin is updated here. Thanks.

Now the FitError will be dedicated used in the situation that no cluster fit, right?

yes, then the fitError information will be shown in the event and RB's status, the same behavior as k8s.

Garrybest · 2022-12-12T12:20:37Z

/lgtm

jwcesign · 2022-12-12T12:22:12Z

cc @RainbowMango for final checking

RainbowMango · 2022-12-13T02:18:25Z

@XiShanYongYe-Chang Have you confirmed the E2E part?

XiShanYongYe-Chang · 2022-12-13T02:31:51Z

@XiShanYongYe-Chang Have you confirmed the E2E part?

Yes, I've confirmed it.

RainbowMango · 2022-12-13T02:38:28Z

pkg/scheduler/scheduler.go

@@ -476,13 +478,14 @@ func (s *Scheduler) scheduleResourceBinding(resourceBinding *workv1alpha2.Resour
 	}

 	scheduleResult, err := s.Algorithm.Schedule(context.TODO(), &placement, &resourceBinding.Spec, &core.ScheduleAlgorithmOption{EnableEmptyWorkloadPropagation: s.enableEmptyWorkloadPropagation})
-	if err != nil {
+	if err != nil && reflect.TypeOf(err) != reflect.TypeOf(&framework.FitError{}) {


Suggested change

if err != nil && reflect.TypeOf(err) != reflect.TypeOf(&framework.FitError{}) {

var noClusterFit *framework.FitError

// in case of no cluster fit, can not return but continue to patch(cleanup) the result.

if err != nil && !errors.As(err, &noClusterFit) {

Does this make it a little clearer?

…nges Signed-off-by: jwcesign <jiangwei115@huawei.com>

RainbowMango · 2022-12-13T07:30:14Z

Have you figured out the root cause of the failing tests? I didn't see any change from the force-push.

jwcesign · 2022-12-13T07:34:05Z

Have you figured out the root cause of the failing tests? I didn't see any change from the force push.

it's my mistake, should be if err != nil && !errors.As(err, &noClusterFit) { instead of if err != nil && errors.As(err, &noClusterFit) {.

RainbowMango

/lgtm
/approve

Please cherry-pick this patch to release branches.
And we are going to tag a new release after that.

karmada-bot · 2022-12-13T09:05:57Z

[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

…-upstream-release-1.4 Automated cherry pick of #2912: fix a corner case that re-schedule be skipped in case of the cluster becomes not fit

…-upstream-release-1.3 Automated cherry pick of #2912: fix a corner case that re-schedule be skipped in case of the cluster becomes not fit

karmada-bot added the kind/bug Categorizes issue or PR as related to a bug. label Dec 7, 2022

karmada-bot requested review from qianjun1993 and XiShanYongYe-Chang December 7, 2022 07:40

karmada-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 7, 2022

jwcesign changed the title ~~Fix resource hangover in member cluster when propagating matching changes~~ Fix resource hangover in member cluster when propagating missmatch Dec 7, 2022

karmada-bot assigned XiShanYongYe-Chang Dec 7, 2022

jwcesign force-pushed the fix-pp-rb branch from f8584fe to 7b4eeb7 Compare December 7, 2022 07:56

karmada-bot changed the title ~~Fix resource hangover in member cluster when propagating missmatch~~ fix a corner case that re-schedule be skipped in case of the cluster becomes not fit. Dec 7, 2022

jwcesign force-pushed the fix-pp-rb branch from 7b4eeb7 to 2b33ea0 Compare December 7, 2022 08:48

XiShanYongYe-Chang reviewed Dec 8, 2022

View reviewed changes

pkg/scheduler/core/generic_scheduler.go Outdated Show resolved Hide resolved

test/e2e/propagationpolicy_test.go Outdated Show resolved Hide resolved

jwcesign force-pushed the fix-pp-rb branch from 2b33ea0 to f3ce4c2 Compare December 8, 2022 07:56

XiShanYongYe-Chang reviewed Dec 9, 2022

View reviewed changes

pkg/scheduler/core/generic_scheduler.go Outdated Show resolved Hide resolved

test/e2e/rescheduling_test.go Outdated Show resolved Hide resolved

test/e2e/framework/cluster.go Show resolved Hide resolved

test/e2e/rescheduling_test.go Show resolved Hide resolved

RainbowMango added this to the v1.5 milestone Dec 9, 2022

RainbowMango reviewed Dec 9, 2022

View reviewed changes

pkg/scheduler/event_handler.go Outdated Show resolved Hide resolved

jwcesign force-pushed the fix-pp-rb branch from f3ce4c2 to 7fc3180 Compare December 9, 2022 03:20

chaunceyjiang reviewed Dec 9, 2022

View reviewed changes

pkg/scheduler/event_handler.go Outdated Show resolved Hide resolved

jwcesign force-pushed the fix-pp-rb branch 2 times, most recently from 452595a to 24bc5a0 Compare December 9, 2022 06:56

RainbowMango reviewed Dec 9, 2022

View reviewed changes

pkg/scheduler/core/generic_scheduler.go Outdated Show resolved Hide resolved

XiShanYongYe-Chang reviewed Dec 9, 2022

View reviewed changes

test/e2e/framework/cluster.go Outdated Show resolved Hide resolved

test/e2e/framework/cluster.go Outdated Show resolved Hide resolved

test/e2e/rescheduling_test.go Outdated Show resolved Hide resolved

jwcesign force-pushed the fix-pp-rb branch 3 times, most recently from 95beefb to ba18160 Compare December 12, 2022 07:10

karmada-bot assigned Garrybest Dec 12, 2022

karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Dec 12, 2022

Garrybest reviewed Dec 12, 2022

View reviewed changes

RainbowMango reviewed Dec 13, 2022

View reviewed changes

jwcesign force-pushed the fix-pp-rb branch from ba18160 to 05ed154 Compare December 13, 2022 02:53

karmada-bot removed the lgtm Indicates that a PR is ready to be merged. label Dec 13, 2022

jwcesign force-pushed the fix-pp-rb branch from 05ed154 to 9bf7c0e Compare December 13, 2022 02:54

Fix resource hangover in member cluster when propagating matching cha…

950b614

…nges Signed-off-by: jwcesign <jiangwei115@huawei.com>

jwcesign force-pushed the fix-pp-rb branch from 9bf7c0e to 950b614 Compare December 13, 2022 06:56

RainbowMango approved these changes Dec 13, 2022

View reviewed changes

karmada-bot assigned RainbowMango Dec 13, 2022

karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Dec 13, 2022

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

karmada-bot merged commit f029394 into karmada-io:master Dec 13, 2022

jwcesign mentioned this pull request Dec 13, 2022

Automated cherry pick of #2912: fix a corner case that re-schedule be skipped in case of the cluster becomes not fit #2955

Merged

jwcesign mentioned this pull request Dec 14, 2022

Automated cherry pick of #2912: fix a corner case that re-schedule be skipped in case of the cluster becomes not fit #2959

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix a corner case that re-schedule be skipped in case of the cluster becomes not fit. #2912

fix a corner case that re-schedule be skipped in case of the cluster becomes not fit. #2912

jwcesign commented Dec 7, 2022 •

edited by RainbowMango

Loading

jwcesign commented Dec 7, 2022

XiShanYongYe-Chang commented Dec 7, 2022

codecov-commenter commented Dec 7, 2022 •

edited

Loading

RainbowMango commented Dec 7, 2022

jwcesign commented Dec 7, 2022 •

edited

Loading

chaunceyjiang commented Dec 9, 2022

XiShanYongYe-Chang left a comment

XiShanYongYe-Chang commented Dec 12, 2022

Garrybest Dec 12, 2022

jwcesign Dec 12, 2022

Garrybest Dec 12, 2022

RainbowMango Dec 13, 2022

jwcesign Dec 13, 2022

Garrybest commented Dec 12, 2022

jwcesign commented Dec 12, 2022

RainbowMango commented Dec 13, 2022

XiShanYongYe-Chang commented Dec 13, 2022

RainbowMango Dec 13, 2022

jwcesign Dec 13, 2022

RainbowMango commented Dec 13, 2022

jwcesign commented Dec 13, 2022

RainbowMango left a comment

karmada-bot commented Dec 13, 2022

-	if err != nil && reflect.TypeOf(err) != reflect.TypeOf(&framework.FitError{}) {
+	var noClusterFit *framework.FitError
+	// in case of no cluster fit, can not return but continue to patch(cleanup) the result.
+	if err != nil && !errors.As(err, &noClusterFit) {

fix a corner case that re-schedule be skipped in case of the cluster becomes not fit. #2912

fix a corner case that re-schedule be skipped in case of the cluster becomes not fit. #2912

Conversation

jwcesign commented Dec 7, 2022 • edited by RainbowMango Loading

jwcesign commented Dec 7, 2022

XiShanYongYe-Chang commented Dec 7, 2022

codecov-commenter commented Dec 7, 2022 • edited Loading

Codecov Report

RainbowMango commented Dec 7, 2022

jwcesign commented Dec 7, 2022 • edited Loading

chaunceyjiang commented Dec 9, 2022

XiShanYongYe-Chang left a comment

Choose a reason for hiding this comment

XiShanYongYe-Chang commented Dec 12, 2022

Garrybest Dec 12, 2022

Choose a reason for hiding this comment

jwcesign Dec 12, 2022

Choose a reason for hiding this comment

Garrybest Dec 12, 2022

Choose a reason for hiding this comment

RainbowMango Dec 13, 2022

Choose a reason for hiding this comment

jwcesign Dec 13, 2022

Choose a reason for hiding this comment

Garrybest commented Dec 12, 2022

jwcesign commented Dec 12, 2022

RainbowMango commented Dec 13, 2022

XiShanYongYe-Chang commented Dec 13, 2022

RainbowMango Dec 13, 2022

Choose a reason for hiding this comment

jwcesign Dec 13, 2022

Choose a reason for hiding this comment

RainbowMango commented Dec 13, 2022

jwcesign commented Dec 13, 2022

RainbowMango left a comment

Choose a reason for hiding this comment

karmada-bot commented Dec 13, 2022

jwcesign commented Dec 7, 2022 •

edited by RainbowMango

Loading

codecov-commenter commented Dec 7, 2022 •

edited

Loading

jwcesign commented Dec 7, 2022 •

edited

Loading