[pve-devel] [PATCH proxmx-nvidia-vgpu-helper 0/2] reduce setup steps for nvidia vgpu drivers

[pve-devel] [PATCH proxmx-nvidia-vgpu-helper 0/2] reduce setup steps for nvidia vgpu drivers

[Hannes Duerr]

The patches apply to the repository proxmox-nvidia-vgpu-helper which
is currently only available in my staff folder
`staff/h.duerr/proxmox-nvidia-vgpu-helper`. The aim of the repository
is to reduce the necessary installation steps for the Nvidia VGPU
drivers [0]. The repository contains an install script which can be
used to check and install necessary dependencies and a systemd
template service which can be used to configure the SR-IOV per pci-id

Part of the changes would later be the adjustment of the wiki page

[0] https://pve.proxmox.com/wiki/NVIDIA_vGPU_on_Proxmox_VE


Hannes Duerr (2):
  debian/control: adjust description and dependency to new purpose
  add script to help with the installation of the nvidia vgpu
    dependencies

 debian/control               | 11 +++---
 pve-install-nvidia-vgpu-deps | 66 ++++++++++++++++++++++++++++++++++++
 2 files changed, 71 insertions(+), 6 deletions(-)
 create mode 100755 pve-install-nvidia-vgpu-deps

-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH proxmx-nvidia-vgpu-helper 1/2] debian/control: adjust description and pve-manager dependency

[Hannes Duerr]

remove the dependency of proxmox-dkms, since this package does not
exist, and add the dependency of pve-manager, which should be installed
with every reasonable Proxmox VE installation, so that the package can
already be installed during the installation.

Signed-off-by: Hannes Duerr <h.duerr@proxmox.com>
---
 debian/control | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/debian/control b/debian/control
index 1c19a50..f119bdc 100644
--- a/debian/control
+++ b/debian/control
@@ -8,11 +8,10 @@ Homepage: https://www.proxmox.com
 
 Package: proxmox-nvidia-vgpu-helper
 Architecture: all
-Depends: proxmox-dkms,
+Depends: libapt-pkg-perl,
+         pve-manager,
          ${misc:Depends},
-Description: Proxmox Nvidia vGPU systemd service
- This package helps with the configuration of Nvidia vGPU
- drivers by providing a systemd template service which
+Description: Proxmox Nvidia vGPU helper script and systemd service
+ This package provides a script, that helps with installing all required
+ packages for the Nvidia vGPU driver, and also a systemd template service which
  configures the option SRI-OV per pci-id
-
-
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH proxmx-nvidia-vgpu-helper 1/2] debian/control: adjust description and pve-manager dependency

[Dominik Csapak]

On 11/20/24 12:26, Hannes Duerr wrote:
> remove the dependency of proxmox-dkms, since this package does not
> exist, and add the dependency of pve-manager, which should be installed
> with every reasonable Proxmox VE installation, so that the package can
> already be installed during the installation.
> 

i think that's in reverse? we don't need pve-manager here AFAICT,
but if we want a dependency somewhere, we'd want to add this package
as a dependency of pve-manager, no?

from the current state of this package I don't see a reason to depend on any pve/proxmox packages


> Signed-off-by: Hannes Duerr <h.duerr@proxmox.com>
> ---
>   debian/control | 11 +++++------
>   1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/debian/control b/debian/control
> index 1c19a50..f119bdc 100644
> --- a/debian/control
> +++ b/debian/control
> @@ -8,11 +8,10 @@ Homepage: https://www.proxmox.com
>   
>   Package: proxmox-nvidia-vgpu-helper
>   Architecture: all
> -Depends: proxmox-dkms,
> +Depends: libapt-pkg-perl,
> +         pve-manager,
>            ${misc:Depends},
> -Description: Proxmox Nvidia vGPU systemd service
> - This package helps with the configuration of Nvidia vGPU
> - drivers by providing a systemd template service which
> +Description: Proxmox Nvidia vGPU helper script and systemd service
> + This package provides a script, that helps with installing all required
> + packages for the Nvidia vGPU driver, and also a systemd template service which
>    configures the option SRI-OV per pci-id
> -
> -



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH proxmx-nvidia-vgpu-helper 1/2] debian/control: adjust description and pve-manager dependency

[Hannes Dürr]

On 12/2/24 09:36, Dominik Csapak wrote:
> i think that's in reverse? we don't need pve-manager here AFAICT,
> but if we want a dependency somewhere, we'd want to add this package
> as a dependency of pve-manager, no?
>
> from the current state of this package I don't see a reason to depend 
> on any pve/proxmox packages
Yes, thomas has already given me that as feedback, I just wanted to wait 
for your feedback before sending a V2


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH proxmx-nvidia-vgpu-helper 2/2] add script to help with the installation of the nvidia vgpu dependencies

