Re: [Qemu-devel] [PATCH 1/3] migration: Create block capabilities for shared and enable

[Markus Armbruster]

Juan Quintela  writes:

> Markus Armbruster  wrote:
>> Eric Blake  writes:
>>
>>> On 05/15/2017 04:46 AM, Dr. David Alan Gilbert wrote:
 * Juan Quintela (quint...@redhat.com) wrote:
> Eric Blake  wrote:
>> On 05/11/2017 11:32 AM, Juan Quintela wrote:
>>> Those two capabilities were added through the command line.  Notice that
>>> we just created them.  This is just the boilerplate.
>>>
>>> Signed-off-by: Juan Quintela 
>>> Reviewed-by: Eric Blake 
>>>
>>> --
>>>
>>> Make migrate_set_block_* take a boolean argument.
>>
>> Question - do we support the orthogonal selection of all 4 combinations
>> under HMP 'migrate' (no argument, -b alone, -i alone, -b and -i
>> together), or are there only 3 actual states? If the latter, should we
>> represent this as a single enum-valued property, rather than as two
>> independent boolean properties?
>
> { 'enum': 'MigrationCapability',
>   'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks',
>'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram'] 
> }
>
>
> My understanding is that we can only have boolean capabilities here.
> Or, how could we put a non-boolean capability?
>>>
>>> If we want a non-boolean, then we make it a migration parameter rather
>>> than a migration capability.  There may be other advantages to using
>>> MigrationParameter instead of MigrationCapability (such as making it
>>> easier to figure out whether the parameter settings are persistent or
>>> apply per-migration).
>>
>> What makes a migration knob a MigrationCapability rather than a
>> MigrationParameter?  Type bool, or is there more to it?
>
> I didn't started this, but *my* undersanding:
>
> Migration capability: we have to set this up before migration starts.
> It is like a property that we can't change later.  We can't add a
> compression thread in the middle of migration (notice that we "could"
> but current code can't).
>
> So, block enabled migration is a capability.
>
> Block shared on the other hand is weird.  *My* understading of the code
> is that we have a qcow2 overlay on top of the base image, and:
>
> - without shared: we migrate all the block device
> - with shared: we migrate only the top overlay

Plausible.  The thing that's "shared" is the backing file, between
migration source and destination.

However, we can't change "shared" in the middle of migration, either.

> So, thisk could be a parameter of block migration.  If anyone understand
> better than me, please stand up.
>
 Lets keep this simple and stick with the booleans.
 
 Dave
 
> There are three states as far as I can see.
>>
>> Begs the question how the fourth state behaves.  Documentation is of no
>> help:
>>
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index 5728b7f..109852e 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -894,11 +894,16 @@
>>  # @release-ram: if enabled, qemu will free the migrated ram pages on 
>> the source
>>  #during postcopy-ram migration. (since 2.9)
>>  #
>> +# @block-enabled: enable block migration (Since 2.10)
>> +#
>> +# @block-shared: enable block shared migration (Since 2.10)
>> +#
>>  # Since: 1.2
>>  ##
>>
>> Please explain all four states clearly there.
>
> Is the previous explanation enough?

The new comments can serve as reminders for readers who already know
what "block migration" and "block shared migration" is.  Surely we can
do better for readers who don't, and I'd like you to try.

Note that the comments aren't just for QAPI schema readers, but also for
QMP documentation readers (generated docs/qemu-qmp-ref.*).

>>> I'll leave it up to you as maintainers which way you prefer, I'm just
>>> offering the potential design tradeoffs for simplicity of booleans (but
>>> complexity in an unused state) vs. simplicity of design (but complexity
>>> in code).
>>
>> For what it's worth, I dislike entangled booleans.
>
> Some here, but except if we put shared as a migration parameter, I don't
> know what to do here.
>
> { 'enum': 'MigrationParameter',
>   'data': ['compress-level', 'compress-threads', 'decompress-threads',
>'cpu-throttle-initial', 'cpu-throttle-increment',
>'tls-creds', 'tls-hostname', 'max-bandwidth',
>'downtime-limit', 'x-checkpoint-delay' ] }
>
>
> I don't see either how to define that block_shared will be a boolean 
> parameter.

Here:

  { 'struct': 'MigrationParameters',
'data': { '*compress-level': 'int',
  '*compress-threads': 'int',
  '*decompress-threads': 'int',
  '*cpu-throttle-initial': 'int',
  '*cpu-throttle-increment': 'int',
  '*tls-creds': 'str',
  '*tls-hostname': 'str',
 

Re: [Qemu-devel] [PATCH 1/3] migration: Create block capabilities for shared and enable

[Juan Quintela]

Markus Armbruster  wrote:
> Eric Blake  writes:
>
>> On 05/15/2017 04:46 AM, Dr. David Alan Gilbert wrote:
>>> * Juan Quintela (quint...@redhat.com) wrote:
 Eric Blake  wrote:
> On 05/11/2017 11:32 AM, Juan Quintela wrote:
>> Those two capabilities were added through the command line.  Notice that
>> we just created them.  This is just the boilerplate.
>>
>> Signed-off-by: Juan Quintela 
>> Reviewed-by: Eric Blake 
>>
>> --
>>
>> Make migrate_set_block_* take a boolean argument.
>
> Question - do we support the orthogonal selection of all 4 combinations
> under HMP 'migrate' (no argument, -b alone, -i alone, -b and -i
> together), or are there only 3 actual states? If the latter, should we
> represent this as a single enum-valued property, rather than as two
> independent boolean properties?

 { 'enum': 'MigrationCapability',
   'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks',
'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram'] }


 My understanding is that we can only have boolean capabilities here.
 Or, how could we put a non-boolean capability?
>>
>> If we want a non-boolean, then we make it a migration parameter rather
>> than a migration capability.  There may be other advantages to using
>> MigrationParameter instead of MigrationCapability (such as making it
>> easier to figure out whether the parameter settings are persistent or
>> apply per-migration).
>
> What makes a migration knob a MigrationCapability rather than a
> MigrationParameter?  Type bool, or is there more to it?

I didn't started this, but *my* undersanding:

Migration capability: we have to set this up before migration starts.
It is like a property that we can't change later.  We can't add a
compression thread in the middle of migration (notice that we "could"
but current code can't).

So, block enabled migration is a capability.

Block shared on the other hand is weird.  *My* understading of the code
is that we have a qcow2 overlay on top of the base image, and:

- without shared: we migrate all the block device
- with shared: we migrate only the top overlay

So, thisk could be a parameter of block migration.  If anyone understand
better than me, please stand up.

>>> Lets keep this simple and stick with the booleans.
>>> 
>>> Dave
>>> 
 There are three states as far as I can see.
>
> Begs the question how the fourth state behaves.  Documentation is of no
> help:
>
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 5728b7f..109852e 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -894,11 +894,16 @@
>  # @release-ram: if enabled, qemu will free the migrated ram pages on the 
> source
>  #during postcopy-ram migration. (since 2.9)
>  #
> +# @block-enabled: enable block migration (Since 2.10)
> +#
> +# @block-shared: enable block shared migration (Since 2.10)
> +#
>  # Since: 1.2
>  ##
>
> Please explain all four states clearly there.

Is the previous explanation enough?

>> I'll leave it up to you as maintainers which way you prefer, I'm just
>> offering the potential design tradeoffs for simplicity of booleans (but
>> complexity in an unused state) vs. simplicity of design (but complexity
>> in code).
>
> For what it's worth, I dislike entangled booleans.

Some here, but except if we put shared as a migration parameter, I don't
know what to do here.

{ 'enum': 'MigrationParameter',
  'data': ['compress-level', 'compress-threads', 'decompress-threads',
   'cpu-throttle-initial', 'cpu-throttle-increment',
   'tls-creds', 'tls-hostname', 'max-bandwidth',
   'downtime-limit', 'x-checkpoint-delay' ] }


I don't see either how to define that block_shared will be a boolean parameter.

Later, Juan.

Re: [Qemu-devel] [PATCH 1/3] migration: Create block capabilities for shared and enable

[Juan Quintela]

Eric Blake  wrote:
> On 05/15/2017 04:46 AM, Dr. David Alan Gilbert wrote:
>> * Juan Quintela (quint...@redhat.com) wrote:
>>> Eric Blake  wrote:
 On 05/11/2017 11:32 AM, Juan Quintela wrote:
> Those two capabilities were added through the command line.  Notice that
> we just created them.  This is just the boilerplate.
>
> Signed-off-by: Juan Quintela 
> Reviewed-by: Eric Blake 
>
> --
>
> Make migrate_set_block_* take a boolean argument.

 Question - do we support the orthogonal selection of all 4 combinations
 under HMP 'migrate' (no argument, -b alone, -i alone, -b and -i
 together), or are there only 3 actual states? If the latter, should we
 represent this as a single enum-valued property, rather than as two
 independent boolean properties?
>>>
>>> { 'enum': 'MigrationCapability',
>>>   'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks',
>>>'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram'] }
>>>
>>>
>>> My understanding is that we can only have boolean capabilities here.
>>> Or, how could we put a non-boolean capability?
>
> If we want a non-boolean, then we make it a migration parameter rather
> than a migration capability.  There may be other advantages to using
> MigrationParameter instead of MigrationCapability (such as making it
> easier to figure out whether the parameter settings are persistent or
> apply per-migration).

Block migration is a capability as far as I can see/understand.  The
block shared bit could be a parameter, though.  Not sure if that would
be better/worse.

>
>> 
>> Lets keep this simple and stick with the booleans.
>> 
>> Dave
>> 
>>> There are three states as far as I can see.
>
> I'll leave it up to you as maintainers which way you prefer, I'm just
> offering the potential design tradeoffs for simplicity of booleans (but
> complexity in an unused state) vs. simplicity of design (but complexity
> in code).

As I expect to deprecate the old interface, I think that the best thing
to do is to use two capabilities or a capability(block enabled) and a
parameter (block shared).

What do you think?

Later, Juan.

Re: [Qemu-devel] [PATCH 1/3] migration: Create block capabilities for shared and enable

[Markus Armbruster]

Eric Blake  writes:

> On 05/15/2017 04:46 AM, Dr. David Alan Gilbert wrote:
>> * Juan Quintela (quint...@redhat.com) wrote:
>>> Eric Blake  wrote:
 On 05/11/2017 11:32 AM, Juan Quintela wrote:
> Those two capabilities were added through the command line.  Notice that
> we just created them.  This is just the boilerplate.
>
> Signed-off-by: Juan Quintela 
> Reviewed-by: Eric Blake 
>
> --
>
> Make migrate_set_block_* take a boolean argument.

 Question - do we support the orthogonal selection of all 4 combinations
 under HMP 'migrate' (no argument, -b alone, -i alone, -b and -i
 together), or are there only 3 actual states? If the latter, should we
 represent this as a single enum-valued property, rather than as two
 independent boolean properties?
>>>
>>> { 'enum': 'MigrationCapability',
>>>   'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks',
>>>'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram'] }
>>>
>>>
>>> My understanding is that we can only have boolean capabilities here.
>>> Or, how could we put a non-boolean capability?
>
> If we want a non-boolean, then we make it a migration parameter rather
> than a migration capability.  There may be other advantages to using
> MigrationParameter instead of MigrationCapability (such as making it
> easier to figure out whether the parameter settings are persistent or
> apply per-migration).

What makes a migration knob a MigrationCapability rather than a
MigrationParameter?  Type bool, or is there more to it?

>> 
>> Lets keep this simple and stick with the booleans.
>> 
>> Dave
>> 
>>> There are three states as far as I can see.

Begs the question how the fourth state behaves.  Documentation is of no
help:

diff --git a/qapi-schema.json b/qapi-schema.json
index 5728b7f..109852e 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -894,11 +894,16 @@
 # @release-ram: if enabled, qemu will free the migrated ram pages on the 
source
 #during postcopy-ram migration. (since 2.9)
 #
+# @block-enabled: enable block migration (Since 2.10)
+#
+# @block-shared: enable block shared migration (Since 2.10)
+#
 # Since: 1.2
 ##

Please explain all four states clearly there.

> I'll leave it up to you as maintainers which way you prefer, I'm just
> offering the potential design tradeoffs for simplicity of booleans (but
> complexity in an unused state) vs. simplicity of design (but complexity
> in code).

For what it's worth, I dislike entangled booleans.

Re: [Qemu-devel] [PATCH 1/3] migration: Create block capabilities for shared and enable

[Eric Blake]

On 05/15/2017 04:46 AM, Dr. David Alan Gilbert wrote:
> * Juan Quintela (quint...@redhat.com) wrote:
>> Eric Blake  wrote:
>>> On 05/11/2017 11:32 AM, Juan Quintela wrote:
 Those two capabilities were added through the command line.  Notice that
 we just created them.  This is just the boilerplate.

 Signed-off-by: Juan Quintela 
 Reviewed-by: Eric Blake 

 --

 Make migrate_set_block_* take a boolean argument.
>>>
>>> Question - do we support the orthogonal selection of all 4 combinations
>>> under HMP 'migrate' (no argument, -b alone, -i alone, -b and -i
>>> together), or are there only 3 actual states? If the latter, should we
>>> represent this as a single enum-valued property, rather than as two
>>> independent boolean properties?
>>
>> { 'enum': 'MigrationCapability',
>>   'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks',
>>'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram'] }
>>
>>
>> My understanding is that we can only have boolean capabilities here.
>> Or, how could we put a non-boolean capability?

