| Message ID | 20240502154729.370938-1-peterx@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | hmp/migration: Fix documents for "migrate" command | expand |
Peter Xu <peterx@redhat.com> writes: > Peter missed the Sphinx HMP document for the "resume/-r" flag in commit > 7a4da28b26 ("qmp: hmp: add migrate "resume" option"). Add it. Avoid > adding a Fixes to make life easier for the stable maintainer. > > When at it, slightly cleanup the lines, move "detach/-d" to a separate > section rather than appending it at the end of the command description. > > Cc: Dr. David Alan Gilbert <dave@treblig.org> > Cc: Fabiano Rosas <farosas@suse.de> > Cc: Markus Armbruster <armbru@redhat.com> > Signed-off-by: Peter Xu <peterx@redhat.com> Reviewed-by: Fabiano Rosas <farosas@suse.de>
Peter Xu <peterx@redhat.com> writes: > Peter missed the Sphinx HMP document for the "resume/-r" flag in commit > 7a4da28b26 ("qmp: hmp: add migrate "resume" option"). Add it. Avoid > adding a Fixes to make life easier for the stable maintainer. I'm curious: how does not adding Fixes: make life easier? > When at it, slightly cleanup the lines, move "detach/-d" to a separate > section rather than appending it at the end of the command description. > > Cc: Dr. David Alan Gilbert <dave@treblig.org> > Cc: Fabiano Rosas <farosas@suse.de> > Cc: Markus Armbruster <armbru@redhat.com> > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > > Based-on: <20240430142737.29066-1-farosas@suse.de> > ("[PATCH v3 0/6] migration removals & deprecations") > --- > hmp-commands.hx | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/hmp-commands.hx b/hmp-commands.hx > index ebca2cdced..484a8a1c3a 100644 > --- a/hmp-commands.hx > +++ b/hmp-commands.hx > @@ -918,8 +918,13 @@ ERST { .name = "migrate", .args_type = "detach:-d,blk:-b,inc:-i,resume:-r,uri:s", .params = "[-d] [-b] [-i] [-r] uri", .help = "migrate to URI (using -d to not wait for completion)" "\n\t\t\t -b for migration without shared storage with" " full copy of disk\n\t\t\t -i for migration without " "shared storage with incremental copy of disk " "(base image shared between src and destination)" "\n\t\t\t -r to resume a paused migration", .cmd = hmp_migrate, }, > > > SRST > -``migrate [-d]`` *uri* > - Migrate to *uri* (using -d to not wait for completion). > +``migrate [-d] [-r]`` *uri* > + Migrate the current VM to *uri*. Could there be any other VM than the current one? Scratch "current"? > + > + ``-d`` > + Run this command asynchronously, so that the command doesn't wait for completion. What is run asynchronously, and what isn't waiting? These are two different entities, aren't they? Calling them "this command" and "the command" is confusing :) Perhaps Start the migration process, but do not wait for its completion. Maybe add a hint on how to wait or poll for completion? > + ``-r`` > + Resume a paused postcopy migration. .help doesn't have "postcopy". Should it? > ERST > > {
On Fri, May 03, 2024 at 08:58:09AM +0200, Markus Armbruster wrote: > Peter Xu <peterx@redhat.com> writes: > > > Peter missed the Sphinx HMP document for the "resume/-r" flag in commit > > 7a4da28b26 ("qmp: hmp: add migrate "resume" option"). Add it. Avoid > > adding a Fixes to make life easier for the stable maintainer. > > I'm curious: how does not adding Fixes: make life easier? Because if I attach Fixes then IIUC Michael will read it through and judge whether it should apply to stable, where I want to skip that for him because I think this doesn't apply to stable. Reasons: - This is a document update, IIUC we normally only keep the latest document uptodate, not all the stable versions (especiailly for HMP, which isn't a stable ABI)? I assume it applies the same when a qtest case got a slight fixup. - This patch is even more special as it will need explicit backport due to the removal of block migration, and I really don't think any of us should spend time on that.. > > > When at it, slightly cleanup the lines, move "detach/-d" to a separate > > section rather than appending it at the end of the command description. > > > > Cc: Dr. David Alan Gilbert <dave@treblig.org> > > Cc: Fabiano Rosas <farosas@suse.de> > > Cc: Markus Armbruster <armbru@redhat.com> > > Signed-off-by: Peter Xu <peterx@redhat.com> > > --- > > > > Based-on: <20240430142737.29066-1-farosas@suse.de> > > ("[PATCH v3 0/6] migration removals & deprecations") > > --- > > hmp-commands.hx | 9 +++++++-- > > 1 file changed, 7 insertions(+), 2 deletions(-) > > > > diff --git a/hmp-commands.hx b/hmp-commands.hx > > index ebca2cdced..484a8a1c3a 100644 > > --- a/hmp-commands.hx > > +++ b/hmp-commands.hx > > @@ -918,8 +918,13 @@ ERST > { > .name = "migrate", > .args_type = "detach:-d,blk:-b,inc:-i,resume:-r,uri:s", > .params = "[-d] [-b] [-i] [-r] uri", > .help = "migrate to URI (using -d to not wait for completion)" > "\n\t\t\t -b for migration without shared storage with" > " full copy of disk\n\t\t\t -i for migration without " > "shared storage with incremental copy of disk " > "(base image shared between src and destination)" > "\n\t\t\t -r to resume a paused migration", > .cmd = hmp_migrate, > }, > > > > > > SRST > > -``migrate [-d]`` *uri* > > - Migrate to *uri* (using -d to not wait for completion). > > +``migrate [-d] [-r]`` *uri* > > + Migrate the current VM to *uri*. > > Could there be any other VM than the current one? Scratch "current"? I didn't have "current" until I generated the doc and read, then I see right below "migrate_cancel" has it: SRST ``migrate_cancel`` Cancel the current VM migration. ERST But maybe it means "current migration", not "current VM".. So yeah I can drop it. > > > + > > + ``-d`` > > + Run this command asynchronously, so that the command doesn't wait for completion. > > What is run asynchronously, and what isn't waiting? These are two > different entities, aren't they? Calling them "this command" and "the > command" is confusing :) > > Perhaps > > Start the migration process, but do not wait for its completion. > > Maybe add a hint on how to wait or poll for completion? Yes this reads better; I will add the hint too. > > > + ``-r`` > > + Resume a paused postcopy migration. > > .help doesn't have "postcopy". Should it? It should. This is the fixup I'll squash when sending v2, let me know if there's other early comments, thanks. ===8<=== diff --git a/hmp-commands.hx b/hmp-commands.hx index 484a8a1c3a..06746f0afc 100644 --- a/hmp-commands.hx +++ b/hmp-commands.hx @@ -912,17 +912,18 @@ ERST .args_type = "detach:-d,resume:-r,uri:s", .params = "[-d] [-r] uri", .help = "migrate to URI (using -d to not wait for completion)" - "\n\t\t\t -r to resume a paused migration", + "\n\t\t\t -r to resume a paused postcopy migration", .cmd = hmp_migrate, }, SRST ``migrate [-d] [-r]`` *uri* - Migrate the current VM to *uri*. + Migrate the VM to *uri*. ``-d`` - Run this command asynchronously, so that the command doesn't wait for completion. + Start the migration process, but do not wait for its completion. To + query an ongoing migration process, use "info migrate". ``-r`` Resume a paused postcopy migration. ERST
If there's still time, suggest to tweak the subject to
hmp/migration: Fix "migrate" command's documentation
Peter Xu <peterx@redhat.com> writes:
> On Fri, May 03, 2024 at 08:58:09AM +0200, Markus Armbruster wrote:
>> Peter Xu <peterx@redhat.com> writes:
>>
>> > Peter missed the Sphinx HMP document for the "resume/-r" flag in commit
>> > 7a4da28b26 ("qmp: hmp: add migrate "resume" option"). Add it. Avoid
>> > adding a Fixes to make life easier for the stable maintainer.
>>
>> I'm curious: how does not adding Fixes: make life easier?
>
> Because if I attach Fixes then IIUC Michael will read it through and judge
> whether it should apply to stable, where I want to skip that for him
> because I think this doesn't apply to stable. Reasons:
>
> - This is a document update, IIUC we normally only keep the latest
> document uptodate, not all the stable versions (especiailly for HMP,
> which isn't a stable ABI)? I assume it applies the same when a qtest
> case got a slight fixup.
>
> - This patch is even more special as it will need explicit backport due
> to the removal of block migration, and I really don't think any of us
> should spend time on that..
Right. But Fixes: is also for downstreams, who may want to make their
own decisions.
I think I'd always add Fixes:. When I think there's a need to steer
stable away from it, I'd say so in the commit message. I doubt needed
here, as the subject states it's just a doc fix for HMP.
>> > When at it, slightly cleanup the lines, move "detach/-d" to a separate
>> > section rather than appending it at the end of the command description.
>> >
>> > Cc: Dr. David Alan Gilbert <dave@treblig.org>
>> > Cc: Fabiano Rosas <farosas@suse.de>
>> > Cc: Markus Armbruster <armbru@redhat.com>
>> > Signed-off-by: Peter Xu <peterx@redhat.com>
>> > ---
>> >
>> > Based-on: <20240430142737.29066-1-farosas@suse.de>
>> > ("[PATCH v3 0/6] migration removals & deprecations")
>> > ---
>> > hmp-commands.hx | 9 +++++++--
>> > 1 file changed, 7 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/hmp-commands.hx b/hmp-commands.hx
>> > index ebca2cdced..484a8a1c3a 100644
>> > --- a/hmp-commands.hx
>> > +++ b/hmp-commands.hx
>> > @@ -918,8 +918,13 @@ ERST
>> {
>> .name = "migrate",
>> .args_type = "detach:-d,blk:-b,inc:-i,resume:-r,uri:s",
>> .params = "[-d] [-b] [-i] [-r] uri",
>> .help = "migrate to URI (using -d to not wait for completion)"
>> "\n\t\t\t -b for migration without shared storage with"
>> " full copy of disk\n\t\t\t -i for migration without "
>> "shared storage with incremental copy of disk "
>> "(base image shared between src and destination)"
>> "\n\t\t\t -r to resume a paused migration",
>> .cmd = hmp_migrate,
>> },
>> >
>> >
>> > SRST
>> > -``migrate [-d]`` *uri*
>> > - Migrate to *uri* (using -d to not wait for completion).
>> > +``migrate [-d] [-r]`` *uri*
>> > + Migrate the current VM to *uri*.
>>
>> Could there be any other VM than the current one? Scratch "current"?
>
> I didn't have "current" until I generated the doc and read, then I see
> right below "migrate_cancel" has it:
>
> SRST
> ``migrate_cancel``
> Cancel the current VM migration.
> ERST
>
> But maybe it means "current migration", not "current VM".. So yeah I can
> drop it.
>
>>
>> > +
>> > + ``-d``
>> > + Run this command asynchronously, so that the command doesn't wait for completion.
>>
>> What is run asynchronously, and what isn't waiting? These are two
>> different entities, aren't they? Calling them "this command" and "the
>> command" is confusing :)
>>
>> Perhaps
>>
>> Start the migration process, but do not wait for its completion.
>>
>> Maybe add a hint on how to wait or poll for completion?
>
> Yes this reads better; I will add the hint too.
>
>>
>> > + ``-r``
>> > + Resume a paused postcopy migration.
>>
>> .help doesn't have "postcopy". Should it?
>
> It should.
>
> This is the fixup I'll squash when sending v2, let me know if there's other
> early comments, thanks.
>
> ===8<===
>
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 484a8a1c3a..06746f0afc 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -912,17 +912,18 @@ ERST
> .args_type = "detach:-d,resume:-r,uri:s",
> .params = "[-d] [-r] uri",
> .help = "migrate to URI (using -d to not wait for completion)"
> - "\n\t\t\t -r to resume a paused migration",
> + "\n\t\t\t -r to resume a paused postcopy migration",
> .cmd = hmp_migrate,
> },
>
>
> SRST
> ``migrate [-d] [-r]`` *uri*
> - Migrate the current VM to *uri*.
> + Migrate the VM to *uri*.
>
> ``-d``
> - Run this command asynchronously, so that the command doesn't wait for completion.
> + Start the migration process, but do not wait for its completion. To
> + query an ongoing migration process, use "info migrate".
> ``-r``
> Resume a paused postcopy migration.
> ERST
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Thanks!
On Fri, May 03, 2024 at 04:08:45PM +0200, Markus Armbruster wrote: > If there's still time, suggest to tweak the subject to > > hmp/migration: Fix "migrate" command's documentation Yes there is. :) > > Peter Xu <peterx@redhat.com> writes: > > > On Fri, May 03, 2024 at 08:58:09AM +0200, Markus Armbruster wrote: > >> Peter Xu <peterx@redhat.com> writes: > >> > >> > Peter missed the Sphinx HMP document for the "resume/-r" flag in commit > >> > 7a4da28b26 ("qmp: hmp: add migrate "resume" option"). Add it. Avoid > >> > adding a Fixes to make life easier for the stable maintainer. > >> > >> I'm curious: how does not adding Fixes: make life easier? > > > > Because if I attach Fixes then IIUC Michael will read it through and judge > > whether it should apply to stable, where I want to skip that for him > > because I think this doesn't apply to stable. Reasons: > > > > - This is a document update, IIUC we normally only keep the latest > > document uptodate, not all the stable versions (especiailly for HMP, > > which isn't a stable ABI)? I assume it applies the same when a qtest > > case got a slight fixup. > > > > - This patch is even more special as it will need explicit backport due > > to the removal of block migration, and I really don't think any of us > > should spend time on that.. > > Right. But Fixes: is also for downstreams, who may want to make their > own decisions. > > I think I'd always add Fixes:. When I think there's a need to steer > stable away from it, I'd say so in the commit message. I doubt needed > here, as the subject states it's just a doc fix for HMP. OK, I'll attach a Fixes. > > >> > When at it, slightly cleanup the lines, move "detach/-d" to a separate > >> > section rather than appending it at the end of the command description. > >> > > >> > Cc: Dr. David Alan Gilbert <dave@treblig.org> > >> > Cc: Fabiano Rosas <farosas@suse.de> > >> > Cc: Markus Armbruster <armbru@redhat.com> > >> > Signed-off-by: Peter Xu <peterx@redhat.com> > >> > --- > >> > > >> > Based-on: <20240430142737.29066-1-farosas@suse.de> > >> > ("[PATCH v3 0/6] migration removals & deprecations") > >> > --- > >> > hmp-commands.hx | 9 +++++++-- > >> > 1 file changed, 7 insertions(+), 2 deletions(-) > >> > > >> > diff --git a/hmp-commands.hx b/hmp-commands.hx > >> > index ebca2cdced..484a8a1c3a 100644 > >> > --- a/hmp-commands.hx > >> > +++ b/hmp-commands.hx > >> > @@ -918,8 +918,13 @@ ERST > >> { > >> .name = "migrate", > >> .args_type = "detach:-d,blk:-b,inc:-i,resume:-r,uri:s", > >> .params = "[-d] [-b] [-i] [-r] uri", > >> .help = "migrate to URI (using -d to not wait for completion)" > >> "\n\t\t\t -b for migration without shared storage with" > >> " full copy of disk\n\t\t\t -i for migration without " > >> "shared storage with incremental copy of disk " > >> "(base image shared between src and destination)" > >> "\n\t\t\t -r to resume a paused migration", > >> .cmd = hmp_migrate, > >> }, > >> > > >> > > >> > SRST > >> > -``migrate [-d]`` *uri* > >> > - Migrate to *uri* (using -d to not wait for completion). > >> > +``migrate [-d] [-r]`` *uri* > >> > + Migrate the current VM to *uri*. > >> > >> Could there be any other VM than the current one? Scratch "current"? > > > > I didn't have "current" until I generated the doc and read, then I see > > right below "migrate_cancel" has it: > > > > SRST > > ``migrate_cancel`` > > Cancel the current VM migration. > > ERST > > > > But maybe it means "current migration", not "current VM".. So yeah I can > > drop it. > > > >> > >> > + > >> > + ``-d`` > >> > + Run this command asynchronously, so that the command doesn't wait for completion. > >> > >> What is run asynchronously, and what isn't waiting? These are two > >> different entities, aren't they? Calling them "this command" and "the > >> command" is confusing :) > >> > >> Perhaps > >> > >> Start the migration process, but do not wait for its completion. > >> > >> Maybe add a hint on how to wait or poll for completion? > > > > Yes this reads better; I will add the hint too. > > > >> > >> > + ``-r`` > >> > + Resume a paused postcopy migration. > >> > >> .help doesn't have "postcopy". Should it? > > > > It should. > > > > This is the fixup I'll squash when sending v2, let me know if there's other > > early comments, thanks. > > > > ===8<=== > > > > diff --git a/hmp-commands.hx b/hmp-commands.hx > > index 484a8a1c3a..06746f0afc 100644 > > --- a/hmp-commands.hx > > +++ b/hmp-commands.hx > > @@ -912,17 +912,18 @@ ERST > > .args_type = "detach:-d,resume:-r,uri:s", > > .params = "[-d] [-r] uri", > > .help = "migrate to URI (using -d to not wait for completion)" > > - "\n\t\t\t -r to resume a paused migration", > > + "\n\t\t\t -r to resume a paused postcopy migration", > > .cmd = hmp_migrate, > > }, > > > > > > SRST > > ``migrate [-d] [-r]`` *uri* > > - Migrate the current VM to *uri*. > > + Migrate the VM to *uri*. > > > > ``-d`` > > - Run this command asynchronously, so that the command doesn't wait for completion. > > + Start the migration process, but do not wait for its completion. To > > + query an ongoing migration process, use "info migrate". > > ``-r`` > > Resume a paused postcopy migration. > > ERST > > Reviewed-by: Markus Armbruster <armbru@redhat.com> Thank you!
diff --git a/hmp-commands.hx b/hmp-commands.hx index ebca2cdced..484a8a1c3a 100644 --- a/hmp-commands.hx +++ b/hmp-commands.hx @@ -918,8 +918,13 @@ ERST SRST -``migrate [-d]`` *uri* - Migrate to *uri* (using -d to not wait for completion). +``migrate [-d] [-r]`` *uri* + Migrate the current VM to *uri*. + + ``-d`` + Run this command asynchronously, so that the command doesn't wait for completion. + ``-r`` + Resume a paused postcopy migration. ERST {
Peter missed the Sphinx HMP document for the "resume/-r" flag in commit 7a4da28b26 ("qmp: hmp: add migrate "resume" option"). Add it. Avoid adding a Fixes to make life easier for the stable maintainer. When at it, slightly cleanup the lines, move "detach/-d" to a separate section rather than appending it at the end of the command description. Cc: Dr. David Alan Gilbert <dave@treblig.org> Cc: Fabiano Rosas <farosas@suse.de> Cc: Markus Armbruster <armbru@redhat.com> Signed-off-by: Peter Xu <peterx@redhat.com> --- Based-on: <20240430142737.29066-1-farosas@suse.de> ("[PATCH v3 0/6] migration removals & deprecations") --- hmp-commands.hx | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)