skip the filter if the cluster is already in the list of scheduling result even if the API is missed

[yanfeng1992]

Signed-off-by: huangyanfeng [email protected]

What type of PR is this?
/kind bug

What this PR does / why we need it:

Because when obtaining APIEnablements, it may be empty or partial.
This will cause the scheduler to delete the scheduling results, resulting in incorrect deletion

So if the cluster is already in the list of scheduling results, even if the API is not hit, skip the filter

karmada/pkg/controllers/status/cluster_status_controller.go

Lines 233 to 239 in a87ec2a

	apiEnables, err := getAPIEnablements(clusterClient)
	if len(apiEnables) == 0 {
	klog.Errorf("Failed to get any APIs installed in Cluster %s. Error: %v.", cluster.GetName(), err)
	} else if err != nil {
	klog.Warningf("Maybe get partial(%d) APIs installed in Cluster %s. Error: %v.", len(apiEnables), cluster.GetName(), err)
	}
	currentClusterStatus.APIEnablements = apiEnables

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

`karmada-scheduler`: Avoid filtering out clusters if the API enablement is incomplete during re-scheduling.

[yanfeng1992]

/retest

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 0% with 7 lines in your changes missing coverage. Please review.

Project coverage is 34.14%. Comparing base (e76ce63) to head (4c416ce).
Report is 4 commits behind head on master.

Files with missing lines	Patch %	Lines
.../framework/plugins/apienablement/api_enablement.go	0.00%	7 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5216      +/-   ##
==========================================
- Coverage   34.14%   34.14%   -0.01%     
==========================================
  Files         643      643              
  Lines       44522    44527       +5     
==========================================
+ Hits        15204    15205       +1     
- Misses      28162    28166       +4     
  Partials     1156     1156              

Flag	Coverage Δ
unittests	34.14% <0.00%> (-0.01%)	⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[XiShanYongYe-Chang]

Thanks for your feedback @yanfeng1992

Because when obtaining APIEnablements, it may be empty or partial.

When will that happen?

[chaunceyjiang]

When will that happen?

I guess it's when the status of AA is false?

[yanfeng1992]

When will that happen?

I guess it's when the status of AA is false?

When the AA status is false and the cluster status is healthy

[XiShanYongYe-Chang]

Thanks @chaunceyjiang @yanfeng1992 , I get it.

[chaunceyjiang]

The core meaning of your PR is: If the binding has already been scheduled to a certain cluster, then we should ignore this apienablement plugin, right?

I have a question: If the AA service is no longer healthy, can it still create corresponding resources?

[XiShanYongYe-Chang]

Can we determine whether the cluster status needs to be updated based on the returned error?

karmada/pkg/controllers/status/cluster_status_controller.go

Lines 233 to 239 in a87ec2a

	apiEnables, err := getAPIEnablements(clusterClient)
	if len(apiEnables) == 0 {
	klog.Errorf("Failed to get any APIs installed in Cluster %s. Error: %v.", cluster.GetName(), err)
	} else if err != nil {
	klog.Warningf("Maybe get partial(%d) APIs installed in Cluster %s. Error: %v.", len(apiEnables), cluster.GetName(), err)
	}
	currentClusterStatus.APIEnablements = apiEnables

Only when the err description indicates that some APIs fail to be installed, the returned apiEnables will be updated to the cluster status.

[yanfeng1992]

/assign @XiShanYongYe-Chang

[whitewindmills]

@RainbowMango @XiShanYongYe-Chang
since #5400 has been merged, could you help move this forward?

[XiShanYongYe-Chang]

Of course, let me take a review.

[RainbowMango]

/assign
I want to take a look.

[RainbowMango]

It seems not the one I suggested.

[RainbowMango]

/lgtm

@yanfeng1992 Have you tested it?

[yanfeng1992]

@yanfeng1992 Have you tested it?

step1. stop karmada-controller-manager and patch cluster
kubectl patch cluster cluster5715 --type='merge' -p "{ \"status\": { \"apiEnablements\": [], \"conditions\": [ { \"message\": \"test\", \"reason\": \"Empty\", \"status\": \"False\", \"type\": \"CompleteAPIEnablements\", \"lastTransitionTime\": \"$CURRENT_TIME\" } ] } }" --kubeconfig /etc/karmada/karmada-apiserver.config

step2: edit crb add label for test

log:

clusterResourceBinding(test-clusterrolebinding) scheduled to clusters [{cluster5715 0}]

[RainbowMango]

/approve
Thanks. I assume @whitewindmills has no further comments.

[karmada-bot]

[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:

• pkg/scheduler/OWNERS [RainbowMango]

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