[Hannes Duerr]

The script should help with the dependency installation for the nvidia
vgpu driver, also if the driver is already installed but the system has
been updated

Signed-off-by: Hannes Duerr <h.duerr@proxmox.com>
---
 pve-install-nvidia-vgpu-deps | 66 ++++++++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)
 create mode 100755 pve-install-nvidia-vgpu-deps

diff --git a/pve-install-nvidia-vgpu-deps b/pve-install-nvidia-vgpu-deps
new file mode 100755
index 0000000..fc0856e
--- /dev/null
+++ b/pve-install-nvidia-vgpu-deps
@@ -0,0 +1,66 @@
+#!/usr/bin/perl
+
+use strict;
+use warnings;
+
+use PVE::Tools qw(run_command);
+use AptPkg::Cache;
+
+my @apt_install = qw(apt-get --no-install-recommends -o Dpkg:Options::=--force-confnew install --);
+my @dependencies = qw(dkms libc6-dev);
+my @missing_packages;
+
+die "Please execute the script with root privileges\n" if $>;
+
+my $apt_cache = AptPkg::Cache->new();
+die "unable to initialize AptPkg::Cache\n" if !$apt_cache; 
+
+sub package_is_installed {
+    my ($package) = @_;
+    my $p = $apt_cache->{$package};
+    if (!defined($p->{CurrentState}) || $p->{CurrentState} ne "Installed") {
+	push(@missing_packages, $package);
+    }
+}
+
+foreach my $dependency (@dependencies) {
+    package_is_installed($dependency);
+}
+
+
+my $running_kernel;
+run_command( ['/usr/bin/uname', '-r' ],
+    outfunc => sub { $running_kernel = shift } );
+
+my $default_major_minor_version;
+run_command(['/usr/bin/dpkg-query', '-f', '${Depends}', '-W', 'proxmox-default-kernel'],
+    outfunc => sub { $default_major_minor_version = shift } );
+
+my $default_full_version;
+run_command(['/usr/bin/dpkg-query', '-f', '${Version}', '-W', $default_major_minor_version],
+    outfunc => sub { $default_full_version = shift } );
+
+if ($running_kernel =~ /$default_full_version-pve/) {
+    print "You are running the proxmox default kernel `proxmox-kernel-$running_kernel`\n";
+    package_is_installed("proxmox-default-headers");
+} elsif ($running_kernel =~ /pve/) {
+    print "You are running the non default proxmox kernel `proxmox-kernel-$running_kernel`\n";
+    package_is_installed("proxmox-headers-$running_kernel");
+} else {
+    die "You are not using a proxmox-kernel, please make sure that the appropriate header package is installed.\n";
+}
+
+if (!@missing_packages){
+    print "All required packages are installed, you can continue with the Nvidia vGPU driver installation.\n";
+    exit;
+} else {
+    print "The following packages are missing:\n" . join("\n", @missing_packages) ."\n";
+    print "Would you like to install them now (y/n)?\n";
+}
+
+my $answer = <STDIN>;
+if (defined($answer) && $answer =~ m/^\s*y(?:es)?\s*$/i) {
+    if (system(@apt_install, @missing_packages) != 0) {
+	die "apt failed during the installation: ($?)\n";
+    }
+}
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH proxmx-nvidia-vgpu-helper 2/2] add script to help with the installation of the nvidia vgpu dependencies

[Dominik Csapak]

high level comments first:

* the script is actually not installed with the package, because it's not
installed anywhere and not in any .install file

(you'd either have to use an 'install' target in the makefile to install it to some folder
or use a .install file in the debian folder for the package to specify
which files should be installed
(see most of our other packages how they do it exactly)

* i'd like to go a bit further than this script.

What I'd have imagined is more like our 'pveceph' tool that can
include multiple commands, even if we might only have one or two for now

so e.g.

pve-nvidia-vgpu-helper install-prerequisites
pve-nvidia-vgpu-helper gpus list
pve-nvidia-vgpu-helper gpus enable-sriov
etc..

some of these (e.g. the sriov enablement) could even be interactive, but something
like that could be done in the future.

thir

* not sure about that, but couldn't this be a rust binary?
i think we have everything we need already there.
It is not a requirement for this IMHO, but personally i tend to write new things
in rust these days.

* it would be very nice to also have some general prerequisites checked, maybe
as a separate command or all-in-one (does not matter much imo). For example
if iommu/vt-d/etc. is turned on and the cards are separated, if there are even
nvidia cards installed, etc.

one comment inline

On 11/20/24 12:26, Hannes Duerr wrote:
> The script should help with the dependency installation for the nvidia
> vgpu driver, also if the driver is already installed but the system has
> been updated
> 
> Signed-off-by: Hannes Duerr <h.duerr@proxmox.com>
> ---
>   pve-install-nvidia-vgpu-deps | 66 ++++++++++++++++++++++++++++++++++++
>   1 file changed, 66 insertions(+)
>   create mode 100755 pve-install-nvidia-vgpu-deps
> 
> diff --git a/pve-install-nvidia-vgpu-deps b/pve-install-nvidia-vgpu-deps
> new file mode 100755
> index 0000000..fc0856e
> --- /dev/null
> +++ b/pve-install-nvidia-vgpu-deps
> @@ -0,0 +1,66 @@
> +#!/usr/bin/perl
> +
> +use strict;
> +use warnings;
> +
> +use PVE::Tools qw(run_command);
> +use AptPkg::Cache;
> +
> +my @apt_install = qw(apt-get --no-install-recommends -o Dpkg:Options::=--force-confnew install --);

imho i find that line a bit dangerous, because it'll overwrite user configs with new default configs
AFAIU

Our approach to installs is that they can be interactive, so adding '--force-confnew' is not a good idea

> +my @dependencies = qw(dkms libc6-dev);
> +my @missing_packages;
> +
> +die "Please execute the script with root privileges\n" if $>;
> +
> +my $apt_cache = AptPkg::Cache->new();
> +die "unable to initialize AptPkg::Cache\n" if !$apt_cache;
> +
> +sub package_is_installed {
> +    my ($package) = @_;
> +    my $p = $apt_cache->{$package};
> +    if (!defined($p->{CurrentState}) || $p->{CurrentState} ne "Installed") {
> +	push(@missing_packages, $package);
> +    }
> +}
> +
> +foreach my $dependency (@dependencies) {
> +    package_is_installed($dependency);
> +}
> +
> +
> +my $running_kernel;
> +run_command( ['/usr/bin/uname', '-r' ],
> +    outfunc => sub { $running_kernel = shift } );
> +
> +my $default_major_minor_version;
> +run_command(['/usr/bin/dpkg-query', '-f', '${Depends}', '-W', 'proxmox-default-kernel'],
> +    outfunc => sub { $default_major_minor_version = shift } );
> +
> +my $default_full_version;
> +run_command(['/usr/bin/dpkg-query', '-f', '${Version}', '-W', $default_major_minor_version],
> +    outfunc => sub { $default_full_version = shift } );
> +
> +if ($running_kernel =~ /$default_full_version-pve/) {
> +    print "You are running the proxmox default kernel `proxmox-kernel-$running_kernel`\n";
> +    package_is_installed("proxmox-default-headers");
> +} elsif ($running_kernel =~ /pve/) {
> +    print "You are running the non default proxmox kernel `proxmox-kernel-$running_kernel`\n";
> +    package_is_installed("proxmox-headers-$running_kernel");
> +} else {
> +    die "You are not using a proxmox-kernel, please make sure that the appropriate header package is installed.\n";
> +}
> +
> +if (!@missing_packages){
> +    print "All required packages are installed, you can continue with the Nvidia vGPU driver installation.\n";
> +    exit;
> +} else {
> +    print "The following packages are missing:\n" . join("\n", @missing_packages) ."\n";
> +    print "Would you like to install them now (y/n)?\n";
> +}
> +
> +my $answer = <STDIN>;
> +if (defined($answer) && $answer =~ m/^\s*y(?:es)?\s*$/i) {
> +    if (system(@apt_install, @missing_packages) != 0) {
> +	die "apt failed during the installation: ($?)\n";
> +    }
> +}



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH proxmx-nvidia-vgpu-helper 2/2] add script to help with the installation of the nvidia vgpu dependencies

[Hannes Dürr]

On 12/2/24 09:47, Dominik Csapak wrote:
> high level comments first:
>
> * the script is actually not installed with the package, because it's not
> installed anywhere and not in any .install file
>
> (you'd either have to use an 'install' target in the makefile to 
> install it to some folder
> or use a .install file in the debian folder for the package to specify
> which files should be installed
> (see most of our other packages how they do it exactly)
>
good point, forgot that

> * i'd like to go a bit further than this script.
>
> What I'd have imagined is more like our 'pveceph' tool that can
> include multiple commands, even if we might only have one or two for now
>
> so e.g.
>
> pve-nvidia-vgpu-helper install-prerequisites
> pve-nvidia-vgpu-helper gpus list
> pve-nvidia-vgpu-helper gpus enable-sriov
> etc..
>
> some of these (e.g. the sriov enablement) could even be interactive, 
> but something
> like that could be done in the future.
>
> thir
Unfortunately, I didn't understand it that way, but of course it makes 
more sense to write a small tool if it is to take on more tasks in the 
future.
>
> * not sure about that, but couldn't this be a rust binary?
> i think we have everything we need already there.
> It is not a requirement for this IMHO, but personally i tend to write 
> new things
> in rust these days.
>
and yes, if it should contain even more functionality in the future, I 
would rather implement it in Rust.

> * it would be very nice to also have some general prerequisites 
> checked, maybe
> as a separate command or all-in-one (does not matter much imo). For 
> example
> if iommu/vt-d/etc. is turned on and the cards are separated, if there 
> are even
> nvidia cards installed, etc.
>
agree, would make sense to include this as well.
> one comment inline
>
> On 11/20/24 12:26, Hannes Duerr wrote:
>> The script should help with the dependency installation for the nvidia
>> vgpu driver, also if the driver is already installed but the system has
>> been updated
>>
>> Signed-off-by: Hannes Duerr <h.duerr@proxmox.com>
>> ---
>>   pve-install-nvidia-vgpu-deps | 66 ++++++++++++++++++++++++++++++++++++
>>   1 file changed, 66 insertions(+)
>>   create mode 100755 pve-install-nvidia-vgpu-deps
>>
>> diff --git a/pve-install-nvidia-vgpu-deps b/pve-install-nvidia-vgpu-deps
>> new file mode 100755
>> index 0000000..fc0856e
>> --- /dev/null
>> +++ b/pve-install-nvidia-vgpu-deps
>> @@ -0,0 +1,66 @@
>> +#!/usr/bin/perl
>> +
>> +use strict;
>> +use warnings;
>> +
>> +use PVE::Tools qw(run_command);
>> +use AptPkg::Cache;
>> +
>> +my @apt_install = qw(apt-get --no-install-recommends -o 
>> Dpkg:Options::=--force-confnew install --);
>
> imho i find that line a bit dangerous, because it'll overwrite user 
> configs with new default configs
> AFAIU
>
> Our approach to installs is that they can be interactive, so adding 
> '--force-confnew' is not a good idea
good point, that's probably a bit exaggerated


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH proxmx-nvidia-vgpu-helper 2/2] add script to help with the installation of the nvidia vgpu dependencies

[Thomas Lamprecht]

Am 02.12.24 um 09:47 schrieb Dominik Csapak:
> What I'd have imagined is more like our 'pveceph' tool that can
> include multiple commands, even if we might only have one or two for now
> 
> so e.g.
> 
> pve-nvidia-vgpu-helper install-prerequisites

Yes, that was my idea as well, but I'd just do above for now, all else can follow,
let's not blow up the scope here for the initial implementation.

> pve-nvidia-vgpu-helper gpus list
> pve-nvidia-vgpu-helper gpus enable-sriov
> etc..

> thir
> 
> * not sure about that, but couldn't this be a rust binary?

Not sure if it wins us much here, it certainly has it's cost too, and for such
simple things it does not always balance out, like having to rebuild it on
bigger toolchain changes to profit from them and do not cause issues on major
upgrades; a interpreted language avoids that and perl is an OK choice here.
Also, Hannes already re-wrote this from bash to perl, let's not make him run in
circles. If it really becomes a problem to use perl, which I doubt, then it can
be still re-written later on, which is as much work as doing it now but then
has some actual arguments for making that worthwhile.

> i think we have everything we need already there.
> It is not a requirement for this IMHO, but personally i tend to write new things
> in rust these days.
> 
> * it would be very nice to also have some general prerequisites checked, maybe
> as a separate command or all-in-one (does not matter much imo). For example
> if iommu/vt-d/etc. is turned on and the cards are separated, if there are even
> nvidia cards installed, etc.

IMO it does matter quite a bit, installing base-line software support and doing
basic checks are very different things, I'd not couple those unconditionally.
As it's odd that one cannot prepare a host by installing the basic package set
if it IOMMU is turned off, why force a reboot there? The other way is also a bit
odd to try to install stuff if one just wants to check if they meet some basic
checks (after doing some changes to HW, kernel cmdline, ...).

So please do those in different commands, and the basic checks can be nice to have
but I'd really focus on the command to install the basic package set first.


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel