[pve-devel] [PATCH v6 qemu-server 2/3] add QemuMigrateExternal.pm

[Alexandre DERUMIER]

>>yes, with me idea you'd still separate it, have this in the new QemuMigrateExternal,
>>_but_ the base() class wouldn't be "PVE::AbstractMigrate" but "PVE::QemuMigrate",
>>we won't touch PVE::QemuMigrate besides some splitting up of things (to get a nicer
>>interface to overwrite in the child-class), but also won't copying the full module
>>over. AFAIS, you didn't had this approach first were all was integrated in QemuMigrate
>>directly.

oh ok, got it !
I'll look at this.

(and thanks for the review)


----- Mail original -----
De: "Thomas Lamprecht" <t.lamprecht at proxmox.com>
À: "aderumier" <aderumier at odiso.com>
Cc: "pve-devel" <pve-devel at pve.proxmox.com>
Envoyé: Lundi 1 Avril 2019 07:45:45
Objet: Re: [pve-devel] [PATCH v6 qemu-server 2/3] add QemuMigrateExternal.pm

On 4/1/19 6:55 AM, Alexandre DERUMIER wrote: 
>>> I do not like the completel full duplication of QemuMigrate, how about using 
>>> the exisitng "PVE::QemuMigrate" migrate as base for this and changing only really 
>>> those methods which need to be changed? Maybe we even can split it up a little bit 
>>> more so that you only need to overwrite a handful of rather small methods (no full 
>>> phases)? 
>>> 
>>> Looking at a diff between existing QemuMigrate and your External one, maybe: 
>>> 
>>> * one to get the ssh command 
>>> * one to get the target vmid (for the common case it would just return $vmid) 
>>> * one to get the remote start command 
>>> * ...? 
> 
> Well, First versions of this patch was based on QemuMigrate, but David asked me 
> to separate it. (and maybe merge it later). 

yes, with me idea you'd still separate it, have this in the new QemuMigrateExternal, 
_but_ the base() class wouldn't be "PVE::AbstractMigrate" but "PVE::QemuMigrate", 
we won't touch PVE::QemuMigrate besides some splitting up of things (to get a nicer 
interface to overwrite in the child-class), but also won't copying the full module 
over. AFAIS, you didn't had this approach first were all was integrated in QemuMigrate 
directly. 

> 
> I can rework it again, on current master QemuMigrate if you want. 
> 
> 



> 
> 
> ----- Mail original ----- 
> De: "Thomas Lamprecht" <t.lamprecht at proxmox.com> 
> À: "pve-devel" <pve-devel at pve.proxmox.com>, "aderumier" <aderumier at odiso.com> 
> Envoyé: Samedi 30 Mars 2019 17:27:09 
> Objet: Re: [pve-devel] [PATCH v6 qemu-server 2/3] add QemuMigrateExternal.pm 
> 
> On 2/20/19 1:22 AM, Alexandre Derumier wrote: 
>> --- 
>> PVE/Makefile | 1 + 
>> PVE/QemuMigrateExternal.pm | 872 +++++++++++++++++++++++++++++++++++++++++++++ 
>> 2 files changed, 873 insertions(+) 
>> create mode 100644 PVE/QemuMigrateExternal.pm 
>> 
>> diff --git a/PVE/Makefile b/PVE/Makefile 
>> index 2c800f6..0494cfb 100644 
>> --- a/PVE/Makefile 
>> +++ b/PVE/Makefile 
>> @@ -1,6 +1,7 @@ 
>> PERLSOURCE = \ 
>> QemuServer.pm \ 
>> QemuMigrate.pm \ 
>> + QemuMigrateExternal.pm \ 
>> QMPClient.pm \ 
>> QemuConfig.pm 
>> 
>> diff --git a/PVE/QemuMigrateExternal.pm b/PVE/QemuMigrateExternal.pm 
>> new file mode 100644 
>> index 0000000..dd6a1d9 
>> --- /dev/null 
>> +++ b/PVE/QemuMigrateExternal.pm 
>> @@ -0,0 +1,872 @@ 
>> +package PVE::QemuMigrateExternal; 
>> + 
>> +use strict; 
>> +use warnings; 
>> +use PVE::AbstractMigrate; 
>> +use IO::File; 
>> +use IPC::Open2; 
>> +use POSIX qw( WNOHANG ); 
>> +use PVE::INotify; 
>> +use PVE::Tools; 
>> +use PVE::Cluster; 
>> +use PVE::Storage; 
>> +use PVE::QemuServer; 
>> +use Time::HiRes qw( usleep ); 
>> +use PVE::RPCEnvironment; 
>> +use PVE::ReplicationConfig; 
>> +use PVE::ReplicationState; 
>> +use PVE::Replication; 
>> +use Storable qw(dclone); 
>> + 
>> +use base qw(PVE::AbstractMigrate); 
> 
> I do not like the completel full duplication of QemuMigrate, how about using 
> the exisitng "PVE::QemuMigrate" migrate as base for this and changing only really 
> those methods which need to be changed? Maybe we even can split it up a little bit 
> more so that you only need to overwrite a handful of rather small methods (no full 
> phases)? 
> 
> Looking at a diff between existing QemuMigrate and your External one, maybe: 
> 
> * one to get the ssh command 
> * one to get the target vmid (for the common case it would just return $vmid) 
> * one to get the remote start command 
> * ...? 
> 
> Do you think that could be dooable and make sense? We can also expand AbstractMigrate 
> if we can formulate the stuff we need for this in more general ways, i.e., I'd like 
> to not put to much "for ext. migrate only" methods there as long as this is seen 
> experimental. 
> 
>