all lists on lists.proxmox.com
 help / color / mirror / Atom feed
* [pve-devel] [PATCH container] setup: Fix architecture detection for NixOS containers
@ 2023-02-28 10:59 Christoph Heiss
  2023-07-03 10:00 ` Christoph Heiss
  2023-08-21 12:50 ` Fiona Ebner
  0 siblings, 2 replies; 4+ messages in thread
From: Christoph Heiss @ 2023-02-28 10:59 UTC (permalink / raw)
  To: pve-devel

NixOS is special and deviates in many places from a "standard" Linux
system. In this case, /bin/sh does not exist on the actual filesystem,
at least not before the initial activation - which creates a symlink at
/bin/sh.

Thus try a bit harder to detect the architecture for NixOS containers by
inspecting the init script, which contains a shebang-line with the full
path to the system shell.

This moves the architecture detection code to the end of the container
creation lifecycle, so that it can be implemented as a plugin
subroutine. Therefore this mechanism is now generic enough that it can
be adapted to other container OS's in the future if needed. AFAICS
`arch` is only used when writing the actual LXC config, so determining
it later during creation does not change anything.

Tested by creating a NixOS and a Debian container (to verify that
nothing regressed) and checking if the warning "Architecure detection
failed: [..]" no longer appears for the NixOS CT and if  `arch` in the
CT config is correct. Also tested restoring both containers from a local
and a PBS backup, as well as migrating both container.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 src/PVE/LXC/Create.pm       | 76 -------------------------------------
 src/PVE/LXC/Setup.pm        | 64 +++++++++++++++++++++++++++++++
 src/PVE/LXC/Setup/Base.pm   |  8 ++++
 src/PVE/LXC/Setup/NixOS.pm  | 17 +++++++++
 src/PVE/LXC/Setup/Plugin.pm |  5 +++
 5 files changed, 94 insertions(+), 76 deletions(-)

diff --git a/src/PVE/LXC/Create.pm b/src/PVE/LXC/Create.pm
index b2e3d00..adf917c 100644
--- a/src/PVE/LXC/Create.pm
+++ b/src/PVE/LXC/Create.pm
@@ -16,72 +16,6 @@ use PVE::VZDump::ConvertOVZ;
 use PVE::Tools;
 use POSIX;
 
