Add metadataSufficient status to indicate wether cluster have get all api-resource #1804

duanmengkk · 2022-05-17T01:21:29Z

Signed-off-by: duanmeng [email protected]

What type of PR is this?

/kind bug

What this PR does / why we need it:

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

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

duanmengkk · 2022-05-17T08:20:45Z

/retitle Add warn status of the member cluster when failed get all supported resources from server

karmada-bot · 2022-05-17T08:20:54Z

@duanmengkk: Re-titling can only be requested by trusted users, like repository collaborators.

In response to this:

/retitle Add warn status of the member cluster when failed get all supported resources from server

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

duanmengkk · 2022-05-17T08:42:05Z

/assign @XiShanYongYe-Chang
before
#1673
now

and schedule is ok

duanmengkk · 2022-05-17T08:59:33Z

/cc @lonelyCZ

duanmengkk · 2022-05-18T08:42:50Z

/assign @RainbowMango

pkg/apis/cluster/v1alpha1/types.go

karmada-bot · 2022-05-23T01:50:19Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please ask for approval from rainbowmango after the PR has been reviewed.

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

Needs approval from an approver in each of these files:

OWNERS

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

duanmengkk · 2022-05-25T01:21:49Z

/close

karmada-bot · 2022-05-25T01:21:52Z

@duanmengkk: Closed this PR.

In response to this:

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

RainbowMango · 2022-05-25T01:48:39Z

The latest code judge the health status of the cluster by /readyz and /healthz and ignore the error when get pod and get node,get api-resource ,just log the error and do not fail over.But it does solve the problem #1673

It is #1829. But as we discussed at the meeting, we still need a condition to represent the metadata collection status.

duanmengkk · 2022-05-25T02:21:50Z

/reopen ok,i will do it later

karmada-bot · 2022-05-25T02:21:53Z

@duanmengkk: Reopened this PR.

In response to this:

/reopen

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

duanmengkk · 2022-05-25T05:16:56Z

/cc @dddddai if cluster is oline and healty ,but get pod and get node is failed, should not we handle the error and mark cluster status as unready?

RainbowMango · 2022-05-25T06:12:13Z

if cluster is oline and healty ,but get pod and get node is failed, should not we handle the error and mark cluster status as unready?

I don't think so.

dddddai · 2022-05-25T07:38:31Z

if cluster is oline and healty ,but get pod and get node is failed, should not we handle the error and mark cluster status as unready?

I don't think so.

+1, I think the ready condition should only represent the health check result

duanmengkk · 2022-05-27T03:25:00Z

/hold for e2e test

duanmengkk · 2022-05-28T13:08:41Z

/unhold
/cc @RainbowMango

pkg/apis/cluster/v1alpha1/types.go

… supported resources from server Signed-off-by: duanmeng <[email protected]>

RainbowMango · 2022-06-02T10:01:31Z

pkg/controllers/status/cluster_status_controller.go

