| Message ID | 20230428194928.1426370-11-vsementsov@yandex-team.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | COLO: improve build options | expand |
On Fri, Apr 28, 2023 at 10:49:28PM +0300, Vladimir Sementsov-Ogievskiy wrote: > We generally require same set of capabilities on source and target. > Let's require x-colo capability to use COLO on target. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Reviewed-by: Peter Xu <peterx@redhat.com>
> -----Original Message----- > From: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> > Sent: Saturday, April 29, 2023 3:49 AM > To: qemu-devel@nongnu.org > Cc: lukasstraub2@web.de; quintela@redhat.com; Zhang, Chen > <chen.zhang@intel.com>; vsementsov@yandex-team.ru; Peter Xu > <peterx@redhat.com>; Leonardo Bras <leobras@redhat.com> > Subject: [PATCH v4 10/10] migration: block incoming colo when capability is > disabled > > We generally require same set of capabilities on source and target. > Let's require x-colo capability to use COLO on target. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> > Reviewed-by: Zhang Chen <chen.zhang@intel.com> Thanks Chen --- > migration/migration.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/migration/migration.c b/migration/migration.c index > 8c5bbf3e94..5e162c0622 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -395,6 +395,12 @@ int migration_incoming_enable_colo(void) > return -ENOTSUP; > #endif > > + if (!migrate_colo()) { > + error_report("ENABLE_COLO command come in migration stream, but > c-colo " > + "capability is not set"); > + return -EINVAL; > + } > + > if (ram_block_discard_disable(true)) { > error_report("COLO: cannot disable RAM discard"); > return -EBUSY; > -- > 2.34.1
On Fri, 28 Apr 2023 22:49:28 +0300 Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> wrote: > We generally require same set of capabilities on source and target. > Let's require x-colo capability to use COLO on target. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Good patch, this is needed anyway for COLO with multifd. Also, it allows to remove a some code, like migration_incoming_enable_colo(), qemu_savevm_send_colo_enable() etc. I will send patches for that. Or you can do it if you like. Please update the docs like below, then: Reviewed-by: Lukas Straub <lukasstraub2@web.de> diff --git a/docs/COLO-FT.txt b/docs/COLO-FT.txt index 8ec653f81c..2e760a4aee 100644 --- a/docs/COLO-FT.txt +++ b/docs/COLO-FT.txt @@ -210,6 +210,7 @@ children.0=childs0 \ 3. On Secondary VM's QEMU monitor, issue command {"execute":"qmp_capabilities"} +{"execute": "migrate-set-capabilities", "arguments": {"capabilities": [ {"capability": "x-colo", "state": true } ] } } {"execute": "nbd-server-start", "arguments": {"addr": {"type": "inet", "data": {"host": "0.0.0.0", "port": "9999"} } } } {"execute": "nbd-server-add", "arguments": {"device": "parent0", "writable": true } } > --- > migration/migration.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/migration/migration.c b/migration/migration.c > index 8c5bbf3e94..5e162c0622 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -395,6 +395,12 @@ int migration_incoming_enable_colo(void) > return -ENOTSUP; > #endif > > + if (!migrate_colo()) { > + error_report("ENABLE_COLO command come in migration stream, but c-colo " > + "capability is not set"); > + return -EINVAL; > + } > + > if (ram_block_discard_disable(true)) { > error_report("COLO: cannot disable RAM discard"); > return -EBUSY; --
On 05.05.23 01:10, Lukas Straub wrote: > On Fri, 28 Apr 2023 22:49:28 +0300 > Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> wrote: > >> We generally require same set of capabilities on source and target. >> Let's require x-colo capability to use COLO on target. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> > > Good patch, this is needed anyway for COLO with multifd. > > Also, it allows to remove a some code, like > migration_incoming_enable_colo(), qemu_savevm_send_colo_enable() etc. > I will send patches for that. Great! But with such changes we should be careful to not break compatibility between versions.. On the other hand, x-colo - is still experimental with that x-, so there is no guarantee how it works. > Or you can do it if you like. To be honest, I don't :) > > Please update the docs like below, then: > Reviewed-by: Lukas Straub <lukasstraub2@web.de> > > diff --git a/docs/COLO-FT.txt b/docs/COLO-FT.txt > index 8ec653f81c..2e760a4aee 100644 > --- a/docs/COLO-FT.txt > +++ b/docs/COLO-FT.txt > @@ -210,6 +210,7 @@ children.0=childs0 \ > > 3. On Secondary VM's QEMU monitor, issue command > {"execute":"qmp_capabilities"} > +{"execute": "migrate-set-capabilities", "arguments": {"capabilities": [ {"capability": "x-colo", "state": true } ] } } > {"execute": "nbd-server-start", "arguments": {"addr": {"type": "inet", "data": {"host": "0.0.0.0", "port": "9999"} } } } > {"execute": "nbd-server-add", "arguments": {"device": "parent0", "writable": true } } > I'll resend with this addition, thanks for reviewing! > >> --- >> migration/migration.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/migration/migration.c b/migration/migration.c >> index 8c5bbf3e94..5e162c0622 100644 >> --- a/migration/migration.c >> +++ b/migration/migration.c >> @@ -395,6 +395,12 @@ int migration_incoming_enable_colo(void) >> return -ENOTSUP; >> #endif >> >> + if (!migrate_colo()) { >> + error_report("ENABLE_COLO command come in migration stream, but c-colo " >> + "capability is not set"); >> + return -EINVAL; >> + } >> + >> if (ram_block_discard_disable(true)) { >> error_report("COLO: cannot disable RAM discard"); >> return -EBUSY; > > >
On Fri, 5 May 2023 01:30:34 +0300 Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> wrote: > On 05.05.23 01:10, Lukas Straub wrote: > > On Fri, 28 Apr 2023 22:49:28 +0300 > > Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> wrote: > > > >> We generally require same set of capabilities on source and target. > >> Let's require x-colo capability to use COLO on target. > >> > >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> > > > > Good patch, this is needed anyway for COLO with multifd. > > > > Also, it allows to remove a some code, like > > migration_incoming_enable_colo(), qemu_savevm_send_colo_enable() etc. > > I will send patches for that. > > Great! But with such changes we should be careful to not break compatibility between versions.. On the other hand, x-colo - is still experimental with that x-, so there is no guarantee how it works. Given COLO's usecase, I think it should only be run with the same qemu version on both sides anyway. Maybe we should even enforce that somehow, while we're at it doing breaking changes. For upgrading qemu without downtime, normal migration can still be used. Zhang Cheng, what do you think? > > Or you can do it if you like. > > To be honest, I don't :) > > > > > Please update the docs like below, then: > > Reviewed-by: Lukas Straub <lukasstraub2@web.de> > > > > diff --git a/docs/COLO-FT.txt b/docs/COLO-FT.txt > > index 8ec653f81c..2e760a4aee 100644 > > --- a/docs/COLO-FT.txt > > +++ b/docs/COLO-FT.txt > > @@ -210,6 +210,7 @@ children.0=childs0 \ > > > > 3. On Secondary VM's QEMU monitor, issue command > > {"execute":"qmp_capabilities"} > > +{"execute": "migrate-set-capabilities", "arguments": {"capabilities": [ {"capability": "x-colo", "state": true } ] } } > > {"execute": "nbd-server-start", "arguments": {"addr": {"type": "inet", "data": {"host": "0.0.0.0", "port": "9999"} } } } > > {"execute": "nbd-server-add", "arguments": {"device": "parent0", "writable": true } } > > > > I'll resend with this addition, thanks for reviewing! > > > > >> --- > >> migration/migration.c | 6 ++++++ > >> 1 file changed, 6 insertions(+) > >> > >> diff --git a/migration/migration.c b/migration/migration.c > >> index 8c5bbf3e94..5e162c0622 100644 > >> --- a/migration/migration.c > >> +++ b/migration/migration.c > >> @@ -395,6 +395,12 @@ int migration_incoming_enable_colo(void) > >> return -ENOTSUP; > >> #endif > >> > >> + if (!migrate_colo()) { > >> + error_report("ENABLE_COLO command come in migration stream, but c-colo " > >> + "capability is not set"); > >> + return -EINVAL; > >> + } > >> + > >> if (ram_block_discard_disable(true)) { > >> error_report("COLO: cannot disable RAM discard"); > >> return -EBUSY; > > > > > > > --
> -----Original Message----- > From: Lukas Straub <lukasstraub2@web.de> > Sent: Friday, May 5, 2023 6:46 AM > To: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> > Cc: qemu-devel@nongnu.org; quintela@redhat.com; Zhang, Chen > <chen.zhang@intel.com>; Peter Xu <peterx@redhat.com>; Leonardo Bras > <leobras@redhat.com> > Subject: Re: [PATCH v4 10/10] migration: block incoming colo when capability > is disabled > > On Fri, 5 May 2023 01:30:34 +0300 > Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> wrote: > > > On 05.05.23 01:10, Lukas Straub wrote: > > > On Fri, 28 Apr 2023 22:49:28 +0300 > > > Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> wrote: > > > > > >> We generally require same set of capabilities on source and target. > > >> Let's require x-colo capability to use COLO on target. > > >> > > >> Signed-off-by: Vladimir Sementsov-Ogievskiy > > >> <vsementsov@yandex-team.ru> > > > > > > Good patch, this is needed anyway for COLO with multifd. > > > > > > Also, it allows to remove a some code, like > > > migration_incoming_enable_colo(), qemu_savevm_send_colo_enable() > etc. > > > I will send patches for that. > > > > Great! But with such changes we should be careful to not break > compatibility between versions.. On the other hand, x-colo - is still > experimental with that x-, so there is no guarantee how it works. > > Given COLO's usecase, I think it should only be run with the same qemu > version on both sides anyway. Maybe we should even enforce that somehow, > while we're at it doing breaking changes. > > For upgrading qemu without downtime, normal migration can still be used. > > Zhang Cheng, what do you think? It's OK for me. We can add the same version requirement comments to the docs/COLO-FT.txt. Thanks Chen > > > > Or you can do it if you like. > > > > To be honest, I don't :) > > > > > > > > Please update the docs like below, then: > > > Reviewed-by: Lukas Straub <lukasstraub2@web.de> > > > > > > diff --git a/docs/COLO-FT.txt b/docs/COLO-FT.txt index > > > 8ec653f81c..2e760a4aee 100644 > > > --- a/docs/COLO-FT.txt > > > +++ b/docs/COLO-FT.txt > > > @@ -210,6 +210,7 @@ children.0=childs0 \ > > > > > > 3. On Secondary VM's QEMU monitor, issue command > > > {"execute":"qmp_capabilities"} > > > +{"execute": "migrate-set-capabilities", "arguments": > > > +{"capabilities": [ {"capability": "x-colo", "state": true } ] } } > > > {"execute": "nbd-server-start", "arguments": {"addr": {"type": "inet", > "data": {"host": "0.0.0.0", "port": "9999"} } } } > > > {"execute": "nbd-server-add", "arguments": {"device": "parent0", > > > "writable": true } } > > > > > > > I'll resend with this addition, thanks for reviewing! > > > > > > > >> --- > > >> migration/migration.c | 6 ++++++ > > >> 1 file changed, 6 insertions(+) > > >> > > >> diff --git a/migration/migration.c b/migration/migration.c index > > >> 8c5bbf3e94..5e162c0622 100644 > > >> --- a/migration/migration.c > > >> +++ b/migration/migration.c > > >> @@ -395,6 +395,12 @@ int migration_incoming_enable_colo(void) > > >> return -ENOTSUP; > > >> #endif > > >> > > >> + if (!migrate_colo()) { > > >> + error_report("ENABLE_COLO command come in migration stream, > but c-colo " > > >> + "capability is not set"); > > >> + return -EINVAL; > > >> + } > > >> + > > >> if (ram_block_discard_disable(true)) { > > >> error_report("COLO: cannot disable RAM discard"); > > >> return -EBUSY; > > > > > > > > > > > > > > > --
Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> wrote: > We generally require same set of capabilities on source and target. > Let's require x-colo capability to use COLO on target. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Reviewed-by: Juan Quintela <quintela@redhat.com> I will update the docs as said by lukas. > --- > migration/migration.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/migration/migration.c b/migration/migration.c > index 8c5bbf3e94..5e162c0622 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -395,6 +395,12 @@ int migration_incoming_enable_colo(void) > return -ENOTSUP; > #endif > > + if (!migrate_colo()) { > + error_report("ENABLE_COLO command come in migration stream, but c-colo " > + "capability is not set"); > + return -EINVAL; > + } > + > if (ram_block_discard_disable(true)) { > error_report("COLO: cannot disable RAM discard"); > return -EBUSY;
diff --git a/migration/migration.c b/migration/migration.c index 8c5bbf3e94..5e162c0622 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -395,6 +395,12 @@ int migration_incoming_enable_colo(void) return -ENOTSUP; #endif + if (!migrate_colo()) { + error_report("ENABLE_COLO command come in migration stream, but c-colo " + "capability is not set"); + return -EINVAL; + } + if (ram_block_discard_disable(true)) { error_report("COLO: cannot disable RAM discard"); return -EBUSY;
We generally require same set of capabilities on source and target. Let's require x-colo capability to use COLO on target. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> --- migration/migration.c | 6 ++++++ 1 file changed, 6 insertions(+)