On Thu, Aug 15, 2019 at 12:49:28 +0200, Kevin Wolf wrote:

Am 14.08.2019 um 21:27 hat John Snow geschrieben:

[...]

...

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.

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.

Who would read this, though? In the best case it ends up deep in a libvirt log that nobody will look at because there was no error. In the more common case, the debug level is configured so that QMP traffic isn't even logged.

The best we could do here is to log a warning. Thankfully we have one central function which always checks the returned JSON from qemu so we could do that universally. The would end up in the system log and alternatively also in the VM log file. I agree with Kevin that the possibility of it being noticed is rather small. From my experience users report non-fatal messages mostly only if it is spamming the system log. One of instances are very unlikely to be noticed. In my experience it's better to notify us in libvirt of such change and we will try our best to fix it.

Markus Armbruster writes:

Peter Krempa <pkrempa@redhat.com> writes:

[...]

...

From my experience users report non-fatal messages mostly only if it is spamming the system log. One of instances are very unlikely to be noticed.

In my experience it's better to notify us in libvirt of such change and we will try our best to fix it.

How to best alert the layers above QEMU was one of the topic of 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

Relevant part:

* We need to communicate "you're using something that is deprecated". How? Right now, we print a deprecation message. Okay when humans use QEMU directly in a shell. However, when QEMU sits at the bottom of a software stack, the message will likely end up in a log file that is effectively write-only.

- The one way to get people read log files is crashing their application. A command line option --future could make QEMU crash right after printing a deprecation message. This could help with finding use of deprecated features in a testing environment.

- A less destructive way to grab people's attention is to make things run really, really slow: have QEMU go to sleep for a while after printing a deprecation message.

- We can also pass the buck to the next layer up: emit a QMP event.

Sadly, by the time the next layer connects to QMP, plenty of stuff already happened. We'd have to buffer deprecation events somehow.

What would libvirt do with such an event? Log it, taint the domain, emit a (libvirt) event to pass it on to the next layer up.

- A completely different idea is to have a configuratin linter. To support doing this at the libvirt level, QEMU could expose "is deprecated" in interface introspection. Feels feasible for QMP, where we already have sufficiently expressive introspection. For CLI, we'd first have to provide that (but we want that anyway).

- We might also want to dispay deprecation messages in QEMU's GUI somehow, or on serial consoles.

Sorry for catching up late, this mail thread happened during my PTO. I remember bringing up at the time [1] that the correct solution needs to take into account usage models that vary from - a workstation case, where displaying an error box is easy and convenient, - to local headless VMs where system-level notification would do the job better, allowing us to leverage things like system-wide email notifications - to large-scale collections of VMs managed by some layered product, where the correct reporting would be through something like Insights, i.e. you don't scan individual logs, you want something like "913 VMs are using deprecated X" To me, that implies that we need to have a clear division of roles, with a standard way to a) produce the errors, b) propagate them, c) consume them (at least up to libvirt) Notice that this work has already been done for "real" errors, i.e. there is a real QAPI notion of "errors". AFAICT, warn_report does not connect to it, though, it goes through error_vprintf which is really just basic logging. So would it make sense to: 1. Add a deprecation_report() alongside warn_report()? 2. Connect warn_report() and all the error_vprintf output to QAPI, e.g. using John's suggestion of adding the messages using some "warning" or "deprecated" tag? 3. Teach libvirt how to consume that new tag and pass it along? [1] https://lists.nongnu.org/archive/html/qemu-devel/2018-10/msg06131.html -- Cheers, Christophe de Dinechin (IRC c3d)

John Snow writes:

On 8/29/19 12:45 PM, Christophe de Dinechin wrote:

...

[...]

...

Sorry for catching up late, this mail thread happened during my PTO.

I remember bringing up at the time [1] that the correct solution needs to take into account usage models that vary from

- a workstation case, where displaying an error box is easy and convenient,

- to local headless VMs where system-level notification would do the job better, allowing us to leverage things like system-wide email notifications

- to large-scale collections of VMs managed by some layered product, where the correct reporting would be through something like Insights, i.e. you don't scan individual logs, you want something like "913 VMs are using deprecated X"

To me, that implies that we need to have a clear division of roles, with a standard way to

a) produce the errors, b) propagate them,

