Re: [Qemu-devel] [PATCH RFC] blkdebug: make the fault injection functionality callable from QMP

[Hitoshi Mitake <mitake.hitoshi@lab.ntt.co.jp>]

At Wed, 13 Aug 2014 08:20:53 -0600,
Eric Blake wrote:
> 
> [1  <text/plain; UTF-8 (quoted-printable)>]
> On 08/12/2014 10:48 PM, Hitoshi Mitake wrote:
> > This patch makes the fault injection functionality of blkdebug
> > callable from QMP. Motivation of this change is for testing and
> > debugging distributed systems. Ordinal distributed systems must handle
> > hardware faults because of its reason for existence, but testing
> > whether the systems can hanle such faults and recover in a correct
> > manner is really hard.
> > 
> 
> > 
> > {"execute": "set-block-fault-state", "arguments": {"id": "ide0-hd0",
> > "state": "error"}} # <- inject error to /dev/sda
> > {"return": {}}
> > 
> 
> Sounds interesting.

Hi Eric, thanks a lot for your review.

> 
> > Now the guest OS on the VM finds the disk is broken.
> > 
> > Of course, using QMP directly is painful for users (developers and
> > admins of distributed systems). I'm implementing user friendly
> > interface in vagrant-kvm [4] for blackbox testing. In addition, a
> > testing framework for injecting faults at critical timing (which
> > requires solid understanding of target systems) is in progress.
> > 
> > [1] http://ucare.cs.uchicago.edu/pdf/socc13-limplock.pdf
> > [2] http://docs.openstack.org/developer/swift/howto_installmultinode.html
> > [3] http://www.amazon.com/dp/B00C93QFHI
> > [4] https://github.com/adrahon/vagrant-kvm
> > 
> > Cc: Kevin Wolf <kwolf@redhat.com>
> > Cc: Stefan Hajnoczi <stefanha@redhat.com>
> > Signed-off-by: Hitoshi Mitake <mitake.hitoshi@lab.ntt.co.jp>
> > ---
> 
> > +++ b/qapi-schema.json
> > @@ -3481,3 +3481,42 @@
> >  # Since: 2.1
> >  ##
> >  { 'command': 'rtc-reset-reinjection' }
> > +
> > +##
> > +# @BlockFaultState:
> > +#
> > +# Injected fault state for a block device.
> > +#
> > +# @none: no fault
> > +#
> > +# @error: visible error
> > +#
> 
> Needs a Since: 2.2 line, as well as the ## trailing marker.

Thanks for pointing, I'll update it in v2.

> 
> > +{ 'enum': 'BlockFaultState', 'data': [ 'none', 'error' ] }
> > +
> > +##
> > +# @query-block-fault-state:
> > +#
> > +# Return block fault state information for the given block device.
> > +#
> > +# @name: name of block device
> > +#
> > +# Returns: @BlockFaultState of a block device.
> > +#
> > +# Since: 1.6
> 
> s/1.6/2.2/
> 
> Personally, I think this command is overkill.  Just enhance the existing
> query-block to output the fault state alongside everything else it
> already does.  There's no need to add a new command that outputs a
> subset of the existing command.

I agree. The above command is needless as you say. I'll enhance
query-block in v2.

> 
> > +##
> > +{ 'command': 'query-block-fault-state', 'data': { 'id': 'str' },
> > +  'returns': 'BlockFaultState' }
> > +##
> > +# @set-block-fault-state:
> > +#
> > +# Set fault state for the given block device.
> > +#
> > +# @name: the chardev's ID, must exist and not be in use
> > +# @state: fault state
> > +#
> > +# Returns: Nothing on success
> > +#
> > +# Since: 1.6
> 
> s/1.6/2.2/

Thanks, I'll fix it in v2, too.

> 
> > +##
> > +{ 'command': 'set-block-fault-state', 'data': {'id': 'str',
> > +  'state': 'BlockFaultState' }}
> > diff --git a/qmp-commands.hx b/qmp-commands.hx
> > index 4be4765..96ae7e2 100644
> > --- a/qmp-commands.hx
> > +++ b/qmp-commands.hx
> > @@ -3755,3 +3755,27 @@ Example:
> >  <- { "return": {} }
> >  
> >  EQMP
> > +    {
> > +        .name       = "query-block-fault-state",
> > +        .args_type  = "id:s",
> > +        .mhandler.cmd_new = qmp_marshal_input_query_block_fault_state,
> > +    },
> > +
> > +SQMP
> > +query-block-fault-state
> > +-----------------------
> > +
> > +Show block fault information.
> > +
> > +EQMP
> > +    {
> > +        .name       = "set-block-fault-state",
> > +        .args_type  = "id:s,state:q",
> > +        .mhandler.cmd_new = qmp_marshal_input_set_block_fault_state,
> > +    },
> > +
> > +SQMP
> > +set-block-fault-state
> > +-----------------------
> > +
> > +Set block fault information.
> > 
> 
> It would be nice to include an example section for the new command(s)
> (well, I argue that only one new command is needed).

OK, I'll add an example in v2.

Thanks,
Hitoshi