-sub detect_architecture {
-    my ($rootdir) = @_;
-
-    # see https://en.wikipedia.org/wiki/Executable_and_Linkable_Format
-
-    my $supported_elf_machine = {
-	0x03 => 'i386',
-	0x3e => 'amd64',
-	0x28 => 'armhf',
-	0xb7 => 'arm64',
-	0xf3 => 'riscv',
-    };
-
-    my $elf_fn = '/bin/sh'; # '/bin/sh' is POSIX mandatory
-    my $detect_arch = sub {
-	# chroot avoids a problem where we check the binary of the host system
-	# if $elf_fn is an absolut symlink (e.g. $rootdir/bin/sh -> /bin/bash)
-	chroot($rootdir) or die "chroot '$rootdir' failed: $!\n";
-	chdir('/') or die "failed to change to root directory\n";
-
-	open(my $fh, "<", $elf_fn) or die "open '$elf_fn' failed: $!\n";
-	binmode($fh);
-
-	my $length = read($fh, my $data, 20) or die "read failed: $!\n";
-
-	# 4 bytes ELF magic number and 1 byte ELF class, padding, machine
-	my ($magic, $class, undef, $machine) = unpack("A4CA12n", $data);
-
-	die "'$elf_fn' does not resolve to an ELF!\n"
-	    if (!defined($class) || !defined($magic) || $magic ne "\177ELF");
-
-	my $arch = $supported_elf_machine->{$machine};
-	die "'$elf_fn' has unknown ELF machine '$machine'!\n"
-	    if !defined($arch);
-
-	if ($arch eq 'riscv') {
-	    if ($class eq 1) {
-		$arch = 'riscv32';
-	    } elsif ($class eq 2) {
-		$arch = 'riscv64';
-	    } else {
-		die "'$elf_fn' has invalid class '$class'!\n";
-	    }
-	}
-
-	return $arch;
-    };
-
-    my $arch = eval { PVE::Tools::run_fork_with_timeout(10, $detect_arch); };
-    my $err = $@;
-
-    if (!defined($arch) && !defined($err)) {
-	# on timeout
-	die "Architecture detection failed: timeout\n";
-    } elsif ($err) {
-	# any other error
-	$arch = 'amd64';
-	print "Architecture detection failed: $err\nFalling back to $arch.\n" .
-	      "Use `pct set VMID --arch ARCH` to change.\n";
-    } else {
-	print "Detected container architecture: $arch\n";
-    }
-
-    return $arch;
-}
-
 sub restore_archive {
     my ($storage_cfg, $archive, $rootdir, $conf, $no_unpack_error, $bwlimit) = @_;
 
@@ -118,11 +52,6 @@ sub restore_proxmox_backup_archive {
 
     PVE::Storage::PBSPlugin::run_raw_client_cmd(
 	$scfg, $storeid, $cmd, $param, userns_cmd => $userns_cmd);
-
-    # if arch is set, we do not try to autodetect it
-    return if defined($conf->{arch});
-
-    $conf->{arch} = detect_architecture($rootdir);
 }
 
 sub restore_tar_archive {
@@ -183,11 +112,6 @@ sub restore_tar_archive {
     my $err = $@;
     close($archive_fh) if defined $archive_fh;
     die $err if $err && !$no_unpack_error;
-
-    # if arch is set, we do not try to autodetect it
-    return if defined($conf->{arch});
-
-    $conf->{arch} = detect_architecture($rootdir);
 }
 
 sub recover_config {
diff --git a/src/PVE/LXC/Setup.pm b/src/PVE/LXC/Setup.pm
index 891231f..4346c5e 100644
--- a/src/PVE/LXC/Setup.pm
+++ b/src/PVE/LXC/Setup.pm
@@ -131,6 +131,20 @@ sub new {
 	$plugin->{rootgid} = $rootgid;
     }
 
+    # if arch is unset, we try to autodetect it
+    if (!defined($conf->{arch})) {
+	my $arch = eval { $self->protected_call(sub { $plugin->detect_architecture() }) };
+
+	if (!defined($arch)) {
+	    $arch = 'amd64';
+	    print "Falling back to $arch.\nUse `pct set VMID --arch ARCH` to change.\n";
+	} else {
+	    print "Detected container architecture: $arch\n";
+	}
+
+	$conf->{arch} = $arch;
+    }
+
     return $self;
 }
 
@@ -346,4 +360,54 @@ sub get_ct_init_path {
     return $init;
 }
 
+sub detect_architecture {
+    my ($elf_fn) = @_;
+
+    # see https://en.wikipedia.org/wiki/Executable_and_Linkable_Format
+
+    my $supported_elf_machine = {
+	0x03 => 'i386',
+	0x3e => 'amd64',
+	0x28 => 'armhf',
+	0xb7 => 'arm64',
+	0xf3 => 'riscv',
+    };
+
+    my $detect_arch = sub {
+	open(my $fh, "<", $elf_fn) or die "open '$elf_fn' failed: $!\n";
+	binmode($fh);
+
+	my $length = read($fh, my $data, 20) or die "read failed: $!\n";
+
+	# 4 bytes ELF magic number and 1 byte ELF class, padding, machine
+	my ($magic, $class, undef, $machine) = unpack("A4CA12n", $data);
+
+	die "'$elf_fn' does not resolve to an ELF!\n"
+	    if (!defined($class) || !defined($magic) || $magic ne "\177ELF");
+
+	my $arch = $supported_elf_machine->{$machine};
+	die "'$elf_fn' has unknown ELF machine '$machine'!\n"
+	    if !defined($arch);
+
+	if ($arch eq 'riscv') {
+	    if ($class eq 1) {
+		$arch = 'riscv32';
+	    } elsif ($class eq 2) {
+		$arch = 'riscv64';
+	    } else {
+		die "'$elf_fn' has invalid class '$class'!\n";
+	    }
+	}
+
+	return $arch;
+    };
+
+    my $arch = eval { PVE::Tools::run_fork_with_timeout(10, $detect_arch); };
+    my $err = $@ // "timeout\n";
+
+    die "Architecture detection failed: $err" if !defined($arch);
+
+    return $arch;
+}
+
 1;
diff --git a/src/PVE/LXC/Setup/Base.pm b/src/PVE/LXC/Setup/Base.pm
index 547abdb..93c1d34 100644
--- a/src/PVE/LXC/Setup/Base.pm
+++ b/src/PVE/LXC/Setup/Base.pm
@@ -18,6 +18,7 @@ use PVE::INotify;
 use PVE::Tools;
 use PVE::Network;
 
+use PVE::LXC::Setup;
 use PVE::LXC::Setup::Plugin;
 use base qw(PVE::LXC::Setup::Plugin);
 
@@ -574,6 +575,13 @@ sub ssh_host_key_types_to_generate {
     };
 }
 
+sub detect_architecture {
+    my ($self) = @_;
+
+    # '/bin/sh' is POSIX mandatory
+    return PVE::LXC::Setup::detect_architecture('/bin/sh');
+}
+
 sub pre_start_hook {
     my ($self, $conf) = @_;
 
diff --git a/src/PVE/LXC/Setup/NixOS.pm b/src/PVE/LXC/Setup/NixOS.pm
index 845d2d5..4f7b93e 100644
--- a/src/PVE/LXC/Setup/NixOS.pm
+++ b/src/PVE/LXC/Setup/NixOS.pm
@@ -5,6 +5,7 @@ use warnings;
 
 use File::Path 'make_path';
 
+use PVE::LXC::Setup;
 use PVE::LXC::Setup::Base;
 
 use base qw(PVE::LXC::Setup::Base);
@@ -37,4 +38,20 @@ sub setup_init {
     my ($self, $conf) = @_;
 }
 
+sub detect_architecture {
+    my ($self) = @_;
+
+    # /bin/sh only exists as a symlink after the initial system activaction on first boot.
+    # To detect the actual architecture of the system, examine the shebang line of the /sbin/init
+    # script, which has the full path to the system shell.
+    my $init_path = '/sbin/init';
+    open(my $fh, '<', $init_path) or die "open '$init_path' failed: $!\n";
+
+    if (<$fh> =~ /^#! ?(\S*)/) {
+	return PVE::LXC::Setup::detect_architecture($1);
+    }
+
+    die "Architecture detection failed: could not find a shell\n";
+}
+
 1;
diff --git a/src/PVE/LXC/Setup/Plugin.pm b/src/PVE/LXC/Setup/Plugin.pm
index 3d968e7..b9d9c2d 100644
--- a/src/PVE/LXC/Setup/Plugin.pm
+++ b/src/PVE/LXC/Setup/Plugin.pm
@@ -62,6 +62,11 @@ sub ssh_host_key_types_to_generate {
     croak "implement me in sub-class\n";
 }
 
+sub detect_architecture {
+    my ($self) = @_;
+    croak "implement me in sub-class\n";
+}
+
 # hooks
 
 sub pre_start_hook {
-- 
2.39.2





^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [pve-devel] [PATCH container] setup: Fix architecture detection for NixOS containers
  2023-02-28 10:59 [pve-devel] [PATCH container] setup: Fix architecture detection for NixOS containers Christoph Heiss
@ 2023-07-03 10:00 ` Christoph Heiss
  2023-08-21 12:50 ` Fiona Ebner
  1 sibling, 0 replies; 4+ messages in thread
From: Christoph Heiss @ 2023-07-03 10:00 UTC (permalink / raw)
  To: Proxmox VE development discussion

Ping? Still applies & works fine on current master.

On Tue, Feb 28, 2023 at 11:59:11AM +0100, Christoph Heiss wrote:
> NixOS is special and deviates in many places from a "standard" Linux
> system. In this case, /bin/sh does not exist on the actual filesystem,
> at least not before the initial activation - which creates a symlink at
> /bin/sh.
>
> Thus try a bit harder to detect the architecture for NixOS containers by
> inspecting the init script, which contains a shebang-line with the full
> path to the system shell.
>
> This moves the architecture detection code to the end of the container
> creation lifecycle, so that it can be implemented as a plugin
> subroutine. Therefore this mechanism is now generic enough that it can
> be adapted to other container OS's in the future if needed. AFAICS
> `arch` is only used when writing the actual LXC config, so determining
> it later during creation does not change anything.
>
> Tested by creating a NixOS and a Debian container (to verify that
> nothing regressed) and checking if the warning "Architecure detection
> failed: [..]" no longer appears for the NixOS CT and if  `arch` in the
> CT config is correct. Also tested restoring both containers from a local
> and a PBS backup, as well as migrating both container.
>
> Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
> ---
>  src/PVE/LXC/Create.pm       | 76 -------------------------------------
>  src/PVE/LXC/Setup.pm        | 64 +++++++++++++++++++++++++++++++
>  src/PVE/LXC/Setup/Base.pm   |  8 ++++
>  src/PVE/LXC/Setup/NixOS.pm  | 17 +++++++++
>  src/PVE/LXC/Setup/Plugin.pm |  5 +++
>  5 files changed, 94 insertions(+), 76 deletions(-)
>
> diff --git a/src/PVE/LXC/Create.pm b/src/PVE/LXC/Create.pm
> index b2e3d00..adf917c 100644
> --- a/src/PVE/LXC/Create.pm
> +++ b/src/PVE/LXC/Create.pm
> @@ -16,72 +16,6 @@ use PVE::VZDump::ConvertOVZ;
>  use PVE::Tools;
>  use POSIX;
>
> -sub detect_architecture {
> -    my ($rootdir) = @_;
> -
> -    # see https://en.wikipedia.org/wiki/Executable_and_Linkable_Format
> -
> -    my $supported_elf_machine = {
> -	0x03 => 'i386',
> -	0x3e => 'amd64',
> -	0x28 => 'armhf',
> -	0xb7 => 'arm64',
> -	0xf3 => 'riscv',
> -    };
> -
> -    my $elf_fn = '/bin/sh'; # '/bin/sh' is POSIX mandatory
> -    my $detect_arch = sub {
> -	# chroot avoids a problem where we check the binary of the host system
> -	# if $elf_fn is an absolut symlink (e.g. $rootdir/bin/sh -> /bin/bash)
> -	chroot($rootdir) or die "chroot '$rootdir' failed: $!\n";
> -	chdir('/') or die "failed to change to root directory\n";
> -
> -	open(my $fh, "<", $elf_fn) or die "open '$elf_fn' failed: $!\n";
> -	binmode($fh);
> -
> -	my $length = read($fh, my $data, 20) or die "read failed: $!\n";
> -
> -	# 4 bytes ELF magic number and 1 byte ELF class, padding, machine
> -	my ($magic, $class, undef, $machine) = unpack("A4CA12n", $data);
> -
> -	die "'$elf_fn' does not resolve to an ELF!\n"
> -	    if (!defined($class) || !defined($magic) || $magic ne "\177ELF");
> -
> -	my $arch = $supported_elf_machine->{$machine};
> -	die "'$elf_fn' has unknown ELF machine '$machine'!\n"
> -	    if !defined($arch);
> -
> -	if ($arch eq 'riscv') {
> -	    if ($class eq 1) {
> -		$arch = 'riscv32';
> -	    } elsif ($class eq 2) {
> -		$arch = 'riscv64';
> -	    } else {
> -		die "'$elf_fn' has invalid class '$class'!\n";
> -	    }
> -	}
> -
> -	return $arch;
> -    };
> -
> -    my $arch = eval { PVE::Tools::run_fork_with_timeout(10, $detect_arch); };
> -    my $err = $@;
> -
> -    if (!defined($arch) && !defined($err)) {
> -	# on timeout
> -	die "Architecture detection failed: timeout\n";
> -    } elsif ($err) {
> -	# any other error
> -	$arch = 'amd64';
> -	print "Architecture detection failed: $err\nFalling back to $arch.\n" .
> -	      "Use `pct set VMID --arch ARCH` to change.\n";
> -    } else {
> -	print "Detected container architecture: $arch\n";
> -    }
> -
> -    return $arch;
> -}
> -
>  sub restore_archive {
>      my ($storage_cfg, $archive, $rootdir, $conf, $no_unpack_error, $bwlimit) = @_;
>
> @@ -118,11 +52,6 @@ sub restore_proxmox_backup_archive {
>
>      PVE::Storage::PBSPlugin::run_raw_client_cmd(
>  	$scfg, $storeid, $cmd, $param, userns_cmd => $userns_cmd);
> -
> -    # if arch is set, we do not try to autodetect it
> -    return if defined($conf->{arch});
> -
> -    $conf->{arch} = detect_architecture($rootdir);
>  }
>
>  sub restore_tar_archive {
> @@ -183,11 +112,6 @@ sub restore_tar_archive {
>      my $err = $@;
>      close($archive_fh) if defined $archive_fh;
>      die $err if $err && !$no_unpack_error;
> -
> -    # if arch is set, we do not try to autodetect it
> -    return if defined($conf->{arch});
> -
> -    $conf->{arch} = detect_architecture($rootdir);
>  }
>
>  sub recover_config {
> diff --git a/src/PVE/LXC/Setup.pm b/src/PVE/LXC/Setup.pm
> index 891231f..4346c5e 100644
> --- a/src/PVE/LXC/Setup.pm
> +++ b/src/PVE/LXC/Setup.pm
> @@ -131,6 +131,20 @@ sub new {
>  	$plugin->{rootgid} = $rootgid;
>      }
>
> +    # if arch is unset, we try to autodetect it
> +    if (!defined($conf->{arch})) {
> +	my $arch = eval { $self->protected_call(sub { $plugin->detect_architecture() }) };
> +
> +	if (!defined($arch)) {
> +	    $arch = 'amd64';
> +	    print "Falling back to $arch.\nUse `pct set VMID --arch ARCH` to change.\n";
> +	} else {
> +	    print "Detected container architecture: $arch\n";
> +	}
> +
> +	$conf->{arch} = $arch;
> +    }
> +
>      return $self;
>  }
>
> @@ -346,4 +360,54 @@ sub get_ct_init_path {
>      return $init;
>  }
>
> +sub detect_architecture {
> +    my ($elf_fn) = @_;
> +
> +    # see https://en.wikipedia.org/wiki/Executable_and_Linkable_Format
> +
> +    my $supported_elf_machine = {
> +	0x03 => 'i386',
> +	0x3e => 'amd64',
> +	0x28 => 'armhf',
> +	0xb7 => 'arm64',
> +	0xf3 => 'riscv',
> +    };
> +
> +    my $detect_arch = sub {
> +	open(my $fh, "<", $elf_fn) or die "open '$elf_fn' failed: $!\n";
> +	binmode($fh);
> +
> +	my $length = read($fh, my $data, 20) or die "read failed: $!\n";
> +
> +	# 4 bytes ELF magic number and 1 byte ELF class, padding, machine
> +	my ($magic, $class, undef, $machine) = unpack("A4CA12n", $data);
> +
> +	die "'$elf_fn' does not resolve to an ELF!\n"
> +	    if (!defined($class) || !defined($magic) || $magic ne "\177ELF");
> +
> +	my $arch = $supported_elf_machine->{$machine};
> +	die "'$elf_fn' has unknown ELF machine '$machine'!\n"
> +	    if !defined($arch);
> +
> +	if ($arch eq 'riscv') {
> +	    if ($class eq 1) {
> +		$arch = 'riscv32';
> +	    } elsif ($class eq 2) {
> +		$arch = 'riscv64';
> +	    } else {
> +		die "'$elf_fn' has invalid class '$class'!\n";
> +	    }
> +	}
> +
> +	return $arch;
> +    };
> +
> +    my $arch = eval { PVE::Tools::run_fork_with_timeout(10, $detect_arch); };
> +    my $err = $@ // "timeout\n";
> +
> +    die "Architecture detection failed: $err" if !defined($arch);
> +
> +    return $arch;
> +}
> +
>  1;
> diff --git a/src/PVE/LXC/Setup/Base.pm b/src/PVE/LXC/Setup/Base.pm
> index 547abdb..93c1d34 100644
> --- a/src/PVE/LXC/Setup/Base.pm
> +++ b/src/PVE/LXC/Setup/Base.pm
> @@ -18,6 +18,7 @@ use PVE::INotify;
>  use PVE::Tools;
>  use PVE::Network;
>
> +use PVE::LXC::Setup;
>  use PVE::LXC::Setup::Plugin;
>  use base qw(PVE::LXC::Setup::Plugin);
>
> @@ -574,6 +575,13 @@ sub ssh_host_key_types_to_generate {
>      };
>  }
>
> +sub detect_architecture {
> +    my ($self) = @_;
> +
> +    # '/bin/sh' is POSIX mandatory
> +    return PVE::LXC::Setup::detect_architecture('/bin/sh');
> +}
> +
>  sub pre_start_hook {
>      my ($self, $conf) = @_;
>
> diff --git a/src/PVE/LXC/Setup/NixOS.pm b/src/PVE/LXC/Setup/NixOS.pm
> index 845d2d5..4f7b93e 100644
> --- a/src/PVE/LXC/Setup/NixOS.pm
> +++ b/src/PVE/LXC/Setup/NixOS.pm
> @@ -5,6 +5,7 @@ use warnings;
>
>  use File::Path 'make_path';
>
> +use PVE::LXC::Setup;
>  use PVE::LXC::Setup::Base;
>
>  use base qw(PVE::LXC::Setup::Base);
> @@ -37,4 +38,20 @@ sub setup_init {
>      my ($self, $conf) = @_;
>  }
>
> +sub detect_architecture {
> +    my ($self) = @_;
> +
> +    # /bin/sh only exists as a symlink after the initial system activaction on first boot.
> +    # To detect the actual architecture of the system, examine the shebang line of the /sbin/init
> +    # script, which has the full path to the system shell.
> +    my $init_path = '/sbin/init';
> +    open(my $fh, '<', $init_path) or die "open '$init_path' failed: $!\n";
> +
> +    if (<$fh> =~ /^#! ?(\S*)/) {
> +	return PVE::LXC::Setup::detect_architecture($1);
> +    }
> +
> +    die "Architecture detection failed: could not find a shell\n";
> +}
> +
>  1;
> diff --git a/src/PVE/LXC/Setup/Plugin.pm b/src/PVE/LXC/Setup/Plugin.pm
> index 3d968e7..b9d9c2d 100644
> --- a/src/PVE/LXC/Setup/Plugin.pm
> +++ b/src/PVE/LXC/Setup/Plugin.pm
> @@ -62,6 +62,11 @@ sub ssh_host_key_types_to_generate {
>      croak "implement me in sub-class\n";
>  }
>
> +sub detect_architecture {
> +    my ($self) = @_;
> +    croak "implement me in sub-class\n";
> +}
> +
>  # hooks
>
>  sub pre_start_hook {
> --
> 2.39.2
>
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>




^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [pve-devel] [PATCH container] setup: Fix architecture detection for NixOS containers
  2023-02-28 10:59 [pve-devel] [PATCH container] setup: Fix architecture detection for NixOS containers Christoph Heiss
  2023-07-03 10:00 ` Christoph Heiss
@ 2023-08-21 12:50 ` Fiona Ebner
  2023-08-22 12:35   ` Christoph Heiss
  1 sibling, 1 reply; 4+ messages in thread
From: Fiona Ebner @ 2023-08-21 12:50 UTC (permalink / raw)
  To: Proxmox VE development discussion, Christoph Heiss

Am 28.02.23 um 11:59 schrieb Christoph Heiss:
> diff --git a/src/PVE/LXC/Setup.pm b/src/PVE/LXC/Setup.pm
> index 891231f..4346c5e 100644
> --- a/src/PVE/LXC/Setup.pm
> +++ b/src/PVE/LXC/Setup.pm
> @@ -131,6 +131,20 @@ sub new {
>  	$plugin->{rootgid} = $rootgid;
>      }
>  
> +    # if arch is unset, we try to autodetect it
> +    if (!defined($conf->{arch})) {
> +	my $arch = eval { $self->protected_call(sub { $plugin->detect_architecture() }) };
> +

The error from the eval should be logged here.

> +	if (!defined($arch)) {
> +	    $arch = 'amd64';
> +	    print "Falling back to $arch.\nUse `pct set VMID --arch ARCH` to change.\n";
> +	} else {
> +	    print "Detected container architecture: $arch\n";
> +	}
> +

(...)

> diff --git a/src/PVE/LXC/Setup/NixOS.pm b/src/PVE/LXC/Setup/NixOS.pm
> index 845d2d5..4f7b93e 100644
> --- a/src/PVE/LXC/Setup/NixOS.pm
> +++ b/src/PVE/LXC/Setup/NixOS.pm
> @@ -5,6 +5,7 @@ use warnings;
>  
>  use File::Path 'make_path';
>  
> +use PVE::LXC::Setup;

This is a cyclic include, please let us avoid those. We could either use
a new helper module for the detect_architecture() helper or otherwise
move it to pve-guest-common or even pve-common. The detection itself is
pretty generic after all.

An alternative to that would be to just add a function get_elf_fn() to
the plugin interface and change the existing detect_architecture() to
use that.

>  use PVE::LXC::Setup::Base;
>  
>  use base qw(PVE::LXC::Setup::Base);




^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [pve-devel] [PATCH container] setup: Fix architecture detection for NixOS containers
  2023-08-21 12:50 ` Fiona Ebner
@ 2023-08-22 12:35   ` Christoph Heiss
  0 siblings, 0 replies; 4+ messages in thread
From: Christoph Heiss @ 2023-08-22 12:35 UTC (permalink / raw)
  To: Fiona Ebner; +Cc: Proxmox VE development discussion


Thanks for the review!

On Mon, Aug 21, 2023 at 02:50:28PM +0200, Fiona Ebner wrote:
>
> Am 28.02.23 um 11:59 schrieb Christoph Heiss:
> > diff --git a/src/PVE/LXC/Setup.pm b/src/PVE/LXC/Setup.pm
> > index 891231f..4346c5e 100644
> > --- a/src/PVE/LXC/Setup.pm
> > +++ b/src/PVE/LXC/Setup.pm
> > @@ -131,6 +131,20 @@ sub new {
> >  	$plugin->{rootgid} = $rootgid;
> >      }
> >
> > +    # if arch is unset, we try to autodetect it
> > +    if (!defined($conf->{arch})) {
> > +	my $arch = eval { $self->protected_call(sub { $plugin->detect_architecture() }) };
> > +
>
> The error from the eval should be logged here.
Ack, will do.

>
> > +	if (!defined($arch)) {
> > +	    $arch = 'amd64';
> > +	    print "Falling back to $arch.\nUse `pct set VMID --arch ARCH` to change.\n";
> > +	} else {
> > +	    print "Detected container architecture: $arch\n";
> > +	}
> > +
>
> (...)
>
> > diff --git a/src/PVE/LXC/Setup/NixOS.pm b/src/PVE/LXC/Setup/NixOS.pm
> > index 845d2d5..4f7b93e 100644
> > --- a/src/PVE/LXC/Setup/NixOS.pm
> > +++ b/src/PVE/LXC/Setup/NixOS.pm
> > @@ -5,6 +5,7 @@ use warnings;
> >
> >  use File::Path 'make_path';
> >
> > +use PVE::LXC::Setup;
>
> This is a cyclic include, please let us avoid those. We could either use
> a new helper module for the detect_architecture() helper or otherwise
> move it to pve-guest-common or even pve-common. The detection itself is
> pretty generic after all.
I will probably go this route and simply move it to another module in
pve-container. As long as it is not needed anywhere else, I think it is
sensible to letting it stay close it is only user.

>
> An alternative to that would be to just add a function get_elf_fn() to
> the plugin interface and change the existing detect_architecture() to
> use that.
>
> >  use PVE::LXC::Setup::Base;
> >
> >  use base qw(PVE::LXC::Setup::Base);




^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2023-08-22 12:36 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-28 10:59 [pve-devel] [PATCH container] setup: Fix architecture detection for NixOS containers Christoph Heiss
2023-07-03 10:00 ` Christoph Heiss
2023-08-21 12:50 ` Fiona Ebner
2023-08-22 12:35   ` Christoph Heiss

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal