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

[Kevin Wolf <kwolf@redhat.com>]

Am 28.02.2024 um 19:07 hat Vladimir Sementsov-Ogievskiy geschrieben:
> On 03.11.23 18:56, Markus Armbruster wrote:
> > 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.
> 
> Hi! I didn't notice, that the series was finally merged.
> 
> About the APIs, I think, of course we should deprecate block-job-* API, because we already have jobs which are not block-jobs, so we can't deprecate job-* API.
> 
> So I suggest a plan:
> 
> 1. Add job-change command simply in block-core.json, as a simple copy
>    of block-job-change, to not care with resolving inclusion loops.
>    (ha we could simply name our block-job-change to be job-change and
>    place it in block-core.json, but now is too late)
> 
> 2. Support changing speed in a new job-chage command. (or both in
>    block-job-change and job-change, keeping them equal)

It should be both block-job-change and job-change.

Having job-change in block-core.json rather than job.json is ugly, but
if Markus doesn't complain, why would I.

> 3. Deprecate block-job-* APIs
> 
> 4. Wait 3 releases
> 
> 5. Drop block-job-* APIs

I consider these strictly optional. We don't really have strong reasons
to deprecate these commands (they are just thin wrappers), and I think
libvirt still uses block-job-* in some places.

We also need to check if the interfaces are really the same. For
example, JobInfo is only a small subset of BlockJobInfo. Some things
could be added to JobInfo, other things like BlockDeviceIoStatus don't
really have a place there, so we would have to introduce job type
specific data in query-jobs first.

I'm sure it's all doable, but it might be more work than your list above
would make you think.

> 6. Move all job-related stuff to job.json, drop `{ 'include':
>    'job.json' }` from block-core.json, and instead include
>    block-core.json into job.json

Of course, this cleanup assumes that steps 3.-5. are really implemented.
If not, you would end up moving a lot more block related things to
job.json than after them.

Kevin