fix: make sure nvidia utility and compute bins can be mounted into co… by delulu · Pull Request #3029 · Azure/aks-engine

delulu · 2020-04-06T13:31:38Z

Reason for Change:

Make sure nvidia utility and compute bins can be mounted into container as documented in nvidia-container-runtime readme.

Issue Fixed:

NVIDIA utility/compute bins are not mounted into containers when they're required in container env variable as NVIDIA_DRIVER_CAPABILITIES=compute,utility

Requirements:

uses conventional commit messages
includes documentation
adds unit tests
tested upgrade from previous version

Notes:

welcome · 2020-04-06T13:31:40Z

💖 Thanks for opening your first pull request! 💖 We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should make sure your first commit and PR title start with a semantic prefix. Examples of commit messages with semantic prefixes: - fix: change azure disk cachingMode to ReadOnly - feat: make maximumLoadBalancerRuleCount configurable - docs: add note on AKS Engine and AKS relationship
Make sure to check out the developer guide for guidance on testing your change.

delulu · 2020-04-06T13:32:24Z

more details can be found from #2837

mboersma · 2020-04-06T15:51:29Z

/azp run pr-e2e

azure-pipelines · 2020-04-06T15:51:39Z

Azure Pipelines successfully started running 1 pipeline(s).

mboersma · 2020-04-06T16:26:23Z

@delulu could you run make generate and then commit and push the code that creates? You can make sure your PR will pass this check by running make test-style test locally.

jackfrancis · 2020-04-06T18:25:39Z

parts/k8s/cloud-init/artifacts/cse_config.sh

  retrycmd_if_failure 120 5 25 mount -t overlay -o lowerdir=/usr/lib/x86_64-linux-gnu,upperdir=${GPU_DEST}/lib64,workdir=${GPU_DEST}/overlay-workdir none /usr/lib/x86_64-linux-gnu || exit {{GetCSEErrorCode "ERR_GPU_DRIVERS_CONFIG"}}
  retrycmd_if_failure 3 1 600 sh $GPU_DEST/nvidia-drivers-$GPU_DV --silent --accept-license --no-drm --dkms --utility-prefix="${GPU_DEST}" --opengl-prefix="${GPU_DEST}" || exit {{GetCSEErrorCode "ERR_GPU_DRIVERS_START_FAIL"}}
  echo "${GPU_DEST}/lib64" >/etc/ld.so.conf.d/nvidia.conf
+  cp "${GPU_DEST}/bin" /usr/bin


What is special about /usr/bin as opposed to /usr/local/nvidia/bin?

nvidia-container-runtime and nvidia-container-cli bins are located in /usr/bin.

It seems it will only search nvidia utility/compute bins in the same folder and then mounted them accordingly into container. Because I've tried to add /usr/local/nvidia into system path, and it does not work as expected.

I also opened an issue NVIDIA/nvidia-docker#1226, but there's no response yet.

So this's a quick fix for this issue.

delulu · 2020-04-07T03:02:15Z

@delulu could you run make generate and then commit and push the code that creates? You can make sure your PR will pass this check by running make test-style test locally.

I tried, but there's some unexpected issue as shown below, maybe some more settings are required to setup local box. So could you help run it at your side, thx!

delzh@MININT-DELU:/mnt/e/git/aks-engine$ make test-style test

==> Running Go linter <==
golangci-lint has version 1.23.7 built from 96964db on 2020-02-28T12:07:44Z
==> Running shell linter <==
ShellCheck - shell script analysis tool
version: 0.3.7
license: GNU Affero General Public License, version 3
website: http://www.shellcheck.net

In ./parts/k8s/cloud-init/artifacts/cse_customcloud.sh line 63:
      jq --arg K8S_CLIENT_CERT_PATH ${K8S_CLIENT_CERT_PATH} '. + {aadClientCertPath:($K8S_CLIENT_CERT_PATH)}' |
                                                            ^-- SC2016: Expressions don't expand in single quotes, use double quotes for that.
...
Makefile:79: recipe for target 'validate-shell' failed
make: *** [validate-shell] Error 1

msftclas · 2020-04-07T16:12:06Z

All CLA requirements met.

mboersma · 2020-04-07T16:15:03Z

@delulu no problem, I did make generate and pushed a commit.

I think the make test-style failure was because we assume shellcheck 0.7.0 but you have version 0.3.7. You can do make dev to run commands from our toolbox container, which has that version, but we don't have a way to enforce tool versions on workstations in general.

jackfrancis · 2020-04-08T16:25:29Z

/azp run pr-e2e

azure-pipelines · 2020-04-08T16:25:39Z

Azure Pipelines successfully started running 1 pipeline(s).

jackfrancis · 2020-04-08T16:26:51Z

