gpu: copy nfdhook functionality to gpu-plugin #1492

tkatila · 2023-08-04T08:23:10Z

In order to support v0.14+ NFD and extended resources, write node labels into a file and store it for NFD's features.d directory.

The old functionality still exists in the initcontainer, but if it's used NFD has to be configured to allow it.

Fixes #1181

codecov-commenter · 2023-08-04T11:41:59Z

Codecov Report

Merging #1492 (9006fff) into main (8d3ef43) will decrease coverage by 0.52%.
Report is 16 commits behind head on main.
The diff coverage is 39.62%.

❗ Current head 9006fff differs from pull request most recent head 55e57e3. Consider uploading reports for the commit 55e57e3 to get more accurate results

@@            Coverage Diff             @@
##             main    #1492      +/-   ##
==========================================
- Coverage   50.04%   49.52%   -0.52%     
==========================================
  Files          43       42       -1     
  Lines        4884     4917      +33     
==========================================
- Hits         2444     2435       -9     
- Misses       2301     2341      +40     
- Partials      139      141       +2

Files Changed	Coverage Δ
cmd/gpu_plugin/rm/gpu_plugin_resource_manager.go	`69.88% <0.00%> (ø)`
cmd/internal/labeler/labeler.go	`63.63% <8.19%> (ø)`
cmd/gpu_plugin/gpu_plugin.go	`86.74% <50.00%> (-1.54%)`	⬇️
pkg/controllers/gpu/controller.go	`47.70% <94.11%> (+11.05%)`	⬆️

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

cmd/gpu_plugin/gpu_plugin.go

deployments/gpu_plugin/overlays/initcontainer/use-initcontainer.yaml

cmd/gpu_nfdhook/main.go

cmd/internal/labeler/labeler.go

cmd/gpu_plugin/gpu_plugin.go

cmd/internal/labeler/labeler.go

cmd/gpu_plugin/gpu_plugin.go

cmd/internal/labeler/labeler.go

eero-t · 2023-08-21T12:03:27Z

My main issues have been fixed (last two items are more of nit-picking), so I think it would be time for other(s) review this too. @uniemimu ?

cmd/gpu_plugin/gpu_plugin.go

tkatila · 2023-09-05T07:08:03Z

Changes in the PR:

debugfs access removed, replaced with NFD's rule
- Some deprecated labels removed in the process
Initcontainer not used anymore: support left in-place but deployments do not use it.
Ext resource labels are now added when resource management is enabled
- No additional cli argument
NFD feature file path is static

mythi · 2023-09-05T07:47:52Z

Changes in the PR:

LGTM.

* debugfs access removed, replaced with NFD's rule
  
  * Some deprecated labels removed in the process

Update the README table to match this change?

tkatila · 2023-09-05T08:03:58Z

Update the README table to match this change?

Yes, that's needed. It might make sense to add the labeling info to GPU plugin also. Or create a new .md to which both initcontainer and plugin points to.

eero-t

