[Qemu-devel] [CFR 0/10] QMP specification review

[Qemu-devel] [CFR 0/10] QMP specification review

[Anthony Liguori]

This is the first set of commands as part of the QMP specification review.
Please comment on the individual commands specifications and Stefan and I will
try to fold the comments back into the command documentation.

[Qemu-devel] [CFR 1/10] qmp: balloon command

[Anthony Liguori]

balloon
-------

Request VM to change its memory allocation (in bytes).

Arguments:

- "value": New memory allocation (json-int)

Example:

-> { "execute": "balloon", "arguments": { "value": 536870912 } }
<- { "return": {} }

[Qemu-devel] [CFR 2/10] qmp: block_passwd command

[Anthony Liguori]

block_passwd
------------

Set the password of encrypted block devices.

Arguments:

- "device": device name (json-string)
- "password": password (json-string)

Example:

-> { "execute": "block_passwd", "arguments": { "device": "ide0-hd0",
                                               "password": "12345" } }
<- { "return": {} }

[Qemu-devel] [CFR 3/10] command

[Anthony Liguori]

qmp_capabilities
----------------

Enable QMP capabilities.

Arguments: None.

Example:

-> { "execute": "qmp_capabilities" }
<- { "return": {} }

Note: This command must be issued before issuing any other command.

[Qemu-devel] [CFR 4/10] command

[Anthony Liguori]

change
------

Change a removable medium or VNC configuration.

Arguments:

- "device": device name (json-string)
- "target": filename or item (json-string)
- "arg": additional argument (json-string, optional)

Examples:

1. Change a removable medium

-> { "execute": "change",
             "arguments": { "device": "ide1-cd0",
                            "target": "/srv/images/Fedora-12-x86_64-DVD.iso" } }
<- { "return": {} }

2. Change VNC password

-> { "execute": "change",
             "arguments": { "device": "vnc", "target": "password",
                            "arg": "foobar1" } }
<- { "return": {} }

[Qemu-devel] [CFR 5/10] closefd command

[Anthony Liguori]

closefd
-------

Close a file descriptor previously passed via SCM rights.

Arguments:

- "fdname": file descriptor name (json-string)

Example:

-> { "execute": "closefd", "arguments": { "fdname": "fd1" } }
<- { "return": {} }

[Qemu-devel] [CFR 6/10] cont command

[Anthony Liguori]

cont
----

Resume emulation.

Arguments: None.

Example:

-> { "execute": "cont" }
<- { "return": {} }

[Qemu-devel] [CFR 7/10] cpu command

[Anthony Liguori]

cpu
---

Set the default CPU.

Arguments:

- "index": the CPU's index (json-int)

Example:

-> { "execute": "cpu", "arguments": { "index": 0 } }
<- { "return": {} }

Note: CPUs' indexes are obtained with the 'query-cpus' command.

[Qemu-devel] [CFR 8/10] device_add command

[Anthony Liguori]

device_add
----------

Add a device.

Arguments:

- "driver": the name of the new device's driver (json-string)
- "bus": the device's parent bus (device tree path, json-string, optional)
- "id": the device's ID, must be unique (json-string)
- device properties

Example:

-> { "execute": "device_add", "arguments": { "driver": "e1000", "id": "net1" } }
<- { "return": {} }

Notes:

(1) For detailed information about this command, please refer to the
    'docs/qdev-device-use.txt' file.

(2) It's possible to list device properties by running QEMU with the
    "-device DEVICE,\?" command-line argument, where DEVICE is the device's name

[Qemu-devel] [CFR 9/10] device_del command

[Anthony Liguori]

device_del
----------

Remove a device.

Arguments:

- "id": the device's ID (json-string)

Example:

-> { "execute": "device_del", "arguments": { "id": "net1" } }
<- { "return": {} }

[Qemu-devel] [CFR 10/10] eject command

[Anthony Liguori]

eject
-----

Eject a removable medium.

Arguments: 

- force: force ejection (json-bool, optional)
- device: device name (json-string)

Example:

-> { "execute": "eject", "arguments": { "device": "ide1-cd0" } }
<- { "return": {} }

Note: The "force" argument defaults to false.

Re: [Qemu-devel] [CFR 1/10] qmp: balloon command

[Anthony Liguori]

On 06/15/2010 11:30 AM, Anthony Liguori wrote:
> balloon
> -------
>
> Request VM to change its memory allocation (in bytes).
>    

We ought to clarify a few points:

1) What happens if there isn't a balloon device?
2) What happens upon reboot?
3) What is the valid range for value?
4) How does a user determine when the guest has actually responded to 
the balloon request?

This is a case where I think we ought to use async events because the 
guest can adjust it's balloon allotment without an explicit request from 
the host.  In fact, if we add a shrinker callback to the balloon driver 
(and we should) it will do exactly this.

Regards,

Anthony Liguori

> Arguments:
>
> - "value": New memory allocation (json-int)
>
> Example:
>
> ->  { "execute": "balloon", "arguments": { "value": 536870912 } }
> <- { "return": {} }
>
>
>    

Re: [Qemu-devel] [CFR 2/10] qmp: block_passwd command

[Anthony Liguori]

On 06/15/2010 11:30 AM, Anthony Liguori wrote:
> block_passwd
> ------------
>    

I dislike abbreviations.  I also think that we should make commands 
verbs.  So I'd like to change the name to set_block_password and then we 
can alias block_passwd to that command if we need to.

> Set the password of encrypted block devices.
>
> Arguments:
>
> - "device": device name (json-string)
> - "password": password (json-string)
>    

What happens if:

1) device does not exist
2) media is not present
3) block format does not support encryption
4) password is invalid

Regards,

Anthony Liguori

> Example:
>
> ->  { "execute": "block_passwd", "arguments": { "device": "ide0-hd0",
>                                                 "password": "12345" } }
> <- { "return": {} }
>
>
>    

Re: [Qemu-devel] [CFR 5/10] closefd command

[Anthony Liguori]

On 06/15/2010 11:30 AM, Anthony Liguori wrote:
> closefd
> -------
>
> Close a file descriptor previously passed via SCM rights.
>
> Arguments:
>
> - "fdname": file descriptor name (json-string)
>    

I wonder if setfd/closefd should be QMP functions or whether we should 
make it core features of the protocol?

Regards,

Anthony Liguori

> Example:
>
> ->  { "execute": "closefd", "arguments": { "fdname": "fd1" } }
> <- { "return": {} }
>
>
>    

Re: [Qemu-devel] [CFR 6/10] cont command

[Anthony Liguori]

On 06/15/2010 11:30 AM, Anthony Liguori wrote:
> cont
> ----
>    

continue -> aliases cont.

> Resume emulation.
>    

What happens if execution is not stopped?

Regards,

Anthony Liguori

> Arguments: None.
>
> Example:
>
> ->  { "execute": "cont" }
> <- { "return": {} }
>
>    

Re: [Qemu-devel] [CFR 8/10] device_add command

[Anthony Liguori]

On 06/15/2010 11:30 AM, Anthony Liguori wrote:
> device_add
> ----------
>
> Add a device.
>
> Arguments:
>
> - "driver": the name of the new device's driver (json-string)
>    

What class of name is this?  I believe this is a qdev name but the 
example is misleading because someone could reasonable do { "driver": 
"virtio", "id": "net1"}

> - "bus": the device's parent bus (device tree path, json-string, optional)
> - "id": the device's ID, must be unique (json-string)
> - device properties
>    

I think we need to document all of the supported devices and their 
properties as part of the spec.

What happens if we cannot add the device?  How does one use this for hot 
add?

Is the device available within the guest immediately after the 
device_add operation completes?

Regards,

Anthony Liguori

> Example:
>
> ->  { "execute": "device_add", "arguments": { "driver": "e1000", "id": "net1" } }
> <- { "return": {} }
>
> Notes:
>
> (1) For detailed information about this command, please refer to the
>      'docs/qdev-device-use.txt' file.
>
> (2) It's possible to list device properties by running QEMU with the
>      "-device DEVICE,\?" command-line argument, where DEVICE is the device's name
>
>    

Re: [Qemu-devel] [CFR 9/10] device_del command

[Anthony Liguori]

On 06/15/2010 11:30 AM, Anthony Liguori wrote:
> device_del
> ----------
>
> Remove a device.
>
> Arguments:
>
> - "id": the device's ID (json-string)
>    

How does one discover a device's ID?

What are the failure conditions?

Is the device removed from the guest immediately upon receiving the return?

Regards,

Anthony Liguori

> Example:
>
> ->  { "execute": "device_del", "arguments": { "id": "net1" } }
> <- { "return": {} }
>
>
>    

Re: [Qemu-devel] [CFR 7/10] cpu command

[Anthony Liguori]

On 06/15/2010 11:30 AM, Anthony Liguori wrote:
> cpu
> ---
>
> Set the default CPU.
>    

We need to remove this from QMP IMHO.  All QMP commands should take a 
CPU index.

Regards,

Anthony Liguori

> Arguments:
>
> - "index": the CPU's index (json-int)
>
> Example:
>
> ->  { "execute": "cpu", "arguments": { "index": 0 } }
> <- { "return": {} }
>
> Note: CPUs' indexes are obtained with the 'query-cpus' command.
>
>    

[Qemu-devel] Re: [CFR 9/10] device_del command

[Jan Kiszka]

Anthony Liguori wrote:
> device_del
> ----------
> 
> Remove a device.
> 
> Arguments:
> 
> - "id": the device's ID (json-string)

"id" should become "device" (I hope to send the corresponding patches
this night). The idea is to not only allow global device IDs but also
qtree paths here.

> 
> Example:
> 
> -> { "execute": "device_del", "arguments": { "id": "net1" } }
> <- { "return": {} }
> 
> 
> 