If we want a non-boolean, then we make it a migration parameter rather
than a migration capability.  There may be other advantages to using
MigrationParameter instead of MigrationCapability (such as making it
easier to figure out whether the parameter settings are persistent or
apply per-migration).

> 
> Lets keep this simple and stick with the booleans.
> 
> Dave
> 
>> There are three states as far as I can see.

I'll leave it up to you as maintainers which way you prefer, I'm just
offering the potential design tradeoffs for simplicity of booleans (but
complexity in an unused state) vs. simplicity of design (but complexity
in code).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 1/3] migration: Create block capabilities for shared and enable

[Dr. David Alan Gilbert]

* Juan Quintela (quint...@redhat.com) wrote:
> Eric Blake  wrote:
> > On 05/11/2017 11:32 AM, Juan Quintela wrote:
> >> Those two capabilities were added through the command line.  Notice that
> >> we just created them.  This is just the boilerplate.
> >> 
> >> Signed-off-by: Juan Quintela 
> >> Reviewed-by: Eric Blake 
> >> 
> >> --
> >> 
> >> Make migrate_set_block_* take a boolean argument.
> >
> > Question - do we support the orthogonal selection of all 4 combinations
> > under HMP 'migrate' (no argument, -b alone, -i alone, -b and -i
> > together), or are there only 3 actual states? If the latter, should we
> > represent this as a single enum-valued property, rather than as two
> > independent boolean properties?
> 
> { 'enum': 'MigrationCapability',
>   'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks',
>'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram'] }
> 
> 
> My understanding is that we can only have boolean capabilities here.
> Or, how could we put a non-boolean capability?