Reviewed other than the operator code (as I'm not familiar enough with that).

cmd/internal/labeler/labeler.go

deployments/nfd/overlays/node-feature-rules/platform-labeling-rules.yaml

cmd/internal/labeler/labeler.go

cmd/gpu_plugin/labels.md

cmd/internal/labeler/labeler.go

tkatila · 2023-09-07T13:23:06Z

PTAL

I'll squash the commits once approved.

mythi · 2023-09-08T07:34:45Z

LGTM

eero-t · 2023-09-08T09:27:25Z

FYI: we've had some internal discussion on how to best label things, so label rules & names have been changing a bit, but we should have now consensus on final ones (i.e. there will be one more small update to them with squash).

cmd/gpu_plugin/labels.md

tkatila · 2023-09-11T11:53:34Z

@hj-johannes-lee can you please review?

cmd/internal/labeler/labeler.go

mythi · 2023-09-12T08:02:59Z

@uniemimu this should be OK to merge I think

NFD v0.14+ doesn't support binary NFD hooks by default, so there is a need to move the label creation away from the GPU nfdhook. Move extended resource label creation to plugin, and drop labels that were already marked deprecated (platform_gen, media_version etc.). Drop init-container from deployment files and operator. It is still possible to use an initcontainer, but the default deployments do not support it. Signed-off-by: Tuomas Katila <[email protected]>

Signed-off-by: Tuomas Katila <[email protected]>

tkatila requested review from bart0sh, hj-johannes-lee, mythi and uniemimu as code owners August 4, 2023 08:23

tkatila force-pushed the gpu/labeling-integration-to-gpuplugin branch from bbc8d79 to 218a29d Compare August 4, 2023 10:58

tkatila requested a review from ozhuraki as a code owner August 4, 2023 10:58

eero-t reviewed Aug 14, 2023

View reviewed changes

tkatila force-pushed the gpu/labeling-integration-to-gpuplugin branch 2 times, most recently from c8a62e9 to 107d8a0 Compare August 21, 2023 06:24

eero-t reviewed Aug 21, 2023

View reviewed changes

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

eero-t reviewed Aug 21, 2023

View reviewed changes

cmd/internal/labeler/labeler.go Show resolved Hide resolved

tkatila force-pushed the gpu/labeling-integration-to-gpuplugin branch from 107d8a0 to 302a14b Compare August 21, 2023 12:59

mythi reviewed Aug 25, 2023

View reviewed changes

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

uniemimu previously approved these changes Aug 25, 2023

View reviewed changes

tkatila dismissed uniemimu’s stale review via fc959f6 September 5, 2023 06:58

tkatila force-pushed the gpu/labeling-integration-to-gpuplugin branch from 302a14b to fc959f6 Compare September 5, 2023 06:58

tkatila requested a review from kad as a code owner September 5, 2023 06:58

tkatila force-pushed the gpu/labeling-integration-to-gpuplugin branch from fc959f6 to 1511b06 Compare September 5, 2023 06:59

tkatila force-pushed the gpu/labeling-integration-to-gpuplugin branch from 1511b06 to 5532976 Compare September 5, 2023 07:10

eero-t requested changes Sep 5, 2023

View reviewed changes

tkatila force-pushed the gpu/labeling-integration-to-gpuplugin branch from d43b1f4 to b8dc72a Compare September 6, 2023 10:52

eero-t reviewed Sep 6, 2023

View reviewed changes

cmd/gpu_plugin/labels.md Outdated Show resolved Hide resolved

eero-t reviewed Sep 6, 2023

View reviewed changes

cmd/internal/labeler/labeler.go Show resolved Hide resolved

eero-t reviewed Sep 8, 2023

View reviewed changes

cmd/gpu_plugin/labels.md Outdated Show resolved Hide resolved

cmd/gpu_plugin/labels.md Outdated Show resolved Hide resolved

uniemimu previously approved these changes Sep 11, 2023

View reviewed changes

tkatila force-pushed the gpu/labeling-integration-to-gpuplugin branch from 55e57e3 to 4235e69 Compare September 11, 2023 10:43

eero-t reviewed Sep 11, 2023

View reviewed changes

cmd/internal/labeler/labeler.go Show resolved Hide resolved

mythi previously approved these changes Sep 12, 2023

View reviewed changes

tkatila dismissed stale reviews from mythi and uniemimu via 01d5042 September 12, 2023 10:43

tkatila force-pushed the gpu/labeling-integration-to-gpuplugin branch from 4235e69 to 01d5042 Compare September 12, 2023 10:43

tkatila added 3 commits September 12, 2023 14:21

nfd: add rules to label nodes with different GPUs

9b3b881

Signed-off-by: Tuomas Katila <[email protected]>

fix crash with rm when kubelet request timeouts

bd2a35d

Signed-off-by: Tuomas Katila <[email protected]>

tkatila force-pushed the gpu/labeling-integration-to-gpuplugin branch from 01d5042 to bd2a35d Compare September 12, 2023 11:22

mythi approved these changes Sep 12, 2023

View reviewed changes

uniemimu approved these changes Sep 12, 2023

View reviewed changes

uniemimu merged commit 827b9a0 into intel:main Sep 12, 2023

gpu: copy nfdhook functionality to gpu-plugin #1492

gpu: copy nfdhook functionality to gpu-plugin #1492

Uh oh!

Conversation

tkatila commented Aug 4, 2023

Uh oh!

codecov-commenter commented Aug 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eero-t commented Aug 21, 2023

Uh oh!

Uh oh!

tkatila commented Sep 5, 2023

Uh oh!

mythi commented Sep 5, 2023

Uh oh!

tkatila commented Sep 5, 2023

Uh oh!

eero-t left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tkatila commented Sep 7, 2023

Uh oh!

mythi commented Sep 8, 2023

Uh oh!

eero-t commented Sep 8, 2023

Uh oh!

Uh oh!

Uh oh!

tkatila commented Sep 11, 2023

Uh oh!

Uh oh!

mythi commented Sep 12, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-commenter commented Aug 4, 2023 •

edited

Loading