@@ -47,6 +47,9 @@ const (
 	clusterNotReachableReason = "ClusterNotReachable"
 	clusterNotReachableMsg    = "cluster is not reachable"
 	statusCollectionFailed    = "StatusCollectionFailed"
+	metadataSufficientMsg     = "all APIs installed in the cluster have been obtained"


metadata sufficient not only include APIs, see the condition comments.

RainbowMango · 2022-06-02T10:10:15Z

pkg/printers/internalversion/printers.go

@@ -18,6 +18,7 @@ func AddHandlers(h printers.PrintHandler) {
 		{Name: "Version", Type: "string", Description: "KubernetesVersion represents version of the member cluster."},
 		{Name: "Mode", Type: "string", Description: "SyncMode describes how a cluster sync resources from karmada control plane."},
 		{Name: "Ready", Type: "string", Description: "The aggregate readiness state of this cluster for accepting workloads."},
+		{Name: "MetadataSufficient", Type: "string", Description: "The status indicate that all APIs are obtained"},


Is this used to format the output for kubectl get ?

Is there a test report?

pkg/util/membercluster_client.go

RainbowMango · 2022-06-02T10:20:07Z

pkg/controllers/status/cluster_status_controller.go

+// getAPIEnablements get the list of APIs installed in the member cluster
 func getAPIEnablements(clusterClient *util.ClusterClient) ([]clusterv1alpha1.APIEnablement, error) {
+	// This can return an error *and* the results it was able to find.
+	// If the length of api resources is not zero,we don't need to fail on the error.
 	_, apiResourceList, err := clusterClient.KubeClient.Discovery().ServerGroupsAndResources()
-	if err != nil {
+	if len(apiResourceList) == 0 {
+		klog.Errorf("unable to get any supported resources from server. Error: %v.", err)
 		return nil, err
 	}


Can you split a PR for the partially APIs? This is our key mission, and shouldn't be blocked by the optional new condition.

Can you split a PR for the partially APIs?

I am not clearly about what you sayed.Do you mean this PR should not contain e2e test,just focus on the feature of metadataSufficient status.Or is there a problem in line 385 and 386 ?

Do you mean this pr should not contain e2e test,just focus on the feature of metadataSufficient status

If my understand is right,I will split this PR in three:1. Add metadataSufficient status 2. Add output of metadataSufficient in kubectl get cluster 3.Add new e2e test for metadataSufficient status

Can you split a PR for the partially APIs? This is our key mission, and shouldn't be blocked by the optional new condition.

If you do not mind,Can you describe it in Chinese?

I mean the change for func getAPIEnablements is what we urgently needed. We can do it in a separate PR.

For the MetadataSufficient condition, I'm a little bit hesitant as it seems to lack enough benefits to users.

Thanks. I got it

A separate PR in #1968
/cc @RainbowMango

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

karmada-bot requested review from Garrybest and pigletfly May 17, 2022 01:21

karmada-bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label May 17, 2022

duanmengkk changed the title ~~ignore the error when getting the list of APIs installed in the membe…~~ WIP:ignore the error when getting the list of APIs installed in the membe… May 17, 2022

karmada-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 17, 2022

duanmengkk changed the title ~~WIP:ignore the error when getting the list of APIs installed in the membe…~~ Add warn status of the member cluster when failed get all supported resources from server May 17, 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 May 17, 2022

karmada-bot assigned XiShanYongYe-Chang May 17, 2022

karmada-bot requested a review from lonelyCZ May 17, 2022 08:59

karmada-bot assigned RainbowMango May 18, 2022

RainbowMango reviewed May 18, 2022

View reviewed changes

pkg/apis/cluster/v1alpha1/types.go Outdated Show resolved Hide resolved

karmada-bot closed this May 25, 2022

karmada-bot reopened this May 25, 2022

karmada-bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 27, 2022

karmada-bot requested a review from RainbowMango May 28, 2022 13:08

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

RainbowMango reviewed May 30, 2022

View reviewed changes

pkg/apis/cluster/v1alpha1/types.go Outdated Show resolved Hide resolved

Add metadataSufficient status to indicate wether cluster have get all…

2a2bac6

… supported resources from server Signed-off-by: duanmeng <[email protected]>

duanmengkk changed the title ~~Add warn status of the member cluster when failed get all supported resources from server~~ # This is the commit message #1: Add warn status of the member cluster when failed get all supported resources from server May 31, 2022

duanmengkk changed the title ~~# This is the commit message #1: Add warn status of the member cluster when failed get all supported resources from server~~ Add warn status of the member cluster when failed get all supported resources from server May 31, 2022

duanmengkk changed the title ~~Add warn status of the member cluster when failed get all supported resources from server~~ Add metadataSufficient status to indicate wether cluster have get all api-resource May 31, 2022

RainbowMango reviewed Jun 2, 2022

View reviewed changes

karmada-bot requested a review from RainbowMango June 6, 2022 13:37

RainbowMango mentioned this pull request Sep 13, 2024

add new cluster condition: CompleteAPIEnablements #5400

Merged

karmada-bot closed this in #5400 Sep 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add metadataSufficient status to indicate wether cluster have get all api-resource #1804

Add metadataSufficient status to indicate wether cluster have get all api-resource #1804

duanmengkk commented May 17, 2022 •

edited

Loading

duanmengkk commented May 17, 2022

karmada-bot commented May 17, 2022

duanmengkk commented May 17, 2022 •

edited

Loading

duanmengkk commented May 17, 2022

duanmengkk commented May 18, 2022

karmada-bot commented May 23, 2022

duanmengkk commented May 25, 2022

karmada-bot commented May 25, 2022

RainbowMango commented May 25, 2022

duanmengkk commented May 25, 2022 •

edited

Loading

karmada-bot commented May 25, 2022

duanmengkk commented May 25, 2022

RainbowMango commented May 25, 2022

dddddai commented May 25, 2022

duanmengkk commented May 27, 2022

duanmengkk commented May 28, 2022

RainbowMango Jun 2, 2022

RainbowMango Jun 2, 2022

RainbowMango Jun 2, 2022

duanmengkk Jun 3, 2022 •

edited

Loading

duanmengkk Jun 3, 2022 •

edited

Loading

duanmengkk Jun 3, 2022

RainbowMango Jun 6, 2022

duanmengkk Jun 6, 2022

duanmengkk Jun 6, 2022

Add metadataSufficient status to indicate wether cluster have get all api-resource #1804

Add metadataSufficient status to indicate wether cluster have get all api-resource #1804

Conversation

duanmengkk commented May 17, 2022 • edited Loading

duanmengkk commented May 17, 2022

karmada-bot commented May 17, 2022

duanmengkk commented May 17, 2022 • edited Loading

duanmengkk commented May 17, 2022

duanmengkk commented May 18, 2022

karmada-bot commented May 23, 2022

duanmengkk commented May 25, 2022

karmada-bot commented May 25, 2022

RainbowMango commented May 25, 2022

duanmengkk commented May 25, 2022 • edited Loading

karmada-bot commented May 25, 2022

duanmengkk commented May 25, 2022

RainbowMango commented May 25, 2022

dddddai commented May 25, 2022

duanmengkk commented May 27, 2022

duanmengkk commented May 28, 2022

RainbowMango Jun 2, 2022

Choose a reason for hiding this comment

RainbowMango Jun 2, 2022

Choose a reason for hiding this comment

RainbowMango Jun 2, 2022

Choose a reason for hiding this comment

duanmengkk Jun 3, 2022 • edited Loading

Choose a reason for hiding this comment

duanmengkk Jun 3, 2022 • edited Loading

Choose a reason for hiding this comment

duanmengkk Jun 3, 2022

Choose a reason for hiding this comment

RainbowMango Jun 6, 2022

Choose a reason for hiding this comment

duanmengkk Jun 6, 2022

Choose a reason for hiding this comment

duanmengkk Jun 6, 2022

Choose a reason for hiding this comment

duanmengkk commented May 17, 2022 •

edited

Loading

duanmengkk commented May 17, 2022 •

edited

Loading

duanmengkk commented May 25, 2022 •

edited

Loading

duanmengkk Jun 3, 2022 •

edited

Loading

duanmengkk Jun 3, 2022 •

edited

Loading