Re: [PATCH 3/3] dump: support cancel dump process

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 28.07.2022 um 14:37 hat Marc-André Lureau geschrieben:
>> Hi
>> 
>> On Wed, Jul 27, 2022 at 6:02 PM Hogan Wang via <qemu-devel@nongnu.org>
>> wrote:
>> 
>> > Break saving pages or dump iterate when dump job in cancel state,
>> > make sure dump process exits as soon as possible.
>> >
>> > Signed-off-by: Hogan Wang <hogan.wang@huawei.com>
>> >
>> 
>> Overall, the series looks good to me. Please send it with a top cover
>> letter, so it can be processed by patchew too.
>> 
>> I am not familiar with the job infrastructure, it would be nice if Kevin
>> could check the usage or give some advice.
>
> There is one big point I see with this series, which is that it could be
> considered an incompatible change to 'dump-guest-memory'. Clients are
> expected to manage the job now. Though in practice with the default
> settings, maybe it actually just results in clients receiving additional
> QMP events. (Technically, it is still incompatible because the command
> will now fail if you have another job called 'memory-guest-dump' - no
> good reason to have that, but it's a scenario that worked and breaks
> after this series.)
>
> Markus, do you have an opinion on whether job creation must be
> explicitly enabled with a new option or if we can just switch existing
> callers?

The conservative answer is to create a job only when new optional
argument @job-id is present, else maintain the traditional behavior.
This means we get to maintain two variations of the command: with and
without a job.

I can see the following alternatives:

(1) Keep both variations forever.  This could make sense if we believe
    the additional complexity and maintenance burden is insignificant.

(2) Deprecate "without a job", and remove it after a suitable grace
    period.  @job-id becomes mandatory then.

(3) Create a job even when the user doesn't ask for it (@job-id absent),
    accept the change in behavior.  This could make sense if we're
    confident the change is harmless in practice.  First step would be
    documenting the change in behavior.  Second step would be the
    argument why it's harmless.

    We may want to deprecate absent @job-id then, so we can get rid of
    the special case after a suitable grace period.

Does this answer your question, Kevin?

> The implementation of a very basic job looks mostly okay to me, though
> of course it doesn't support a few things like pausing the job and
> proper progress monitoring. But these things are optional, so it's not a
> blocker.

We can always improve on top.

> The one thing I would strongly recommend to include is an auto-dismiss
> option like every other job has. It is required so that management tools
> can query the final job status before it goes away. Most of the
> information is in QMP events, too, but events can be lost.

Concur.  Transmitting important information in QMP events only is an
interface design flaw.

>                                                            auto-finalize
> isn't required for this job because it doesn't actually do anything in
> the finalize phase.
>
> I'm not sure how safe the whole thing is when it runs in the background
> and you can send additional QMP commands while it's running, but that is
> preexisting with detach=true.
>
> Kevin