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

gpu_plugin: fractional resource management #638

Merged

rojkov merged 3 commits into intel:main from uniemimu:fractional

Jun 9, 2021

Contributor

uniemimu commented May 21, 2021 •

edited

Fractional GPU resource management feature

Signed-off-by: Ukri Niemimuukko ukri.niemimuukko@intel.com

uniemimu requested review from rojkov and mythi

May 21, 2021 13:34

rojkov reviewed

View reviewed changes

pkg/controllers/reconciler.go Outdated Show resolved Hide resolved

cmd/gpu_plugin/gpu_plugin_resource_manager.go Outdated Show resolved Hide resolved

uniemimu force-pushed the fractional branch from b936664 to 4c2c0aa Compare

May 21, 2021 18:48

rojkov reviewed

View reviewed changes

cmd/gpu_plugin/gpu_plugin.go Outdated Show resolved Hide resolved

cmd/gpu_plugin/gpu_plugin_resource_manager.go Outdated Show resolved Hide resolved

pkg/controllers/reconciler.go Outdated Show resolved Hide resolved

cmd/gpu_plugin/gpu_plugin_resource_manager.go Outdated Show resolved Hide resolved

cmd/gpu_plugin/gpu_plugin_resource_manager.go Outdated Show resolved Hide resolved

cmd/gpu_plugin/gpu_plugin_resource_manager.go Outdated Show resolved Hide resolved

uniemimu force-pushed the fractional branch from 4c2c0aa to c32cdac Compare

May 24, 2021 14:03

uniemimu requested a review from rojkov

May 24, 2021 14:11

codecov-commenter commented May 25, 2021 •

edited

Codecov Report

Merging #638 (96749b8) into main (47f5b34) will increase coverage by 1.33%.
The diff coverage is 75.13%.

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

@@            Coverage Diff             @@
##             main     #638      +/-   ##
==========================================
+ Coverage   57.61%   58.95%   +1.33%     
==========================================
  Files          31       32       +1     
  Lines        2192     2373     +181     
==========================================
+ Hits         1263     1399     +136     
- Misses        855      891      +36     
- Partials       74       83       +9

