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

[Thomas Lamprecht]

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. 
> 
>