Re: [RFC] migration: Remove old compression code

[Juan Quintela]

Markus Armbruster <address@hidden> wrote:
> Cc'ing like David did, plus Eric and Mike for additional QAPI expertise.

Thanks.

>> diff --git a/qapi/migration.json b/qapi/migration.json
>> index 65db85970e..5477d4d20b 100644
>> --- a/qapi/migration.json
>> +++ b/qapi/migration.json
>> @@ -79,27 +79,6 @@
>>             'cache-miss': 'int', 'cache-miss-rate': 'number',
>>             'overflow': 'int' } }
>>  
>> -##
>> -# @CompressionStats:
>> -#
>> -# Detailed migration compression statistics
>> -#
>> -# @pages: amount of pages compressed and transferred to the target VM
>> -#
>> -# @busy: count of times that no free thread was available to compress data
>> -#
>> -# @busy-rate: rate of thread busy
>> -#
>> -# @compressed-size: amount of bytes after compression
>> -#
>> -# @compression-rate: rate of compressed size
>> -#
>> -# Since: 3.1
>> -##
>> -{ 'struct': 'CompressionStats',
>> -  'data': {'pages': 'int', 'busy': 'int', 'busy-rate': 'number',
>> -       'compressed-size': 'int', 'compression-rate': 'number' } }
>> -
>
> Only user is MigrationInfo, which is next.
>
>>  ##
>>  # @MigrationStatus:
>>  #
>> @@ -200,9 +179,6 @@
>    ##
>    # @MigrationInfo:
> [...]
>>  #           only present when the postcopy-blocktime migration capability
>>  #           is enabled. (Since 3.0)
>>  #
>> -# @compression: migration compression statistics, only returned if 
>> compression
>> -#           feature is on and status is 'active' or 'completed' (Since 3.1)
>> -#
>>  # @socket-address: Only used for tcp, to know what the real port is (Since 
>> 4.0)
>>  #
>>  # Since: 0.14.0
>> @@ -219,7 +195,6 @@
>>             '*error-desc': 'str',
>>             '*postcopy-blocktime' : 'uint32',
>>             '*postcopy-vcpu-blocktime': ['uint32'],
>> -           '*compression': 'CompressionStats',
>>             '*socket-address': ['SocketAddress'] } }
>
> MigrationInfo is returned by  query-migrate.  No other users.
>
> Doc comment gives us wiggle room: "only returned if compression feature
> is on".  Can it be on after this patch?
>
> If no, the member can go without breaking query-migrate compatibility.

We can't setup it anymore.
Notice that even if we don't agree in this patch, I would change this to
compile out compression method, so it is going to be the same.

