Re: [RFC PATCH 07/13] migration: Introduce new MigrationConfig structure

[Markus Armbruster <armbru@redhat.com>]

Fabiano Rosas <farosas@suse.de> writes:

> Markus Armbruster <armbru@redhat.com> writes:
>
> Markus, sorry for the delay here. I had vacations and holidays, plus a
> pile of patches to review.

No problem.  Hope you enjoyed your time off!

>> Fabiano Rosas <farosas@suse.de> writes:
>>
>>> Add a new migration structure to consolidate the capabilities and
>>> parameters. This structure will be used in place of the s->parameters
>>> and s->capabilities data structures in the next few patches.
>>>
>>> The QAPI migration types now look like this:
>>>
>>> /* options previously known as parameters */
>>
>> Configuration previously known as parameters less the TLS stuff.
>>
>>> { 'struct': 'MigrationConfigBase',
>>>   'data': {
>>>       <parameters>
>>> } }
>>>
>>>
>>> /* for compat with query-migrate-parameters */
>>> { 'struct': 'MigrationParameters',
>>>   'base': 'MigrationConfigBase',
>>>   'data': {
>>>       <TLS options in 'str' format>
>>> } }
>>>
>>> /* for compat with migrate-set-parameters */
>>> { 'struct': 'MigrateSetParameters',
>>>   'base': 'MigrationConfigBase',
>>>   'data': {
>>>       <TLS options in 'StrOrNull' format>
>>> } }
>>
>> Yes, this is the state since PATCH 05.
>>
>>> /* to replace MigrationParameters in the MigrationState */
>>> { 'struct': 'MigrationConfig',
>>>   'base': 'MigrationConfigBase'
>>>   'data': {
>>>     <TLS options in 'str' format>
>>> } }
>>
>> This is new in this patch.
>>
>> Your description doesn't cover optionalness.  Here's my understanding:
>>
>> * MigrationSetParameters has optional members, because
>>   migrate-set-parameters needs that.
>>
>
> Yes.
>
>> * MigrationParameters would ideally have these members non-optional,
>>   because query-migrate-parameters wants that.
>>
>
> Yes.
>
>> * But to enable sharing via common base type MigrationConfigBase, we
>>   accept unwanted optional in MigrationParameters and thus
>>   query-migrate-parameters.
>>
>
> Yes.
>
>> * This doesn't apply to the non-shared members of MigrationParameters,
>>   so you made them non-optional.  These are @tls-creds, @tls-hostname,
>>   @tls-authz.
>>
>
> Yes.
>
>> * But in MigrationConfig they're optional again, because "empty string
>>   means absent" is silly; we want "NULL means absent".
>>
>
> Yes. But mostly because MigrationConfig will become the type for the new
> '*config' argument to migrate/migrate_incoming (patches 12 & 13) and we
> want to keep all members optional. Otherwise the user would have to pass
> all ~50 migration options in every migrate command, which is bad IMO.

Got it.

>> Correct?
>>
>> Up to here, this enables cleanup of some "empty string means absent"
>> silliness in later patches.
>>
>> The remainder is about unifying capabilities into parameters.  I'd split
>> the patch (but I'm a compulsive patch splitter).
>>
>>> The above keeps the query/set-parameters commands stable. For the
>>> capabilities as well as the options added in the future, we have a
>>> choice of where to put them:
>>>
>>> 1) In MigrationConfigBase, this means that the existing
>>>    query/set-parameters commands will be updated to deal with
>>>    capabilities/new options.
>>>
>>>   { 'struct': 'MigrationConfigBase',
>>>     'data': {
>>>       <parameters>
>>>       <capabilities>
>>>       <new opts>
>>>   } }
>>>
>>>   { 'struct': 'MigrationConfig',
>>>     'base': 'MigrationConfigBase'
>>>     'data': {
>>>       <TLS options in 'str' format>
>>>   } }
>>>
>>> 2) In MigrationConfig, this means that the existing commands will be
>>>    frozen in time.
>>>
>>>   { 'struct': 'MigrationConfigBase',
>>>     'data': {
>>>       <parameters>
>>>   } }
>>>
>>>   { 'struct': 'MigrationConfig',
>>>     'base': 'MigrationConfigBase'
>>>     'data': {
>>>       <TLS options in 'str' format>
>>>       <capabilities>
>>>       <new opts>
>>>   } }
>>>
>>> For now, I've chosen the option 1, all capabilities and new options go
>>> into MigrationConfigBase. This gives the option to keep the existing
>>> commands for as long as we'd like.
>>
>> Perhaps this would be slightly easier to digest for the reader if you
>> talked just about capabilities at first.  Once that's understood,
>> mention we have the same choice for new configuration bits.
>>
>
> Ok, I'll reorganize, along with the other comments you've made.
>
>>> Note that the query/set capabilities commands will have to go, we can
>>> treat parameters as generic configuration options, but capabilities
>>> are just too different.
>>
>> I think the argument is that migration capabilities are a pointless
>> interface complication.  One mechanism (parameters) is better than two
>> (parameters and capabilities).
>>
>
> Yes, that's the main point indeed.

Perhaps you can make this point more prominently.

>>> Signed-off-by: Fabiano Rosas <farosas@suse.de>

[...]