Re: [PATCH v2 00/10] mirror: allow switching from background to active mode

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 03.11.2023 um 10:36 hat Markus Armbruster geschrieben:
>> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
>> 
>> > On 11.10.23 13:18, Fiona Ebner wrote:
>> >> Am 10.10.23 um 19:55 schrieb Vladimir Sementsov-Ogievskiy:
>> >>> On 09.10.23 12:46, Fiona Ebner wrote:
>> >>>>
>> >>>> Initially, I tried to go for a more general 'job-change' command, but
>> >>>> I couldn't figure out a way to avoid mutual inclusion between
>> >>>> block-core.json and job.json.
>> >>>>
>> >>>
>> >>> What is the problem with it? I still think that job-change would be 
>> >>> better.
>> >>>
>> >> If going for job-change in job.json, the dependencies would be
>> >> job-change -> JobChangeOptions -> JobChangeOptionsMirror -> MirrorCopyMode
>> >> query-jobs -> JobInfo -> JobInfoMirror
>> >> and we can't include block-core.json in job.json, because an inclusion
>> >> loop gives a build error.
>> 
>> Let me try to understand this.
>> 
>> Command job-change needs its argument type JobChangeOptions.
>> 
>> JobChangeOptions is a union, and JobChangeOptionsMirror is one of its
>> branches.
>> 
>> JobChangeOptionsMirror needs MirrorCopyMode from block-core.json.
>> 
>> block-core.json needs job.json for JobType and JobStatus.
>> 
>> >> Could be made to work by moving MirrorCopyMode (and
>> >> JobChangeOptionsMirror, JobInfoMirror) to job.json or some place that
>> >> can be included by both job.json and block-core.json. Moving the
>> >> type-specific definitions to the general job.json didn't feel right to
>> >> me. Including another file with type-specific definitions in job.json
>> >> feels slightly less wrong, but still not quite right and I didn't want
>> >> to create a new file just for MirrorCopyMode (and
>> >> JobChangeOptionsMirror, JobInfoMirror).
>> >> And going further and moving all mirror-related things to a separate
>> >> file would require moving along things like NewImageMode with it or
>> >> create yet another file for such general things used by multiple 
>> >> block-jobs.
>> >> If preferred, I can try and go with some version of the above.
>> >> 
>> >
>> > OK, I see the problem. Seems, that all requires some good refactoring. But 
>> > that's a preexisting big work, and should not hold up your series. I'm OK 
>> > to proceed with block-job-change.
>> 
>> Saving ourselves some internal refactoring is a poor excuse for
>> undesirable external interfaces.
>
> I'm not sure how undesirable it is. We have block-job-* commands for
> pretty much every other operation, so it's only consistent to have
> block-job-change, too.

Is the job abstraction a failure?

We have

    block-job- command      since   job- command    since
    -----------------------------------------------------
    block-job-set-speed     1.1
    block-job-cancel        1.1     job-cancel      3.0
    block-job-pause         1.3     job-pause       3.0
    block-job-resume        1.3     job-resume      3.0
    block-job-complete      1.3     job-complete    3.0
    block-job-dismiss       2.12    job-dismiss     3.0
    block-job-finalize      2.12    job-finalize    3.0
    block-job-change        8.2
    query-block-jobs        1.1     query-jobs

I was under the impression that we added the (more general) job-
commands to replace the (less general) block-job commands, and we're
keeping the latter just for compatibility.  Am I mistaken?

Which one should be used?

Why not deprecate the one that shouldn't be used?

The addition of block-job-change without even trying to do job-change
makes me wonder: have we given up on the job- interface?

I'm okay with giving up on failures.  All I want is clarity.  Right now,
I feel thoroughly confused about the status block-jobs and jobs, and how
they're related.

> Having job-change, too, might be nice in theory, but we don't have even
> a potential user for it at this point (i.e. a job type that isn't a
> block job, but for which changing options at runtime makes sense).
>
>> We need to answer two questions before we do that:
>> 
>> 1. How much work would the refactoring be?
>> 
>> 2. Is the interface improvement this enables worth the work?
>> 
>> Let's start with 1.
>> 
>> An obvious solution is to split JobType and JobStatus off job.json to
>> break the dependency of block-core.json on job.json.
>> 
>> But I'd like us to investigate another one.  block-core.json is *huge*.
>> It's almost a quarter of the entire QAPI schema.  Can we spin out block
>> jobs into block-job.json?  Moves the dependency on job.json from
>> block-core.json to block-job.json.
>
> It also makes job.json depend on block-job.json instead of
> block-core.json, so you only moved the problem without solving it.

block-job.json needs block-core.json and job.json.

job.json needs block-core.json.

No circle so far.