Allow specifying the variant for the ubuntu drivers

[guenhter]

For ubuntu machines there exists "normal" and a "server" nvidia packages. Up to now, the -server variant was the only supported one. This PR makes it configurable if the -server or non-server is used.

[guenhter]

On a second thought, I don't find it very pretty how my solution is implemented.

Maybe something like this is better:

nvidia_driver_ubuntu_packages_variant: server
nvidia_driver_ubuntu_packages:
    server: 
        - "nvidia-headless-{{ nvidia_driver_ubuntu_branch }}-server"
        - "nvidia-utils-{{ nvidia_driver_ubuntu_branch }}-server"
        - "nvidia-headless-no-dkms-{{ nvidia_driver_ubuntu_branch }}-server"
        - "nvidia-kernel-source-{{ nvidia_driver_ubuntu_branch }}-server"
    non-server: 
        - "nvidia-headless-{{ nvidia_driver_ubuntu_branch }}"
        - "nvidia-utils-{{ nvidia_driver_ubuntu_branch }}"
        - "nvidia-headless-no-dkms-{{ nvidia_driver_ubuntu_branch }}"
        - "nvidia-kernel-source-{{ nvidia_driver_ubuntu_branch }}"

What variant do you prefer?

[ajdecon]

What variant do you prefer?

I don't have a strong preference here. I agree your second variant is prettier from a code perspective. But your first variant feels slightly more future-proof in the case if any future packages are introduced with a different suffix. A user could just configure nvidia_driver_variant_ubuntu as a different string without having to make changes to nvidia_driver_ubuntu_packages.

(I have no idea if there will ever be other variant package names, though! 😉 )

Either of these is fine with me and I'd be happy to merge, assuming passing tests.

[ajdecon]

Speaking of tests... CI tests for CentOS 7 and Ubuntu installs from the Canonical repos are currently failing on this PR. In all the cases the failure is due to a failure to correctly parse the unit file for nvidia-persistenced. Sample error:

[centos-7]: FAILED! => {"changed": false, "msg": "Error loading unit file 'nvidia-persistenced': org.freedesktop.DBus.Error.InvalidArgs \"Invalid argument\""}

Looks like that first line ExecStart= may have been necessary after all? I'm not sure why and I'll investigate that, but for now you may want to revert your change to that file.

[guenhter]

Thx. I will work this in accordingly. One question to the tests: Do you want me to add a molecule test for server/noserver for every target platform or just one?

[ajdecon]

Do you want me to add a molecule test for server/noserver for every target platform or just one?

I think just one target platform is fine, just so that we're testing the relevant code path. I'd suggest using Ubuntu 20.04 since that's our most commonly-used platform right now.

[guenhter]

I'll wait for the other PR to be merged so that I can then rebase on top of it.

[guenhter]

@ajdecon This one is also ready and the Molecule test for 20.04 (noserver) is also there now.

[ajdecon]

Thanks @guenhter , can you also sign-off this commit per the contributing guide?

[guenhter]

I forget this every time. Thx for making me aware. It is signed off now.

[noaho]

What about being able to specify the non-headless variant? For me there is a huge performance difference in eg Youtube in Firefox.

eg nvidia-driver-515 instead of nvidia-headless-515

[guenhter]

That's a really good point. This is not covered by this Merge Request.