diff mbox

[v2,2.1,3/3] docs/qmp: Fix documentation of BLOCK_JOB_READY to match code

Message ID 1403889855-5740-4-git-send-email-armbru@redhat.com
State New
Headers show

Commit Message

Markus Armbruster June 27, 2014, 5:24 p.m. UTC
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 docs/qmp/qmp-events.txt | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

Comments

Eric Blake July 1, 2014, 5:12 p.m. UTC | #1
On 06/27/2014 11:24 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  docs/qmp/qmp-events.txt | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/docs/qmp/qmp-events.txt b/docs/qmp/qmp-events.txt
> index 22fea58..44be891 100644
> --- a/docs/qmp/qmp-events.txt
> +++ b/docs/qmp/qmp-events.txt
> @@ -157,12 +157,20 @@ Emitted when a block job is ready to complete.
>  
>  Data:
>  
> -- "device": device name (json-string)
> +- "type":     Job type (json-string; "stream" for image streaming
> +                                     "commit" for block commit)
> +- "device":   Device name (json-string)
> +- "len":      Maximum progress value (json-int)
> +- "offset":   Current progress value (json-int)
> +              On success this is equal to len.
> +              On failure this is less than len.
> +- "speed":    Rate limit, bytes per second (json-int)
>  

Design question - if BLOCK_JOB_READY reports failure (that is, offset <
len), are we still guaranteed to get a BLOCK_JOB_COMPLETED that also
reports failure, or does 'query-blockjobs' completely forget about the
job? If the job is completely lost, what recourse does management have
to learn about the failure (that is, if libvirtd restarts, how will it
learn whether a previously running job was aborted due to an error, if
it missed the event)?
Markus Armbruster July 2, 2014, 6:49 a.m. UTC | #2
Eric Blake <eblake@redhat.com> writes:

> On 06/27/2014 11:24 AM, Markus Armbruster wrote:
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  docs/qmp/qmp-events.txt | 12 ++++++++++--
>>  1 file changed, 10 insertions(+), 2 deletions(-)
>> 
>> diff --git a/docs/qmp/qmp-events.txt b/docs/qmp/qmp-events.txt
>> index 22fea58..44be891 100644
>> --- a/docs/qmp/qmp-events.txt
>> +++ b/docs/qmp/qmp-events.txt
>> @@ -157,12 +157,20 @@ Emitted when a block job is ready to complete.
>>  
>>  Data:
>>  
>> -- "device": device name (json-string)
>> +- "type":     Job type (json-string; "stream" for image streaming
>> +                                     "commit" for block commit)
>> +- "device":   Device name (json-string)
>> +- "len":      Maximum progress value (json-int)
>> +- "offset":   Current progress value (json-int)
>> +              On success this is equal to len.
>> +              On failure this is less than len.
>> +- "speed":    Rate limit, bytes per second (json-int)
>>  
>
> Design question - if BLOCK_JOB_READY reports failure (that is, offset <
> len), are we still guaranteed to get a BLOCK_JOB_COMPLETED that also
> reports failure, or does 'query-blockjobs' completely forget about the
> job?

Good one.  It's underspecified, as far as I can tell.  First step to fix
that is to find out what the code does.

>      If the job is completely lost, what recourse does management have
> to learn about the failure (that is, if libvirtd restarts, how will it
> learn whether a previously running job was aborted due to an error, if
> it missed the event)?

Let's worry about that when we know what the code does.
Paolo Bonzini July 2, 2014, 7:55 a.m. UTC | #3
Il 01/07/2014 19:12, Eric Blake ha scritto:
> On 06/27/2014 11:24 AM, Markus Armbruster wrote:
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  docs/qmp/qmp-events.txt | 12 ++++++++++--
>>  1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/docs/qmp/qmp-events.txt b/docs/qmp/qmp-events.txt
>> index 22fea58..44be891 100644
>> --- a/docs/qmp/qmp-events.txt
>> +++ b/docs/qmp/qmp-events.txt
>> @@ -157,12 +157,20 @@ Emitted when a block job is ready to complete.
>>
>>  Data:
>>
>> -- "device": device name (json-string)
>> +- "type":     Job type (json-string; "stream" for image streaming
>> +                                     "commit" for block commit)
>> +- "device":   Device name (json-string)
>> +- "len":      Maximum progress value (json-int)
>> +- "offset":   Current progress value (json-int)
>> +              On success this is equal to len.
>> +              On failure this is less than len.
>> +- "speed":    Rate limit, bytes per second (json-int)
>>
>
> Design question - if BLOCK_JOB_READY reports failure (that is, offset <
> len), are we still guaranteed to get a BLOCK_JOB_COMPLETED that also
> reports failure, or does 'query-blockjobs' completely forget about the
> job? If the job is completely lost, what recourse does management have
> to learn about the failure (that is, if libvirtd restarts, how will it
> learn whether a previously running job was aborted due to an error, if
> it missed the event)?

If you use rerror=stop/werror=stop you do have the behavior you request. 
  The job will not abort, it will be paused and libvirt will be able to 
see the error.  In fact, that was the rationale for adding 
rerror=stop/werror=stop to streaming (where you can just restart a 
failed job and only have to do minimal extra work, unlike mirror and 
commit).

There may be a couple of holes in the logic (for example a failure in 
bdrv_change_backing_file), but they can be plugged if necessary.

Paolo
Kevin Wolf July 2, 2014, 8:48 a.m. UTC | #4
Am 02.07.2014 um 08:49 hat Markus Armbruster geschrieben:
> Eric Blake <eblake@redhat.com> writes:
> 
> > On 06/27/2014 11:24 AM, Markus Armbruster wrote:
> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >> ---
> >>  docs/qmp/qmp-events.txt | 12 ++++++++++--
> >>  1 file changed, 10 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/docs/qmp/qmp-events.txt b/docs/qmp/qmp-events.txt
> >> index 22fea58..44be891 100644
> >> --- a/docs/qmp/qmp-events.txt
> >> +++ b/docs/qmp/qmp-events.txt
> >> @@ -157,12 +157,20 @@ Emitted when a block job is ready to complete.
> >>  
> >>  Data:
> >>  
> >> -- "device": device name (json-string)
> >> +- "type":     Job type (json-string; "stream" for image streaming
> >> +                                     "commit" for block commit)
> >> +- "device":   Device name (json-string)
> >> +- "len":      Maximum progress value (json-int)
> >> +- "offset":   Current progress value (json-int)
> >> +              On success this is equal to len.
> >> +              On failure this is less than len.
> >> +- "speed":    Rate limit, bytes per second (json-int)
> >>  
> >
> > Design question - if BLOCK_JOB_READY reports failure (that is, offset <
> > len), are we still guaranteed to get a BLOCK_JOB_COMPLETED that also
> > reports failure, or does 'query-blockjobs' completely forget about the
> > job?
> 
> Good one.  It's underspecified, as far as I can tell.  First step to fix
> that is to find out what the code does.

As far as I can see, BLOCK_JOB_READY implies success up to the point
where the event was emitted. Should we document that len = offset in all
cases? Perhaps add an assertion to block_job_event_ready(), too?

Kevin
diff mbox

Patch

diff --git a/docs/qmp/qmp-events.txt b/docs/qmp/qmp-events.txt
index 22fea58..44be891 100644
--- a/docs/qmp/qmp-events.txt
+++ b/docs/qmp/qmp-events.txt
@@ -157,12 +157,20 @@  Emitted when a block job is ready to complete.
 
 Data:
 
-- "device": device name (json-string)
+- "type":     Job type (json-string; "stream" for image streaming
+                                     "commit" for block commit)
+- "device":   Device name (json-string)
+- "len":      Maximum progress value (json-int)
+- "offset":   Current progress value (json-int)
+              On success this is equal to len.
+              On failure this is less than len.
+- "speed":    Rate limit, bytes per second (json-int)
 
 Example:
 
 { "event": "BLOCK_JOB_READY",
-    "data": { "device": "ide0-hd1" },
+    "data": { "device": "drive0", "type": "mirror", "speed": 0,
+              "len": 2097152, "offset": 2097152 }
     "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
 
 Note: The "ready to complete" status is always reset by a BLOCK_JOB_ERROR