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.

Sign up for GitHub

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

Jump to bottom

Precise execution according to waterline #216

Merged
merged 1 commit into from
Jun 20, 2022
Merged

Precise execution according to waterline #216

merged 1 commit into from
Jun 20, 2022

Conversation

chenkaiyue
Copy link
Contributor

@chenkaiyue chenkaiyue commented Mar 21, 2022 •
edited
Loading

What type of PR is this?

Feature

What this PR does / why we need it:

Add waterline for precise execution, select how many pods will be executed.

The sort logic will be put into executor module use with waterline.

Proposal: #357

Some things to complete

@chenkaiyue chenkaiyue changed the title [WIP]Add waterline for precise execution [WIP]Precise execution according to waterline Mar 21, 2022
@github-actions
Copy link
Contributor

github-actions bot commented May 8, 2022 •
edited
Loading

🎉 Successfully Build Images.

Overview: https://finops.coding.net/public-artifacts/gocrane/crane/packages

Image Pull Command
crane-agent:pr-216-e2d0b85 docker pull finops-docker.pkg.coding.net/gocrane/crane/crane-agent:pr-216-e2d0b85
dashboard:pr-216-e2d0b85 docker pull finops-docker.pkg.coding.net/gocrane/crane/dashboard:pr-216-e2d0b85
metric-adapter:pr-216-e2d0b85 docker pull finops-docker.pkg.coding.net/gocrane/crane/metric-adapter:pr-216-e2d0b85
craned:pr-216-e2d0b85 docker pull finops-docker.pkg.coding.net/gocrane/crane/craned:pr-216-e2d0b85

@chenkaiyue chenkaiyue changed the title [WIP]Precise execution according to waterline Precise execution according to waterline May 9, 2022
@yan234280533
Copy link

Please have a square for the commits.

@yan234280533
Copy link

What relationship between this PR and PR #201 ? Is this PR depends on PR #201 to be merged into the main branch?