I started replying to this thread to the other mail you sent; I think this is going to be fairly involved. I wouldn't mind being proven wrong though.

Yes, I think it does look involved, but mostly for historical reasons. In other words, what is complicated is preserving the historical behaviors so as to not break existing consumers.

...

c) consume them (at least up to libvirt)

Notice that this work has already been done for "real" errors, i.e. there is a real QAPI notion of "errors". AFAICT, warn_report does not connect to it, though, it goes through error_vprintf which is really just basic logging.

So would it make sense to:

1. Add a deprecation_report() alongside warn_report()?

Where's that get routed to? just an error_vprintf style situation?

Yes, but see below.

...

2. Connect warn_report() and all the error_vprintf output to QAPI, e.g. using John's suggestion of adding the messages using some "warning" or "deprecated" tag?

How do you correlate them?

Without having looked at the code much, I think I would 1. extend the existing QAPI error to support warnings, deprecations and info messages. The first problem I see is that there is no error, so we may sometimes need to create one when there was none before. And of course make sure that this does not ultimately show as an error, but as a success with additional annotations. 2. replace the current "link + if" switching for error_vprintf with some actual notification mechanism, with one option routine to monitor_vprintf, one to stderr, one to log file, and then an additional path that would post a newly minted qapi warning.

...

3. Teach libvirt how to consume that new tag and pass it along?

I think it's not libvirt's job to pass it along, exactly -- libvirt made the decision for which features to engage in QEMU, not the end user.

First, by "pass along", I meant to possible layered products or management software. We don't necessarily need a new virErrorLevel, deprecation could be a warning with some special domain, e.g. VIR_FROM_DEPRECATION. There may be a need to add some API here. Looking at the code, it's not obvious to me that libvirt has any notion of error priority. In other words, if you raise an error then a warning, you get the warning as the last error, right? Second, why not report the use of deprecated features? I don't fully buy the rationale that libvirt engages the features, because it does not do it on its own, it does it because the user made some specific request. This point of view also seems to require that libvirt or the user should know ahead of time it's about to engage a deprecated feature. To me, the problem is precisely that neither libvirt nor the user knows, which is why we are discussing how to best make it known.

If the user upgrades QEMU but not libvirt, it's not really anything they have control over and they shouldn't be pestered with such things.

However, if libvirt accidentally released a version that engages deprecated behavior (and were unaware of it), it'd be nice to get user reports, surely?

Logging messages for libvirt might be the best that can be done there in that case.

I personally would treat that like any warning.

In contrast, power user tools like QMP libraries, qmp-shell and others allow more direct and meaningful access to QMP, so those should report deprecation messages to the user.

Agreed.

...

[1] https://lists.nongnu.org/archive/html/qemu-devel/2018-10/msg06131.html

-- Thanks, Christophe de Dinechin (IRC c3d)

Am 30.08.2019 um 20:11 hat John Snow geschrieben:

On 8/30/19 6:07 AM, Christophe de Dinechin wrote:

...

Without having looked at the code much, I think I would

1. extend the existing QAPI error to support warnings, deprecations and info messages. The first problem I see is that there is no error, so we may sometimes need to create one when there was none before. And of course make sure that this does not ultimately show as an error, but as a success with additional annotations.

I assume this might be a chance to consolidate all of the methodologies we use for actually checking if there was an error or not. There have been many and I am sure Markus can give us a history lesson if it's warranted.

Generally, there's a few paradigms I see a lot:

1. Rely on an error return code being produced by the called function. The caller trusts that errp was set. This is one of my favorite methods, because it has the least scaffolding.

This one is convenient to use, but the problem is that nobody enforces that errp is always set if ret < 0, and that it's not set for ret == 0. So in a way it is error-prone because it allows inconsistencies.

2. Pass errp directly to the called function, and check for null after return. I don't like this method very much, because of confusion with:

I mainly don't like this very much because it's simply wrong. Callers can pass errp = NULL if they aren't interested in error information. If you directly pass errp, you can't check *errp because errp could be NULL. So directly passing errp makes the code simpler, but only use it in functions where you don't intend to check whether an error is set.

3. Create a local error object; check THAT for null, and propagate the error to the common error object. I think Markus has explained why we have this code 50 times, and I forget again minutes later.

local_err exists for those cases where you need to check the error object before passing it to the caller. (And obviously for those cases where you don't want to pass it to the caller, but do something like error_report_err().)

If we want to expand the concept of the error object into something that encompasses hints, warnings and deprecations*, checking for null is no longer appropriate. It might be a good chance to make our error propagation story more consistent, too.

We could unify with a helper like this, I think, if I'm not forgetting some crucial usage detail:

subroutine(foo, bar, errp); if (failure(errp)) { error_append_hint(errp, "Lorem ipsum, ..."); cleanup(); return; }

We would then always use this pattern that operates directly on the caller's errp instead of creating local error objects to allow hints and warnings to accumulate.

There are two parts to the change that you imply: 1. Forbid passing errp = NULL to any function so that *errp can always be checked. This gets rid of local_err in the intermediate function, but may require the introduction of new local_err variables in top-level callers which ignore the error information. 2. Introduce failure(errp) to replace errp != NULL because we want Error to contain warnings and notes, too. Currently, it can contain only exactly one error, so this would be a major change. Note that the change wouldn't make the existing 'if (errp)' checks build failures, so getting confident that we found and replaced all of them is going to be hard. Essentially, you'd probably want to replace Error with a new type so that the compiler will at least be able to tell which places have been converted and which haven't. And then, you'd have to touch every single function that does something with errors. This is a huge change across the whole source tree. I doubt it's worth the effort.

...

Second, why not report the use of deprecated features? I don't fully buy the rationale that libvirt engages the features, because it does not do it on its own, it does it because the user made some specific request.

Because the user didn't request those specific QMP features, they asked for the VM to start, or to stop, or they asked for a backup, or a snapshot.

On my desktop, I am not really too interested in knowing if XFCE is using deprecated features of xorg or wayland. I didn't tell it to use them and I have no real power or control over that. It's nice if I'm a developer, but as a user, it's noise.

So a development log seems right for these, but not user-visible interruptions.

I agree, it's not the high-level operation the user requested that is deprecated, but just the specific implementation libvirt uses to perform the operation on a QEMU VM. The expected response to a deprecation notice is that a libvirt update makes it go away by using more modern interfaces, not that the user changes their workflow. Kevin

On 22 Nov 2019, at 09:41, Markus Armbruster <armbru@redhat.com> wrote:

Reviving this old thread, because I'd like to connect it to more recent discussions.

Christophe de Dinechin <dinechin@redhat.com <mailto:dinechin@redhat.com>> writes:

...

Markus Armbruster writes:

...

Peter Krempa <pkrempa@redhat.com> writes:

[...]

...

...

From my experience users report non-fatal messages mostly only if it is spamming the system log. One of instances are very unlikely to be noticed.

In my experience it's better to notify us in libvirt of such change and we will try our best to fix it.

How to best alert the layers above QEMU was one of the topic of 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

Relevant part:

* We need to communicate "you're using something that is deprecated". How? Right now, we print a deprecation message. Okay when humans use QEMU directly in a shell. However, when QEMU sits at the bottom of a software stack, the message will likely end up in a log file that is effectively write-only.

- The one way to get people read log files is crashing their application. A command line option --future could make QEMU crash right after printing a deprecation message. This could help with finding use of deprecated features in a testing environment.

- A less destructive way to grab people's attention is to make things run really, really slow: have QEMU go to sleep for a while after printing a deprecation message.

- We can also pass the buck to the next layer up: emit a QMP event.

Sadly, by the time the next layer connects to QMP, plenty of stuff already happened. We'd have to buffer deprecation events somehow.

What would libvirt do with such an event? Log it, taint the domain, emit a (libvirt) event to pass it on to the next layer up.

- A completely different idea is to have a configuratin linter. To support doing this at the libvirt level, QEMU could expose "is deprecated" in interface introspection. Feels feasible for QMP, where we already have sufficiently expressive introspection. For CLI, we'd first have to provide that (but we want that anyway).

- We might also want to dispay deprecation messages in QEMU's GUI somehow, or on serial consoles.

Sorry for catching up late, this mail thread happened during my PTO.

I remember bringing up at the time [1] that the correct solution needs to take into account usage models that vary from

- a workstation case, where displaying an error box is easy and convenient,

- to local headless VMs where system-level notification would do the job better, allowing us to leverage things like system-wide email notifications

- to large-scale collections of VMs managed by some layered product, where the correct reporting would be through something like Insights, i.e. you don't scan individual logs, you want something like "913 VMs are using deprecated X"

To me, that implies that we need to have a clear division of roles, with a standard way to

a) produce the errors, b) propagate them, c) consume them (at least up to libvirt)

