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

feat(tide): support tide nodepool #387

Merged
merged 2 commits into from
Dec 20, 2023
Merged

Conversation

xjh1996
Copy link
Contributor

@xjh1996 xjh1996 commented Nov 30, 2023

What type of PR is this?

#213

What this PR does / why we need it:

Support tide node pool in katalyst. When a workload is scaled down, the vacant nodes will be identified and assigned to another workload that is experiencing higher demand. This will facilitate the efficient utilization of nodes and prevent resource wastage.

Which issue(s) this PR fixes:

Special notes for your reviewer:

Copy link

codecov bot commented Nov 30, 2023

Codecov Report

Attention: 541 lines in your changes are missing coverage. Please review.

Comparison is base (871cdd7) 53.88% compared to head (44eca05) 53.69%.
Report is 20 commits behind head on main.

Files Patch % Lines
pkg/controller/tide/tide.go 41.97% 255 Missing and 45 partials ⚠️
...ce/models/borwein/inferencesvc/inference_svc.pb.go 48.16% 95 Missing and 18 partials ⚠️
...server/agent/metric/malachite/fetcher_calculate.go 65.68% 23 Missing and 12 partials ⚠️
pkg/metaserver/agent/metric/malachite/fetcher.go 67.50% 20 Missing and 6 partials ⚠️
pkg/controller/tide/nodepool_wrapper.go 80.73% 18 Missing and 3 partials ⚠️
...tions/sysadvisor/qosaware/model/borwein/borwein.go 38.46% 14 Missing and 2 partials ⚠️
pkg/util/general/file.go 0.00% 10 Missing ⚠️
...in/inference/modelresultfetcher/borwein/borwein.go 67.85% 6 Missing and 3 partials ⚠️
...aware/resource/helper/modelctrl/borwein/borwein.go 74.19% 4 Missing and 4 partials ⚠️
pkg/agent/sysadvisor/plugin/inference/inference.go 0.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #387      +/-   ##
==========================================
- Coverage   53.88%   53.69%   -0.19%     
==========================================
  Files         445      448       +3     
  Lines       49054    50121    +1067     
==========================================
+ Hits        26432    26912     +480     
- Misses      19656    20156     +500     
- Partials     2966     3053      +87     
Flag Coverage Δ
unittest 53.69% <52.29%> (-0.19%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@xjh1996 xjh1996 force-pushed the feat-tide branch 2 times, most recently from 00a0bf0 to 3f4ec51 Compare November 30, 2023 11:53
@waynepeking348 waynepeking348 self-assigned this Dec 1, 2023
@waynepeking348 waynepeking348 added the enhancement New feature or request label Dec 1, 2023
@waynepeking348
Copy link
Collaborator

you should add enough ut logic for this pr @xjh1996

@xjh1996 xjh1996 force-pushed the feat-tide branch 5 times, most recently from 92eadaa to 6afd1db Compare December 11, 2023 08:17
k8s.io/client-go v0.24.6
k8s.io/component-base v0.24.6
k8s.io/component-helpers v0.24.6
k8s.io/api v0.24.16
Copy link
Collaborator

Choose a reason for hiding this comment

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

is it inevitable to upgrade go mod dependency for k8s-related repo?

Copy link
Collaborator

Choose a reason for hiding this comment

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

mainly considered for stability, prefer not to do it if possible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is triggered by the necessary dependency cluster-autoscaler. At the bottom of the go mod, there is a replace directive. Theoretically, the version used is still v0.24.6.

@waynepeking348 waynepeking348 added the workflow/need-review review: test succeeded, need to review label Dec 12, 2023
waynepeking348
waynepeking348 previously approved these changes Dec 18, 2023
@waynepeking348
Copy link
Collaborator

lgtm @caohe pls have a look for this pr

@waynepeking348 waynepeking348 added the workflow/merge-ready merge-ready: code is ready and can be merged label Dec 18, 2023
return nodeUsageList, nil
}

// clearUnexpectedCNR is used to clear unexpected cnr
Copy link
Member

Choose a reason for hiding this comment

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

It seems that this comment may be not relevant to this method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@caohe
Copy link
Member

caohe commented Dec 19, 2023

Could you add more information to the description part?

@xjh1996
Copy link
Contributor Author

xjh1996 commented Dec 19, 2023

Could you add more information to the description part?
@caohe done

@waynepeking348 waynepeking348 merged commit 04e4909 into kubewharf:main Dec 20, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request workflow/merge-ready merge-ready: code is ready and can be merged workflow/need-review review: test succeeded, need to review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants