On 12.03.24 18:49, Kevin Wolf wrote:
Am 12.03.2024 um 14:44 hat Vladimir Sementsov-Ogievskiy geschrieben:
On 11.03.24 18:15, Vladimir Sementsov-Ogievskiy wrote:
On 08.03.24 11:52, Kevin Wolf wrote:
Am 07.03.2024 um 20:42 hat Vladimir Sementsov-Ogievskiy geschrieben:
On 04.03.24 14:09, Peter Krempa wrote:
On Mon, Mar 04, 2024 at 11:48:54 +0100, Kevin Wolf wrote:
Am 28.02.2024 um 19:07 hat Vladimir Sementsov-Ogievskiy geschrieben:
On 03.11.23 18:56, Markus Armbruster wrote:
Kevin Wolf<kw...@redhat.com> writes:
[...]
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 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.
Libvirt uses 'block-job-cancel' because it has different semantics from
'job-cancel' which libvirt documented as the behaviour of the API that
uses it. (Semantics regarding the expectation of what is written to the
destination node at the point when the job is cancelled).
That's the following semantics:
# Note that if you issue 'block-job-cancel' after 'drive-mirror' has
# indicated (via the event BLOCK_JOB_READY) that the source and
# destination are synchronized, then the event triggered by this
# command changes to BLOCK_JOB_COMPLETED, to indicate that the
# mirroring has ended and the destination now has a point-in-time copy
# tied to the time of the cancellation.
Hmm. Looking at this, it looks for me, that should probably a
'block-job-complete" command (as leading to BLOCK_JOB_COMPLETED).
Yes, it's just a different completion mode.
Actually, what is the difference between block-job-complete and
block-job-cancel(force=false) for mirror in ready state?
I only see the following differencies:
1. block-job-complete documents that it completes the job
synchronously.. But looking at mirror code I see it just set
s->should_complete = true, which will be then handled
asynchronously.. So I doubt that documentation is correct.
2. block-job-complete will trigger final graph changes.
block-job-cancel will not.
Is [2] really useful? Seems yes: in case of some failure before
starting migration target, we'd like to continue executing source. So,
no reason to break block-graph in source, better keep it unchanged.
But I think, such behavior better be setup by mirror-job start
parameter, rather then by special option for cancel (or even
compelete) command, useful only for mirror.
I'm not sure, having the option on the complete command makes more sense
to me than having it in blockdev-mirror.
I do see the challenge of representing this meaningfully in QAPI,
though. Semantically it should be a union with job-specific options and
only mirror adds the graph-changes option. But the union variant
can't be directly selected from another option - instead we have a job
ID, and the variant is the job type of the job with this ID.
We already have such command: block-job-change. Which has id and type
parameters, so user have to pass both, to identify the job itself and pick
corresponding variant of the union type.
That would be good to somehow teach QAPI to get the type automatically from the
job itself...
Seems, that's easy enough to implement such a possibility, I'll try. At least
now I have a prototype, which compiles
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 0ae8ae62dc..332de67e52 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3116,13 +3116,11 @@
#
# @id: The job identifier
#
-# @type: The job type
-#
# Since: 8.2
##
{ 'union': 'BlockJobChangeOptions',
- 'base': { 'id': 'str', 'type': 'JobType' },
- 'discriminator': 'type',
+ 'base': { 'id': 'str' },
+ 'discriminator': 'JobType',
'data': { 'mirror': 'BlockJobChangeOptionsMirror' } }
We probably need some different syntax because I think in theory we
could get conflicts between option names and type names.
I think we shouldn't, as enum types in QAPI are CamelCase and members are lowercase.
Still, it's probably a bad way to rely on text case (I just imagine documenting this:))
It could be "'discriminator-type': 'JobType'" instead.
But I'll leave
this discussion to Markus. I hope you can figure out something that he
is willing to accept, getting the variant from a callback looks like
useful functionality.
For output, your implementation is probably not optimal because you're
going to look up things the calling code already knows, but it's
probably not the end of the world.
Yes I thought about it, we'll call find_block_job_locked() twice. That's
probably solvable by including some opaque pointer to QAPI structure, which
could be set during json parsing and then reused by qmp_* command itself.. But
I don't think it worth doing for simple search for a job. Simpler approach
would be to cache globally last result of find_block_job().. But again that
would be extra optimization
--
Best regards,
Vladimir