Notice that this work has already been done for "real" errors, i.e. there is a real QAPI notion of "errors". AFAICT, warn_report does not connect to it, though, it goes through error_vprintf which is really just basic logging.

So would it make sense to:

1. Add a deprecation_report() alongside warn_report()?

"Grepability" alone would make that worthwhile, I think.

...

2. Connect warn_report() and all the error_vprintf output to QAPI, e.g. using John's suggestion of adding the messages using some "warning" or "deprecated" tag?

This is the difficult part. See my "Exposing feature deprecation to machine clients (was: [Qemu-devel] [PATCH 2/2] qapi: deprecate implicit filters)" in this thread. Quote:

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.

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.

This looks swampy the way you describe it, but this make me think that the recorder library may provide a somewhat elegant solution to this. First, my presupposition here is that part of the issues you are describing is because there is no “one size fits all” reporting of errors, warnings, etc. Essentially, it’s a policy that is defined outside of libvirt or qemu, and not all consumers will want the same policy. The way the recorder library addresses that is to have as many recorders as you need, and allowing the behavior for each recorder to be configured individually at run-time, e.g. through env variables. If you couple that with a naming convention for the recorders, you can set the policy externally in a rather flexible way. For example, I will typically have recorders that end with _error or _warning or _info or _verbose. We could have a _deprecated convention just the same. With a simple syntax, you can state that you want tracing on all warnings and errors by activating a regexp like “.*_(warning|error)”, or disable anything related to deprecation with “.*_deprecated.*=0”. It wouldn’t be much of a stretch to have similar policies for “create a qapi error object” or even “abort the process”. This approach means that we would get to define the default policy for the various deprecation cases, but that the user would ultimately be able to override, maybe even individually, e.g.: // I don’t care about deprecation at all .*_deprecated=0 // Log all deprecation messages, do nothing else with it .*_deprecated=<log> // Log most deprecation, but ignore “blink_tag” deprecation // messages and abort on “serial_uart” deprecation message .*_deprecated=<log> blink_tag_deprecated=0 serial_uart_deprecated=<abort>

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.

The recorder above (as it exists today) only deals with logging and reporting messages. It provides no help whatsoever for introspection, which I assume you would use to warn “ahead of time” that something is deprecated, right?

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.

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.

For these cases, we can still fallback to C code, no? As long as the “report_deprecated()” being used is somewhat consistent between the two, the fact that there is a “short, common case” for optiions and a “complicated, less common case” would be invisible to users.

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've since explored this idea in

[RFC PATCH 00/19] Configurable policy for handling deprecated interfaces Message-Id: <20191024123458.13505-1-armbru@redhat.com <mailto:20191024123458.13505-1-armbru@redhat.com>> https://lists.gnu.org/archive/html/qemu-devel/2019-10/msg06534.html <https://lists.gnu.org/archive/html/qemu-devel/2019-10/msg06534.html>

I had not paid attention to this one. I’ll take a look and get back to you.

...

3. Teach libvirt how to consume that new tag and pass it along?

Pertinent: Message-ID: <20191024140146.GC8381@redhat.com <mailto:20191024140146.GC8381@redhat.com>> https://lists.gnu.org/archive/html/qemu-devel/2019-10/msg06604.html <https://lists.gnu.org/archive/html/qemu-devel/2019-10/msg06604.html>

Quoting Dan's conclusion:

In summary, it is probably reasonable to include this info in the QMP command reply, but don't expect much to be done with it beyond possibly writing it to a log file.

...

[1] https://lists.nongnu.org/archive/html/qemu-devel/2018-10/msg06131.html <https://lists.nongnu.org/archive/html/qemu-devel/2018-10/msg06131.html>

Thanks Christophe

...

-- Cheers, Christophe de Dinechin (IRC c3d)

Kevin Wolf <kwolf@redhat.com> writes:

Am 15.08.2019 um 21:24 hat Markus Armbruster geschrieben: [...]

...

Let's assume all libvirt ever does with deprecation notices is logging them. Would that solve the problem of reliably alerting libvirt developers to deprecation issues? Nope. But it could help occasionally.

