Markus Armbruster <arm...@redhat.com> 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.