Re: [PATCH v4 10/10] migration: block incoming colo when capability is disabled

From: Lukas Straub <lukasstraub2@web.de>
To: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Cc: qemu-devel@nongnu.org, quintela@redhat.com, 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
Date: Fri, 5 May 2023 00:46:11 +0200	[thread overview]
Message-ID: <20230504224611.2ea16229@gecko.fritz.box> (raw)
In-Reply-To: <35dd6e24-543f-e36e-5130-4f1a92be6a40@yandex-team.ru>

[-- Attachment #1: Type: text/plain, Size: 2945 bytes --]

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

-- 

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]