I'm not saying that deprecation notices would hurt, just that they probably won't solve problem alone.

No argument.

Crashing if --future is given and logging otherwise seems reasonable enough to me. Whether we need to wire up a new deprecation mechanism in QMP for the logging or if we can just keep printing to stderr is debatable. stderr already ends up in a log file, a QMP extension would require new libvirt code. If libvirt would log deprecation notices more prominently, or use the information for tainting or any other kind of processing, a dedicated QMP mechanism could be justified.

I'd like to start with two tasks: * A CLI option to configure what to do on use of a deprecated feature. We currently warn. We want to be able to crash instead. Silencing the warnings might be useful. Turning them into errors might be useful. The existing ad hoc warnings need to be replaced by a call of a common function that implements the configurable behavior. * QAPI feature flag "deprecated", for introspectable deprecation, and without ad hoc code. Then see whether our users need more.

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.

Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

07.11.2019 21:52, Philippe Mathieu-Daudé wrote: [...]

...

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.

4.2.0-rc0 has been tagged, i.e. we're in hard freeze already. Only bug fixes are accepted during hard freeze. We've occasionally bent this rule after -rc0 for borderline cases, e.g. to tweak a new external interface before the release calcifies it. Making a case for bending the rules becomes harder with each -rc. Ideally, we'd double-check new interfaces for gaffes before a release, and whether old interfaces that have been replaced now should be deprecated. There's rarely time for that, and pretty much never for releases right after KVM Forum. So no, I don't have anything in mind for 4.2. We intend to tag -rc1 next Tuesday. To make that deadline, we'd need patches, not just ideas.

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?

See above.

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

Sounds workable to me, but I'm not the maintainer.

or 1.2 Update iotests to not use deprecated things.. Not appropriate for hard freeze.

Unnecessarily risky compared to 1.1.

or 2. Commit deprecations without warnings.. But how do people find out about this?

Not nice. We do it for QMP, but only because we still lack the means to warn there.

Next, what exactly to deprecate? As I understand, we can't deprecate drive-mirror now? So I propose to:

1. deprecate drive-backup 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.

To have a chance there, we need patches a.s.a.p.

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.

On 8/29/19 11:59 AM, Christophe de Dinechin wrote:

John Snow writes: [...]

...

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.

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.

I like this approach even if there is no consumer today. It does not hurt, and it is indeed a motivation to develop consumers that care.

I personally find this much easier to swallow than any kind of crash on deprecation, which already at the BoF seemed like a really big hammer to kill a fly.

CC'ing Andrea as well, because we discussed recently about how to deal with error checking in general, and if a new error checking framework is being put in place, adding deprecation to the thinking could be a good idea.

The most convincing argument against deprecation notices like this is not that they won't be consumed, but that they are difficult to plumb through the C infrastructure. Sadly, I think I have to agree there -- we can't even really model it like hints, because these are cases where there was no /error/ but instead a success -- but our error propagation doesn't work on those terms generally and we'd need a rather extensive audit to allow warnings. We could always fudge it with a kind of global warning log: clear the log at the beginning of a QMP interaction and if the log is non-empty when we return, amend the return with that information. That's not really the nicest thing to do in a multi-process, multi-threaded, multi-stacked application, though, so... --js

On 8/23/19 5:22 AM, Vladimir Sementsov-Ogievskiy wrote:

14.08.2019 13:07, Vladimir Sementsov-Ogievskiy wrote:

...

To get rid of implicit filters related workarounds in future let's deprecate them now.

Interesting, could we deprecate implicit filter without deprecation of unnecessity of parameter? As actually, it's good when this parameter is not necessary, in most cases user is not interested in node-name.

https://en.wiktionary.org/wiki/unnecessity -- I am surprised to learn that this a real word in the language I speak. :) I assume you're referring to making the optional argument mandatory.

Obviously we can do the following:

