Re: [Qemu-devel] Exposing feature deprecation to machine clients

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 9084 bytes --]

On 07.11.19 20:13, Vladimir Sementsov-Ogievskiy wrote:
> 07.11.2019 21:52, Philippe Mathieu-Daudé wrote:
>> Hi Markus,
>>
>> On 8/15/19 7:40 PM, John Snow wrote:
>>> On 8/15/19 10:16 AM, Markus Armbruster wrote:
>>>> John Snow <jsnow@redhat.com> writes:
>> [...]
>>>>> I asked Markus this not too long ago; do we want to amend the QAPI
>>>>> schema specification to allow commands to return with "Warning" strings,
>>>>> or "Deprecated" stings to allow in-band deprecation notices for cases
>>>>> like these?
>>>>>
>>>>> example:
>>>>>
>>>>> { "return": {},
>>>>>    "deprecated": True,
>>>>>    "warning": "Omitting filter-node-name parameter is deprecated, it will
>>>>> be required in the future"
>>>>> }
>>>>>
>>>>> There's no "error" key, so this should be recognized as success by
>>>>> compatible clients, but they'll definitely see the extra information.
>>>>
>>>> This is a compatible evolution of the QMP protocol.
>>>>
>>>>> Part of my motivation is to facilitate a more aggressive deprecation of
>>>>> legacy features by ensuring that we are able to rigorously notify users
>>>>> through any means that they need to adjust their scripts.
>>>>
>>>> Yes, we should help libvirt etc. with detecting use of deprecated
>>>> features.  We discussed this at the KVM Forum 2018 BoF on deprecating
>>>> stuff.  Minutes:
>>>>
>>>>      Message-ID: <87mur0ls8o.fsf@dusky.pond.sub.org>
>>>>      https://lists.nongnu.org/archive/html/qemu-devel/2018-10/msg05828.html
>>>>
>>>> Last item is relevant here.
>>>>
>>>> Adding deprecation information to QMP's success response belongs to "We
>>>> can also pass the buck to the next layer up", next to "emit a QMP
>>>> event".
>>>>
>>>> Let's compare the two, i.e. "deprecation info in success response"
>>>> vs. "deprecation event".
>>>>
>>>> 1. Possible triggers
>>>>
>>>> Anything we put in the success response should only ever apply to the
>>>> (successful) command.  So this one's limited to QMP commands.
>>>>
>>>> A QMP event is not limited to QMP commands.  For instance, it could be
>>>> emitted for deprecated CLI features (long after the fact, in addition to
>>>> human-readable warnings on stderr), or when we detect use of a
>>>> deprecated feature only after we sent the success response, say in a
>>>> job.  Neither use case is particularly convincing.  Reporting use of
>>>> deprecated CLI in QMP feels like a work-around for the CLI's
>>>> machine-unfriendliness.  Job-like commands should really check their
>>>> arguments upfront.
>>>>
>>>> 2. Connection to trigger
>>>>
>>>> Connecting responses to commands is the QMP protocol's responsibility.
>>>> Transmitting deprecation information in the response trivially ties it
>>>> to the offending command.
>>>>
>>>> The QMP protocol doesn't tie events to anything.  Thus, a deprecation
>>>> event needs an event-specific tie to its trigger.
>>>>
>>>> The obvious way to tie it to a command mirrors how the QMP protocol ties
>>>> responses to commands: by command ID.  The event either has to be sent
>>>> just to the offending monitor (currently, all events are broadcast to
>>>> all monitors), or include a suitable monitor ID.
>>>>
>>>> For non-command triggers, we'd have to invent some other tie.
>>>>
>>>> 3. Interface complexity
>>>>
>>>> Tying the event to some arbitrary trigger adds complexity.
>>>>
>>>> Do we need non-command triggers, and badly enough to justify the
>>>> additional complexity?
>>>>
>>>> 4. Implementation complexity
>>>>
>>>> Emitting an event could be as simple as
>>>>
>>>>      qapi_event_send_deprecated(qmp_command_id(),
>>>>                                 "Omitting 'filter-node-name'");
>>>>
>>>> where qmp_command_id() returns the ID of the currently executing
>>>> command.  Making qmp_command_id() work is up to the QMP core.  Simple
>>>> enough as long as each QMP command runs to completion before its monitor
>>>> starts the next one.
>>>>
>>>> The event is "fire and forget".  There is no warning object propagated
>>>> up the call chain into the QMP core like errors objects are.
>>>>
>>>> "Fire and forget" is ideal for letting arbitrary code decide "this is
>>>> deprecated".
>>>>
>>>> Note the QAPI schema remains untouched.
>>>>
>>>> Unlike an event, which can be emitted anywhere, the success response
>>>> gets built in the QMP core.  To have the core add deprecation info to
>>>> it, we need to get the info to the core.
>>>>
>>>> If deprecation info originates in command code, like errors do, we need
>>>> to propagate it up the call chain into the QMP core like errors.
>>>>
>>>> Propagating errors is painful.  It has caused massive churn all over the
>>>> place.
>>>>
>>>> I don't think we can hitch deprecation info to the existing error
>>>> propagation, since we need to take the success path back to the QMP
>>>> core, not an error path.
>>>>
>>>> Propagating a second object for warnings... thanks, but no thanks.
>>>>
>>>
>>> Probably the best argument against it. Fire-and-forget avoids the
>>> problem. Events might work just fine, but the "tie" bit seems like a yak
>>> in need of a shave.
>>>
>>>> The QMP core could provide a function for recording deprecation info for
>>>> the currently executing QMP command.  This is how we used to record
>>>> errors in QMP commands, until Anthony rammed through what we have now.
>>>> The commit messages (e.g. d5ec4f27c38) provide no justification.  I
>>>> dimly recall adamant (oral?) claims that recording errors in the Monitor
>>>> object cannot work for us.
>>>>
>>>> I smell a swamp.
>>>>
>>>> Can we avoid plumbing deprecation info from command code to QMP core?
>>>> Only if the QMP core itself can recognize deprecated interfaces.  I
>>>> believe it can for the cases we can expose in introspecion.  Let me
>>>> explain.
>>>>
>>>> Kevin recently added "features" to the QAPI schema language.  The
>>>> implementation is incomplete, but that's detail.  The idea is to tack a
>>>> "deprecated" feature to deprecated stuff in the QAPI schema.
>>>>
>>>
>>> That's a good idea too; but the semantics of exactly *what* was
>>> deprecated may be hard to capture.
>>>
>>>> Commands and arguments need to support features for that.
>>>> Implementation should be relatively straightforward.
>>>>
>>>> Deprecating an argument's optionalness may require a
>>>> "optional-deprecated" feature.  I've seen more elegant designs, but I've
>>>> also seen plenty of uglier ones.
>>>>
>>>> Note that features are tied to schema syntax.  To express semantically
>>>> conditional deprecation like "if you specify argument FOO, then not
>>>> specifying argument BAR is deprecated", we'd have to add a language for
>>>> these conditions.  Uh, not now, maybe never.
>>>>
>>>> The primary use of having deprecation defined in the QAPI schema is
>>>> introspection.  The BoF minutes mention this, too.
>>>>
>>>> A secondary use could be detecting use of deprecated features right in
>>>> the QMP core.  No need for ad hoc code in commands, no need for plumbing
>>>> information from there to the QMP core.
>>>>
>>>> I'd like to pursue this idea, then see how well it suits our deprecation
>>>> needs.
>>>>
>>>
>>> I should clearly remember to talk to you before thinking about QMP in
>>> public, because you've thought about it much more.
>>
>> Pre-release period, time to deprecate some stuffs :)
>>
>> How should we proceed? Do you have something in mind?
>>
>> There are older threads about this. Should we start a new thread? Gather the different ideas on the Wiki?
>>
>> (Obviously you are not the one responsible of this topic, you just happen to be the last one worried about it on the list).
>>
>> Regards,
>>
>> Phil.
> 
> Hi!
> 
> I wanted to resend, but faced some problems, and understand that I can't do it in time before soft-freeze..
> But you say, that we can deprecate something even after hard-freeze?
> 
> Ok, the problem that I faced, is that deprecation warnings breaks some iotests.. What can we do?
> 
> 1. Update iotests...
>    1.1 Just update iotests outputs to show warnings. Then, in next release cycle, update iotests, to not use deprecated things
>    or
>    1.2 Update iotests to not use deprecated things.. Not appropriate for hard freeze.

I personally don’t have a problem with any test patches during freeze,
but maybe I should be more careful with auto-grouped patches.

> or
> 2. Commit deprecations without warnings.. But how do people find out about this?
> 
> Next, what exactly to deprecate? As I understand, we can't deprecate drive-mirror now?
> So I propose to:
> 
> 1. deprecate drive-backup

I suspect I missed something at KVM Forum, but what’s the hurry here?

Max

> 2. add optional filter-node-name parameter to drive-mirror, to correspond to commit and mirror
> 3. deprecate that filter-node-name is optional for commit and mirror.
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]