WaitAPIServiceReady use watch #3415

helen-frank · 2023-04-17T08:43:52Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
Avoid too many logs
Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

karmada-bot · 2023-04-17T08:44:03Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign prodanlabs after the PR has been reviewed.
You can assign the PR to them by writing /assign @prodanlabs in a comment when ready.

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

Needs approval from an approver in each of these files:

pkg/karmadactl/cmdinit/OWNERS

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

codecov-commenter · 2023-04-17T09:00:36Z

Codecov Report

Merging #3415 (978486b) into master (4552a19) will decrease coverage by 0.05%.
The diff coverage is 0.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##           master    #3415      +/-   ##
==========================================
- Coverage   51.79%   51.75%   -0.05%     
==========================================
  Files         210      210              
  Lines       18926    18938      +12     
==========================================
- Hits         9803     9801       -2     
- Misses       8581     8595      +14     
  Partials      542      542

Flag	Coverage Δ
unittests	`51.75% <0.00%> (-0.05%)`	⬇️

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

Impacted Files	Coverage Δ
pkg/karmadactl/cmdinit/karmada/check.go	`0.00% <0.00%> (ø)`

... and 1 file with indirect coverage changes

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

helen-frank · 2023-04-17T13:28:23Z

/retest

Signed-off-by: helen <[email protected]>

RainbowMango · 2023-04-18T03:33:52Z

How many logs avoided by this patch?

helen-frank · 2023-04-18T03:59:35Z

How many logs avoided by this patch?

The new cluster installed karmada, and measured the init before and after the change, the measured log volume dropped from 77 to 18

RainbowMango · 2023-04-18T07:11:17Z

the measured log volume dropped from 77 to 18

What kind of log is avoided?
I believe you have tested it before sending the PR, it'd be helpful if you can give more details.

helen-frank · 2023-04-18T08:09:38Z

the measured log volume dropped from 77 to 18

What kind of log is avoided? I believe you have tested it before sending the PR, it'd be helpful if you can give more details.

The original code will print this log every 1s.

I0418 06:33:32.788899  375506 check.go:26] Waiting for APIService(v1alpha1.cluster.karmada.io) condition(Available), will try

helen-frank · 2023-06-05T06:18:49Z

@RainbowMango @lonelyCZ hi, After troubleshooting errors for #3418, I think it is necessary to reduce the useless duplicate logs of WaitAPIServiceReady and request merge.

RainbowMango · 2023-06-05T09:54:26Z

Do you mean #3418 rely on this PR?

RainbowMango · 2023-06-05T10:04:55Z

pkg/karmadactl/cmdinit/karmada/check.go

+	apiService, err := c.ApiregistrationV1().APIServices().Get(context.TODO(), name, metav1.GetOptions{})
+	if err != nil {
+		return err
+	}


What if the APIService not exist at that time?

I think when this situation occurs, the normal error can be thrown, WaitAPIServiceReady premise is that there is already APIService

Please @lonelyCZ check if the behavior change is acceptable.

Before executing WaitAPIServiceReady, we firstly CreateOrUpdateAPIService in general.

karmada/pkg/karmadactl/cmdinit/karmada/deploy.go

Lines 302 to 308 in 3604123

if err = util.CreateOrUpdateAPIService(apiRegistrationClient, aaAPIService); err != nil {

return err

}

if err := WaitAPIServiceReady(apiRegistrationClient, aaAPIServiceObjName, 120*time.Second); err != nil {

return err

}

helen-frank · 2023-06-05T10:15:00Z

Do you mean #3418 rely on this PR?

Not interdependent, just found useful when troubleshooting errors and reducing duplicate error logs

RainbowMango · 2023-06-05T12:04:54Z

pkg/karmadactl/cmdinit/karmada/check.go

+		}
+
+		if apiregistrationv1helper.IsAPIServiceConditionTrue(updatedAPIService, apiregistrationv1.Available) {
+			return nil
 		}

 		klog.Infof("Waiting for APIService(%s) condition(%s), will try", name, apiregistrationv1.Available)


Here will dump info logs, does it seems too verbose?

The length looks ok, for example I0601 05:59:14.497568 20024 check.go:26] Waiting for APIService(v1alpha1.cluster.karmada.io) condition(Available), will try

Maybe we shouldn't print this log, it just tells the user that karmadactl is executing and the current shell is not disconnected, so it's not really that useful

PS: I prefer to use kwokctl circle waiting method, but this seems to be a bit of a big change, I don't know what you think about the evolution of karmadactl later

Maybe we shouldn't print this log, it just tells the user that karmadactl is executing and the current shell is not disconnected, so it's not really that useful

Yes, I think that is one of the benefits that keep telling users that karmadactl isn't hanging there. :)

PS: I prefer to use kwokctl circle waiting method, but this seems to be a bit of a big change, I don't know what you think about the evolution of karmadactl later

Don't know about kwokctl circle.

The kworkctl circle is cool, many project CLI use this way, like subctl of submariner that are interesting and clear. I think we also use this way in the future.

