add new cluster condition: IncompleteAPIEnablements

[whitewindmills]

What type of PR is this? /kind feature

What this PR does / why we need it:

since https://github.com/karmada-io/karmada/pull/5325 is closed, I open this PR to do it.

Which issue(s) this PR fixes: Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

[whitewindmills]

@XiShanYongYe-Chang could you help with this PR?

[XiShanYongYe-Chang]

Thanks @whitewindmills /assign

[codecov-commenter]

:warning: Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 64.70588% with 6 lines in your changes missing coverage. Please review.

Project coverage is 34.15%. Comparing base (4dfff39) to head (3307a52). Report is 6 commits behind head on master.

Files with missing lines	Patch %	Lines
...kg/controllers/status/cluster_status_controller.go	64.70%	6 Missing :warning:

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

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5400      +/-   ##
==========================================
+ Coverage   33.86%   34.15%   +0.29%     
==========================================
  Files         643      643              
  Lines       44509    44522      +13     
==========================================
+ Hits        15072    15206     +134     
+ Misses      28288    28160     -128     
- Partials     1149     1156       +7     

Flag	Coverage Δ
unittests	34.15% <64.70%> (+0.29%)	:arrow_up:

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[whitewindmills]

@XiShanYongYe-Chang PTAL

[whitewindmills]

/assign @RainbowMango

[whitewindmills]

I'm thinking what if we just skip the .status.apiEnablements updating in case of we can't get full list of APIs? Is it doable?

yes, this pr won't block the controller update.

[XiShanYongYe-Chang]

I'm thinking what if we just skip the .status.apiEnablements updating in case of we can't get full list of APIs? Is it doable?

I had the same idea. Analyze the length of err or apienablements. Only update the status if err is not empty and apienablements is not empty.

[whitewindmills]

I'm thinking what if we just skip the .status.apiEnablements updating in case of we can't get full list of APIs? Is it doable?

assuming this is the case, if a user installs a new CRD, this means that the new CRD will not be updated to the cluster state, resulting in the failure to propagate such CRs.

[whitewindmills]

at least the current situation is that if an API exists in the cluster state, we can consider it reliable, but if an API does not exist, we cannot consider it reliable.

[chaunceyjiang]

I'm thinking what if we just skip the .status.apiEnablements updating in case of we can't get full list of APIs? Is it doable?

Our scheduling plugin APIEnablement checks if the resource kind is present in cluster.status.apiEnablements. If not, it won't propagate to the member clusters.

[whitewindmills]

ping @RainbowMango

[whitewindmills]

@RainbowMango @XiShanYongYe-Chang @chaunceyjiang could you help move it forward, it's very important for us.

[RainbowMango]

Sure! Sorry, too many on my plate recently, it is also very important to the project.

[RainbowMango]

Please help to rebase this as the test matrix changed after release-1.11.

[RainbowMango]

Also, we need a release notes for this, this kind of API change.

[RainbowMango]

I happen to see the conditions on my local environment:

  - lastTransitionTime: "2024-09-13T09:04:15Z"
    message: might collect partial APIEnablements(22) from the cluster
    reason: PartialAPIEnablements
    status: "False"
    type: CompleteAPIEnablements

And

  - lastTransitionTime: "2024-09-13T09:05:35Z"
    message: collected complete APIEnablements from the cluster
    reason: CompleteAPIEnablements
    status: "True"
    type: CompleteAPIEnablements

Do you think we should have a shorter reason? Like Complete/Partial/Empty?

[whitewindmills]

Do you think we should have a shorter reason? Like Complete/Partial/Empty?

looks reasonable.

[whitewindmills]

have you tested it locally?

yes, I did test it but forgot to paste the result here.

[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/apis/OWNERS~~ [RainbowMango]
• ~~pkg/controllers/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]

@yanfeng1992 @whitewindmills @chaunceyjiang Although this PR includes API changes and seems unsuitable for backport, given the significant potential impact, I think it is necessary to backport it to the release branches. What do you think?

By the way, introducing the condition doesn't break backward compatibility, and @XiShanYongYe-Chang has run some tests with it.

[whitewindmills]

sounds good!