Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimization WaitForStatefulSetRollout and WaitForDeploymentRollout #3418

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

helen-frank
Copy link
Contributor

What type of PR is this?
/kind feature

What this PR does / why we need it:

  1. use watch
  2. use kubernetes-sigs/application judge status

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

@karmada-bot karmada-bot added the kind/feature Categorizes issue or PR as related to a new feature. label Apr 17, 2023
@karmada-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign lonelycz after the PR has been reviewed.
You can assign the PR to them by writing /assign @lonelycz 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:

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

@karmada-bot karmada-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 17, 2023
@helen-frank
Copy link
Contributor Author

/retest

@helen-frank helen-frank force-pushed the feature/WaitUseWatch branch 2 times, most recently from 934bdf9 to 9a386a2 Compare April 17, 2023 15:16
@helen-frank
Copy link
Contributor Author

/ok-to-test
/retest

@helen-frank helen-frank force-pushed the feature/WaitUseWatch branch 6 times, most recently from 4aba2ef to 04f9a70 Compare April 18, 2023 08:53
@codecov-commenter
Copy link

codecov-commenter commented Apr 18, 2023

Codecov Report

Merging #3418 (565eeed) into master (dff7a5b) will decrease coverage by 0.11%.
The diff coverage is 1.03%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master    #3418      +/-   ##
==========================================
- Coverage   55.22%   55.12%   -0.11%     
==========================================
  Files         221      221              
  Lines       20831    20870      +39     
==========================================
  Hits        11504    11504              
- Misses       8717     8756      +39     
  Partials      610      610              
Flag Coverage Δ
unittests 55.12% <1.03%> (-0.11%) ⬇️

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

Impacted Files Coverage Δ
pkg/karmadactl/util/check.go 0.00% <0.00%> (ø)
pkg/karmadactl/cmdinit/kubernetes/deploy.go 41.01% <100.00%> (ø)

@helen-frank
Copy link
Contributor Author

/cc @RainbowMango @lonelyCZ

Copy link
Member

@RainbowMango RainbowMango left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain why we need to do this a little bit?

@helen-frank
Copy link
Contributor Author

Can you explain why we need to do this a little bit?

More accurate status determination and event-dependent waiting instead of violent polling

Copy link
Member

@RainbowMango RainbowMango left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More accurate status determination and event-dependent waiting instead of violent polling

Yes, agree, that's the benefit we can get from this. I'd like to invite @yanfeng1992 to take a look as he just optimized this piece of code at #3221.

pkg/karmadactl/util/check.go Show resolved Hide resolved
@helen-frank
Copy link
Contributor Author

/cc @yanfeng1992

@karmada-bot karmada-bot removed the lgtm Indicates that a PR is ready to be merged. label Jun 1, 2023
@karmada-bot
Copy link
Collaborator

New changes are detected. LGTM label has been removed.

@helen-frank
Copy link
Contributor Author

/retest

@helen-frank helen-frank force-pushed the feature/WaitUseWatch branch 6 times, most recently from 2480ad0 to b0c0104 Compare June 1, 2023 05:51
@RainbowMango
Copy link
Member

I'm looking at the CLI/init tests. Don't worry about that.

@helen-frank
Copy link
Contributor Author

I'm looking at the CLI/init tests. Don't worry about that.

I previously looked at the updated code is relatively simple, did not do the test in their own environment
I self-tested and found that it would report an error "error: prepare karmada failed. failed download file. url: https://github.com/karmada-io/karmada/releases/download/v1.7.0/crds. tar.gz code: 404"
The reason is

// ParseGitVersion parses a git version string, such as:
// - v1.1.0-73-g7e6d4f69
// - v1.1.0
func ParseGitVersion(gitVersion string) (*ReleaseVersion, error) {
v, err := utilversion.ParseGeneric(gitVersion)
if err != nil {
return nil, err
}
return &ReleaseVersion{
Version: v,
}, nil
}

But I see that cicd is using the local crds.tar.gz

@RainbowMango
Copy link
Member

I self-tested and found that it would report an error "error: prepare karmada failed. failed download file.

Yes, because I tagged a pre-release [v1.7.0-alpha.1](https://github.com/karmada-io/karmada/releases/tag/v1.7.0-alpha.1) this morning, init will use the v1.7.0 as the default version. I'm thinking about how to deal with this.
cc @jwcesign who also looking at this issue.

By the way. I just disabled the CLI/init workflow temporarily, so that it won't block the following PRs.

@helen-frank
Copy link
Contributor Author

I self-tested and found that it would report an error "error: prepare karmada failed. failed download file.

Yes, because I tagged a pre-release [v1.7.0-alpha.1](https://github.com/karmada-io/karmada/releases/tag/v1.7.0-alpha.1) this morning, init will use the v1.7.0 as the default version. I'm thinking about how to deal with this. cc @jwcesign who also looking at this issue.

By the way. I just disabled the CLI/init workflow temporarily, so that it won't block the following PRs.

I found out that the reason for waiting for APIService is that karmada-aggregated-apiserver uses v1.7.0, so it can't pull the image and apiservice v1alpha1.cluster.karmada.io is not ready.

By the way request to merge #3415 to reduce the log volume

@jwcesign
Copy link
Member

jwcesign commented Jun 1, 2023

I found out that the reason for waiting for APIService is that karmada-aggregated-apiserver uses v1.7.0, so it can't pull the image and apiservice v1alpha1.cluster.karmada.io is not ready.

The main reason is that when building karmadactl, the latest tag is used as the image version. However, when parsing the v1.7.0-alpha.1 tag, it results in a parsed version of v1.7.0, which is not what was expected.

@helen-frank
Copy link
Contributor Author

helen-frank commented Jun 1, 2023

An idea: use the committed code to build images and crds.tar.gz for testing? @RainbowMango

@jwcesign
Copy link
Member

jwcesign commented Jun 1, 2023

An idea: use the committed code to build images and crds.tar.gz for testing?

It makes sense, now the crd is from the committed code, but images are not.

@helen-frank helen-frank force-pushed the feature/WaitUseWatch branch 2 times, most recently from ac692ed to 0332a74 Compare June 3, 2023 02:13
@yanfeng1992 yanfeng1992 removed their assignment Jun 8, 2023
@helen-frank
Copy link
Contributor Author

/cc @jwcesign @lonelyCZ

@helen-frank
Copy link
Contributor Author

ping @RainbowMango

Copy link
Member

@RainbowMango RainbowMango left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this will conflict with #3665, I'll look at it after #3665 get merged.

…Use kubernetes-sigs/application judge status

Signed-off-by: helen <haitao.zhang@daocloud.io>
Copy link
Member

@RainbowMango RainbowMango left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm hesitant to move this forward because it lacks enough benefits.

@helen-frank
Copy link
Contributor Author

I'm hesitant to move this forward because it lacks enough benefits.

My idea is to use as many kubernetes mechanisms as possible to make code more cloud-native and faster. And using watch is cooler than wait.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. ok-to-test size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants