>>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" <[email protected]> À: "aderumier" <[email protected]> Cc: "pve-devel" <[email protected]> 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" <[email protected]> > À: "pve-devel" <[email protected]>, "aderumier" <[email protected]> > 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. > > _______________________________________________ pve-devel mailing list [email protected] https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
