From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 0E6267EB25 for ; Thu, 11 Nov 2021 12:07:49 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id C5E0EA45E for ; Thu, 11 Nov 2021 12:07:48 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 9D6D0A3EE for ; Thu, 11 Nov 2021 12:07:46 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 749A442F4F for ; Thu, 11 Nov 2021 12:07:46 +0100 (CET) Date: Thu, 11 Nov 2021 12:07:39 +0100 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Fabian Ebner , pve-devel@lists.proxmox.com References: <20211105130359.40803-1-f.gruenbichler@proxmox.com> <20211105130359.40803-20-f.gruenbichler@proxmox.com> <06755979-9217-f572-384a-2825631f4f8f@proxmox.com> <97332e83-f7e1-aa6e-3932-2ba9de52b5cc@proxmox.com> In-Reply-To: <97332e83-f7e1-aa6e-3932-2ba9de52b5cc@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.15.0 (https://github.com/astroidmail/astroid) Message-Id: <1636628093.z9q3o3cluj.astroid@nora.none> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-SPAM-LEVEL: Spam detection results: 0 AWL 0.281 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pve-devel] [PATCH qemu-server 07/10] mtunnel: add API endpoints X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 11 Nov 2021 11:07:49 -0000 On November 10, 2021 8:40 am, Fabian Ebner wrote: > Am 09.11.21 um 13:46 schrieb Fabian Ebner: >> Am 05.11.21 um 14:03 schrieb Fabian Gr=C3=BCnbichler: >=20 > ---snip--- >=20 >>> =C2=A0 use IO::Socket::IP; >>> +use IO::Socket::UNIX; >>> +use IPC::Open3; >>> +use JSON; >>> +use MIME::Base64; >=20 > Forgot to ask: is this import needed or a left-over from development? yes >=20 > ---snip--- >=20 >>=20 >>> + >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 my = $migration_snapshot; >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if = ($scfg->{type} eq 'zfspool' || $scfg->{type} eq=20 >>> 'btrfs') { >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 $mi= gration_snapshot =3D '__migration__'; >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 } >>> + >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 my = $volid =3D "$storeid:$volname"; >>> + >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 # f= ind common import/export format, taken from PVE::Storage >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 my = @import_formats =3D=20 >>> PVE::Storage::volume_import_formats($state->{storecfg}, $volid,=20 >>> $migration_snapshot, undef, $with_snapshots); >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 my = @export_formats =3D=20 >>> PVE::Tools::split_list($params->{'export-formats'}); >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 my = %import_hash =3D map { $_ =3D> 1 } @import_formats; >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 my = @common =3D grep { $import_hash{$_} } @export_formats; >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 die= "no matching import/export format found for storage=20 >>> '$storeid'\n" >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if = !@common; >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 $fo= rmat =3D $common[0]; >>> + >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 my = $input =3D IO::File->new(); >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 my = $info =3D IO::File->new(); >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 my = $unix =3D "/run/qemu-server/$vmid.storage"; >>> + >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 my = $import_cmd =3D ['pvesm', 'import', $volid, $format,=20 >>> "unix://$unix", '-with-snapshots', $with_snapshots]; >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if = ($params->{'allow-rename'}) { >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 pus= h @$import_cmd, '-allow-rename',=20 >>> $params->{'allow-rename'}; >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 } >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if = ($migration_snapshot) { >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 pus= h @$import_cmd, '-delete-snapshot', $migration_snapshot; >>=20 >> Missing '-snapshot $migration_snapshot'? While the parameter is ignored=20 >> by our ZFSPoolPlugin, the BTRFSPlugin aborts if it's not specified=20 >> AFAICS. And external plugins might require it too. >=20 > That is, for the 'btrfs' format. In the patch with the export command, a=20 > snapshot is only used for ZFS, so it would already fail on export for=20 > BTRFS with 'btrfs' format. For external plugins we also don't use a=20 > migration snapshot in storage_migrate(), so please disregard that part. done >=20 >>=20 >> In general, we'll need to be careful not to introduce mismatches between= =20 >> the import and the export parameters. Might it be better if the client=20 >> would pass along (most of) the parameters for the import command (which=20 >> basically is how it's done for the existing storage_migrate)? >>=20 >=20 > On the other hand, that would require being very careful with input=20 > validation. yeah, and since we are crossing a trust boundary here (between two=20 clusters) we have to be careful. if we change the export/import code, we=20 can always also bump the tunnel API if needed (either to selectively use=20 new features only if supported, or to error out early if there was a=20 breaking change). just passing in "pass this to `pvesm import`" is=20 potentially dangerous if we don't carefully validate the 'this', and=20 that is easier if it's structured data ;) so I'd rather do this explicit=20 even if it means extending two places when we change the interface. >=20 > ---snip--- >=20