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 hj-johannes-lee, bart0sh, 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 Outdated 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
72 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gpu: copy nfdhook functionality to gpu-plugin #1492

gpu: copy nfdhook functionality to gpu-plugin #1492

tkatila commented Aug 4, 2023

codecov-commenter commented Aug 4, 2023 •

edited

Loading

eero-t commented Aug 21, 2023

tkatila commented Sep 5, 2023

mythi commented Sep 5, 2023

tkatila commented Sep 5, 2023

eero-t left a comment

tkatila commented Sep 7, 2023

mythi commented Sep 8, 2023

eero-t commented Sep 8, 2023

tkatila commented Sep 11, 2023

mythi commented Sep 12, 2023

gpu: copy nfdhook functionality to gpu-plugin #1492

gpu: copy nfdhook functionality to gpu-plugin #1492

Conversation

tkatila commented Aug 4, 2023

codecov-commenter commented Aug 4, 2023 • edited Loading

Codecov Report

eero-t commented Aug 21, 2023

tkatila commented Sep 5, 2023

mythi commented Sep 5, 2023

tkatila commented Sep 5, 2023

eero-t left a comment

Choose a reason for hiding this comment

tkatila commented Sep 7, 2023

mythi commented Sep 8, 2023

eero-t commented Sep 8, 2023

tkatila commented Sep 11, 2023

mythi commented Sep 12, 2023

codecov-commenter commented Aug 4, 2023 •

edited

Loading