check deployment statefulset rollout not use check pod ready #3221

yanfeng1992 · 2023-02-28T07:41:02Z

What type of PR is this?
/kind cleanup

What this PR does / why we need it:

Now the way to check the karmada component is to traverse all the pods according to the label. I think this way is not perfect, and when the for loop checks, the set timeout time is not accurate.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

codecov-commenter · 2023-02-28T07:57:35Z

Codecov Report

Merging #3221 (ab4e1cf) into master (51fbbb0) will increase coverage by 0.01%.
The diff coverage is 0.00%.

❗ Current head ab4e1cf differs from pull request most recent head b7b0116. Consider uploading reports for the commit b7b0116 to get more accurate results

📣 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    #3221      +/-   ##
==========================================
+ Coverage   49.06%   49.07%   +0.01%     
==========================================
  Files         203      203              
  Lines       18354    18352       -2     
==========================================
+ Hits         9005     9007       +2     
+ Misses       8859     8856       -3     
+ Partials      490      489       -1

Flag	Coverage Δ
unittests	`49.07% <0.00%> (+0.01%)`	⬆️

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

Impacted Files	Coverage Δ
pkg/karmadactl/cmdinit/kubernetes/check.go	`0.00% <0.00%> (ø)`
pkg/karmadactl/cmdinit/kubernetes/deploy.go	`7.46% <0.00%> (-0.08%)`	⬇️
pkg/search/proxy/store/util.go	`92.41% <0.00%> (-0.48%)`	⬇️
pkg/util/worker.go	`71.42% <0.00%> (+4.76%)`	⬆️

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

calvin0327 · 2023-02-28T08:09:55Z

and when the for loop checks, the set timeout time is not accurate

@yanfeng1992 why the timeout is not accurate? I don't understand. could you explain that?

yanfeng1992 · 2023-02-28T09:00:10Z

When checking circularly, if the deployment has three replicas, check them serially in order. After checking the first pod, hasn’t the second pod been created for a while? @calvin0327

pkg/karmadactl/cmdinit/kubernetes/check.go

pkg/karmadactl/cmdinit/kubernetes/deploy.go

pkg/karmadactl/cmdinit/options/global.go

pkg/karmadactl/addons/enable.go

yanfeng1992 · 2023-03-01T06:12:17Z

karmada/pkg/karmadactl/addons/init/disable_option.go

Line 19 in a2394cb

WaitPodReadyTimeout int

Does the WaitPodReadyTimeout here work? It feels like it can be removed. @jwcesign

jwcesign · 2023-03-01T06:38:52Z

Does the WaitPodReadyTimeout here work? It feels like it can be removed?

Looks like no place uses this, cc @lonelyCZ for checking

wuyingjun-lucky · 2023-03-02T05:59:42Z

What about move check.go to kamadactl/utils

yanfeng1992 · 2023-03-03T02:21:46Z

Can you confirm these two questions?

1.What about move check.go to kamadactl/utils

2.

karmada/pkg/karmadactl/addons/init/disable_option.go

Line 19 in a2394cb

WaitPodReadyTimeout int

Does the WaitPodReadyTimeout here work? It feels like it can be removed.

@lonelyCZ @wuyingjun-lucky @jwcesign

wuyingjun-lucky · 2023-03-07T04:49:18Z

Can you confirm these two questions?

1.What about move check.go to kamadactl/utils

karmada/pkg/karmadactl/addons/init/disable_option.go

Line 19 in a2394cb

WaitPodReadyTimeout int

Does the WaitPodReadyTimeout here work? It feels like it can be removed.

@lonelyCZ @wuyingjun-lucky @jwcesign

1 sorry my mistake. I mean kamadactl/util
2 Yes. we can clear it up.

yanfeng1992 · 2023-03-07T06:45:08Z

Can you confirm these two questions?
1.What about move check.go to kamadactl/utils

karmada/pkg/karmadactl/addons/init/disable_option.go

Line 19 in a2394cb

WaitPodReadyTimeout int