1. In 4.2 we deprecate unnecessity, which implies deprecation of implicit filters 2. After some releases in 4.x we can drop deprecated functionality, so we drop it together with implicit filters. And, in same release 4.x we return it back (as it's compatible change :) but without implicit filters (so, if filter-node-name not specified, we just create explicit filter with autogenerated node-name)

So, effectively we just drop "deprecation mark" together with implicit filters, which is nice but actually confusing.

Instead, we may do 1. In 4.2 deprecate 2. In 4.x drop optionality together with implicit filters 3. In 4.y (y > x of course) return optionality back

Ah, I see what you're digging at here now...

It's a bit safer, but for users who miss releases [4.x, 4.y) it's no difference..

Or we just write in spec, that implicit filters are deprecated? But we have nothing about implicit filters in spec. More over, we directly write that we have filter, and if parameter is omitted it's node-name is autogenerated. So actually, the fact the filter is hidden when filter-node-name is unspecified is _undocumented_.

So, finally, it looks like nothing to deprecated in specification, we can just drop implicit filters :)

What do you think?

What exactly _IS_ an implicit filter? How does it differ today from an explicit filter? I assumed the only difference was if it was named or not; but I think I must be mistaken now if you're proposing leaving the interface alone entirely. Are they instantiated differently? --js

(Peter: search for "pkrempa" down below.) On 8/28/19 5:20 AM, Vladimir Sementsov-Ogievskiy wrote:

27.08.2019 23:12, John Snow wrote:

...

On 8/23/19 5:22 AM, Vladimir Sementsov-Ogievskiy wrote:

...

14.08.2019 13:07, Vladimir Sementsov-Ogievskiy wrote:

...

To get rid of implicit filters related workarounds in future let's deprecate them now.

Interesting, could we deprecate implicit filter without deprecation of unnecessity of parameter? As actually, it's good when this parameter is not necessary, in most cases user is not interested in node-name.

https://en.wiktionary.org/wiki/unnecessity -- I am surprised to learn that this a real word in the language I speak. :)

I assume you're referring to making the optional argument mandatory.

exactly, it's my a bit "google-translate-driven" English)

It teaches me some fun words!

...

...

Obviously we can do the following:

1. In 4.2 we deprecate unnecessity, which implies deprecation of implicit filters 2. After some releases in 4.x we can drop deprecated functionality, so we drop it together with implicit filters. And, in same release 4.x we return it back (as it's compatible change :) but without implicit filters (so, if filter-node-name not specified, we just create explicit filter with autogenerated node-name)

So, effectively we just drop "deprecation mark" together with implicit filters, which is nice but actually confusing.

Instead, we may do 1. In 4.2 deprecate 2. In 4.x drop optionality together with implicit filters 3. In 4.y (y > x of course) return optionality back

Ah, I see what you're digging at here now...

...

It's a bit safer, but for users who miss releases [4.x, 4.y) it's no difference..

Or we just write in spec, that implicit filters are deprecated? But we have nothing about implicit filters in spec. More over, we directly write that we have filter, and if parameter is omitted it's node-name is autogenerated. So actually, the fact the filter is hidden when filter-node-name is unspecified is _undocumented_.

So, finally, it looks like nothing to deprecated in specification, we can just drop implicit filters :)

What do you think?

What exactly _IS_ an implicit filter? How does it differ today from an explicit filter? I assumed the only difference was if it was named or not; but I think I must be mistaken now if you're proposing leaving the interface alone entirely.

Are they instantiated differently?

As I understand, the only difference is their BlockDriverState.impicit field, and several places in code where we skip implicit filter when trying to find something in a chain starting from a device.

Oh, oh, yes. I see.

Hmm, OK, let's see:

1. the only implicit filters are commit_top and mirror_top if user don't specify filter-node-name.

Where it make sense, i.e., where implicit field used?

`git grep -E '(->|\.)implicit'` is what I used to find usages.

2. bdrv_query_info, bdrv_query_bds_stats, bdrv_block_device_info(only when called from bdrv_query_info), they'll report filter as top node if we don't mark it implicit.

So that's a bit of a change, but only visually. The "reality" is still the same, we just report it more "accurately." libvirt MIGHT need a heads up here. I'm looping pkrempa back in for comment. <pkrempa> Would libvirt be negatively impacted by the revelation of formerly internal ("implicit") nodes created by mirror and commit via query block commands? At the moment, QEMU hides them from you if you do not name them. </pkrempa>

3. bdrv_refresh_filename, bdrv_reopen_parse_backing, bdrv_drop_intermediate: I think it's not a problem, just drop special case for implicit fitlers

I'm much less certain about what the impact of this would be and would need to audit it (and don't have the time to, personally.) Do you have a POC or RFC patch that demonstrates dropping these special cases? It might be nice to see as proof that it's safe to deprecate.

So, seems the only real change is query-block and query-blockstats output when mirror or commit is started without specifying filter-node-name (filter would be on top)

So, how should we deprecate this, or can we just change it?

I'm not sure if it's worth it yet, what does dropping the implicit field buy us? Conceptually I understand that it's simpler without the notion of implicit fields, but I imagine there's some cleanup in particular that motivated this. I'd say to just change the behavior, we should: - Give a standard three-release warning that the behavior will change in an incompatible way - Demonstrate with an RFC patch that special cases around ->implicit in block.c can be removed and do not make the code more complex, - Get blessings from Peter Krempa. As always: Libvirt is not the end-all be-all of QEMU management, but if libvirt is capable of working around design changes then I believe any project out there today also could, so it's a good litmus test. --js

29.08.2019 17:44, Peter Krempa wrote:

On Wed, Aug 28, 2019 at 13:48:10 -0400, John Snow wrote:

...

(Peter: search for "pkrempa" down below.)

On 8/28/19 5:20 AM, Vladimir Sementsov-Ogievskiy wrote:

[....]

...

So that's a bit of a change, but only visually. The "reality" is still the same, we just report it more "accurately." libvirt MIGHT need a heads up here. I'm looping pkrempa back in for comment.

<pkrempa> Would libvirt be negatively impacted by the revelation of formerly internal ("implicit") nodes created by mirror and commit via query block commands? At the moment, QEMU hides them from you if you do not name them.

Currently we would not be able to handle that properly at least definitely in the pre-blockdev case. In blockdev case I must make sure that it will work.

The thing is that I didn't really want to touch the pre-blockdev case code any more,

Aren't you going to deprecate and drop it at some moment? but if you decide that we should do it I'm willing to

investigate this case also for the old commands.

...

</pkrempa>

...

3. bdrv_refresh_filename, bdrv_reopen_parse_backing, bdrv_drop_intermediate: I think it's not a problem, just drop special case for implicit fitlers

I'm much less certain about what the impact of this would be and would need to audit it (and don't have the time to, personally.)

Do you have a POC or RFC patch that demonstrates dropping these special cases? It might be nice to see as proof that it's safe to deprecate.

...

So, seems the only real change is query-block and query-blockstats output when mirror or commit is started without specifying filter-node-name (filter would be on top)

So, how should we deprecate this, or can we just change it?

I'm not sure if it's worth it yet, what does dropping the implicit field buy us? Conceptually I understand that it's simpler without the notion of implicit fields, but I imagine there's some cleanup in particular that motivated this.

I'd say to just change the behavior, we should:

- Give a standard three-release warning that the behavior will change in an incompatible way - Demonstrate with an RFC patch that special cases around ->implicit in block.c can be removed and do not make the code more complex, - Get blessings from Peter Krempa.

As always: Libvirt is not the end-all be-all of QEMU management, but if libvirt is capable of working around design changes then I believe any project out there today also could, so it's a good litmus test.

For libvirt we really care more whether a node is format/protocol related or not rather than whether it's implicit or not.

In this case we could filter it by the known protocol and format driver types and filter out the rest in cases when we e.g. detect the node names for the pre-blockdev era cases.

(Note that even with new qemu, if an SD card is used blockdev will be disabled).

-- Best regards, Vladimir

28.08.2019 20:48, John Snow wrote:

(Peter: search for "pkrempa" down below.)

On 8/28/19 5:20 AM, Vladimir Sementsov-Ogievskiy wrote:

...

27.08.2019 23:12, John Snow wrote:

...

On 8/23/19 5:22 AM, Vladimir Sementsov-Ogievskiy wrote:

...

14.08.2019 13:07, Vladimir Sementsov-Ogievskiy wrote:

...

To get rid of implicit filters related workarounds in future let's deprecate them now.

Interesting, could we deprecate implicit filter without deprecation of unnecessity of parameter? As actually, it's good when this parameter is not necessary, in most cases user is not interested in node-name.

https://en.wiktionary.org/wiki/unnecessity -- I am surprised to learn that this a real word in the language I speak. :)

I assume you're referring to making the optional argument mandatory.

exactly, it's my a bit "google-translate-driven" English)

It teaches me some fun words!

...

...

...

Obviously we can do the following:

