diff mbox series

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

Message ID 20230428194928.1426370-11-vsementsov@yandex-team.ru
State New
Headers show
Series COLO: improve build options | expand

Commit Message

Vladimir Sementsov-Ogievskiy April 28, 2023, 7:49 p.m. UTC 
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(+)

Comments

Peter Xu May 2, 2023, 8:57 p.m. UTC | #1 
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>
Zhang, Chen May 4, 2023, 9:25 a.m. UTC | #2 
> -----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
Lukas Straub May 4, 2023, 10:10 p.m. UTC | #3 
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;



--
Vladimir Sementsov-Ogievskiy May 4, 2023, 10:30 p.m. UTC | #4 
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;
> 
> 
>
Lukas Straub May 4, 2023, 10:46 p.m. UTC | #5 
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;  
> > 
> > 
> >   
> 



--
Zhang, Chen May 5, 2023, 7:51 a.m. UTC | #6 
> -----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;
> > >
> > >
> > >
> >
> 
> 
> 
> --
Juan Quintela May 9, 2023, 6:23 p.m. UTC | #7 
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 mbox series

Patch

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;