helen-frank · 2023-06-09T08:11:52Z

/cc @lonelyCZ

lonelyCZ · 2023-06-09T09:11:13Z

Thanks @helen-frank , I will look it ASAP

/assign

lonelyCZ · 2023-06-11T17:46:11Z

Hi, @helen-frank , Could you paste your test result, how many logs do we reduce?

helen-frank · 2023-06-12T01:02:36Z

Hi, @helen-frank , Could you paste your test result, how many logs do we reduce?

The log volume was reduced from 77 to 18 when karmada-aggregated-apiserver failed to start properly
(Maybe we can no longer output this log

helen-frank · 2023-06-20T03:00:54Z

ping @lonelyCZ

karmada-bot added the kind/feature label Apr 17, 2023

karmada-bot requested review from carlory and prodanlabs April 17, 2023 08:44

karmada-bot added the size/M label Apr 17, 2023

helen-frank force-pushed the feature/WaitAPIServiceReadyUseWatch branch from 978486b to 153aeec Compare April 17, 2023 10:28

helen-frank force-pushed the feature/WaitAPIServiceReadyUseWatch branch 3 times, most recently from 40143d5 to 9b523b0 Compare April 17, 2023 15:17

WaitAPIServiceReady use watch.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

ae1f3d8

Signed-off-by: helen <[email protected]>

helen-frank force-pushed the feature/WaitAPIServiceReadyUseWatch branch from 9b523b0 to ae1f3d8 Compare April 17, 2023 15:26

helen-frank mentioned this pull request Jun 1, 2023

Optimization WaitForStatefulSetRollout and WaitForDeploymentRollout #3418

Open

RainbowMango reviewed Jun 5, 2023

View reviewed changes

karmada-bot requested a review from lonelyCZ June 9, 2023 08:11

karmada-bot assigned lonelyCZ Jun 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WaitAPIServiceReady use watch #3415

WaitAPIServiceReady use watch #3415

helen-frank commented Apr 17, 2023

karmada-bot commented Apr 17, 2023

codecov-commenter commented Apr 17, 2023

helen-frank commented Apr 17, 2023

RainbowMango commented Apr 18, 2023

helen-frank commented Apr 18, 2023

RainbowMango commented Apr 18, 2023

helen-frank commented Apr 18, 2023

helen-frank commented Jun 5, 2023

RainbowMango commented Jun 5, 2023

RainbowMango Jun 5, 2023

helen-frank Jun 5, 2023 •

edited

Loading

RainbowMango Jun 5, 2023

lonelyCZ Jun 11, 2023

helen-frank commented Jun 5, 2023

RainbowMango Jun 5, 2023

helen-frank Jun 5, 2023

RainbowMango Jun 5, 2023

lonelyCZ Jun 11, 2023

helen-frank commented Jun 9, 2023

lonelyCZ commented Jun 9, 2023

lonelyCZ commented Jun 11, 2023

helen-frank commented Jun 12, 2023 •

edited

Loading

helen-frank commented Jun 20, 2023

	if err = util.CreateOrUpdateAPIService(apiRegistrationClient, aaAPIService); err != nil {
	return err
	}

	if err := WaitAPIServiceReady(apiRegistrationClient, aaAPIServiceObjName, 120*time.Second); err != nil {
	return err
	}

WaitAPIServiceReady use watch #3415

Are you sure you want to change the base?

WaitAPIServiceReady use watch #3415

Conversation

helen-frank commented Apr 17, 2023

karmada-bot commented Apr 17, 2023

codecov-commenter commented Apr 17, 2023

Codecov Report

helen-frank commented Apr 17, 2023

RainbowMango commented Apr 18, 2023

helen-frank commented Apr 18, 2023

RainbowMango commented Apr 18, 2023

helen-frank commented Apr 18, 2023

helen-frank commented Jun 5, 2023

RainbowMango commented Jun 5, 2023

RainbowMango Jun 5, 2023

Choose a reason for hiding this comment

helen-frank Jun 5, 2023 • edited Loading

Choose a reason for hiding this comment

RainbowMango Jun 5, 2023

Choose a reason for hiding this comment

lonelyCZ Jun 11, 2023

Choose a reason for hiding this comment

helen-frank commented Jun 5, 2023

RainbowMango Jun 5, 2023

Choose a reason for hiding this comment

helen-frank Jun 5, 2023

Choose a reason for hiding this comment

RainbowMango Jun 5, 2023

Choose a reason for hiding this comment

lonelyCZ Jun 11, 2023

Choose a reason for hiding this comment

helen-frank commented Jun 9, 2023

lonelyCZ commented Jun 9, 2023

lonelyCZ commented Jun 11, 2023

helen-frank commented Jun 12, 2023 • edited Loading

helen-frank commented Jun 20, 2023

helen-frank Jun 5, 2023 •

edited

Loading

helen-frank commented Jun 12, 2023 •

edited

Loading