Re: [libvirt] Exposing feature deprecation to machine clients (was: [Qemu-devel] [PATCH 2/2] qapi: deprecate implicit filters)
On 8/15/19 10:16 AM, Markus Armbruster wrote:
> John Snow writes:
>
>> On 8/14/19 6:07 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> To get rid of implicit filters related workarounds in future let's
>>> deprecate them now.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy
>>> ---
> [...]
>>> diff --git a/blockdev.c b/blockdev.c
>>> index 36e9368e01..b3cfaccce1 100644
>>> --- a/blockdev.c
>>> +++ b/blockdev.c
>>> @@ -3292,6 +3292,11 @@ void qmp_block_commit(bool has_job_id, const char
>>> *job_id, const char *device,
>>> BlockdevOnError on_error = BLOCKDEV_ON_ERROR_REPORT;
>>> int job_flags = JOB_DEFAULT;
>>>
>>> +if (!has_filter_node_name) {
>>> +warn_report("Omitting filter-node-name parameter is deprecated, it
>>> "
>>> +"will be required in future");
>>> +}
>>> +
>>> if (!has_speed) {
>>> speed = 0;
>>> }
>>> @@ -3990,6 +3995,11 @@ void qmp_blockdev_mirror(bool has_job_id, const char
>>> *job_id,
>>> Error *local_err = NULL;
>>> int ret;
>>>
>>> +if (!has_filter_node_name) {
>>> +warn_report("Omitting filter-node-name parameter is deprecated, it
>>> "
>>> +"will be required in future");
>>> +}
>>> +
>>> bs = qmp_get_root_bs(device, errp);
>>> if (!bs) {
>>> return;
>>>
>>
>> This might be OK to do right away, though.
>>
>> 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@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
>
[libvirt] Exposing feature deprecation to machine clients (was: [Qemu-devel] [PATCH 2/2] qapi: deprecate implicit filters)
John Snow writes:
> On 8/14/19 6:07 AM, Vladimir Sementsov-Ogievskiy wrote:
>> To get rid of implicit filters related workarounds in future let's
>> deprecate them now.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy
>> ---
[...]
>> diff --git a/blockdev.c b/blockdev.c
>> index 36e9368e01..b3cfaccce1 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -3292,6 +3292,11 @@ void qmp_block_commit(bool has_job_id, const char
>> *job_id, const char *device,
>> BlockdevOnError on_error = BLOCKDEV_ON_ERROR_REPORT;
>> int job_flags = JOB_DEFAULT;
>>
>> +if (!has_filter_node_name) {
>> +warn_report("Omitting filter-node-name parameter is deprecated, it "
>> +"will be required in future");
>> +}
>> +
>> if (!has_speed) {
>> speed = 0;
>> }
>> @@ -3990,6 +3995,11 @@ void qmp_blockdev_mirror(bool has_job_id, const char
>> *job_id,
>> Error *local_err = NULL;
>> int ret;
>>
>> +if (!has_filter_node_name) {
>> +warn_report("Omitting filter-node-name parameter is deprecated, it "
>> +"will be required in future");
>> +}
>> +
>> bs = qmp_get_root_bs(device, errp);
>> if (!bs) {
>> return;
>>
>
> This might be OK to do right away, though.
>
> 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@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
