Re: [pve-devel] vma extract force flag
The hope would be to get the benefits of nop-write from the underlying ZFS filesystem therefore increasing performance and reducing snapshot size without the need for deduplication being enabled. I have often found large deletes on ZFS to sometimes not be very performant as well, especially if deduplication is enabled. https://openzfs.org/wiki/Features#nop-write Praise the Lord! Ben Dailey Associate Technology Director Bluffton-Harrison MSD bdai...@bhmsd.org On Tue, Apr 30, 2024 at 4:26 AM Fiona Ebner wrote: > Hi, > > Am 24.04.24 um 22:44 schrieb Ben Dailey: > > I would like to keep extracted backups on an NFS server backed with ZFS > and > > retain the benefits of differential snapshots and it would be helpful to > > have the vma extract replace the previous backups directly. > > why not just remove the previously extracted archive and then extract > the new archive? > > Best Regards, > Fiona > > ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH v1 pve-manager 7/8] api: ceph: add build commit of host to Ceph osd index endpoint data
This is required in order to avoid making multiple API calls in the
following commit.
Signed-off-by: Max Carrara
---
PVE/API2/Ceph/OSD.pm | 1 +
1 file changed, 1 insertion(+)
diff --git a/PVE/API2/Ceph/OSD.pm b/PVE/API2/Ceph/OSD.pm
index 2893456a..de4cc72b 100644
--- a/PVE/API2/Ceph/OSD.pm
+++ b/PVE/API2/Ceph/OSD.pm
@@ -206,6 +206,7 @@ __PACKAGE__->register_method ({
if ($name && $e->{type} eq 'host') {
$new->{version} = $hostversions->{$name}->{version}->{str};
+ $new->{buildcommit} = $hostversions->{$name}->{buildcommit};
}
}
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH v1 pve-manager 8/8] ui: ceph: osd: rework rendering of version field & show build commit
The logic of the `render_version` function is split up in order to
handle how the version is displayed depending on the type of the row.
If the parsed version is `undefined` or the row marks the beginning of
the tree, an empty string is now returned. This behaviour is
equivalent to before, it just makes the overall logic easier.
If the row belongs to a node, the build commit is now additionally
displayed in parentheses next to the installed Ceph version:
18.2.2 (abcd1234)
If the row belongs to an osd, the build commit is also additionally
displayed in parentheses next to the installed Ceph version.
Furthermore, should the build commit of the running osd differ from
the one that's installed on the host, the new hash will also be shown
in parentheses:
18.2.2 (abcd1234 -> 5678fedc)
The icon displayed for running an osd with an outdated build is the
same as for running an outdated version. The conditional display of
cluster health-related icons remains the same otherwise.
Signed-off-by: Max Carrara
---
www/manager6/ceph/OSD.js | 55
1 file changed, 45 insertions(+), 10 deletions(-)
diff --git a/www/manager6/ceph/OSD.js b/www/manager6/ceph/OSD.js
index d2caafa4..988962b1 100644
--- a/www/manager6/ceph/OSD.js
+++ b/www/manager6/ceph/OSD.js
@@ -642,23 +642,58 @@ Ext.define('PVE.node.CephOsdTree', {
},
render_version: function(value, metadata, rec) {
+ if (value === undefined || rec.data.type === 'root') {
+ return '';
+ }
+
let vm = this.getViewModel();
- let versions = vm.get('versions');
let icon = "";
- let version = value || "";
- let maxversion = vm.get('maxversion');
- if (value && PVE.Utils.compare_ceph_versions(value, maxversion) !==
0) {
- let host_version = rec.parentNode?.data?.version ||
versions[rec.data.host] || "";
- if (rec.data.type === 'host' ||
PVE.Utils.compare_ceph_versions(host_version, maxversion) !== 0) {
+ const maxversion = vm.get('maxversion');
+ const mixedversions = vm.get('mixedversions');
+
+ if (rec.data.type === 'host') {
+ const bc = rec.data.buildcommit ?? '';
+
+ if (PVE.Utils.compare_ceph_versions(maxversion, value) > 0) {
icon = PVE.Utils.get_ceph_icon_html('HEALTH_UPGRADE');
- } else {
- icon = PVE.Utils.get_ceph_icon_html('HEALTH_OLD');
+ } else if (mixedversions) {
+ icon = PVE.Utils.get_ceph_icon_html('HEALTH_OK');
}
- } else if (value && vm.get('mixedversions')) {
+
+ if (bc === '') {
+ return `${icon}${value}`;
+ }
+
+ return `${icon}${value} (${bc.substring(0, 9)})`;
+ }
+
+ const versionNode = rec.parentNode?.data?.version ?? '';
+
+ const bc = PVE.Utils.parse_ceph_buildcommit(rec.data) ?? '';
+ const bcNode = rec.parentNode?.data?.buildcommit ?? '';
+
+ let bcChanged = bc !== '' && bcNode !== '' && bc !== bcNode;
+
+ if (PVE.Utils.compare_ceph_versions(maxversion, value) > 0) {
+ icon = PVE.Utils.get_ceph_icon_html('HEALTH_UPGRADE');
+ } else if (versionNode &&
PVE.Utils.compare_ceph_versions(versionNode, value) > 0) {
+ icon = PVE.Utils.get_ceph_icon_html('HEALTH_OLD');
+ } else if (mixedversions && !bcChanged) {
icon = PVE.Utils.get_ceph_icon_html('HEALTH_OK');
}
- return icon + version;
+ let bcPart = bc.substring(0, 9);
+ if (bcChanged) {
+ const arrow = '';
+ icon ||= PVE.Utils.get_ceph_icon_html('HEALTH_OLD');
+ bcPart = `${bc.substring(0, 9)}${arrow}${bcNode.substring(0,
9)}`;
+ }
+
+ if (bcPart === '') {
+ return `${icon}${value}`;
+ }
+
+ return `${icon}${value} (${bcPart})`;
},
render_osd_val: function(value, metaData, rec) {
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH v1 pve-manager 6/8] ui: ceph: services: parse and display build commit
This commit adds `PVE.Utils.parse_ceph_buildcommit`, which can be used
to get the full hash "eccf199d..." in parentheses from a string like
the following:
ceph version 17.2.7 (eccf199d63457659c09677399928203b7903c888) quincy (stable)
This hash is displayed and taken into account when comparing monitor
and manager versions in the client. Specifically, the shortened build
commit is now displayed in parentheses next to the version for both
monitors and managers like so:
18.2.2 (abcd1234)
Should the build commit of the running service differ from the one
that's installed on the host, the newer build commit will also be
shown in parentheses:
18.2.2 (abcd1234 -> 5678fedc)
The icon displayed for running a service with an outdated build is the
same as for running an outdated version. The conditional display of
cluster health-related icons remains the same otherwise.
The Ceph summary view also displays the hash and will show a warning
if a service is running with a build commit that doesn't match the one
that's advertised by the host.
Signed-off-by: Max Carrara
---
www/manager6/Utils.js| 14 ++
www/manager6/ceph/ServiceList.js | 28 ++--
www/manager6/ceph/Services.js| 14 +-
3 files changed, 49 insertions(+), 7 deletions(-)
diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
index 74e46694..435c79d7 100644
--- a/www/manager6/Utils.js
+++ b/www/manager6/Utils.js
@@ -128,6 +128,20 @@ Ext.define('PVE.Utils', {
return undefined;
},
+parse_ceph_buildcommit: function(service) {
+ if (service.ceph_version) {
+ // See PVE/Ceph/Tools.pm - get_local_version
+ const match = service.ceph_version.match(
+ /^ceph.*\sv?(?:\d+(?:\.\d+)+)\s+(?:\(([a-zA-Z0-9]+)\))/,
+ );
+ if (match) {
+ return match[1];
+ }
+ }
+
+ return undefined;
+},
+
compare_ceph_versions: function(a, b) {
let avers = [];
let bvers = [];
diff --git a/www/manager6/ceph/ServiceList.js b/www/manager6/ceph/ServiceList.js
index 76710146..30f455ed 100644
--- a/www/manager6/ceph/ServiceList.js
+++ b/www/manager6/ceph/ServiceList.js
@@ -102,21 +102,37 @@ Ext.define('PVE.node.CephServiceController', {
if (value === undefined) {
return '';
}
+
+ const bc = PVE.Utils.parse_ceph_buildcommit(rec.data) ?? '';
+
let view = this.getView();
- let host = rec.data.host, nodev = [0];
+ const host = rec.data.host;
+
+ let versionNode = [0];
+ let bcNode = '';
if (view.nodeversions[host] !== undefined) {
- nodev = view.nodeversions[host].version.parts;
+ versionNode = view.nodeversions[host].version.parts;
+ bcNode = view.nodeversions[host].buildcommit;
}
+ let bcChanged = bc !== '' && bcNode !== '' && bc !== bcNode;
let icon = '';
- if (PVE.Utils.compare_ceph_versions(view.maxversion, nodev) > 0) {
+ if (PVE.Utils.compare_ceph_versions(view.maxversion, versionNode) > 0) {
icon = PVE.Utils.get_ceph_icon_html('HEALTH_UPGRADE');
- } else if (PVE.Utils.compare_ceph_versions(nodev, value) > 0) {
+ } else if (PVE.Utils.compare_ceph_versions(versionNode, value) > 0) {
icon = PVE.Utils.get_ceph_icon_html('HEALTH_OLD');
- } else if (view.mixedversions) {
+ } else if (view.mixedversions && !bcChanged) {
icon = PVE.Utils.get_ceph_icon_html('HEALTH_OK');
}
- return icon + value;
+
+ let bcPart = bc.substring(0, 9);
+ if (bcChanged) {
+ const arrow = '';
+ icon ||= PVE.Utils.get_ceph_icon_html('HEALTH_OLD');
+ bcPart = `${bc.substring(0, 9)}${arrow}${bcNode.substring(0, 9)}`;
+ }
+
+ return `${icon}${value} (${bcPart})`;
},
getMaxVersions: function(store, records, success) {
diff --git a/www/manager6/ceph/Services.js b/www/manager6/ceph/Services.js
index b9fc52c8..410b28bf 100644
--- a/www/manager6/ceph/Services.js
+++ b/www/manager6/ceph/Services.js
@@ -155,6 +155,7 @@ Ext.define('PVE.ceph.Services', {
title: metadata[type][id].name || name,
host: host,
version: PVE.Utils.parse_ceph_version(metadata[type][id]),
+ buildcommit:
PVE.Utils.parse_ceph_buildcommit(metadata[type][id]),
service: metadata[type][id].service,
addr: metadata[type][id].addr || metadata[type][id].addrs
|| Proxmox.Utils.unknownText,
};
@@ -181,7 +182,10 @@ Ext.define('PVE.ceph.Services', {
}
if (result.version) {
- result.statuses.push(gettext('Version') + ": " +
result.version);
+ const host_buildcommit = metadata.node[host]?.buildcommit
|| "";
+
+ const buildcommit_short =
[pve-devel] [PATCH v1 pve-manager 3/8] ceph: services: remove old cluster broadcast
The `ceph-version` key is not used anymore, so it can go.
Double-checked by `rg`ing through all of our repositories.
Signed-off-by: Max Carrara
---
PVE/Ceph/Services.pm | 2 --
1 file changed, 2 deletions(-)
diff --git a/PVE/Ceph/Services.pm b/PVE/Ceph/Services.pm
index e0f31e8e..f5109655 100644
--- a/PVE/Ceph/Services.pm
+++ b/PVE/Ceph/Services.pm
@@ -60,8 +60,6 @@ sub broadcast_ceph_versions {
return; # up to date, nothing to do so avoid (not exactly
cheap) broadcast
}
}
- # FIXME: remove with 8.0 (or 7.2, its not _that_ bad) - for backward
compat only
- PVE::Cluster::broadcast_node_kv("ceph-version", $version);
my $node_versions = {
version => {
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH v1 pve-manager 5/8] utils: align regex of parse_ceph_version with Perl equivalent
Signed-off-by: Max Carrara
---
"Unfortunately" JS doesn't support comments in its regexes akin to
Perl's 'x' flag, but oh well ...
www/manager6/Utils.js | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
index f5608944..74e46694 100644
--- a/www/manager6/Utils.js
+++ b/www/manager6/Utils.js
@@ -118,7 +118,8 @@ Ext.define('PVE.Utils', {
}
if (service.ceph_version) {
- var match = service.ceph_version.match(/version (\d+(\.\d+)*)/);
+ // See PVE/Ceph/Tools.pm - get_local_version
+ const match =
service.ceph_version.match(/^ceph.*\sv?(\d+(?:\.\d+)+)/);
if (match) {
return match[1];
}
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH v1 pve-manager 4/8] ceph: services: refactor version existence check as guard clause
Signed-off-by: Max Carrara
---
PVE/Ceph/Services.pm | 36 ++--
1 file changed, 18 insertions(+), 18 deletions(-)
diff --git a/PVE/Ceph/Services.pm b/PVE/Ceph/Services.pm
index f5109655..0b8c207e 100644
--- a/PVE/Ceph/Services.pm
+++ b/PVE/Ceph/Services.pm
@@ -50,26 +50,26 @@ sub broadcast_ceph_services {
sub broadcast_ceph_versions {
my ($version, $buildcommit, $vers_parts) =
PVE::Ceph::Tools::get_local_version(1);
-if ($version) {
- my $nodename = PVE::INotify::nodename();
- my $old = PVE::Cluster::get_node_kv("ceph-versions", $nodename);
- if (defined($old->{$nodename})) {
- $old = eval { decode_json($old->{$nodename}) };
- warn $@ if $@; # should not happen
- if (defined($old) && $old->{buildcommit} eq $buildcommit &&
$old->{version}->{str} eq $version) {
- return; # up to date, nothing to do so avoid (not exactly
cheap) broadcast
- }
+return undef if !$version;
+
+my $nodename = PVE::INotify::nodename();
+my $old = PVE::Cluster::get_node_kv("ceph-versions", $nodename);
+if (defined($old->{$nodename})) {
+ $old = eval { decode_json($old->{$nodename}) };
+ warn $@ if $@; # should not happen
+ if (defined($old) && $old->{buildcommit} eq $buildcommit &&
$old->{version}->{str} eq $version) {
+ return; # up to date, nothing to do so avoid (not exactly cheap)
broadcast
}
-
- my $node_versions = {
- version => {
- str => $version,
- parts => $vers_parts,
- },
- buildcommit => $buildcommit,
- };
- PVE::Cluster::broadcast_node_kv("ceph-versions",
encode_json($node_versions));
}
+
+my $node_versions = {
+ version => {
+ str => $version,
+ parts => $vers_parts,
+ },
+ buildcommit => $buildcommit,
+};
+PVE::Cluster::broadcast_node_kv("ceph-versions",
encode_json($node_versions));
}
sub get_ceph_versions {
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH v1 pve-manager 2/8] ceph: tools: update Ceph version regex
Make the regex more maintainable declaring it as a variable, breaking it
up and commenting it by using the x flag.
Also remove the part that parses our Debian revision (e.g. -pve1) from
the version, as we do not actually include that in our Ceph builds.
The part of the regex that parses the build commit hash is made
mandatory (remove '?' after its group).
Signed-off-by: Max Carrara
---
PVE/Ceph/Tools.pm | 13 -
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/PVE/Ceph/Tools.pm b/PVE/Ceph/Tools.pm
index 087c4ef3..a00d23e1 100644
--- a/PVE/Ceph/Tools.pm
+++ b/PVE/Ceph/Tools.pm
@@ -68,7 +68,18 @@ sub get_local_version {
return undef if !defined $ceph_version;
-if ($ceph_version =~
/^ceph.*\sv?(\d+(?:\.\d+)+(?:-pve\d+)?)\s+(?:\(([a-zA-Z0-9]+)\))?/) {
+my $re_ceph_version = qr/
+ # Skip ahead to the version, which may optionally start with 'v'
+ ^ceph.*\sv?
+
+ # Parse the version X.Y, X.Y.Z, etc.
+ ( \d+ (?:\.\d+)+ ) \s+
+
+ # Parse the git commit hash between parentheses
+ (?: \( ([a-zA-Z0-9]+) \) )
+/x;
+
+if ($ceph_version =~ /$re_ceph_version/) {
my ($version, $buildcommit) = ($1, $2);
my $subversions = [ split(/\.|-/, $version) ];
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH v1 pve-manager 1/8] ceph: tools: refactor installation check as guard clause
Signed-off-by: Max Carrara
---
PVE/Ceph/Tools.pm | 37 +++--
1 file changed, 19 insertions(+), 18 deletions(-)
diff --git a/PVE/Ceph/Tools.pm b/PVE/Ceph/Tools.pm
index 9b97171e..087c4ef3 100644
--- a/PVE/Ceph/Tools.pm
+++ b/PVE/Ceph/Tools.pm
@@ -57,24 +57,25 @@ my $config_files = {
sub get_local_version {
my ($noerr) = @_;
-if (check_ceph_installed('ceph_bin', $noerr)) {
- my $ceph_version;
- run_command(
- [ $ceph_service->{ceph_bin}, '--version' ],
- noerr => $noerr,
- outfunc => sub { $ceph_version = shift if !defined $ceph_version },
- );
- return undef if !defined $ceph_version;
-
- if ($ceph_version =~
/^ceph.*\sv?(\d+(?:\.\d+)+(?:-pve\d+)?)\s+(?:\(([a-zA-Z0-9]+)\))?/) {
- my ($version, $buildcommit) = ($1, $2);
- my $subversions = [ split(/\.|-/, $version) ];
-
- # return (version, buildid, major, minor, ...) : major;
- return wantarray
- ? ($version, $buildcommit, $subversions)
- : $subversions->[0];
- }
+return undef if !check_ceph_installed('ceph_bin', $noerr);
+
+my $ceph_version;
+run_command(
+ [ $ceph_service->{ceph_bin}, '--version' ],
+ noerr => $noerr,
+ outfunc => sub { $ceph_version = shift if !defined $ceph_version },
+);
+
+return undef if !defined $ceph_version;
+
+if ($ceph_version =~
/^ceph.*\sv?(\d+(?:\.\d+)+(?:-pve\d+)?)\s+(?:\(([a-zA-Z0-9]+)\))?/) {
+ my ($version, $buildcommit) = ($1, $2);
+ my $subversions = [ split(/\.|-/, $version) ];
+
+ # return (version, buildid, major, minor, ...) : major;
+ return wantarray
+ ? ($version, $buildcommit, $subversions)
+ : $subversions->[0];
}
return undef;
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH v1 pve-manager 0/8] Ceph Build Commit in UI
Ceph Build Commit in UI - Version 1 === This series adds Ceph's build commit to the UI and lets the user know if a service is running an outdated build and therefore ought to be restarted. The build commit is now displayed next to the version for all Ceph services like so: 18.2.2 (abcd1234) Should a service run an outdated build, the new build commit is also displayed: 18.2.2 (abcd1234 -> 5678fedc) (Icons are omitted here). See the individual patches for more in-depth information. Additionally, some of the code was also cleaned up and refactored a little along the way. I'm not 100% sure if the design I've opted for here is the best, so it would be great to get some opinions on this. The OSD tree/list view especially can get a little noisy if there are a lot of outdated OSDs running. Summary of Changes -- Max Carrara (8): ceph: tools: refactor installation check as guard clause ceph: tools: update Ceph version regex ceph: services: remove old cluster broadcast ceph: services: refactor version existence check as guard clause utils: align regex of parse_ceph_version with Perl equivalent ui: ceph: services: parse and display build commit api: ceph: add build commit of host to Ceph osd index endpoint data ui: ceph: osd: rework rendering of version field & show build commit PVE/API2/Ceph/OSD.pm | 1 + PVE/Ceph/Services.pm | 38 +++--- PVE/Ceph/Tools.pm| 48 +--- www/manager6/Utils.js| 17 +- www/manager6/ceph/OSD.js | 55 ++-- www/manager6/ceph/ServiceList.js | 34 +++- www/manager6/ceph/Services.js| 14 +++- 7 files changed, 149 insertions(+), 58 deletions(-) -- 2.39.2 ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH installer 0/2] fix 2 cosmetic glitches with the tests.
while testing a patch-series today I saw quite a few: ``` Use of uninitialized value... ``` warnings from running the tests. While the issues are cosmetic, and don't harm, the actual build - they were enough to distract me for 10 minutes, which I want to spare others (including my future self). Stoiko Ivanov (2): d/control: add geoip-bin to Build-Depends tests: prevent uninitialized value warning with undef as fqdn debian/control | 1 + test/parse-fqdn.pl | 5 +++-- 2 files changed, 4 insertions(+), 2 deletions(-) -- 2.39.2 ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH installer 2/2] tests: prevent uninitialized value warning with undef as fqdn
cosmetic issue - but was distracting enough to make me look if there's
an error.
Signed-off-by: Stoiko Ivanov
---
test/parse-fqdn.pl | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/test/parse-fqdn.pl b/test/parse-fqdn.pl
index 6638fbe..47e0e21 100755
--- a/test/parse-fqdn.pl
+++ b/test/parse-fqdn.pl
@@ -24,9 +24,10 @@ sub is_parsed {
sub is_invalid {
my ($fqdn, $expected_err) = @_;
+my $print_fqdn = $fqdn // '(undefined)';
my $parsed = eval { parse_fqdn($fqdn) };
-is($parsed, undef, "invalid FQDN did fail parsing: $fqdn");
-is($@, $expected_err, "invalid FQDN threw correct error: $fqdn");
+is($parsed, undef, "invalid FQDN did fail parsing: $print_fqdn");
+is($@, $expected_err, "invalid FQDN threw correct error: $print_fqdn");
}
is_invalid(undef, ERR_EMPTY);
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH installer 1/2] d/control: add geoip-bin to Build-Depends
else the tests running: `./proxmox-low-level-installer -t test.img dump-env` print quite a few warnings about the use of uninitialized values (though they still continue happily). This was a slight distraction for me. Signed-off-by: Stoiko Ivanov --- debian/control | 1 + 1 file changed, 1 insertion(+) diff --git a/debian/control b/debian/control index eb4d3be..afe3c70 100644 --- a/debian/control +++ b/debian/control @@ -4,6 +4,7 @@ Priority: optional Maintainer: Proxmox Support Team Build-Depends: cargo:native, debhelper-compat (= 12), + geoip-bin, iproute2, iso-codes, libgtk3-perl, -- 2.39.2 ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH installer v3 0/2] assistant: keep prepared iso bootable on uefi with flash drives
for completeness sake - gave the v3 a quick spin as well - so also from my side the: Reviewed-by: Stoiko Ivanov Tested-by: Stoiko Ivanov still applies :) On Tue, 30 Apr 2024 12:46:07 +0200 Aaron Lauterer wrote: > booting a prepared iso in UEFI mode from a blockdev (e.g. usb flash > drive) did not work as grub could not find the partition. > > we now read the uuid / volume_date from the source iso and always set it > explictly to the same value when injecting files. > > more details in the actual commit message > > the second patch is a style patch > > this version should now include everything. sorry for the noise :) > > changes since: > v2: > * add import of format_err that was missed in v2 > v1: > * improve error handling in case xorriso does return empty output > > Aaron Lauterer (2): > assistant: keep prepared iso bootable on uefi with flash drives > assistant: use single dash for xorriso parameter > > proxmox-auto-install-assistant/src/main.rs | 48 +++--- > 1 file changed, 43 insertions(+), 5 deletions(-) > ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH v2 container 2/2] setup: unlink default netplan configuration even with Ubuntu >= 23.04
It seems like commit 02d9462 ("setup: enable systemd-networkd via
preset for ubuntu 23.04+") also resulted in the default netplan
configuration no longer being unlinked. That should still happen, even
if systemd-networkd is now enabled via preset. Otherwise, the network
configuration created by Proxmox VE is not ordered before the one
generated by netplan and thus not applied by systemd-networkd.
Reported in the community forum:
https://forum.proxmox.com/threads/145848/post-658058
Fixes: 02d9462 ("setup: enable systemd-networkd via preset for ubuntu 23.04+")
Signed-off-by: Fiona Ebner
---
New in v2.
src/PVE/LXC/Setup/Ubuntu.pm | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/PVE/LXC/Setup/Ubuntu.pm b/src/PVE/LXC/Setup/Ubuntu.pm
index cea8ef5..897eab9 100644
--- a/src/PVE/LXC/Setup/Ubuntu.pm
+++ b/src/PVE/LXC/Setup/Ubuntu.pm
@@ -73,7 +73,9 @@ sub template_fixup {
'/etc/systemd/system/multi-user.target.wants/systemd-networkd.service');
$self->ct_symlink('/lib/systemd/system/systemd-networkd.socket',
'/etc/systemd/system/socket.target.wants/systemd-networkd.socket');
+}
+if ($version >= '17.10') {
# unlink default netplan lxc config
$self->ct_unlink('/etc/netplan/10-lxc.yaml');
}
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH v2 container 1/2] setup: support Ubuntu 24.04 Noble
Minimally tested, that an upgrade from an existing 23.04 container
works, there still is network and no obviously bad messages in the
container's journal.
Reported in the community forum:
https://forum.proxmox.com/threads/145848/
Signed-off-by: Fiona Ebner
---
No changes in v2.
src/PVE/LXC/Setup/Ubuntu.pm | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/PVE/LXC/Setup/Ubuntu.pm b/src/PVE/LXC/Setup/Ubuntu.pm
index 905cacb..cea8ef5 100644
--- a/src/PVE/LXC/Setup/Ubuntu.pm
+++ b/src/PVE/LXC/Setup/Ubuntu.pm
@@ -12,6 +12,7 @@ use PVE::LXC::Setup::Debian;
use base qw(PVE::LXC::Setup::Debian);
my $known_versions = {
+'24.04' => 1, # noble
'23.10' => 1, # mantic
'23.04' => 1, # lunar
'22.10' => 1, # kinetic
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH manager] ui: Remove pveACMEPluginView in favor of pmxACMEPluginView
Sent a patch v2: https://lists.proxmox.com/pipermail/pve-devel/2024-April/063764.html On 29/04/2024 13:14, Thomas Lamprecht wrote: subject: ui: acme: switch plugin view over to the one from widget-toolkit (having internal xtypes in the subject already is not really that useful) Am 29/08/2023 um 13:00 schrieb Filip Schauer: Remove pveACMEPluginView and use the ACMEPluginView from the proxmox-widget-toolkit instead. This leaves pveACMEPluginEditor unused, so remove it as well. when got this moved, would be good to have some references here, which would it also make it easier to decide if we need a new bump of the verisoned dependency in d/control. ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH v2 manager] ui: acme: switch cluster view items over to those from widget-toolkit
The pmxACMEAccountView & pmxACMEPluginView in proxmox-widget-toolkit
were copied from pve-manager in commits 5df894de and 658bfdff. This
makes pveACMEAccountView & pveACMEPluginView redundant, hence remove
them and use pmxACMEAccountView & pmxACMEPluginView instead.
This leaves PVE.node.ACMEAccountView & pveACMEPluginEditor unused, so
remove them too.
Signed-off-by: Filip Schauer
---
Changes since v1:
* Also switch over account view
* Correct acmeUrl (/config/acme -> /cluster/acme)
* Reference introduction of pmxACMEAccountView & pmxACMEPluginView in
commit message
* Remove ACMEPluginEdit.js from the Makefile
www/manager6/Makefile | 1 -
www/manager6/dc/ACMEClusterView.js | 204 +-
www/manager6/dc/ACMEPluginEdit.js | 223 -
www/manager6/node/ACME.js | 66 -
4 files changed, 4 insertions(+), 490 deletions(-)
delete mode 100644 www/manager6/dc/ACMEPluginEdit.js
diff --git a/www/manager6/Makefile b/www/manager6/Makefile
index 2c3a822b..a1ad4698 100644
--- a/www/manager6/Makefile
+++ b/www/manager6/Makefile
@@ -148,7 +148,6 @@ JSSRC=
\
ha/StatusView.js\
dc/ACLView.js \
dc/ACMEClusterView.js \
- dc/ACMEPluginEdit.js\
dc/AuthEditBase.js \
dc/AuthEditAD.js\
dc/AuthEditLDAP.js \
diff --git a/www/manager6/dc/ACMEClusterView.js
b/www/manager6/dc/ACMEClusterView.js
index d02aeef0..d2ffde17 100644
--- a/www/manager6/dc/ACMEClusterView.js
+++ b/www/manager6/dc/ACMEClusterView.js
@@ -18,204 +18,6 @@ Ext.define('pve-acme-plugins', {
idProperty: 'plugin',
});
-Ext.define('PVE.dc.ACMEAccountView', {
-extend: 'Ext.grid.Panel',
-alias: 'widget.pveACMEAccountView',
-
-title: gettext('Accounts'),
-
-controller: {
- xclass: 'Ext.app.ViewController',
-
- addAccount: function() {
- let me = this;
- let view = me.getView();
- let defaultExists = view.getStore().findExact('name', 'default')
!== -1;
- Ext.create('PVE.node.ACMEAccountCreate', {
- defaultExists,
- taskDone: function() {
- me.reload();
- },
- }).show();
- },
-
- viewAccount: function() {
- let me = this;
- let view = me.getView();
- let selection = view.getSelection();
- if (selection.length < 1) return;
- Ext.create('PVE.node.ACMEAccountView', {
- accountname: selection[0].data.name,
- }).show();
- },
-
- reload: function() {
- let me = this;
- let view = me.getView();
- view.getStore().rstore.load();
- },
-
- showTaskAndReload: function(options, success, response) {
- let me = this;
- if (!success) return;
-
- let upid = response.result.data;
- Ext.create('Proxmox.window.TaskProgress', {
- upid,
- taskDone: function() {
- me.reload();
- },
- }).show();
- },
-},
-
-minHeight: 150,
-emptyText: gettext('No Accounts configured'),
-
-columns: [
- {
- dataIndex: 'name',
- text: gettext('Name'),
- renderer: Ext.String.htmlEncode,
- flex: 1,
- },
-],
-
-tbar: [
- {
- xtype: 'proxmoxButton',
- text: gettext('Add'),
- selModel: false,
- handler: 'addAccount',
- },
- {
- xtype: 'proxmoxButton',
- text: gettext('View'),
- handler: 'viewAccount',
- disabled: true,
- },
- {
- xtype: 'proxmoxStdRemoveButton',
- baseurl: '/cluster/acme/account',
- callback: 'showTaskAndReload',
- },
-],
-
-listeners: {
- itemdblclick: 'viewAccount',
-},
-
-store: {
- type: 'diff',
- autoDestroy: true,
- autoDestroyRstore: true,
- rstore: {
- type: 'update',
- storeid: 'pve-acme-accounts',
- model: 'pve-acme-accounts',
- autoStart: true,
- },
- sorters: 'name',
-},
-});
-
-Ext.define('PVE.dc.ACMEPluginView', {
-extend: 'Ext.grid.Panel',
-alias: 'widget.pveACMEPluginView',
-
-title: gettext('Challenge Plugins'),
-
-controller: {
- xclass: 'Ext.app.ViewController',
-
- addPlugin: function() {
- let me = this;
- Ext.create('PVE.dc.ACMEPluginEditor', {
- isCreate: true,
- apiCallDone: function() {
- me.reload();
- },
- }).show();
- },
-
- editPlugin:
[pve-devel] [PATCH installer v3 1/2] assistant: keep prepared iso bootable on uefi with flash drives
By mapping files into the ISO, the UUID for the partitions change as
they depend on the timestamp. The result is, that grub cannot find its
partition anymore and the user ends up on the grub shell.
This only happens when booting from a blockdev in UEFI mode. E.g. a USB
flash drive. Alternatively one can `dd` the ISO to a small (2GiB) VM
disk and mark it as the first boot device.
When booting in legacy mode or via CDROM (e.g. pass through via IPMI),
it worked.
Xorriso can report the commands needed to recreate the source ISO. The
'-volume_date uuid' is the one needed to override the same UUIDs. We
therefore read it first from the source iso and pass it as parameter
whenever we inject a file into the iso.
Signed-off-by: Aaron Lauterer
Reviewed-by: Stoiko Ivanov
Tested-by: Stoiko Ivanov
---
changes since:
v2:
* add missing import of format_err to patch
v1:
* improve error handling should xorriso return empty output
proxmox-auto-install-assistant/src/main.rs | 46 --
1 file changed, 42 insertions(+), 4 deletions(-)
diff --git a/proxmox-auto-install-assistant/src/main.rs
b/proxmox-auto-install-assistant/src/main.rs
index 0debd29..f8e5ed0 100644
--- a/proxmox-auto-install-assistant/src/main.rs
+++ b/proxmox-auto-install-assistant/src/main.rs
@@ -1,4 +1,4 @@
-use anyhow::{bail, Result};
+use anyhow::{bail, format_err, Result};
use clap::{Args, Parser, Subcommand, ValueEnum};
use glob::Pattern;
use regex::Regex;
@@ -276,6 +276,7 @@ fn show_system_info(_args: ) ->
Result<()> {
fn prepare_iso(args: ) -> Result<()> {
check_prepare_requirements(args)?;
+let uuid = get_iso_uuid()?;
if args.fetch_from == FetchAnswerFrom::Iso && args.answer_file.is_none() {
bail!("Missing path to the answer file required for the fetch-from
'iso' mode.");
@@ -331,10 +332,15 @@ fn prepare_iso(args: ) -> Result<()> {
instmode_file_tmp.push("auto-installer-mode.toml");
fs::write(_file_tmp, toml::to_string_pretty()?)?;
-inject_file_to_iso(_iso, _file_tmp,
"/auto-installer-mode.toml")?;
+inject_file_to_iso(
+_iso,
+_file_tmp,
+"/auto-installer-mode.toml",
+,
+)?;
if let Some(answer_file) = _file {
-inject_file_to_iso(_iso, answer_file, "/answer.toml")?;
+inject_file_to_iso(_iso, answer_file, "/answer.toml", )?;
}
println!("Moving prepared ISO to target location...");
@@ -371,11 +377,14 @@ fn final_iso_location(args: ) ->
PathBuf {
target.to_path_buf()
}
-fn inject_file_to_iso(iso: , file: , location: ) ->
Result<()> {
+fn inject_file_to_iso(iso: , file: , location: , uuid:
) -> Result<()> {
let result = Command::new("xorriso")
.arg("--boot_image")
.arg("any")
.arg("keep")
+.arg("-volume_date")
+.arg("uuid")
+.arg(uuid)
.arg("-dev")
.arg(iso)
.arg("-map")
@@ -391,6 +400,35 @@ fn inject_file_to_iso(iso: , file: ,
location: ) -> Result<(
Ok(())
}
+fn get_iso_uuid(iso: ) -> Result {
+let result = Command::new("xorriso")
+.arg("-dev")
+.arg(iso)
+.arg("-report_system_area")
+.arg("cmd")
+.output()?;
+if !result.status.success() {
+bail!(
+"Error determining the UUID of the source ISO: {}",
+String::from_utf8_lossy()
+);
+}
+let mut uuid = String::new();
+for line in String::from_utf8(result.stdout)?.lines() {
+if line.starts_with("-volume_date uuid") {
+uuid = line
+.split(' ')
+.last()
+.ok_or_else(|| format_err!("xorriso did behave unexpextedly"))?
+.replace('\'', "")
+.trim()
+.into();
+break;
+}
+}
+Ok(uuid)
+}
+
fn get_disks() -> Result>> {
let unwantend_block_devs = vec![
"ram[0-9]*",
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH installer v3 0/2] assistant: keep prepared iso bootable on uefi with flash drives
booting a prepared iso in UEFI mode from a blockdev (e.g. usb flash drive) did not work as grub could not find the partition. we now read the uuid / volume_date from the source iso and always set it explictly to the same value when injecting files. more details in the actual commit message the second patch is a style patch this version should now include everything. sorry for the noise :) changes since: v2: * add import of format_err that was missed in v2 v1: * improve error handling in case xorriso does return empty output Aaron Lauterer (2): assistant: keep prepared iso bootable on uefi with flash drives assistant: use single dash for xorriso parameter proxmox-auto-install-assistant/src/main.rs | 48 +++--- 1 file changed, 43 insertions(+), 5 deletions(-) -- 2.39.2 ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH installer v3 2/2] assistant: use single dash for xorriso parameter
while it works with two, one is what is shown in the man page and what
we already use for the other paramters.
Signed-off-by: Aaron Lauterer
---
proxmox-auto-install-assistant/src/main.rs | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/proxmox-auto-install-assistant/src/main.rs
b/proxmox-auto-install-assistant/src/main.rs
index f8e5ed0..1447175 100644
--- a/proxmox-auto-install-assistant/src/main.rs
+++ b/proxmox-auto-install-assistant/src/main.rs
@@ -379,7 +379,7 @@ fn final_iso_location(args: ) -> PathBuf {
fn inject_file_to_iso(iso: , file: , location: , uuid:
) -> Result<()> {
let result = Command::new("xorriso")
-.arg("--boot_image")
+.arg("-boot_image")
.arg("any")
.arg("keep")
.arg("-volume_date")
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH installer v2 0/2] assistant: keep prepared iso bootable on uefi with flash drives
sorry for the noise, I missed one part of the patch... will send a v3 :-/ On 2024-04-30 12:39, Aaron Lauterer wrote: booting a prepared iso in UEFI mode from a blockdev (e.g. usb flash drive) did not work as grub could not find the partition. we now read the uuid / volume_date from the source iso and always set it explictly to the same value when injecting files. more details in the actual commit message the second patch is a style patch Aaron Lauterer (2): assistant: keep prepared iso bootable on uefi with flash drives assistant: use single dash for xorriso parameter proxmox-auto-install-assistant/src/main.rs | 46 -- 1 file changed, 42 insertions(+), 4 deletions(-) ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH installer v2 1/2] assistant: keep prepared iso bootable on uefi with flash drives
By mapping files into the ISO, the UUID for the partitions change as
they depend on the timestamp. The result is, that grub cannot find its
partition anymore and the user ends up on the grub shell.
This only happens when booting from a blockdev in UEFI mode. E.g. a USB
flash drive. Alternatively one can `dd` the ISO to a small (2GiB) VM
disk and mark it as the first boot device.
When booting in legacy mode or via CDROM (e.g. pass through via IPMI), it
worked.
Xorriso can report the commands needed to recreate the source ISO. The
'-volume_date uuid' is the one needed to override the same UUIDs. We
therefore read it first from the source iso and pass it as parameter
whenever we inject a file into the iso.
Signed-off-by: Aaron Lauterer
Reviewed-by: Stoiko Ivanov
Tested-by: Stoiko Ivanov
---
changes since v1:
improve error handling in case xorriso doesn't return anything
proxmox-auto-install-assistant/src/main.rs | 44 --
1 file changed, 41 insertions(+), 3 deletions(-)
diff --git a/proxmox-auto-install-assistant/src/main.rs
b/proxmox-auto-install-assistant/src/main.rs
index 0debd29..ef471f3 100644
--- a/proxmox-auto-install-assistant/src/main.rs
+++ b/proxmox-auto-install-assistant/src/main.rs
@@ -276,6 +276,7 @@ fn show_system_info(_args: ) ->
Result<()> {
fn prepare_iso(args: ) -> Result<()> {
check_prepare_requirements(args)?;
+let uuid = get_iso_uuid()?;
if args.fetch_from == FetchAnswerFrom::Iso && args.answer_file.is_none() {
bail!("Missing path to the answer file required for the fetch-from
'iso' mode.");
@@ -331,10 +332,15 @@ fn prepare_iso(args: ) -> Result<()> {
instmode_file_tmp.push("auto-installer-mode.toml");
fs::write(_file_tmp, toml::to_string_pretty()?)?;
-inject_file_to_iso(_iso, _file_tmp,
"/auto-installer-mode.toml")?;
+inject_file_to_iso(
+_iso,
+_file_tmp,
+"/auto-installer-mode.toml",
+,
+)?;
if let Some(answer_file) = _file {
-inject_file_to_iso(_iso, answer_file, "/answer.toml")?;
+inject_file_to_iso(_iso, answer_file, "/answer.toml", )?;
}
println!("Moving prepared ISO to target location...");
@@ -371,11 +377,14 @@ fn final_iso_location(args: ) ->
PathBuf {
target.to_path_buf()
}
-fn inject_file_to_iso(iso: , file: , location: ) ->
Result<()> {
+fn inject_file_to_iso(iso: , file: , location: , uuid:
) -> Result<()> {
let result = Command::new("xorriso")
.arg("--boot_image")
.arg("any")
.arg("keep")
+.arg("-volume_date")
+.arg("uuid")
+.arg(uuid)
.arg("-dev")
.arg(iso)
.arg("-map")
@@ -391,6 +400,35 @@ fn inject_file_to_iso(iso: , file: ,
location: ) -> Result<(
Ok(())
}
+fn get_iso_uuid(iso: ) -> Result {
+let result = Command::new("xorriso")
+.arg("-dev")
+.arg(iso)
+.arg("-report_system_area")
+.arg("cmd")
+.output()?;
+if !result.status.success() {
+bail!(
+"Error determining the UUID of the source ISO: {}",
+String::from_utf8_lossy()
+);
+}
+let mut uuid = String::new();
+for line in String::from_utf8(result.stdout)?.lines() {
+if line.starts_with("-volume_date uuid") {
+uuid = line
+.split(' ')
+.last()
+.ok_or_else(|| format_err!("xorriso did behave unexpextedly"))?
+.replace('\'', "")
+.trim()
+.into();
+break;
+}
+}
+Ok(uuid)
+}
+
fn get_disks() -> Result>> {
let unwantend_block_devs = vec![
"ram[0-9]*",
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH installer v2 2/2] assistant: use single dash for xorriso parameter
while it works with two, one is what is shown in the man page and what
we already use for the other paramters.
Signed-off-by: Aaron Lauterer
---
proxmox-auto-install-assistant/src/main.rs | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/proxmox-auto-install-assistant/src/main.rs
b/proxmox-auto-install-assistant/src/main.rs
index ef471f3..27e4a45 100644
--- a/proxmox-auto-install-assistant/src/main.rs
+++ b/proxmox-auto-install-assistant/src/main.rs
@@ -379,7 +379,7 @@ fn final_iso_location(args: ) -> PathBuf {
fn inject_file_to_iso(iso: , file: , location: , uuid:
) -> Result<()> {
let result = Command::new("xorriso")
-.arg("--boot_image")
+.arg("-boot_image")
.arg("any")
.arg("keep")
.arg("-volume_date")
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH installer v2 0/2] assistant: keep prepared iso bootable on uefi with flash drives
booting a prepared iso in UEFI mode from a blockdev (e.g. usb flash drive) did not work as grub could not find the partition. we now read the uuid / volume_date from the source iso and always set it explictly to the same value when injecting files. more details in the actual commit message the second patch is a style patch Aaron Lauterer (2): assistant: keep prepared iso bootable on uefi with flash drives assistant: use single dash for xorriso parameter proxmox-auto-install-assistant/src/main.rs | 46 -- 1 file changed, 42 insertions(+), 4 deletions(-) -- 2.39.2 ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH 1/2] assistant: keep prepared iso bootable on uefi with flash drives
On 2024-04-30 11:41, Stefan Sterz wrote:
On Tue Apr 30, 2024 at 10:54 AM CEST, Aaron Lauterer wrote:
By mapping files into the ISO, the UUID for the partitions change as
they depend on the timestamp. The result is, that grub cannot find its
partition anymore and the user ends up on the grub shell.
This only happens when booting from a blockdev in UEFI mode. E.g. a USB
flash drive. Alternatively one can `dd` the ISO to a small (2GiB) VM
disk and mark it as the first boot device.
Booting in legacy mode or via CDROM (e.g. pass through via IPMI), it
worked.
Xorriso can report the commands needed to recreate the source ISO. The
'-volume_date uuid' is the one needed to override the same UUIDs. We
therefore read it first from the source iso and pass it as parameter
whenever we inject a file into the iso.
Signed-off-by: Aaron Lauterer
---
proxmox-auto-install-assistant/src/main.rs | 44 --
1 file changed, 41 insertions(+), 3 deletions(-)
+fn get_iso_uuid(iso: ) -> Result {
+let result = Command::new("xorriso")
+.arg("-dev")
+.arg(iso)
+.arg("-report_system_area")
+.arg("cmd")
+.output()?;
+if !result.status.success() {
+bail!(
+"Error determining the UUID of the source ISO: {}",
+String::from_utf8_lossy()
+);
+}
+let mut uuid = String::new();
+for line in String::from_utf8(result.stdout)?.lines() {
+if line.starts_with("-volume_date uuid") {
+uuid = line
+.split(' ')
+.last()
+.unwrap()
nit: while this probably won't happen, if xorriso ever starts returning
nothing to the above command, this unwrap may panic. it might be nice to
do a `ok_or_else(|| bail!("xorisso command behaved unexpectedly"))?` or
similar here.
thanks. will send a v2
+.replace('\'', "")
+.trim()
+.into();
+break;
+}
+}
+Ok(uuid)
+}
+
fn get_disks() -> Result>> {
let unwantend_block_devs = vec![
"ram[0-9]*",
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH 1/2] assistant: keep prepared iso bootable on uefi with flash drives
gave this and the next patch a spin on 2 test-servers and one VM, where I
could reproduce the issue yesterday -
* execsnoop-bpfcc says the patch does what it says on the tin
* automated installs worked.
tiny nit: subject prefix and a cover-letter would have helped
the following goes for both patches:
Reviewed-by: Stoiko Ivanov
Tested-by: Stoiko Ivanov
On Tue, 30 Apr 2024 10:54:33 +0200
Aaron Lauterer wrote:
> By mapping files into the ISO, the UUID for the partitions change as
> they depend on the timestamp. The result is, that grub cannot find its
> partition anymore and the user ends up on the grub shell.
>
> This only happens when booting from a blockdev in UEFI mode. E.g. a USB
> flash drive. Alternatively one can `dd` the ISO to a small (2GiB) VM
> disk and mark it as the first boot device.
>
> Booting in legacy mode or via CDROM (e.g. pass through via IPMI), it
> worked.
>
> Xorriso can report the commands needed to recreate the source ISO. The
> '-volume_date uuid' is the one needed to override the same UUIDs. We
> therefore read it first from the source iso and pass it as parameter
> whenever we inject a file into the iso.
>
> Signed-off-by: Aaron Lauterer
> ---
> proxmox-auto-install-assistant/src/main.rs | 44 --
> 1 file changed, 41 insertions(+), 3 deletions(-)
>
> diff --git a/proxmox-auto-install-assistant/src/main.rs
> b/proxmox-auto-install-assistant/src/main.rs
> index 0debd29..e9213f7 100644
> --- a/proxmox-auto-install-assistant/src/main.rs
> +++ b/proxmox-auto-install-assistant/src/main.rs
> @@ -276,6 +276,7 @@ fn show_system_info(_args: ) ->
> Result<()> {
>
> fn prepare_iso(args: ) -> Result<()> {
> check_prepare_requirements(args)?;
> +let uuid = get_iso_uuid()?;
>
> if args.fetch_from == FetchAnswerFrom::Iso && args.answer_file.is_none()
> {
> bail!("Missing path to the answer file required for the fetch-from
> 'iso' mode.");
> @@ -331,10 +332,15 @@ fn prepare_iso(args: ) -> Result<()> {
> instmode_file_tmp.push("auto-installer-mode.toml");
> fs::write(_file_tmp, toml::to_string_pretty()?)?;
>
> -inject_file_to_iso(_iso, _file_tmp,
> "/auto-installer-mode.toml")?;
> +inject_file_to_iso(
> +_iso,
> +_file_tmp,
> +"/auto-installer-mode.toml",
> +,
> +)?;
>
> if let Some(answer_file) = _file {
> -inject_file_to_iso(_iso, answer_file, "/answer.toml")?;
> +inject_file_to_iso(_iso, answer_file, "/answer.toml", )?;
> }
>
> println!("Moving prepared ISO to target location...");
> @@ -371,11 +377,14 @@ fn final_iso_location(args: ) ->
> PathBuf {
> target.to_path_buf()
> }
>
> -fn inject_file_to_iso(iso: , file: , location: ) ->
> Result<()> {
> +fn inject_file_to_iso(iso: , file: , location: , uuid:
> ) -> Result<()> {
> let result = Command::new("xorriso")
> .arg("--boot_image")
> .arg("any")
> .arg("keep")
> +.arg("-volume_date")
> +.arg("uuid")
> +.arg(uuid)
> .arg("-dev")
> .arg(iso)
> .arg("-map")
> @@ -391,6 +400,35 @@ fn inject_file_to_iso(iso: , file: ,
> location: ) -> Result<(
> Ok(())
> }
>
> +fn get_iso_uuid(iso: ) -> Result {
> +let result = Command::new("xorriso")
> +.arg("-dev")
> +.arg(iso)
> +.arg("-report_system_area")
> +.arg("cmd")
> +.output()?;
> +if !result.status.success() {
> +bail!(
> +"Error determining the UUID of the source ISO: {}",
> +String::from_utf8_lossy()
> +);
> +}
> +let mut uuid = String::new();
> +for line in String::from_utf8(result.stdout)?.lines() {
> +if line.starts_with("-volume_date uuid") {
> +uuid = line
> +.split(' ')
> +.last()
> +.unwrap()
> +.replace('\'', "")
> +.trim()
> +.into();
> +break;
> +}
> +}
> +Ok(uuid)
> +}
> +
> fn get_disks() -> Result>> {
> let unwantend_block_devs = vec![
> "ram[0-9]*",
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH storage 1/2] don't bail on whitespaces in backing devices
On 4/30/24 11:13, Fiona Ebner wrote: Am 30.04.24 um 10:38 schrieb Wolfgang Bumiller: - technically users could add a disk with a "bad" parent to a storage *manually*, but given the list_images mentioned above, I'd argue the situation isn't really getting worse, as values that *do* match `\S+` don't necessarily match the regexes used *later* on the parent *anyway*... CC Dominik Thinking in the context of uploading OVAs (or uploading disk images), I guess we need a check against arbitrary backing file paths in uploaded qcow2/vmdk images (or do we already have that)? good point, we currently don't, but it shouldn't be hard to add that check before importing/after uploading... i'll look into that ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH 1/2] assistant: keep prepared iso bootable on uefi with flash drives
On Tue Apr 30, 2024 at 10:54 AM CEST, Aaron Lauterer wrote:
> By mapping files into the ISO, the UUID for the partitions change as
> they depend on the timestamp. The result is, that grub cannot find its
> partition anymore and the user ends up on the grub shell.
>
> This only happens when booting from a blockdev in UEFI mode. E.g. a USB
> flash drive. Alternatively one can `dd` the ISO to a small (2GiB) VM
> disk and mark it as the first boot device.
>
> Booting in legacy mode or via CDROM (e.g. pass through via IPMI), it
> worked.
>
> Xorriso can report the commands needed to recreate the source ISO. The
> '-volume_date uuid' is the one needed to override the same UUIDs. We
> therefore read it first from the source iso and pass it as parameter
> whenever we inject a file into the iso.
>
> Signed-off-by: Aaron Lauterer
> ---
> proxmox-auto-install-assistant/src/main.rs | 44 --
> 1 file changed, 41 insertions(+), 3 deletions(-)
>
> diff --git a/proxmox-auto-install-assistant/src/main.rs
> b/proxmox-auto-install-assistant/src/main.rs
> index 0debd29..e9213f7 100644
> --- a/proxmox-auto-install-assistant/src/main.rs
> +++ b/proxmox-auto-install-assistant/src/main.rs
> @@ -276,6 +276,7 @@ fn show_system_info(_args: ) ->
> Result<()> {
>
> fn prepare_iso(args: ) -> Result<()> {
> check_prepare_requirements(args)?;
> +let uuid = get_iso_uuid()?;
>
> if args.fetch_from == FetchAnswerFrom::Iso && args.answer_file.is_none()
> {
> bail!("Missing path to the answer file required for the fetch-from
> 'iso' mode.");
> @@ -331,10 +332,15 @@ fn prepare_iso(args: ) -> Result<()> {
> instmode_file_tmp.push("auto-installer-mode.toml");
> fs::write(_file_tmp, toml::to_string_pretty()?)?;
>
> -inject_file_to_iso(_iso, _file_tmp,
> "/auto-installer-mode.toml")?;
> +inject_file_to_iso(
> +_iso,
> +_file_tmp,
> +"/auto-installer-mode.toml",
> +,
> +)?;
>
> if let Some(answer_file) = _file {
> -inject_file_to_iso(_iso, answer_file, "/answer.toml")?;
> +inject_file_to_iso(_iso, answer_file, "/answer.toml", )?;
> }
>
> println!("Moving prepared ISO to target location...");
> @@ -371,11 +377,14 @@ fn final_iso_location(args: ) ->
> PathBuf {
> target.to_path_buf()
> }
>
> -fn inject_file_to_iso(iso: , file: , location: ) ->
> Result<()> {
> +fn inject_file_to_iso(iso: , file: , location: , uuid:
> ) -> Result<()> {
> let result = Command::new("xorriso")
> .arg("--boot_image")
> .arg("any")
> .arg("keep")
> +.arg("-volume_date")
> +.arg("uuid")
> +.arg(uuid)
> .arg("-dev")
> .arg(iso)
> .arg("-map")
> @@ -391,6 +400,35 @@ fn inject_file_to_iso(iso: , file: ,
> location: ) -> Result<(
> Ok(())
> }
>
> +fn get_iso_uuid(iso: ) -> Result {
> +let result = Command::new("xorriso")
> +.arg("-dev")
> +.arg(iso)
> +.arg("-report_system_area")
> +.arg("cmd")
> +.output()?;
> +if !result.status.success() {
> +bail!(
> +"Error determining the UUID of the source ISO: {}",
> +String::from_utf8_lossy()
> +);
> +}
> +let mut uuid = String::new();
> +for line in String::from_utf8(result.stdout)?.lines() {
> +if line.starts_with("-volume_date uuid") {
> +uuid = line
> +.split(' ')
> +.last()
> +.unwrap()
nit: while this probably won't happen, if xorriso ever starts returning
nothing to the above command, this unwrap may panic. it might be nice to
do a `ok_or_else(|| bail!("xorisso command behaved unexpectedly"))?` or
similar here.
> +.replace('\'', "")
> +.trim()
> +.into();
> +break;
> +}
> +}
> +Ok(uuid)
> +}
> +
> fn get_disks() -> Result>> {
> let unwantend_block_devs = vec![
> "ram[0-9]*",
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH storage 1/2] don't bail on whitespaces in backing devices
On Tue, Apr 30, 2024 at 11:13:02AM +0200, Fiona Ebner wrote: > Am 30.04.24 um 10:38 schrieb Wolfgang Bumiller: > > On Tue, Apr 30, 2024 at 10:14:13AM +0200, Fiona Ebner wrote: > >> > >> So the returned $parent will now just be undef if it contains > >> whitespaces, even though there is a parent. Can't that cause issues > >> further down the line? If it's fine, a comment with the rationale would > >> be nice. > >> > >> Or should we rather allow whitespaces while matching and return it > >> properly? Or are there any issues with proper escaping then? > > > > I was a bit too quick on the send trigger there, but it should be fine > > IMO: > > > > - where we do run into this issue, we never use/need/care about the parent > > Maybe this part of the function could be guarded by wantarray already, > so callers caring only about the size don't even get there? But I > suppose we do notice other unexpected things earlier by always doing the > additional checks, so maybe it's better like it is right now. Unfortunately a lot of callers do also care about the format, specifically the import code, where this causes issues, so a `wantarray` check won't help there. ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH container] setup: support Ubuntu 24.04 Noble
Am 30.04.24 um 10:59 schrieb Thomas Lamprecht: > On 30/04/2024 10:43, Fiona Ebner wrote: >> So this is not new (already present for Ubuntu 23.10) and stems from the >> fact that these images from linuxcontainers.org contain: >> >>> root@CT113:~# cat /etc/netplan/10-lxc.yaml >>> network: >>> version: 2 >>> ethernets: >>> eth0: >>> dhcp4: true >>> dhcp-identifier: mac >> >> and that generates a configuration that will be ordered before >> ours/preferred by systemd-networkd: >> >>> root@CT113:~# networkctl status eth0 >>> ● 2: eth0 >>> >>> Link File: n/a >>> Network File: /run/systemd/network/10-netplan-eth0.network >> >> Should we still change something in the setup code? I suppose our >> template will not have the netplan configuration file and in a way it'd >> just be a race to the bottom of being ordered first. > > Why should there be a incentive for a race to the bottom? > What I mean is that template creators also have an incentive to order their configurations very early. And we have the incentive to order even earlier. But yes, "race" was the wrong word, because they do not have an incentive to order earlier than us. > If we have users running into this then yes, we should do something > about it, we do not have a hard requirement of the Ubuntu templates > being build through DAB and especially as we use the LXC template > builder (or well its artefacts) for other non-Debian images, I'd > see why users take it as a source. > > If the change in ordering is the correct solution I cannot say without > looking into all deeper – but I'm sure you can evaluate that. > One possibility might be disabling netplan on CT creation, if PVE wants > to control network in another way itself. I'll look into disabling netplan on creation. That sounds like a cleaner solution. ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH storage 1/2] don't bail on whitespaces in backing devices
Am 30.04.24 um 10:38 schrieb Wolfgang Bumiller: > On Tue, Apr 30, 2024 at 10:14:13AM +0200, Fiona Ebner wrote: >> >> So the returned $parent will now just be undef if it contains >> whitespaces, even though there is a parent. Can't that cause issues >> further down the line? If it's fine, a comment with the rationale would >> be nice. >> >> Or should we rather allow whitespaces while matching and return it >> properly? Or are there any issues with proper escaping then? > > I was a bit too quick on the send trigger there, but it should be fine > IMO: > > - where we do run into this issue, we never use/need/care about the parent Maybe this part of the function could be guarded by wantarray already, so callers caring only about the size don't even get there? But I suppose we do notice other unexpected things earlier by always doing the additional checks, so maybe it's better like it is right now. > - the parent info of file_size_info is usually discarded, or checked > against whether the disk is a "base volume" according to the storage's > idea of how such a volume has to be named (as in, it's created/managed > by pve) > or, eg. in Plugin.pm's `list_images` the parent is then checked > against a more specific regex and if it does not matched it is simply > discarded as if it was `undef`... (so we already have some logic > around backing-devices which "discards" unexpected values...) Hmm, okay. > - technically users could add a disk with a "bad" parent to a storage > *manually*, but given the list_images mentioned above, I'd argue the > situation isn't really getting worse, as values that *do* match `\S+` > don't necessarily match the regexes used *later* on the parent > *anyway*... > CC Dominik Thinking in the context of uploading OVAs (or uploading disk images), I guess we need a check against arbitrary backing file paths in uploaded qcow2/vmdk images (or do we already have that)? > So we could also just untaint with /^(.+)$/, since IMO if we end up with > actual whitespace issues anywhere *else*, then *that* could is the > broken one, not this one... > > 路 Fine by me, but after what you wrote, so is the current approach :) ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH container] setup: support Ubuntu 24.04 Noble
On 30/04/2024 10:43, Fiona Ebner wrote: > So this is not new (already present for Ubuntu 23.10) and stems from the > fact that these images from linuxcontainers.org contain: > >> root@CT113:~# cat /etc/netplan/10-lxc.yaml >> network: >> version: 2 >> ethernets: >> eth0: >> dhcp4: true >> dhcp-identifier: mac > > and that generates a configuration that will be ordered before > ours/preferred by systemd-networkd: > >> root@CT113:~# networkctl status eth0 >> ● 2: eth0 >> >> Link File: n/a >> Network File: /run/systemd/network/10-netplan-eth0.network > > Should we still change something in the setup code? I suppose our > template will not have the netplan configuration file and in a way it'd > just be a race to the bottom of being ordered first. Why should there be a incentive for a race to the bottom? If we have users running into this then yes, we should do something about it, we do not have a hard requirement of the Ubuntu templates being build through DAB and especially as we use the LXC template builder (or well its artefacts) for other non-Debian images, I'd see why users take it as a source. If the change in ordering is the correct solution I cannot say without looking into all deeper – but I'm sure you can evaluate that. One possibility might be disabling netplan on CT creation, if PVE wants to control network in another way itself. ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH 1/2] assistant: keep prepared iso bootable on uefi with flash drives
By mapping files into the ISO, the UUID for the partitions change as
they depend on the timestamp. The result is, that grub cannot find its
partition anymore and the user ends up on the grub shell.
This only happens when booting from a blockdev in UEFI mode. E.g. a USB
flash drive. Alternatively one can `dd` the ISO to a small (2GiB) VM
disk and mark it as the first boot device.
Booting in legacy mode or via CDROM (e.g. pass through via IPMI), it
worked.
Xorriso can report the commands needed to recreate the source ISO. The
'-volume_date uuid' is the one needed to override the same UUIDs. We
therefore read it first from the source iso and pass it as parameter
whenever we inject a file into the iso.
Signed-off-by: Aaron Lauterer
---
proxmox-auto-install-assistant/src/main.rs | 44 --
1 file changed, 41 insertions(+), 3 deletions(-)
diff --git a/proxmox-auto-install-assistant/src/main.rs
b/proxmox-auto-install-assistant/src/main.rs
index 0debd29..e9213f7 100644
--- a/proxmox-auto-install-assistant/src/main.rs
+++ b/proxmox-auto-install-assistant/src/main.rs
@@ -276,6 +276,7 @@ fn show_system_info(_args: ) ->
Result<()> {
fn prepare_iso(args: ) -> Result<()> {
check_prepare_requirements(args)?;
+let uuid = get_iso_uuid()?;
if args.fetch_from == FetchAnswerFrom::Iso && args.answer_file.is_none() {
bail!("Missing path to the answer file required for the fetch-from
'iso' mode.");
@@ -331,10 +332,15 @@ fn prepare_iso(args: ) -> Result<()> {
instmode_file_tmp.push("auto-installer-mode.toml");
fs::write(_file_tmp, toml::to_string_pretty()?)?;
-inject_file_to_iso(_iso, _file_tmp,
"/auto-installer-mode.toml")?;
+inject_file_to_iso(
+_iso,
+_file_tmp,
+"/auto-installer-mode.toml",
+,
+)?;
if let Some(answer_file) = _file {
-inject_file_to_iso(_iso, answer_file, "/answer.toml")?;
+inject_file_to_iso(_iso, answer_file, "/answer.toml", )?;
}
println!("Moving prepared ISO to target location...");
@@ -371,11 +377,14 @@ fn final_iso_location(args: ) ->
PathBuf {
target.to_path_buf()
}
-fn inject_file_to_iso(iso: , file: , location: ) ->
Result<()> {
+fn inject_file_to_iso(iso: , file: , location: , uuid:
) -> Result<()> {
let result = Command::new("xorriso")
.arg("--boot_image")
.arg("any")
.arg("keep")
+.arg("-volume_date")
+.arg("uuid")
+.arg(uuid)
.arg("-dev")
.arg(iso)
.arg("-map")
@@ -391,6 +400,35 @@ fn inject_file_to_iso(iso: , file: ,
location: ) -> Result<(
Ok(())
}
+fn get_iso_uuid(iso: ) -> Result {
+let result = Command::new("xorriso")
+.arg("-dev")
+.arg(iso)
+.arg("-report_system_area")
+.arg("cmd")
+.output()?;
+if !result.status.success() {
+bail!(
+"Error determining the UUID of the source ISO: {}",
+String::from_utf8_lossy()
+);
+}
+let mut uuid = String::new();
+for line in String::from_utf8(result.stdout)?.lines() {
+if line.starts_with("-volume_date uuid") {
+uuid = line
+.split(' ')
+.last()
+.unwrap()
+.replace('\'', "")
+.trim()
+.into();
+break;
+}
+}
+Ok(uuid)
+}
+
fn get_disks() -> Result>> {
let unwantend_block_devs = vec![
"ram[0-9]*",
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH 2/2] assistant: use single dash for xorriso parameter
while it works with two, one is what is shown in the man page and what
we already use for the other paramters.
Signed-off-by: Aaron Lauterer
---
proxmox-auto-install-assistant/src/main.rs | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/proxmox-auto-install-assistant/src/main.rs
b/proxmox-auto-install-assistant/src/main.rs
index e9213f7..afbac85 100644
--- a/proxmox-auto-install-assistant/src/main.rs
+++ b/proxmox-auto-install-assistant/src/main.rs
@@ -379,7 +379,7 @@ fn final_iso_location(args: ) -> PathBuf {
fn inject_file_to_iso(iso: , file: , location: , uuid:
) -> Result<()> {
let result = Command::new("xorriso")
-.arg("--boot_image")
+.arg("-boot_image")
.arg("any")
.arg("keep")
.arg("-volume_date")
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH container] setup: support Ubuntu 24.04 Noble
Am 29.04.24 um 13:11 schrieb Thomas Lamprecht: > Am 29/04/2024 um 11:56 schrieb Fiona Ebner: >> Am 29.04.24 um 11:36 schrieb Fiona Ebner: >>> Am 29.04.24 um 11:23 schrieb Fiona Ebner: Reported in the community forum: https://forum.proxmox.com/threads/145848/#post-658694 Signed-off-by: Fiona Ebner --- Minimally tested, that an upgrade from an existing 23.04 container works, there still is network and no obviously bad messages in the container's journal. >>> Hmm, while the upgrade did work, starting from an Ubuntu 24.04 template >>> and setting a static IP does not seem to work, like described here: >>> https://forum.proxmox.com/threads/145848/post-658058 >> >> Seems like the ordering of the configuration files is the issue. The >> following would fix it, but probably needs to be special-cased for new >> Ubuntu (or new systemd, would still need to check where the change came >> in exactly) not to mess up existing containers, right? > > Yes, at least that would reduce regression potential of unknown issues. So this is not new (already present for Ubuntu 23.10) and stems from the fact that these images from linuxcontainers.org contain: > root@CT113:~# cat /etc/netplan/10-lxc.yaml > network: > version: 2 > ethernets: > eth0: > dhcp4: true > dhcp-identifier: mac and that generates a configuration that will be ordered before ours/preferred by systemd-networkd: > root@CT113:~# networkctl status eth0 > ● 2: eth0 > > Link File: n/a > Network File: /run/systemd/network/10-netplan-eth0.network Should we still change something in the setup code? I suppose our template will not have the netplan configuration file and in a way it'd just be a race to the bottom of being ordered first. ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH storage 1/2] don't bail on whitespaces in backing devices
On Tue, Apr 30, 2024 at 10:14:13AM +0200, Fiona Ebner wrote:
> Am 30.04.24 um 09:53 schrieb Wolfgang Bumiller:
> > This prevents importing from vmdks with whitespaces in file names.
> > Further, some operations that include file sizes (like listing disks)
> > would potentially fail entirely if a custom disk with a badly name
> > backing device exists in a VM images directory since they don't expect
> > this. Specifically, since we don't necessarily know the actual naming
> > scheme of the current storage in the plain Plugin.pm version, we don't
> > check the full name anyway, so why bother with whitespaces...
> >
> > See-also:
> > https://forum.proxmox.com/threads/new-import-wizard-available-for-migrating-vmware-esxi-based-virtual-machines.144023/page-16#post-658697
> > Signed-off-by: Wolfgang Bumiller
> > ---
> > src/PVE/Storage/Plugin.pm | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
> > index 22a9729..683190b 100644
> > --- a/src/PVE/Storage/Plugin.pm
> > +++ b/src/PVE/Storage/Plugin.pm
> > @@ -982,7 +982,7 @@ sub file_size_info {
> > $used = int($used);
> > ($format) = ($format =~ /^(\S+)$/) or die "format '$format' includes
> > whitespace\n"; # untaint
> > if (defined($parent)) {
> > - ($parent) = ($parent =~ /^(\S+)$/) or die "parent '$parent' includes
> > whitespace\n"; # untaint
> > + ($parent) = ($parent =~ /^(\S+)$/); # untaint
> > }
> > return wantarray ? ($size, $format, $used, $parent, $st->ctime) :
> > $size;
> > }
>
> So the returned $parent will now just be undef if it contains
> whitespaces, even though there is a parent. Can't that cause issues
> further down the line? If it's fine, a comment with the rationale would
> be nice.
>
> Or should we rather allow whitespaces while matching and return it
> properly? Or are there any issues with proper escaping then?
I was a bit too quick on the send trigger there, but it should be fine
IMO:
- where we do run into this issue, we never use/need/care about the parent
- the parent info of file_size_info is usually discarded, or checked
against whether the disk is a "base volume" according to the storage's
idea of how such a volume has to be named (as in, it's created/managed
by pve)
or, eg. in Plugin.pm's `list_images` the parent is then checked
against a more specific regex and if it does not matched it is simply
discarded as if it was `undef`... (so we already have some logic
around backing-devices which "discards" unexpected values...)
- technically users could add a disk with a "bad" parent to a storage
*manually*, but given the list_images mentioned above, I'd argue the
situation isn't really getting worse, as values that *do* match `\S+`
don't necessarily match the regexes used *later* on the parent
*anyway*...
So we could also just untaint with /^(.+)$/, since IMO if we end up with
actual whitespace issues anywhere *else*, then *that* could is the
broken one, not this one...
路
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] vma extract force flag
Hi, Am 24.04.24 um 22:44 schrieb Ben Dailey: > I would like to keep extracted backups on an NFS server backed with ZFS and > retain the benefits of differential snapshots and it would be helpful to > have the vma extract replace the previous backups directly. why not just remove the previously extracted archive and then extract the new archive? Best Regards, Fiona ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied-series: [PATCH storage 1/2] don't bail on whitespaces in backing devices
On 30/04/2024 09:53, Wolfgang Bumiller wrote: > This prevents importing from vmdks with whitespaces in file names. > Further, some operations that include file sizes (like listing disks) > would potentially fail entirely if a custom disk with a badly name > backing device exists in a VM images directory since they don't expect > this. Specifically, since we don't necessarily know the actual naming > scheme of the current storage in the plain Plugin.pm version, we don't > check the full name anyway, so why bother with whitespaces... > > See-also: > https://forum.proxmox.com/threads/new-import-wizard-available-for-migrating-vmware-esxi-based-virtual-machines.144023/page-16#post-658697 FYI, forum links can be de-SEO'd like: https://forum.proxmox.com/threads/144023/page-16#post-658697 > Signed-off-by: Wolfgang Bumiller > --- > src/PVE/Storage/Plugin.pm | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > applied both patches, thanks! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH storage 1/2] don't bail on whitespaces in backing devices
Am 30.04.24 um 09:53 schrieb Wolfgang Bumiller:
> This prevents importing from vmdks with whitespaces in file names.
> Further, some operations that include file sizes (like listing disks)
> would potentially fail entirely if a custom disk with a badly name
> backing device exists in a VM images directory since they don't expect
> this. Specifically, since we don't necessarily know the actual naming
> scheme of the current storage in the plain Plugin.pm version, we don't
> check the full name anyway, so why bother with whitespaces...
>
> See-also:
> https://forum.proxmox.com/threads/new-import-wizard-available-for-migrating-vmware-esxi-based-virtual-machines.144023/page-16#post-658697
> Signed-off-by: Wolfgang Bumiller
> ---
> src/PVE/Storage/Plugin.pm | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
> index 22a9729..683190b 100644
> --- a/src/PVE/Storage/Plugin.pm
> +++ b/src/PVE/Storage/Plugin.pm
> @@ -982,7 +982,7 @@ sub file_size_info {
> $used = int($used);
> ($format) = ($format =~ /^(\S+)$/) or die "format '$format' includes
> whitespace\n"; # untaint
> if (defined($parent)) {
> - ($parent) = ($parent =~ /^(\S+)$/) or die "parent '$parent' includes
> whitespace\n"; # untaint
> + ($parent) = ($parent =~ /^(\S+)$/); # untaint
> }
> return wantarray ? ($size, $format, $used, $parent, $st->ctime) : $size;
> }
So the returned $parent will now just be undef if it contains
whitespaces, even though there is a parent. Can't that cause issues
further down the line? If it's fine, a comment with the rationale would
be nice.
Or should we rather allow whitespaces while matching and return it
properly? Or are there any issues with proper escaping then?
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH storage 1/2] don't bail on whitespaces in backing devices
This prevents importing from vmdks with whitespaces in file names.
Further, some operations that include file sizes (like listing disks)
would potentially fail entirely if a custom disk with a badly name
backing device exists in a VM images directory since they don't expect
this. Specifically, since we don't necessarily know the actual naming
scheme of the current storage in the plain Plugin.pm version, we don't
check the full name anyway, so why bother with whitespaces...
See-also:
https://forum.proxmox.com/threads/new-import-wizard-available-for-migrating-vmware-esxi-based-virtual-machines.144023/page-16#post-658697
Signed-off-by: Wolfgang Bumiller
---
src/PVE/Storage/Plugin.pm | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
index 22a9729..683190b 100644
--- a/src/PVE/Storage/Plugin.pm
+++ b/src/PVE/Storage/Plugin.pm
@@ -982,7 +982,7 @@ sub file_size_info {
$used = int($used);
($format) = ($format =~ /^(\S+)$/) or die "format '$format' includes
whitespace\n"; # untaint
if (defined($parent)) {
- ($parent) = ($parent =~ /^(\S+)$/) or die "parent '$parent' includes
whitespace\n"; # untaint
+ ($parent) = ($parent =~ /^(\S+)$/); # untaint
}
return wantarray ? ($size, $format, $used, $parent, $st->ctime) : $size;
}
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH storage 2/2] fixup error messages in file_size_info
The assignment happens before the 'die', so the error message would
always contain 'undef'.
Signed-off-by: Wolfgang Bumiller
---
src/PVE/Storage/Plugin.pm | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
index 683190b..b5a54c1 100644
--- a/src/PVE/Storage/Plugin.pm
+++ b/src/PVE/Storage/Plugin.pm
@@ -974,13 +974,16 @@ sub file_size_info {
my ($size, $format, $used, $parent) = $info->@{qw(virtual-size format
actual-size backing-filename)};
-($size) = ($size =~ /^(\d+)$/) or die "size '$size' not an integer\n"; #
untaint
+($size) = ($size =~ /^(\d+)$/); # untaint
+die "size '$size' not an integer\n" if !defined($size);
# coerce back from string
$size = int($size);
-($used) = ($used =~ /^(\d+)$/) or die "used '$used' not an integer\n"; #
untaint
+($used) = ($used =~ /^(\d+)$/); # untaint
+die "used '$used' not an integer\n" if !defined($used);
# coerce back from string
$used = int($used);
-($format) = ($format =~ /^(\S+)$/) or die "format '$format' includes
whitespace\n"; # untaint
+($format) = ($format =~ /^(\S+)$/); # untaint
+die "format '$format' includes whitespace\n" if !defined($format);
if (defined($parent)) {
($parent) = ($parent =~ /^(\S+)$/); # untaint
}
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied-series: [PATCH v3 qemu 1/2] fix #5409: backup: fix copy-before-write timeout
On 29/04/2024 17:20, Fiona Ebner wrote: > The type for the copy-before-write timeout in nanoseconds was wrong. > By being just uint32_t, a maximum of slightly over 4 seconds was > possible. Larger values would overflow and thus the 45 seconds set by > Proxmox's backup with fleecing, resulted in effectively 2 seconds > timeout for copy-before-write operations. > > Reported-by: Friedrich Weber > Signed-off-by: Fiona Ebner > --- > > Changes in v3: > * rebase on master > > Upstream submission of this patch: > https://lore.kernel.org/qemu-devel/20240429141934.442154-1-f.eb...@proxmox.com/T/#u > > ...e-write-use-uint64_t-for-timeout-in-.patch | 35 +++ > ...ock-copy-before-write-fix-permission.patch | 2 +- > ...e-write-support-unligned-snapshot-di.patch | 2 +- > ...e-write-create-block_copy-bitmap-in-.patch | 2 +- > ...-backup-add-discard-source-parameter.patch | 4 +-- > ...e-allow-specifying-minimum-cluster-s.patch | 2 +- > ...um-cluster-size-to-performance-optio.patch | 2 +- > debian/patches/series | 1 + > 8 files changed, 43 insertions(+), 7 deletions(-) > create mode 100644 > debian/patches/extra/0005-block-copy-before-write-use-uint64_t-for-timeout-in-.patch > > applied series, thanks! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