A reference to docs/qdev-device-use.txt would be good here as well (as
done for device_add).

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

[Qemu-devel] Re: [CFR 7/10] cpu command

[Jan Kiszka]

Anthony Liguori wrote:
> cpu
> ---
> 
> Set the default CPU.
> 
> Arguments:
> 
> - "index": the CPU's index (json-int)
> 
> Example:
> 
> -> { "execute": "cpu", "arguments": { "index": 0 } }
> <- { "return": {} }
> 
> Note: CPUs' indexes are obtained with the 'query-cpus' command.
> 

To my understanding, this command is supposed to die. All QMP commands
addressing a specific CPU should do this explicitly.

(Will send a patch to enable this without having to modify the HMP
syntax. And another patch to allow masking commands from QMP.)

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [CFR 4/10] command

[Anthony Liguori]

On 06/15/2010 11:30 AM, Anthony Liguori wrote:
> change
> ------
>
> Change a removable medium or VNC configuration.
>    

I think this command is awkward and should be split into a change-media 
and change-vnc-password command.

> Arguments:
>
> - "device": device name (json-string)
> - "target": filename or item (json-string)
> - "arg": additional argument (json-string, optional)
>    

For change-media:

1) what happens if the drive does not support removable media?
2) what happens if the drive is locked?
3) what happens if media is not currently present?
4) what if I want to pass additional options to target like format=raw?
5) is the media change immediately present to the guest upon return of 
the command?

For change-vnc-password:

1) what if VNC is not in use?
2) what if I don't have vnc authentication enabled?
3) does changing the password have any affect on existing sessions?
4) is a new password required immediately after the command completes?

Regards,

Anthony Liguori

> Examples:
>
> 1. Change a removable medium
>
> ->  { "execute": "change",
>               "arguments": { "device": "ide1-cd0",
>                              "target": "/srv/images/Fedora-12-x86_64-DVD.iso" } }
> <- { "return": {} }
>
> 2. Change VNC password
>
> ->  { "execute": "change",
>               "arguments": { "device": "vnc", "target": "password",
>                              "arg": "foobar1" } }
> <- { "return": {} }
>
>
>    

Re: [Qemu-devel] Re: [CFR 9/10] device_del command

[Miguel Di Ciurcio Filho]

On Tue, Jun 15, 2010 at 1:59 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> Anthony Liguori wrote:
>> device_del
>> ----------
>>
>> Remove a device.
>>
>> Arguments:
>>
>> - "id": the device's ID (json-string)
>
> "id" should become "device" (I hope to send the corresponding patches
> this night). The idea is to not only allow global device IDs but also
> qtree paths here.
>

Now I'm confused. On a previous email when discussing about
query-netdev, I initially proposed this[1]:

-> { "execute": "query-netdev" }
<- {
      "return": [
         {
            "device": "tap.0",
            "vlan": 0,
            "info": {
               "script": "/etc/qemu-ifup",
               "downscript": "/etc/qemu-ifdown",
               "ifname": "tap0",
               "model": "tap"
            },
         },

And latter on Anthony suggested to replace "device" by "id" [2].

So my doubt is: we should identify guest/frontend devices in QMP using
the word "device" instead of "id" and identify backend devices by
using the word "id"?

Regards,

Miguel

[1] http://lists.gnu.org/archive/html/qemu-devel/2010-06/msg00619.html
[2] http://lists.gnu.org/archive/html/qemu-devel/2010-06/msg00943.html

Re: [Qemu-devel] Re: [CFR 9/10] device_del command

[Anthony Liguori]

On 06/15/2010 03:48 PM, Miguel Di Ciurcio Filho wrote:
> On Tue, Jun 15, 2010 at 1:59 PM, Jan Kiszka<jan.kiszka@siemens.com>  wrote:
>    
>> Anthony Liguori wrote:
>>      
>>> device_del
>>> ----------
>>>
>>> Remove a device.
>>>
>>> Arguments:
>>>
>>> - "id": the device's ID (json-string)
>>>        
>> "id" should become "device" (I hope to send the corresponding patches
>> this night). The idea is to not only allow global device IDs but also
>> qtree paths here.
>>
>>      
> Now I'm confused. On a previous email when discussing about
> query-netdev, I initially proposed this[1]:
>
> ->  { "execute": "query-netdev" }
> <- {
>        "return": [
>           {
>              "device": "tap.0",
>              "vlan": 0,
>              "info": {
>                 "script": "/etc/qemu-ifup",
>                 "downscript": "/etc/qemu-ifdown",
>                 "ifname": "tap0",
>                 "model": "tap"
>              },
>           },
>
> And latter on Anthony suggested to replace "device" by "id" [2].
>
> So my doubt is: we should identify guest/frontend devices in QMP using
> the word "device" instead of "id" and identify backend devices by
> using the word "id"?
>    

id.

What Jan is referring to is the qdev device path verses the qdev id.  
netdev objects are not qdev devices, they are just QemuOpts.  In netdev 
parlance, the unique identifier is called the 'id'.

Regards,

Anthony Liguori

> Regards,
>
> Miguel
>
> [1] http://lists.gnu.org/archive/html/qemu-devel/2010-06/msg00619.html
> [2] http://lists.gnu.org/archive/html/qemu-devel/2010-06/msg00943.html
>    

[Qemu-devel] Re: [CFR 6/10] cont command

[Juan Quintela]

Anthony Liguori <aliguori@us.ibm.com> wrote:
> cont
> ----
>
> Resume emulation.
>
> Arguments: None.
>
> Example:
>
> -> { "execute": "cont" }
> <- { "return": {} }

This is related to the commands, not QMP per se:

Once that we are talking about "cont" command.  There are two cases that
we need to think of:

- incoming migration:

If you start with -incoming foo, and then run "cont" on the monitor
without having started the migration .... corruption is ensured.

- outgoing migration

After sucessful migration, we can issue "cont" command in source, and
having source and target running at the same time -> disk corruption
again.

My suggestion:
- add a third state "incoming", and cont/stop don't work on that state
- add a fourth state "migrated", and "cont" gives an explicit error, and you
  have to run "cont --force" or "cont" twice (whatever) to get it to continue.

Later, Juan.

Re: [Qemu-devel] [CFR 2/10] qmp: block_passwd command

[Kevin Wolf]

Am 15.06.2010 18:44, schrieb Anthony Liguori:
> On 06/15/2010 11:30 AM, Anthony Liguori wrote:
>> block_passwd
>> ------------
>>    
> 
> I dislike abbreviations.  I also think that we should make commands 
> verbs.  So I'd like to change the name to set_block_password and then we 
> can alias block_passwd to that command if we need to.

Actually, I think this command should be removed and passwords should
always be set as a blkdevice_add parameter when attaching an image.

Your comments on error cases are still relevant for blockdev_add, of course.

Kevin

Re: [Qemu-devel] Re: [CFR 6/10] cont command

[Anthony Liguori]

On 06/16/2010 08:11 AM, Juan Quintela wrote:
> Anthony Liguori<aliguori@us.ibm.com>  wrote:
>    
>> cont
>> ----
>>
>> Resume emulation.
>>
>> Arguments: None.
>>
>> Example:
>>
>> ->  { "execute": "cont" }
>> <- { "return": {} }
>>      
> This is related to the commands, not QMP per se:
>
> Once that we are talking about "cont" command.  There are two cases that
> we need to think of:
>
> - incoming migration:
>
> If you start with -incoming foo, and then run "cont" on the monitor
> without having started the migration .... corruption is ensured.
>    

It's only ensured if you've got the same disk image running on another 
machine.  Considering that we support migrating from a file and we 
support migrating block devices, I don't think it's practical.

> - outgoing migration
>
> After sucessful migration, we can issue "cont" command in source, and
> having source and target running at the same time ->  disk corruption
> again.
>
> My suggestion:
> - add a third state "incoming", and cont/stop don't work on that state
> - add a fourth state "migrated", and "cont" gives an explicit error, and you
>    have to run "cont --force" or "cont" twice (whatever) to get it to continue.
>    

Very few users are going to do manual migration like this and those that 
do have no good reason to execute cont in either of these scenarios.  A 
--force command like this is equivalent to popping up a message box 
saying "are you sure you really want to do this" which most users find 
to be extremely annoying.

We should try to inform users when it's likely that they'll stumble upon 
a dangerous action.  cache=volatile is a good example of this because a 
user could have used it pretty easily and it's a reasonable expectation 
that we wouldn't expose a feature that could lead to corruption in 
obscure cases.

If a user executes cont in either of these scenarios and has two copies 
of a virtual machine running accessing the same resources, then they 
surely ought to expect bad behavior.

Regards,

Anthony LIguori

> Later, Juan.
>
>    

Re: [Qemu-devel] [CFR 2/10] qmp: block_passwd command

[Daniel P. Berrange]

On Wed, Jun 16, 2010 at 03:33:14PM +0200, Kevin Wolf wrote:
> Am 15.06.2010 18:44, schrieb Anthony Liguori:
> > On 06/15/2010 11:30 AM, Anthony Liguori wrote:
> >> block_passwd
> >> ------------
> >>    
> > 
> > I dislike abbreviations.  I also think that we should make commands 
> > verbs.  So I'd like to change the name to set_block_password and then we 
> > can alias block_passwd to that command if we need to.
> 
> Actually, I think this command should be removed and passwords should
> always be set as a blkdevice_add parameter when attaching an image.

That doesn't work for people adding devices on the command line though,
because you don't want the password exposed in ARGV. It could be useful
to separate the passwords from the device config, both here and wrt to 
VNC (and SPICE in future) instead set them by reference.

eg, register two named secrets  with associated keys

    add_secret secret1 123456
    add_secret secret2 123456

Then you can reference that secret in all the devices added

    blockdev_add  file=foo.qcow1,password=secret1
    blockdev_add  file=foo.qcow2,password=secret2
    blockdev_add  file=foo.qcow3,password=secret1
    graphics_add  type=vnc,password=secret1

On the command line the user could easily specify a config file with
the named secrets

   #cat > secrets.cfg <<EOF
   secret1 = 123465
   secret2 = abcd
   EOF
   qemu -secrets secrets.cfg  -blockdev file=foo.qcow1,password=secret1


It also means that if management apps are logging all monitor commands
for purposes of debugging, or bug reporting, they don't have to worry
so much about exposing their password. just blacklist the 'add_secret'
command from logging and still get the full device configs logged with
the important details.

Regards,
Daniel
-- 
|: Red Hat, Engineering, London    -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org        -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

[Qemu-devel] Re: [CFR 6/10] cont command

[Juan Quintela]

Anthony Liguori <anthony@codemonkey.ws> wrote:
> On 06/16/2010 08:11 AM, Juan Quintela wrote:

> It's only ensured if you've got the same disk image running on another
> machine.  Considering that we support migrating from a file and we
> support migrating block devices, I don't think it's practical.
>
>> - outgoing migration
>>
>> After sucessful migration, we can issue "cont" command in source, and
>> having source and target running at the same time ->  disk corruption
>> again.
>>
>> My suggestion:
>> - add a third state "incoming", and cont/stop don't work on that state
>> - add a fourth state "migrated", and "cont" gives an explicit error, and you
>>    have to run "cont --force" or "cont" twice (whatever) to get it to continue.
>>    
>
> Very few users are going to do manual migration like this and those
> that do have no good reason to execute cont in either of these
> scenarios.

as of today, libvirt uses it (guess who filled that bug to me).

>  A --force command like this is equivalent to popping up a
> message box saying "are you sure you really want to do this" which
> most users find to be extremely annoying.

I had to debug this one from testers/field.  They were testing things
and it was very "practical" to launch guest on machine A, configure
whatever they wanted, migrate to machine B.  test whatever on machine B.
back to machine A, continue.

You can guess what happened.  The problem here is that qemu is not
giving user the _minimal_ advise that something could go wrong.  And it
is not going to be wrong, it is going to cause disk corruption for sure :(

> We should try to inform users when it's likely that they'll stumble
> upon a dangerous action.  cache=volatile is a good example of this
> because a user could have used it pretty easily and it's a reasonable
> expectation that we wouldn't expose a feature that could lead to
> corruption in obscure cases.

This is not _so_ obscure if you run qemu by hand :(
you have a nice "(qemu)" prompt, and if you issue "cont", bad things happen.

> If a user executes cont in either of these scenarios and has two
> copies of a virtual machine running accessing the same resources, then
> they surely ought to expect bad behavior.

It is not _so_ easy O:-).
Consider the example that I showed you:

(host A)		(host B)
launch qemu             launch qemu -incoming
migrate host B
                        .....
                        do your things
                        exit/poweroff/...

At this point you have a qemu launched on machine A, with nothing on
machine B.  running "cont" on machine A, have disastreus consecuences,
and there is no way to prevent it :(

As I have received this bug from users a couple of times, I would like
to be able to prevent this case.

Later, Juan.

Re: [Qemu-devel] Re: [CFR 6/10] cont command

[Daniel P. Berrange]

On Wed, Jun 16, 2010 at 03:11:24PM +0200, Juan Quintela wrote:
> Anthony Liguori <aliguori@us.ibm.com> wrote:
> > cont
> > ----
> >
> > Resume emulation.
> >
> > Arguments: None.
> >
> > Example:
> >
> > -> { "execute": "cont" }
> > <- { "return": {} }
> 
> This is related to the commands, not QMP per se:
> 
> Once that we are talking about "cont" command.  There are two cases that
> we need to think of:
> 
> - incoming migration:
> 
> If you start with -incoming foo, and then run "cont" on the monitor
> without having started the migration .... corruption is ensured.

This is why '-incoming' command line arg should die, and be replaced
with a 'incoming' monitor command that would simply not allow 'cont'
to be run until it completed.

For that matter, even with '-incoming' arg on command line we could
refuse to honour 'cont' until the incoming migration had been done.

> - outgoing migration
> 
> After sucessful migration, we can issue "cont" command in source, and
> having source and target running at the same time -> disk corruption
> again.

This doesn't have to mean corruption. eg two machines using cluster-LVM.
The target QEMU is using a writable snapshot of the volume the source
QEMU is using. So you could in fact start the source again and have two
copies of the guest running at once. At the QEMU level I don't think we
should try to force policy of this kind, since it'll prevent people
experimenting with interesting new use cases. There are also soooooooo
many other ways you can  trash your data with multiple hosts. If you
want safe migration, use a management app which adds a level of policy
to protect against stupid decisions

Daniel
-- 
|: Red Hat, Engineering, London    -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org        -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

[Qemu-devel] Re: [CFR 6/10] cont command

[Anthony Liguori]

On 06/16/2010 11:17 AM, Juan Quintela wrote:
> Anthony Liguori<anthony@codemonkey.ws>  wrote:
>    
>> On 06/16/2010 08:11 AM, Juan Quintela wrote:
>>      
>    
>> It's only ensured if you've got the same disk image running on another
>> machine.  Considering that we support migrating from a file and we
>> support migrating block devices, I don't think it's practical.
>>
>>      
>>> - outgoing migration
>>>
>>> After sucessful migration, we can issue "cont" command in source, and
>>> having source and target running at the same time ->   disk corruption
>>> again.
>>>
>>> My suggestion:
>>> - add a third state "incoming", and cont/stop don't work on that state
>>> - add a fourth state "migrated", and "cont" gives an explicit error, and you
>>>     have to run "cont --force" or "cont" twice (whatever) to get it to continue.
>>>
>>>        
>> Very few users are going to do manual migration like this and those
>> that do have no good reason to execute cont in either of these
>> scenarios.
>>      
> as of today, libvirt uses it (guess who filled that bug to me).
>    

libvirt is not a human so I fail to see how forcing it to use a --force 
option would help them.

Either we didn't document migration well enough or their developers are 
not careful enough.  Considering our lack of documentation, I'm sure it 
was the former.

>>   A --force command like this is equivalent to popping up a
>> message box saying "are you sure you really want to do this" which
>> most users find to be extremely annoying.
>>      
> I had to debug this one from testers/field.  They were testing things
> and it was very "practical" to launch guest on machine A, configure
> whatever they wanted, migrate to machine B.  test whatever on machine B.
> back to machine A, continue.
>    

Honestly, that's a terrible testing strategy.  You cannot just execute 
random commands and hope nothing bad happens.

> You can guess what happened.  The problem here is that qemu is not
> giving user the _minimal_ advise that something could go wrong.  And it
> is not going to be wrong, it is going to cause disk corruption for sure :(
>
>    
>> We should try to inform users when it's likely that they'll stumble
>> upon a dangerous action.  cache=volatile is a good example of this
>> because a user could have used it pretty easily and it's a reasonable
>> expectation that we wouldn't expose a feature that could lead to
>> corruption in obscure cases.
>>      
> This is not _so_ obscure if you run qemu by hand :(
> you have a nice "(qemu)" prompt, and if you issue "cont", bad things happen.
>    

And if you issue system_reset, quit, commit, loadvm, pci_del, or any set 
of commands bad things can happen including some form of data loss or 
corruption.

IMHO, there's a significant difference between twiddling something where 
there is a reasonable expectation that the impact is only going to be 
related to performance (like -smp X, -m X, or cache=X) and just trying 
random things.

>> If a user executes cont in either of these scenarios and has two
>> copies of a virtual machine running accessing the same resources, then
>> they surely ought to expect bad behavior.
>>      
> It is not _so_ easy O:-).
> Consider the example that I showed you:
>
> (host A)		(host B)
> launch qemu             launch qemu -incoming
> migrate host B
>                          .....
>                          do your things
>                          exit/poweroff/...
>
> At this point you have a qemu launched on machine A, with nothing on
> machine B.  running "cont" on machine A, have disastreus consecuences,
> and there is no way to prevent it :(
>    

If there was a reasonable belief that it wouldn't result in disaster, I 
would fully support you.  However, I can't think of any rational reason 
why someone would do this.  I can't think of a better analogy to 
shooting yourself in the foot.

> As I have received this bug from users a couple of times, I would like
> to be able to prevent this case.
>    

I've never seen anyone hit run into this before.  Can you show me a bug 
report?  I'd love to see how someone expected this to behave.

Regards,

Anthony Liguori

> Later, Juan.
>    

Re: [Qemu-devel] Re: [CFR 6/10] cont command

[Anthony Liguori]

On 06/16/2010 11:25 AM, Daniel P. Berrange wrote:
>
>> This is related to the commands, not QMP per se:
>>
>> Once that we are talking about "cont" command.  There are two cases that
>> we need to think of:
>>
>> - incoming migration:
>>
>> If you start with -incoming foo, and then run "cont" on the monitor
>> without having started the migration .... corruption is ensured.
>>      
> This is why '-incoming' command line arg should die, and be replaced
> with a 'incoming' monitor command that would simply not allow 'cont'
> to be run until it completed.
>
> For that matter, even with '-incoming' arg on command line we could
> refuse to honour 'cont' until the incoming migration had been done.
>    

If we had an incoming migration command, I think we'd have to think 
careful about it's semantics.  Is it reasonable to allow a machine 
that's otherwise running to do an incoming command?

Regards,

Anthony Liguori

>> - outgoing migration
>>
>> After sucessful migration, we can issue "cont" command in source, and
>> having source and target running at the same time ->  disk corruption
>> again.
>>      
> This doesn't have to mean corruption. eg two machines using cluster-LVM.
> The target QEMU is using a writable snapshot of the volume the source
> QEMU is using. So you could in fact start the source again and have two
> copies of the guest running at once. At the QEMU level I don't think we
> should try to force policy of this kind, since it'll prevent people
> experimenting with interesting new use cases. There are also soooooooo
> many other ways you can  trash your data with multiple hosts. If you
> want safe migration, use a management app which adds a level of policy
> to protect against stupid decisions
>
> Daniel
>    

Re: [Qemu-devel] Re: [CFR 6/10] cont command

[Jamie Lokier]

Anthony Liguori wrote:
> On 06/16/2010 11:17 AM, Juan Quintela wrote:
> >Consider the example that I showed you:
> >
> >(host A)		(host B)
> >launch qemu             launch qemu -incoming
> >migrate host B
> >                         .....
> >                         do your things
> >                         exit/poweroff/...
> >
> >At this point you have a qemu launched on machine A, with nothing on
> >machine B.  running "cont" on machine A, have disastreus consecuences,
> >and there is no way to prevent it :(
> >   
> 
> If there was a reasonable belief that it wouldn't result in disaster, I 
> would fully support you.  However, I can't think of any rational reason 
> why someone would do this.  I can't think of a better analogy to 
> shooting yourself in the foot.

That looks like a useful way to fork a guest for testing, if host B is
launched with -snapshot, or a copy of the disk image, or a qcow2 child of it.

Does it work? :-)

-- Jamie

[Qemu-devel] Re: [CFR 6/10] cont command

[Juan Quintela]

Anthony Liguori <anthony@codemonkey.ws> wrote:
> On 06/16/2010 11:25 AM, Daniel P. Berrange wrote:
>>
>>> This is related to the commands, not QMP per se:
>>>
>>> Once that we are talking about "cont" command.  There are two cases that
>>> we need to think of:
>>>
>>> - incoming migration:
>>>
>>> If you start with -incoming foo, and then run "cont" on the monitor
>>> without having started the migration .... corruption is ensured.
>>>      
>> This is why '-incoming' command line arg should die, and be replaced
>> with a 'incoming' monitor command that would simply not allow 'cont'
>> to be run until it completed.
>>
>> For that matter, even with '-incoming' arg on command line we could
>> refuse to honour 'cont' until the incoming migration had been done.
>>    
>
> If we had an incoming migration command, I think we'd have to think
> careful about it's semantics.  Is it reasonable to allow a machine
> that's otherwise running to do an incoming command?

It is the same problem that loadvm.

And no, loadvm several times don't work well either.

Later, Juan.

[Qemu-devel] Re: [CFR 6/10] cont command

[Juan Quintela]

Anthony Liguori <anthony@codemonkey.ws> wrote:
> On 06/16/2010 11:17 AM, Juan Quintela wrote:
>> Anthony Liguori<anthony@codemonkey.ws>  wrote:
>>    
>>> On 06/16/2010 08:11 AM, Juan Quintela wrote:
>>>      
>>    
>>> It's only ensured if you've got the same disk image running on another
>>> machine.  Considering that we support migrating from a file and we
>>> support migrating block devices, I don't think it's practical.
>>>
>>>      
>>>> - outgoing migration
>>>>
>>>> After sucessful migration, we can issue "cont" command in source, and
>>>> having source and target running at the same time ->   disk corruption
>>>> again.
>>>>
>>>> My suggestion:
>>>> - add a third state "incoming", and cont/stop don't work on that state
>>>> - add a fourth state "migrated", and "cont" gives an explicit error, and you
>>>>     have to run "cont --force" or "cont" twice (whatever) to get it to continue.
>>>>
>>>>        
>>> Very few users are going to do manual migration like this and those
>>> that do have no good reason to execute cont in either of these
>>> scenarios.
>>>      
>> as of today, libvirt uses it (guess who filled that bug to me).
>>    
>
> libvirt is not a human so I fail to see how forcing it to use a
> --force option would help them.

There were two bugs.  The incoming/vs cont were the libvirt one.

> Either we didn't document migration well enough or their developers
> are not careful enough.  Considering our lack of documentation, I'm
> sure it was the former.

>> I had to debug this one from testers/field.  They were testing things
>> and it was very "practical" to launch guest on machine A, configure
>> whatever they wanted, migrate to machine B.  test whatever on machine B.
>> back to machine A, continue.
>>    
>
> Honestly, that's a terrible testing strategy.  You cannot just execute
> random commands and hope nothing bad happens.

As testing strategy: ugly, but I find that what testers do -> customers
do later, so .....

>>> We should try to inform users when it's likely that they'll stumble
>>> upon a dangerous action.  cache=volatile is a good example of this
>>> because a user could have used it pretty easily and it's a reasonable
>>> expectation that we wouldn't expose a feature that could lead to
>>> corruption in obscure cases.
>>>      
>> This is not _so_ obscure if you run qemu by hand :(
>> you have a nice "(qemu)" prompt, and if you issue "cont", bad things happen.
>>    
>
> And if you issue system_reset, quit, commit, loadvm, pci_del, or any
> set of commands bad things can happen including some form of data loss
> or corruption.

pci_del, system_reset, .... looks dangerous.

"cont" don't look dangerous, look as "continue stoped machine".

> IMHO, there's a significant difference between twiddling something
> where there is a reasonable expectation that the impact is only going
> to be related to performance (like -smp X, -m X, or cache=X) and just
> trying random things.

[...]

> If there was a reasonable belief that it wouldn't result in disaster,
> I would fully support you.  However, I can't think of any rational
> reason why someone would do this.  I can't think of a better analogy
> to shooting yourself in the foot.

This has been reported to me several times.  Guess why I want to do it
more difficult.

>> As I have received this bug from users a couple of times, I would like
>> to be able to prevent this case.
>>    
>
> I've never seen anyone hit run into this before.  Can you show me a
> bug report?  I'd love to see how someone expected this to behave.

Will search on bugzilla.  My main problem with it is that it took me ages to
discover that the problem was this.

Later, Juan.

Re: [Qemu-devel] Re: [CFR 6/10] cont command

[Anthony Liguori]

On 06/16/2010 05:05 PM, Juan Quintela wrote:
> Anthony Liguori<anthony@codemonkey.ws>  wrote:
>    
>> On 06/16/2010 11:25 AM, Daniel P. Berrange wrote:
>>      
>>>        
>>>> This is related to the commands, not QMP per se:
>>>>
>>>> Once that we are talking about "cont" command.  There are two cases that
>>>> we need to think of:
>>>>
>>>> - incoming migration:
>>>>
>>>> If you start with -incoming foo, and then run "cont" on the monitor
>>>> without having started the migration .... corruption is ensured.
>>>>
>>>>          
>>> This is why '-incoming' command line arg should die, and be replaced
>>> with a 'incoming' monitor command that would simply not allow 'cont'
>>> to be run until it completed.
>>>
>>> For that matter, even with '-incoming' arg on command line we could
>>> refuse to honour 'cont' until the incoming migration had been done.
>>>
>>>        
>> If we had an incoming migration command, I think we'd have to think
>> careful about it's semantics.  Is it reasonable to allow a machine
>> that's otherwise running to do an incoming command?
>>      
> It is the same problem that loadvm.
>
> And no, loadvm several times don't work well either.
>    

It's supposed to.  If it doesn't, then there's a bug somewhere.

Regards,

Anthony Liguori

> Later, Juan.
>
>    

[Qemu-devel] Re: [CFR 6/10] cont command

[Juan Quintela]

Anthony Liguori <anthony@codemonkey.ws> wrote:
> On 06/16/2010 05:05 PM, Juan Quintela wrote:
>> Anthony Liguori<anthony@codemonkey.ws>  wrote:
>>    
>>> On 06/16/2010 11:25 AM, Daniel P. Berrange wrote:
>>>      
>>>>        
>>>>> This is related to the commands, not QMP per se:
>>>>>
>>>>> Once that we are talking about "cont" command.  There are two cases that
>>>>> we need to think of:
>>>>>
>>>>> - incoming migration:
>>>>>
>>>>> If you start with -incoming foo, and then run "cont" on the monitor
>>>>> without having started the migration .... corruption is ensured.
>>>>>
>>>>>          
>>>> This is why '-incoming' command line arg should die, and be replaced
>>>> with a 'incoming' monitor command that would simply not allow 'cont'
>>>> to be run until it completed.
>>>>
>>>> For that matter, even with '-incoming' arg on command line we could
>>>> refuse to honour 'cont' until the incoming migration had been done.
>>>>
>>>>        
>>> If we had an incoming migration command, I think we'd have to think
>>> careful about it's semantics.  Is it reasonable to allow a machine
>>> that's otherwise running to do an incoming command?
>>>      
>> It is the same problem that loadvm.
>>
>> And no, loadvm several times don't work well either.
>>    
>
> It's supposed to.  If it doesn't, then there's a bug somewhere.

I haven't tried lately, but when I tried (months ago, 0.12.0 timeframe),
it didn't work correctly the 2nd/3rd time.  I ended to always loading
from a fresh image, much easier to get it right.

Later, Juan.

[Qemu-devel] Re: [CFR 0/10] QMP specification review

[Luiz Capitulino]

On Tue, 15 Jun 2010 11:30:20 -0500
Anthony Liguori <aliguori@us.ibm.com> wrote:

> This is the first set of commands as part of the QMP specification review.
> Please comment on the individual commands specifications and Stefan and I will
> try to fold the comments back into the command documentation.

 Very nice!

 A few comments regarding the process in general:

  1. How are the issues going to be addressed? I mean, are you or Stefan going
     to send fixes or should we create a TODO page in the wiki so that we can
     work on the feedback later?

  2. I think we should slow down, so that we give more time to reviewers and
     there's no reason to hurry IMHO, as we won't go stable in 0.13

  3. Avi and Daniel, please join the effort