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

[Juan Quintela <quintela@redhat.com>]

Markus Armbruster <armbru@redhat.com> wrote:
> Eric Blake <eblake@redhat.com> writes:
>
>> On 05/15/2017 04:46 AM, Dr. David Alan Gilbert wrote:
>>> * Juan Quintela (quintela@redhat.com) wrote:
>>>> Eric Blake <eblake@redhat.com> 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 <quintela@redhat.com>
>>>>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>>>>>
>>>>>> --
>>>>>>
>>>>>> 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.