add new cluster condition: CompleteAPIEnablements

[whitewindmills]

What type of PR is this?
/kind feature

What this PR does / why we need it:

since #5325 is closed, I open this PR to do it.

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

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Introduced a new condition `CompleteAPIEnablements` to `Cluster` API to represent the API collection status.

[whitewindmills]

@XiShanYongYe-Chang could you help with this PR?

[XiShanYongYe-Chang]

Thanks @whitewindmills
/assign

[codecov-commenter]

⚠️ 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 ⚠️

❗ 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%)	⬆️

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.

[whitewindmills]

@XiShanYongYe-Chang PTAL

[XiShanYongYe-Chang]

/lgtm
/cc @RainbowMango

[whitewindmills]

/assign @RainbowMango

[RainbowMango]

I'm still thinking another approach:

In some cases, the controller can't collect a full list of API list continuously, such as there is a broken APIService in the member cluster, and thus will block the controller update the status of the Cluster object. And that's exactly the reason that we accept the partial list.

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?

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

/lgtm

Since no tests cover this so far, have you tested it locally?

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

[RainbowMango]

/lgtm
/approve

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