yan234280533
yan234280533 reviewed May 9, 2022
View reviewed changes
pkg/ensurance/analyzer/analyzer.go Show resolved Hide resolved
pkg/utils/util.go Outdated Show resolved Hide resolved
pkg/utils/pod.go Show resolved Hide resolved
pkg/utils/pod.go Outdated Show resolved Hide resolved
pkg/resource/pod_resource_manger.go Show resolved Hide resolved
pkg/ensurance/executor/waterline_test.go Show resolved Hide resolved
yan234280533
yan234280533 reviewed May 9, 2022
View reviewed changes
pkg/ensurance/executor/waterline.go Outdated Show resolved Hide resolved
pkg/ensurance/executor/waterline.go Outdated Show resolved Hide resolved
pkg/ensurance/executor/waterline.go Outdated Show resolved Hide resolved
pkg/ensurance/executor/evict.go Show resolved Hide resolved
pkg/ensurance/executor/waterline.go Outdated Show resolved Hide resolved
pkg/ensurance/executor/waterline.go Outdated Show resolved Hide resolved
pkg/ensurance/executor/waterline.go Outdated Show resolved Hide resolved
pkg/ensurance/executor/waterline.go Outdated Show resolved Hide resolved
pkg/ensurance/executor/waterline.go Outdated
if !evictExist {
delete(eviceGapToWaterLines, string(m))
} else {
eviceGapToWaterLines[string(m)] = maxUsed - float64(evictWaterLine.PopSmallest().Value())
Copy link

Choose a reason for hiding this comment

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

Can we need to sort first of the throttleExecutor, we pop the first, but named smallest

@yan234280533
Copy link

Can we write a proposal, and reviewed the proposal first before reviewing this PR? I had confused by the GapToWaterLine and the sort.

@chenkaiyue chenkaiyue force-pushed the action-on-difference branch 4 times, most recently from 9586ee3 to 045b0c9 Compare May 24, 2022 09:24
@chenkaiyue chenkaiyue force-pushed the action-on-difference branch 6 times, most recently from 5decc43 to 4532054 Compare June 11, 2022 15:42
@chenkaiyue chenkaiyue mentioned this pull request Jun 13, 2022
Merged
@chenkaiyue
Copy link
Contributor Author

chenkaiyue commented Jun 13, 2022 •
edited
Loading

Test result for precisely evict.
企业微信截图_fce37134-caee-40fe-abfe-9d97df16ecb8

@chenkaiyue
Copy link
Contributor Author

chenkaiyue commented Jun 13, 2022 •
edited
Loading

Test result for precisely throttle down.
企业微信截图_f2a88d85-824e-4989-aae3-3d6eec2d75f0

@chenkaiyue
Copy link
Contributor Author

chenkaiyue commented Jun 13, 2022 •
edited
Loading

Test result for precisely throttle up.
企业微信截图_702767de-77e5-4d6c-ad8e-3e5ab7defb8a

@zsnmwy
Copy link
Member

zsnmwy commented Jun 13, 2022

How about writing some overview for these pics?
Makes the whole logic flow smoothly.

@chenkaiyue
Copy link
Contributor Author

How about writing some overview for these pics? Makes the whole logic flow smoothly.

Ok, these pics are test results, i have added some comments. This pr has a related proposal #357, thx~

jordy1024
jordy1024 reviewed Jun 16, 2022
View reviewed changes
Copy link

@jordy1024 jordy1024 left a comment

Choose a reason for hiding this comment

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

Hi,pull requests that are too big can be difficult to review effectively.
here are some suggestions to make code readable.

pkg/utils/util.go Outdated Show resolved Hide resolved
pkg/utils/pod.go Outdated Show resolved Hide resolved
pkg/ensurance/executor/waterline_test.go Show resolved Hide resolved
pkg/ensurance/executor/waterline.go Outdated Show resolved Hide resolved
pkg/ensurance/executor/waterline.go Show resolved Hide resolved
pkg/ensurance/analyzer/analyzer.go Outdated Show resolved Hide resolved
pkg/ensurance/analyzer/analyzer.go Outdated Show resolved Hide resolved
pkg/ensurance/analyzer/analyzer.go Outdated Show resolved Hide resolved
pkg/ensurance/executor/cpu_usage.go Outdated Show resolved Hide resolved
pkg/ensurance/executor/cpu_usage.go Show resolved Hide resolved
@chenkaiyue
Copy link
Contributor Author

Hi,pull requests that are too big can be difficult to review effectively. here are some suggestions to make code readable.

Thanks for your code review! There are so many helpful comments! I have fixed them. Thx!

@chenkaiyue chenkaiyue force-pushed the action-on-difference branch 2 times, most recently from c9fe84b to 60813ac Compare June 17, 2022 06:44
yan234280533
yan234280533 reviewed Jun 19, 2022
View reviewed changes
cmd/crane-agent/app/options/option.go Show resolved Hide resolved
cmd/crane-agent/app/options/option.go Show resolved Hide resolved
pkg/agent/agent.go Show resolved Hide resolved
pkg/ensurance/analyzer/analyzer.go Show resolved Hide resolved
pkg/ensurance/executor/waterline.go Outdated Show resolved Hide resolved
pkg/ensurance/executor/evict.go Show resolved Hide resolved
pkg/ensurance/executor/evict.go Outdated Show resolved Hide resolved
pkg/ensurance/executor/throttle.go Outdated Show resolved Hide resolved
pkg/ensurance/executor/throttle.go Outdated Show resolved Hide resolved
pkg/ensurance/executor/throttle.go Outdated Show resolved Hide resolved
@yan234280533
Copy link

Because of the complexity of this requirement, let's merge into the PR first, and we can continue to discuss it on the PR and propose a new PR for modification. Thanks very much to everyone.

@yan234280533
Copy link

/LGTM

@yan234280533 yan234280533 merged commit bc31728 into gocrane:main Jun 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jordy1024 jordy1024 jordy1024 left review comments

@yan234280533 yan234280533 yan234280533 left review comments

@mfanjie mfanjie Awaiting requested review from mfanjie

Assignees
No one assigned
Labels
kind/feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@chenkaiyue @yan234280533 @zsnmwy @jordy1024