>
> query-qmp-schema shows the change, though.  Could conceivably affect
> users, but it seems unlikely.
>
> Eric, Mike, we might want to grant ourselves explicit license to change
> query-qmp-schema in such ways, by having qapi-code-gen.txt state
> optional members may be removed when they can't be present anymore.
> What do you think?
>
> Alternatively, keep the member, hardcode value to whatever is returned
> before the patch when compression is off, deprecate, remove after grace
> period.  A bit more work, but safer.  Worthwhile?  Not for me to decide.
>
>>  
>>  ##
>> @@ -364,14 +339,6 @@
>    ##
>    # @MigrationCapability:
> [...]
>>  #          to enable the capability on the source VM. The feature is 
>> disabled by
>>  #          default. (since 1.6)
>>  #
>> -# @compress: Use multiple compression threads to accelerate live migration.
>> -#          This feature can help to reduce the migration traffic, by sending
>> -#          compressed pages. Please note that if compress and xbzrle are 
>> both
>> -#          on, compress only takes effect in the ram bulk stage, after that,
>> -#          it will be disabled and only xbzrle takes effect, this can help 
>> to
>> -#          minimize migration traffic. The feature is disabled by default.
>> -#          (since 2.4 )
>> -#
>>  # @events: generate events for each migration state change
>>  #          (since 2.4 )
>>  #
>> @@ -425,7 +392,7 @@
>>  ##
>>  { 'enum': 'MigrationCapability',
>>    'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks',
>> -           'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram',
>> +           'events', 'postcopy-ram', 'x-colo', 'release-ram',
>>             'block', 'return-path', 'pause-before-switchover', 'multifd',
>>             'dirty-bitmaps', 'postcopy-blocktime', 'late-block-activate',
>>             'x-ignore-shared', 'validate-uuid' ] }
>
> Only users are migrate-set-capabilities and query-migrate-capabilities,
> via MigrationCapabilityStatus, which are next.
>
>> @@ -479,7 +446,6 @@
>    ##
>    # @MigrationCapabilityStatus:
>    #
>    # Migration capability information
>    #
>    # @capability: capability enum
>    #
>    # @state: capability state bool
>    #
>    # Since: 1.2
>    ##
>    { 'struct': 'MigrationCapabilityStatus',
>      'data': { 'capability' : 'MigrationCapability', 'state' : 'bool' } }
>
>    ##
>    # @migrate-set-capabilities:
>    #
>    # Enable/Disable the following migration capabilities (like xbzrle)
>    #
>    # @capabilities: json array of capability modifications to make
>    #
>    # Since: 1.2
>    #
>    # Example:
>    #
>    # -> { "execute": "migrate-set-capabilities" , "arguments":
>    #      { "capabilities": [ { "capability": "xbzrle", "state": true } ] } }
>    #
>    ##
>    { 'command': 'migrate-set-capabilities',
>      'data': { 'capabilities': ['MigrationCapabilityStatus'] } }
>
> With MigrationCapability @compress gone, attempting to set it with
> migrate-set-capabilities fails with GenericError, "desc": "Invalid
> parameter 'compress'".
>
> Setting capability "compress" can fail before this patch, but only when
> you do it in the middle of a migration, so this is actually a new error.
>
> Adding errors is explicitly permitted in docs/interop/qmp-spec.txt
> section "Compatibility Considerations":
>
>     However, Clients must not assume any particular:
>     [...]
>     - Amount of errors generated by a command, that is, new errors can be 
> added
>       to any existing command in newer versions of the Server
>
> We're good.
>
>    ##
>    # @query-migrate-capabilities:
>    #
>    # Returns information about the current migration capabilities status
>    #
>    # Returns: @MigrationCapabilitiesStatus
>    #
>    # Since: 1.2
>    #
>    # Example:
>    #
>    # -> { "execute": "query-migrate-capabilities" }
>    # <- { "return": [
>    #       {"state": false, "capability": "xbzrle"},
>>  #       {"state": false, "capability": "rdma-pin-all"},
>>  #       {"state": false, "capability": "auto-converge"},
>>  #       {"state": false, "capability": "zero-blocks"},
>> -#       {"state": false, "capability": "compress"},
>>  #       {"state": true, "capability": "events"},
>>  #       {"state": false, "capability": "postcopy-ram"},
>>  #       {"state": false, "capability": "x-colo"}
>    #    ]}
>    #
>    ##
>    { 'command': 'query-migrate-capabilities', 'returns':   
> ['MigrationCapabilityStatus']}
>
> What capabilties are returned when is not specified.
>
> Aside: it should be, shouldn't it?
>
> qmp_query_migrate_capabilities() returns them all, except it skips
> "block" when !defined CONFIG_LIVE_BLOCK_MIGRATION.  De facto ABI due to
> the gap in the documented ABI.
>
> Removing capability "compress" is a break of this de facto ABI.
> Acceptable when we're confident the ABI's users don't care.
>
> Alternatively, keep them, hardcode value to false, deprecate, remove
> after grace period.
>
> Aside: you might want to make MigrationCapability @block conditional like

Yeap, that is what I was searching for.  How to move from here O;-)

[...]
>
> MigrateSetParameters is returned by query-migrate-parameters.  No other
> users.
>
> Even thought the members are optional, the doc comment specifies all are
> returned.  qmp_query_migrate_parameters() does.
>
> Removing the parameters breaks this promise.  Acceptable when we're
> confident the ABI's users don't care.
>
> Alternatively, keep them, hardcode value to whatever is returned before
> the patch when compression is off, deprecate, remove after grace period.
>
> Questions?

Ok with me.

Let's other people get in.

Thanks a lot, Juan.