Impacted Files	Coverage Δ
pkg/deviceplugin/api.go	`26.31% <0.00%> (-1.47%)`	⬇️
pkg/deviceplugin/server.go	`81.94% <33.33%> (-2.23%)`	⬇️
pkg/deviceplugin/manager.go	`80.76% <50.00%> (-2.91%)`	⬇️
cmd/gpu_plugin/gpu_plugin.go	`83.67% <75.00%> (-1.33%)`	⬇️
cmd/gpu_plugin/rm/gpu_plugin_resource_manager.go	`77.92% <77.92%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 47f5b34...7739484. Read the comment docs.

rojkov reviewed

View reviewed changes

Contributor

rojkov left a comment

Looks good!

Maybe it would make sense to move deviceManager to a separate package and to import it in gpu_plugin. This way we could see clearly the interfaces that need to be tested.

pkg/deviceplugin/server.go Outdated Show resolved Hide resolved

cmd/gpu_plugin/gpu_plugin_resource_manager.go Outdated Show resolved Hide resolved

cmd/gpu_plugin/gpu_plugin_resource_manager.go Outdated Show resolved Hide resolved

cmd/gpu_plugin/gpu_plugin_resource_manager.go Outdated Show resolved Hide resolved

cmd/gpu_plugin/gpu_plugin_resource_manager.go Outdated Show resolved Hide resolved

cmd/gpu_plugin/gpu_plugin_resource_manager.go Outdated Show resolved Hide resolved

cmd/gpu_plugin/gpu_plugin_resource_manager.go Outdated Show resolved Hide resolved

cmd/gpu_plugin/gpu_plugin_resource_manager.go Outdated Show resolved Hide resolved

uniemimu force-pushed the fractional branch 4 times, most recently from 7ac40ec to d8fbc3f Compare

May 25, 2021 17:26

rojkov reviewed

View reviewed changes

Contributor

rojkov left a comment

Looks like unrelated VPU changes got sneaked into this PR.

pkg/deviceplugin/server.go Outdated Show resolved Hide resolved

cmd/gpu_plugin/rm/gpu_plugin_resource_manager.go Outdated Show resolved Hide resolved

cmd/gpu_plugin/rm/gpu_plugin_resource_manager.go Outdated Show resolved Hide resolved

cmd/gpu_plugin/rm/gpu_plugin_resource_manager.go Outdated Show resolved Hide resolved

cmd/gpu_plugin/rm/gpu_plugin_resource_manager.go Outdated Show resolved Hide resolved

cmd/gpu_plugin/rm/gpu_plugin_resource_manager.go Outdated Show resolved Hide resolved

cmd/gpu_plugin/rm/gpu_plugin_resource_manager.go Outdated Show resolved Hide resolved

cmd/gpu_plugin/rm/gpu_plugin_resource_manager.go Outdated Show resolved Hide resolved

cmd/gpu_plugin/rm/gpu_plugin_resource_manager.go Outdated Show resolved Hide resolved

cmd/gpu_plugin/gpu_plugin.go Outdated Show resolved Hide resolved

uniemimu force-pushed the fractional branch 3 times, most recently from 12f505e to 1150e33 Compare

May 26, 2021 16:06

rojkov reviewed

View reviewed changes

pkg/deviceplugin/api.go Outdated Show resolved Hide resolved

cmd/gpu_plugin/rm/gpu_plugin_resource_manager.go Outdated Show resolved Hide resolved

cmd/gpu_plugin/rm/gpu_plugin_resource_manager.go Outdated Show resolved Hide resolved

cmd/gpu_plugin/rm/gpu_plugin_resource_manager.go Outdated Show resolved Hide resolved

cmd/gpu_plugin/rm/gpu_plugin_resource_manager.go Outdated Show resolved Hide resolved

cmd/gpu_plugin/rm/gpu_plugin_resource_manager.go Outdated Show resolved Hide resolved

cmd/gpu_plugin/rm/gpu_plugin_resource_manager.go Outdated Show resolved Hide resolved

cmd/gpu_plugin/gpu_plugin.go Outdated Show resolved Hide resolved

cmd/gpu_plugin/rm/gpu_plugin_resource_manager.go Outdated Show resolved Hide resolved

cmd/gpu_plugin/rm/gpu_plugin_resource_manager.go Outdated Show resolved Hide resolved

uniemimu force-pushed the fractional branch from 1150e33 to 58f7702 Compare

May 27, 2021 09:22

rojkov reviewed

View reviewed changes

Contributor

rojkov left a comment

Looks good to me!

Now tests are missing. Also it would be nice to add a paragraph about the new optional interface to DEVEL.md.

cmd/gpu_plugin/rm/gpu_plugin_resource_manager.go Outdated Show resolved Hide resolved

uniemimu force-pushed the fractional branch from 58f7702 to 1220b1c Compare

May 27, 2021 12:22

mythi reviewed

View reviewed changes

Contributor

mythi left a comment

LGTM too, just a couple of questions and suggestions.

The server Allocate() modifications could go into its own commit.

cmd/gpu_plugin/rm/gpu_plugin_resource_manager.go Outdated Show resolved Hide resolved

cmd/gpu_plugin/gpu_plugin.go Outdated Show resolved Hide resolved

cmd/gpu_plugin/rm/gpu_plugin_resource_manager.go Outdated Show resolved Hide resolved

uniemimu force-pushed the fractional branch from 1220b1c to cd07a93 Compare

May 31, 2021 08:03

mythi reviewed

View reviewed changes

Contributor

mythi left a comment

LGTM now

uniemimu force-pushed the fractional branch from ef0b204 to 61070bf Compare

June 1, 2021 12:37

rojkov reviewed

View reviewed changes

cmd/gpu_plugin/gpu_plugin.go Outdated Show resolved Hide resolved

cmd/gpu_plugin/rm/gpu_plugin_resource_manager.go Outdated Show resolved Hide resolved

cmd/gpu_plugin/rm/gpu_plugin_resource_manager.go Outdated Show resolved Hide resolved

cmd/gpu_plugin/rm/gpu_plugin_resource_manager_test.go Outdated Show resolved Hide resolved

pkg/deviceplugin/api.go Outdated Show resolved Hide resolved

cmd/gpu_plugin/gpu_plugin.go Show resolved Hide resolved

uniemimu force-pushed the fractional branch 2 times, most recently from 2b99b8d to 81b3d36 Compare

June 2, 2021 12:52

rojkov reviewed

View reviewed changes

cmd/gpu_plugin/rm/gpu_plugin_resource_manager_test.go Outdated Show resolved Hide resolved

cmd/gpu_plugin/rm/gpu_plugin_resource_manager_test.go Outdated Show resolved Hide resolved

uniemimu force-pushed the fractional branch from 81b3d36 to c682aa0 Compare

June 2, 2021 16:49

uniemimu force-pushed the fractional branch 2 times, most recently from 8c88389 to a1037fd Compare

June 2, 2021 17:44

rojkov reviewed

View reviewed changes

Contributor

rojkov left a comment

Looks good!

Now pkg/deviceplugin needs to be covered. The coverage of this part should be as close to 100% as possible.

DEVEL.md Outdated Show resolved Hide resolved

uniemimu force-pushed the fractional branch from a1037fd to 93e3115 Compare

June 3, 2021 12:41

rojkov previously approved these changes

View reviewed changes

Contributor

rojkov left a comment

Thanks! Looks good to me. The PR can be squashed now.

uniemimu dismissed rojkov’s stale review via

ec1b556

June 4, 2021 08:37

uniemimu added 3 commits

June 4, 2021 13:06


          optional Allocator interface

518fb93

This adds a new optional Allocator interface which allows the
plugins to implement the whole Allocate functionality.

Signed-off-by: Ukri Niemimuukko <ukri.niemimuukko@intel.com>
Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@intel.com>


          gpu_plugin: fractional resource management

2c4d529

Fractional resource management feature

Signed-off-by: Ukri Niemimuukko <ukri.niemimuukko@intel.com>
Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@intel.com>


          update kubelet to match other 0.21.0 versions

f41ecf3

Signed-off-by: Ukri Niemimuukko <ukri.niemimuukko@intel.com>

uniemimu force-pushed the fractional branch from 7739484 to f41ecf3 Compare

June 4, 2021 10:07

uniemimu marked this pull request as ready for review

June 4, 2021 10:07

uniemimu requested review from bart0sh and kad as code owners

June 4, 2021 10:07

mythi approved these changes

View reviewed changes

rojkov approved these changes

View reviewed changes

rojkov merged commit 6aa1a47 into intel:main

uniemimu deleted the fractional branch

June 11, 2021 16:08

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

rojkov rojkov approved these changes

mythi mythi approved these changes

bart0sh Awaiting requested review from bart0sh bart0sh is a code owner

kad Awaiting requested review from kad kad is a code owner