1. In 4.2 we deprecate unnecessity, which implies deprecation of implicit filters 2. After some releases in 4.x we can drop deprecated functionality, so we drop it together with implicit filters. And, in same release 4.x we return it back (as it's compatible change :) but without implicit filters (so, if filter-node-name not specified, we just create explicit filter with autogenerated node-name)

So, effectively we just drop "deprecation mark" together with implicit filters, which is nice but actually confusing.

Instead, we may do 1. In 4.2 deprecate 2. In 4.x drop optionality together with implicit filters 3. In 4.y (y > x of course) return optionality back

Ah, I see what you're digging at here now...

...

It's a bit safer, but for users who miss releases [4.x, 4.y) it's no difference..

Or we just write in spec, that implicit filters are deprecated? But we have nothing about implicit filters in spec. More over, we directly write that we have filter, and if parameter is omitted it's node-name is autogenerated. So actually, the fact the filter is hidden when filter-node-name is unspecified is _undocumented_.

So, finally, it looks like nothing to deprecated in specification, we can just drop implicit filters :)

What do you think?

What exactly _IS_ an implicit filter? How does it differ today from an explicit filter? I assumed the only difference was if it was named or not; but I think I must be mistaken now if you're proposing leaving the interface alone entirely.

Are they instantiated differently?

As I understand, the only difference is their BlockDriverState.impicit field, and several places in code where we skip implicit filter when trying to find something in a chain starting from a device.

Oh, oh, yes. I see.

...

Hmm, OK, let's see:

1. the only implicit filters are commit_top and mirror_top if user don't specify filter-node-name.

Where it make sense, i.e., where implicit field used?

`git grep -E '(->|\.)implicit'` is what I used to find usages.

...

2. bdrv_query_info, bdrv_query_bds_stats, bdrv_block_device_info(only when called from bdrv_query_info), they'll report filter as top node if we don't mark it implicit.

So that's a bit of a change, but only visually. The "reality" is still the same, we just report it more "accurately." libvirt MIGHT need a heads up here. I'm looping pkrempa back in for comment.

<pkrempa> Would libvirt be negatively impacted by the revelation of formerly internal ("implicit") nodes created by mirror and commit via query block commands? At the moment, QEMU hides them from you if you do not name them. </pkrempa>

...

3. bdrv_refresh_filename, bdrv_reopen_parse_backing, bdrv_drop_intermediate: I think it's not a problem, just drop special case for implicit fitlers

I'm much less certain about what the impact of this would be and would need to audit it (and don't have the time to, personally.)

Do you have a POC or RFC patch that demonstrates dropping these special cases? It might be nice to see as proof that it's safe to deprecate.

...

So, seems the only real change is query-block and query-blockstats output when mirror or commit is started without specifying filter-node-name (filter would be on top)

So, how should we deprecate this, or can we just change it?

I'm not sure if it's worth it yet, what does dropping the implicit field buy us? Conceptually I understand that it's simpler without the notion of implicit fields, but I imagine there's some cleanup in particular that motivated this.

Reviewing Max's "block: Deal with filters" series motivated me.

I'd say to just change the behavior, we should:

- Give a standard three-release warning that the behavior will change in an incompatible way - Demonstrate with an RFC patch that special cases around ->implicit in block.c can be removed and do not make the code more complex, - Get blessings from Peter Krempa.

As always: Libvirt is not the end-all be-all of QEMU management, but if libvirt is capable of working around design changes then I believe any project out there today also could, so it's a good litmus test.

--js

-- Best regards, Vladimir

Am 23.08.2019 um 11:22 hat Vladimir Sementsov-Ogievskiy geschrieben:

14.08.2019 13:07, Vladimir Sementsov-Ogievskiy wrote:

...

To get rid of implicit filters related workarounds in future let's deprecate them now.

Interesting, could we deprecate implicit filter without deprecation of unnecessity of parameter? As actually, it's good when this parameter is not necessary, in most cases user is not interested in node-name.

A modern client is supposed to be interested in node-names. We basically expect it to manage the whole block graph at a node level, so it should certainly make sure to know the node-name of every node. It will need it to take snapshots, insert filter drivers or anything like this on top of the implicit node. So once we make the option mandatory (which means returning an error for legacy clients that don't do node-level management), I don't see a good reason to ever make it optional again. Kevin