Does the WaitPodReadyTimeout here work? It feels like it can be removed.
@lonelyCZ @wuyingjun-lucky @jwcesign

1 sorry my mistake. I mean kamadactl/util 2 Yes. we can clear it up.

@wuyingjun-lucky I solved these two problems

wuyingjun-lucky · 2023-03-08T01:20:53Z

LGTM
/cc @lonelyCZ
Do you have tests for these two commands ？ @yanfeng1992

yanfeng1992 · 2023-03-14T03:23:39Z

Please have a look @lonelyCZ

lonelyCZ · 2023-03-14T03:35:17Z

I'm sorry I missed that. I will look it ASAP.

/assign

pkg/karmadactl/util/check.go

jwcesign · 2023-03-15T08:21:11Z

/lgtm

lonelyCZ · 2023-03-15T14:56:39Z

Thanks @yanfeng1992 , I just tested it that worked fine.

cc @RainbowMango

RainbowMango · 2023-03-16T01:21:04Z

/assign

RainbowMango

It makes sense to me that wait for StatefulSet and Deployment ready instead of the Pod. It's more straightforward.

I suggest letting this PR focus on using WaitForStatefulsetRollout and WaitForDeploymentRollout to replace WaitPodReady, and don't touch any flags.

Please send another PR to handle the flags particularly if you think it needs to, so that we can add more clear release notes to users.

pkg/karmadactl/util/check.go

yanfeng1992 · 2023-03-16T07:40:16Z

@RainbowMango @lonelyCZ Please take a look at the latest commit

RainbowMango

Only some nits, otherwise LGTM.
Thanks.

pkg/karmadactl/util/check.go

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

RainbowMango

/lgtm
/approve
Thanks for your quick response.

karmada-bot · 2023-03-16T08:54:23Z

