Re: [PATCH v5 1/2] migration: switchover-hold parameter

[Peter Xu <peterx@redhat.com>]

Hi, Markus,

On Thu, Jul 06, 2023 at 03:38:13PM +0200, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > Add a new migration parameter switchover-hold which can block src qemu
> > migration from switching over to dest from running.
> >
> > One can set this flag to true so src qemu will keep iterating the VM data,
> > not switching over to dest even if it can.
> >
> > It means now live migration works somehow like COLO; we keep syncing data
> > from src to dst without stopping.
> 
> Out of curiosity: does it share code with COLO?

Nop, it is still purely the migration path.

> 
> > When the user is ready for the switchover, one can set the parameter from
> > true->false.  That'll contain a implicit kick to migration thread to be
> > alive and re-evaluate the switchover decision.
> >
> > This can be used in two cases so far in my mind:
> >
> >   (1) One can use this parameter to start pre-heating migration (but not
> >       really migrating, so a migrate-cancel will cancel the preheat).  When
> >       the user wants to really migrate, just clear the flag.  It'll in most
> >       cases migrate immediately because most pages are already synced.
> >
> >   (2) Can also be used as a clean way to do qtest, in many of the precopy
> >       tests we have requirement to run after 1 iteration without completing
> >       the precopy migration.  Before that we have either set bandwidth to
> >       ridiculous low value, or tricks on detecting guest memory change over
> >       some adhoc guest memory position.  Now we can simply set this flag
> >       then we know precopy won't complete and will just keep going.
> >
> > Here we leveraged a sem to make sure migration thread won't busy spin on a
> > physical cpu, meanwhile provide a timedwait() of 10ms so it can still try
> > its best to sync with dest QEMU from time to time.  Note that the sem is
> > prone to outdated counts but it's benign, please refer to the comment above
> > the semaphore definition for more information.
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  qapi/migration.json            | 25 ++++++++++--
> >  migration/migration.h          | 17 +++++++++
> >  migration/migration-hmp-cmds.c |  7 ++++
> >  migration/migration.c          | 69 ++++++++++++++++++++++++++++++++--
> >  migration/options.c            | 17 +++++++++
> >  5 files changed, 128 insertions(+), 7 deletions(-)
> >
> > diff --git a/qapi/migration.json b/qapi/migration.json
> > index 47dfef0278..c050081555 100644
> > --- a/qapi/migration.json
> > +++ b/qapi/migration.json
> > @@ -789,6 +789,15 @@
> >  #     Nodes are mapped to their block device name if there is one, and
> >  #     to their node name otherwise.  (Since 5.2)
> >  #
> > +# @switchover-hold: Whether we should hold-off precopy switchover from
> > +#     src to dest QEMU, even if we can finish migration in the
> 
> Spell out "source" and "destination", please.
> 
> Recommend to spell it out in the commit message, too.

Will do.

> 
> > +#     downtime specified.  By default off, so precopy migration will
> > +#     complete as soon as possible.  One can set it to explicitly keep
> > +#     iterating during precopy migration until set the flag to false
> > +#     again to kick off the final switchover.  Note, this does not
> 
> "until the flag is set to false again"
> 
> or
> 
> "until the flag is cleared".

Will do.

> 
> > +#     affect postcopy switchover, because the user can control that
> > +#     using "migrate-start-postcopy" command explicitly.  (Since 8.1)
> > +#
> >  # Features:
> >  #
> >  # @unstable: Member @x-checkpoint-delay is experimental.
> > @@ -810,7 +819,7 @@
> >             'xbzrle-cache-size', 'max-postcopy-bandwidth',
> >             'max-cpu-throttle', 'multifd-compression',
> >             'multifd-zlib-level' ,'multifd-zstd-level',
> > -           'block-bitmap-mapping' ] }
> > +           'block-bitmap-mapping', 'switchover-hold' ] }
> >  
> >  ##
> >  # @MigrateSetParameters:
> > @@ -945,6 +954,10 @@
> >  #     Nodes are mapped to their block device name if there is one, and
> >  #     to their node name otherwise.  (Since 5.2)
> >  #
> > +# @switchover-hold: Whether we should hold-off precopy switchover from
> > +#     src to dest QEMU.  For more details, please refer to
> > +#     MigrationParameter entry of the same field.  (Since 8.1)
> 
> We normally duplicate the documentation.  This would be the first
> instance where we reference instead.  Do we want that?

Personally I'd hope to try avoid any form of duplication if possible.  We
have some structure duplications that I cannot avoid (not until someone,
including myself, can dedup it..), so I tried to not duplicate exactly same
thing in the documents at least, instead to put such a reference.

But I agree only referencing other docs in this flag is special.  We may
want to do at least the same across all the parameters.  I have no strong
opinion either, so if we want to keep full descriptions in multiple places
I'll do that in the new spin. It's more QAPI stuff IMHO; I'll just follow
your advise.

Thanks for taking a look!

-- 
Peter Xu