parts/k8s/cloud-init/artifacts/cse_config.sh

  retrycmd_if_failure 120 5 25 mount -t overlay -o lowerdir=/usr/lib/x86_64-linux-gnu,upperdir=${GPU_DEST}/lib64,workdir=${GPU_DEST}/overlay-workdir none /usr/lib/x86_64-linux-gnu || exit {{GetCSEErrorCode "ERR_GPU_DRIVERS_CONFIG"}}
  retrycmd_if_failure 3 1 600 sh $GPU_DEST/nvidia-drivers-$GPU_DV --silent --accept-license --no-drm --dkms --utility-prefix="${GPU_DEST}" --opengl-prefix="${GPU_DEST}" || exit {{GetCSEErrorCode "ERR_GPU_DRIVERS_START_FAIL"}}
  echo "${GPU_DEST}/lib64" >/etc/ld.so.conf.d/nvidia.conf
+  cp "${GPU_DEST}/bin/*" /usr/bin


This seems to be creating a second copy of the file, should we create a symlink at /usr/bin/nvidia-smi to /usr/local/nvidia/bin/nvidia-smi instead?

Actually we can mv "${GPU_DEST}/bin/*" /usr/bin if you has this concern, since utility libraries have been added to system library path with echo "${GPU_DEST}/lib64" >/etc/ld.so.conf.d/nvidia.conf.

It will work with a symlink, but there're also other utility bins and compute bins in ${GPU_DEST}/bin/*, they also might be required in gpu containers, and I would like to keep the change small to not introduce any potential issue.

mv works for me, thanks!

codecov · 2020-04-08T16:38:43Z

Codecov Report

Merging #3029 into master will not change coverage by %.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #3029   +/-   ##
=======================================
  Coverage   70.63%   70.63%           
=======================================
  Files         145      145           
  Lines       25151    25151           
=======================================
  Hits        17765    17765           
  Misses       6283     6283           
  Partials     1103     1103

Impacted Files	Coverage Δ
pkg/engine/templates_generated.go	`31.57% <ø> (ø)`

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 5218a91...ec926bb. Read the comment docs.

…ntainer

delulu · 2020-04-10T13:34:25Z

updated per comments, please have a further check.

delulu · 2020-04-10T13:34:48Z

/azp run pr-e2e

azure-pipelines · 2020-04-10T13:34:53Z

Commenter does not have sufficient privileges for PR 3029 in repo Azure/aks-engine

mboersma · 2020-04-10T14:49:46Z

/azp run pr-e2e

azure-pipelines · 2020-04-10T14:50:14Z

Azure Pipelines successfully started running 1 pipeline(s).

jackfrancis

/lgtm

jackfrancis · 2020-04-10T21:00:46Z

@yangl900 @andyliuliming FYI

welcome · 2020-04-10T22:21:52Z

Congrats on merging your first pull request! 🎉🎉🎉

jackfrancis reviewed Apr 6, 2020

View reviewed changes

delulu force-pushed the nvidia branch from cb8f29c to e5f7176 Compare April 7, 2020 01:36

jackfrancis reviewed Apr 8, 2020

View reviewed changes

delulu added 2 commits April 10, 2020 13:22

fix: make sure nvidia utility and compute bins can be mounted into co…

21d10a4

…ntainer

chore: update generated files

ec926bb

delulu force-pushed the nvidia branch from 0e2cd1b to ec926bb Compare April 10, 2020 13:32

jackfrancis approved these changes Apr 10, 2020

View reviewed changes

jackfrancis merged commit 8fb97a2 into Azure:master Apr 10, 2020

fmotrifork mentioned this pull request Apr 14, 2020

aks-engine 0.49.0 fishworks/fish-food#678

Merged

This was referenced Jun 20, 2020

Make sure nvidia utility and compute bins can be mounted into container Azure/AgentBaker#131

Merged

fix: Make sure nvidia utility and compute bins can be mounted into container #3513

Merged

Conversation

delulu commented Apr 6, 2020

Uh oh!

welcome bot commented Apr 6, 2020

Uh oh!

delulu commented Apr 6, 2020

Uh oh!

mboersma commented Apr 6, 2020

Uh oh!

azure-pipelines bot commented Apr 6, 2020

Uh oh!

mboersma commented Apr 6, 2020

Uh oh!

jackfrancis Apr 6, 2020

Choose a reason for hiding this comment

Uh oh!

delulu Apr 7, 2020

Choose a reason for hiding this comment

Uh oh!

delulu commented Apr 7, 2020

Uh oh!

msftclas commented Apr 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mboersma commented Apr 7, 2020

Uh oh!

jackfrancis commented Apr 8, 2020

Uh oh!

azure-pipelines bot commented Apr 8, 2020

Uh oh!

jackfrancis Apr 8, 2020

Choose a reason for hiding this comment

Uh oh!

delulu Apr 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jackfrancis Apr 9, 2020

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Apr 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

delulu commented Apr 10, 2020

Uh oh!

delulu commented Apr 10, 2020

Uh oh!

azure-pipelines bot commented Apr 10, 2020

Uh oh!

mboersma commented Apr 10, 2020

Uh oh!

azure-pipelines bot commented Apr 10, 2020

Uh oh!

jackfrancis left a comment

Choose a reason for hiding this comment

Uh oh!

jackfrancis commented Apr 10, 2020

Uh oh!

welcome bot commented Apr 10, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

msftclas commented Apr 7, 2020 •

edited

Loading

delulu Apr 9, 2020 •

edited

Loading

codecov bot commented Apr 8, 2020 •

edited

Loading