[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/karmadactl/OWNERS~~ [RainbowMango]

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

karmada-bot added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Feb 28, 2023

karmada-bot requested review from lonelyCZ, prodanlabs and wuyingjun-lucky February 28, 2023 07:41

karmada-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 28, 2023

yanfeng1992 force-pushed the wait-dep-sts-rollout branch 3 times, most recently from b7b0116 to 2da8c8a Compare February 28, 2023 08:57

jwcesign reviewed Mar 1, 2023

View reviewed changes

yanfeng1992 force-pushed the wait-dep-sts-rollout branch from 2da8c8a to 8728681 Compare March 1, 2023 05:34

yanfeng1992 commented Mar 1, 2023

View reviewed changes

pkg/karmadactl/cmdinit/options/global.go Show resolved Hide resolved

yanfeng1992 commented Mar 1, 2023

View reviewed changes

pkg/karmadactl/addons/enable.go Outdated Show resolved Hide resolved

yanfeng1992 force-pushed the wait-dep-sts-rollout branch from 8728681 to eacb416 Compare March 7, 2023 05:34

karmada-bot assigned lonelyCZ Mar 14, 2023

lonelyCZ reviewed Mar 14, 2023

View reviewed changes

pkg/karmadactl/util/check.go Outdated Show resolved Hide resolved

yanfeng1992 force-pushed the wait-dep-sts-rollout branch 2 times, most recently from 29e9d9e to 51c13c5 Compare March 15, 2023 02:21

jwcesign reviewed Mar 15, 2023

View reviewed changes

pkg/karmadactl/util/check.go Show resolved Hide resolved

pkg/karmadactl/util/check.go Outdated Show resolved Hide resolved

yanfeng1992 force-pushed the wait-dep-sts-rollout branch from 51c13c5 to d8f70b2 Compare March 15, 2023 06:52

karmada-bot assigned jwcesign Mar 15, 2023

karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 15, 2023

karmada-bot assigned RainbowMango Mar 16, 2023

RainbowMango reviewed Mar 16, 2023

View reviewed changes

yanfeng1992 force-pushed the wait-dep-sts-rollout branch from d8f70b2 to dfae395 Compare March 16, 2023 06:10

karmada-bot removed the lgtm Indicates that a PR is ready to be merged. label Mar 16, 2023

RainbowMango reviewed Mar 16, 2023

View reviewed changes

pkg/karmadactl/util/check.go Outdated Show resolved Hide resolved

pkg/karmadactl/util/check.go Outdated Show resolved Hide resolved

check deployment statefulset rollout not use check pod ready

fd062ce

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

yanfeng1992 force-pushed the wait-dep-sts-rollout branch from dfae395 to fd062ce Compare March 16, 2023 08:40

RainbowMango approved these changes Mar 16, 2023

View reviewed changes

karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 16, 2023

karmada-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 16, 2023

karmada-bot merged commit 7f5bb9d into karmada-io:master Mar 16, 2023

lonelyCZ mentioned this pull request Mar 19, 2023

feat: add flags of force for addon disable #3266

Merged

yanfeng1992 mentioned this pull request Mar 30, 2023

REQUEST: New membership for yanfeng1992 karmada-io/community#26

Closed

8 tasks

RainbowMango mentioned this pull request Apr 25, 2023

Optimization WaitForStatefulSetRollout and WaitForDeploymentRollout #3418

Open

yanfeng1992 deleted the wait-dep-sts-rollout branch April 2, 2024 12:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

check deployment statefulset rollout not use check pod ready #3221

check deployment statefulset rollout not use check pod ready #3221

yanfeng1992 commented Feb 28, 2023 •

edited by RainbowMango

Loading

codecov-commenter commented Feb 28, 2023 •

edited

Loading

calvin0327 commented Feb 28, 2023 •

edited

Loading

yanfeng1992 commented Feb 28, 2023

yanfeng1992 commented Mar 1, 2023

jwcesign commented Mar 1, 2023

wuyingjun-lucky commented Mar 2, 2023

yanfeng1992 commented Mar 3, 2023 •

edited

Loading

wuyingjun-lucky commented Mar 7, 2023 •

edited

Loading

yanfeng1992 commented Mar 7, 2023

wuyingjun-lucky commented Mar 8, 2023

yanfeng1992 commented Mar 14, 2023

lonelyCZ commented Mar 14, 2023

jwcesign commented Mar 15, 2023

lonelyCZ commented Mar 15, 2023

RainbowMango commented Mar 16, 2023

RainbowMango left a comment

yanfeng1992 commented Mar 16, 2023

RainbowMango left a comment

RainbowMango left a comment

karmada-bot commented Mar 16, 2023

check deployment statefulset rollout not use check pod ready #3221

check deployment statefulset rollout not use check pod ready #3221

Conversation

yanfeng1992 commented Feb 28, 2023 • edited by RainbowMango Loading

codecov-commenter commented Feb 28, 2023 • edited Loading

Codecov Report

calvin0327 commented Feb 28, 2023 • edited Loading

yanfeng1992 commented Feb 28, 2023

yanfeng1992 commented Mar 1, 2023

jwcesign commented Mar 1, 2023

wuyingjun-lucky commented Mar 2, 2023

yanfeng1992 commented Mar 3, 2023 • edited Loading

wuyingjun-lucky commented Mar 7, 2023 • edited Loading

yanfeng1992 commented Mar 7, 2023

wuyingjun-lucky commented Mar 8, 2023

yanfeng1992 commented Mar 14, 2023

lonelyCZ commented Mar 14, 2023

jwcesign commented Mar 15, 2023

lonelyCZ commented Mar 15, 2023

RainbowMango commented Mar 16, 2023

RainbowMango left a comment

Choose a reason for hiding this comment

yanfeng1992 commented Mar 16, 2023

RainbowMango left a comment

Choose a reason for hiding this comment

RainbowMango left a comment

Choose a reason for hiding this comment

karmada-bot commented Mar 16, 2023

yanfeng1992 commented Feb 28, 2023 •

edited by RainbowMango

Loading

codecov-commenter commented Feb 28, 2023 •

edited

Loading

calvin0327 commented Feb 28, 2023 •

edited

Loading

yanfeng1992 commented Mar 3, 2023 •

edited

Loading

wuyingjun-lucky commented Mar 7, 2023 •

edited

Loading