nvidia-container-toolkit: only mount existing paths in the host #319772
Conversation
|
@ereslibre may I ask you to instead consider whether it's possible to write a NixOS test covering all these symptoms we've had to address so far? E.g. maybe we could manually prepare a fake CDI json, and create temporary fake files in place of device nodes and userspace drivers, and then run a podman/docker container with a script ensuring that all of these fake paths are mounted correctly, including referential integrity ( This would be very useful for our migration to |
I think this is a good idea. I have never written NixOS tests but I will take a look at existing ones and update the PR. |
|
@ereslibre actually, I now think I shouldn't have delayed merging this; we'll still need the tests though |
nixos/modules/services/hardware/nvidia-container-toolkit/cdi-generate.nix
Outdated
Show resolved
Hide resolved
Tests are WIP (ereslibre/nixpkgs@nvidia-ctk-mount-existing-paths-only...ereslibre:nixpkgs:nvidia-ctk-mount-existing-paths-only-wip); I'd like to finish them before 29th june. Otherwise I fear I'll have to resume second half of July. The "referential integrity" at this time is pretty "poor man"; not sure if you had any other approach in mind. |
e86c013
to
8d52c10
Compare
nixos/modules/services/hardware/nvidia-container-toolkit/cdi-generate.nix
Show resolved
Hide resolved
| @@ -32,6 +35,18 @@ function cdiGenerate { | |||
| --nvidia-ctk-path ${lib.getExe' nvidia-container-toolkit "nvidia-ctk"} | |||
| } | |||
|
|
|||
| cdiGenerate | \ | |||
| ${lib.concatStringsSep " | " allJqMounts} > $RUNTIME_DIRECTORY/nvidia-container-toolkit.json | |||
| function additionalMount { | |||
There was a problem hiding this comment.
Well one comment is that this could be rewritten with pythonMinimal or a similar language that doesn't try so much to make this painful (you could remove concatMapStringsSep and just export a json for the script)
There was a problem hiding this comment.
But as commented before, if you say this is the current iteration I'll go with it
There was a problem hiding this comment.
Yes, I'll work on a reimplementation of this logic with Python in future PR's, if that's fine with you.
d223970
to
8e19362
Compare
b3ac4d8
to
9d382b5
Compare
9d382b5
to
d665ca4
Compare
|
@SomeoneSerge: is this good to merge on its current status? |
| open = true; | ||
| package = config.boot.kernelPackages.nvidiaPackages.stable.open; |
There was a problem hiding this comment.
Ehhh it's stupid that open doesn't change the default to... an open driver
…us from the context
| open = true; | ||
| package = config.boot.kernelPackages.nvidiaPackages.stable.open; | ||
| }; | ||
| opengl.enable = true; |
There was a problem hiding this comment.
I don't understand why Ofborg isn't complaining, we have a mkRenamedModule for this one
|
I pushed a few cosmetic changes, tell me if they're ok by you |
Thank you @SomeoneSerge! LGTM :) |
Description of changes
Although the final fix for this kind of issues is #290609 (
unbreak: we're currently mounting incomplete closures; need to use exportReferencesGraph.), this adds yet another fix for a symptom.Fixes: #319201
I will address the
exportReferencesGraphsoon that I expect will fix all these problems once and for all, but fix this problem in the meantime.Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.