Lets keep this simple and stick with the booleans.

Dave

> There are three states as far as I can see.
> 
> Later, Juan.
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 1/3] migration: Create block capabilities for shared and enable

[Juan Quintela]

Eric Blake  wrote:
> On 05/11/2017 11:32 AM, Juan Quintela wrote:
>> Those two capabilities were added through the command line.  Notice that
>> we just created them.  This is just the boilerplate.
>> 
>> Signed-off-by: Juan Quintela 
>> Reviewed-by: Eric Blake 
>> 
>> --
>> 
>> Make migrate_set_block_* take a boolean argument.
>
> Question - do we support the orthogonal selection of all 4 combinations
> under HMP 'migrate' (no argument, -b alone, -i alone, -b and -i
> together), or are there only 3 actual states? If the latter, should we
> represent this as a single enum-valued property, rather than as two
> independent boolean properties?

{ 'enum': 'MigrationCapability',
  'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks',
   'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram'] }


My understanding is that we can only have boolean capabilities here.
Or, how could we put a non-boolean capability?

There are three states as far as I can see.

Later, Juan.

Re: [Qemu-devel] [PATCH 1/3] migration: Create block capabilities for shared and enable

[Eric Blake]

On 05/11/2017 11:32 AM, Juan Quintela wrote:
> Those two capabilities were added through the command line.  Notice that
> we just created them.  This is just the boilerplate.
> 
> Signed-off-by: Juan Quintela 
> Reviewed-by: Eric Blake 
> 
> --
> 
> Make migrate_set_block_* take a boolean argument.

Question - do we support the orthogonal selection of all 4 combinations
under HMP 'migrate' (no argument, -b alone, -i alone, -b and -i
together), or are there only 3 actual states? If the latter, should we
represent this as a single enum-valued property, rather than as two
independent boolean properties?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature