| Message ID | 20201116131011.26607-7-r.bolshakov@yadro.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add HMP/QMP commands to query accelerator | expand |
Copying libvir-list for the deprecation warning. On Mon, Nov 16, 2020 at 04:10:11PM +0300, Roman Bolshakov wrote: > 'query-accel' QMP command should be used instead. > > Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com> > --- > qapi/machine.json | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) docs/system/deprecated.rst needs to be updated for any deprecation to be visible to consumers of QEMU. > > diff --git a/qapi/machine.json b/qapi/machine.json > index 5648d8d24d..130b0dbebc 100644 > --- a/qapi/machine.json > +++ b/qapi/machine.json > @@ -579,6 +579,9 @@ > # > # Returns information about KVM acceleration > # > +# Features: > +# @deprecated: This command is deprecated, use 'query-accel' instead. > +# > # Returns: @AccelInfo > # > # Since: 0.14.0 > @@ -589,7 +592,8 @@ > # <- { "return": { "enabled": true, "present": true } } > # > ## > -{ 'command': 'query-kvm', 'returns': 'AccelInfo' } > +{ 'command': 'query-kvm', 'returns': 'AccelInfo', > + 'features': [ 'deprecated' ] } > > ## > # @query-accel: > -- > 2.29.2 > > Regards, Daniel
On Fri, Nov 27, 2020 at 10:50:59 +0000, Daniel Berrange wrote: > Copying libvir-list for the deprecation warning. > > > On Mon, Nov 16, 2020 at 04:10:11PM +0300, Roman Bolshakov wrote: > > 'query-accel' QMP command should be used instead. > > > > Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com> > > --- > > qapi/machine.json | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > docs/system/deprecated.rst needs to be updated for any deprecation > to be visible to consumers of QEMU. On behalf of libvirt I'd like to ask to consider cases where the replacement for a deprecated feature is added in the same release as the deprecation happens, to treat the replacement as API stable at merge time. Any changes to the command after the series is merged with a deprecation of the old should be consulted with the libvirt team as we might actually have already added support for the new approach. Considering it as "it wasn't released so we can change it" may introduce breakage given that the relase cycles of libvirt and qemu are not in sync. We try hard to stay on top of such changes by using the new interface as soon as possible, but that is very hard if the replacement changes during the dev cycle.
On Fri, Nov 27, 2020 at 12:21:54PM +0100, Peter Krempa wrote: > On Fri, Nov 27, 2020 at 10:50:59 +0000, Daniel Berrange wrote: > > Copying libvir-list for the deprecation warning. > > > > > > On Mon, Nov 16, 2020 at 04:10:11PM +0300, Roman Bolshakov wrote: > > > 'query-accel' QMP command should be used instead. > > > > > > Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com> > > > --- > > > qapi/machine.json | 6 +++++- > > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > docs/system/deprecated.rst needs to be updated for any deprecation > > to be visible to consumers of QEMU. > > On behalf of libvirt I'd like to ask to consider cases where the > replacement for a deprecated feature is added in the same release as the > deprecation happens, to treat the replacement as API stable at merge > time. > > Any changes to the command after the series is merged with a deprecation > of the old should be consulted with the libvirt team as we might > actually have already added support for the new approach. Considering it > as "it wasn't released so we can change it" may introduce breakage given > that the relase cycles of libvirt and qemu are not in sync. > > We try hard to stay on top of such changes by using the new interface as > soon as possible, but that is very hard if the replacement changes > during the dev cycle. > I see, thanks for the explanation! Perhaps I'll drop deprecation from the series to avoid the issue. Then as soon as libvirt gets support for queyring accels we might consider deprecation again. Best Regards, Roman
On Fri, Nov 27, 2020 at 14:45:12 +0300, Roman Bolshakov wrote: > On Fri, Nov 27, 2020 at 12:21:54PM +0100, Peter Krempa wrote: > > On Fri, Nov 27, 2020 at 10:50:59 +0000, Daniel Berrange wrote: > > > Copying libvir-list for the deprecation warning. > > > > > > > > > On Mon, Nov 16, 2020 at 04:10:11PM +0300, Roman Bolshakov wrote: > > > > 'query-accel' QMP command should be used instead. > > > > > > > > Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com> > > > > --- [...] > > We try hard to stay on top of such changes by using the new interface as > > soon as possible, but that is very hard if the replacement changes > > during the dev cycle. > > > > I see, thanks for the explanation! Perhaps I'll drop deprecation from > the series to avoid the issue. > > Then as soon as libvirt gets support for queyring accels we might > consider deprecation again. I don't want to imply that it's entirely necessary to postpone it, but in such cases the new API which was added to replace the old one needs to be considered a bit more strongly until the release. The main reason for this is that libvirt has tests whether it uses any deprecated interface. If anything is marked as deprecated and our tests flag it, we add an override. Usually the override is added in the same patchset which actually implements the new approach. We obviously can add an override and then wait for the supported interface, but once the override is added there's nothing to remind us later on, so I generally like to have everything in one series. As you can see this has an issue when we have to add support for a unreleased interface, which may change during the dev cycle or plainly forget that something got deprecated as we've already added an override. This mainly comes from libvirt trying to keep on top of the changes so we refresh the QMP schema during qemu's dev cycle multiple times. Obviously the argument that we try to depend on unreleased functionality can be used, but that would be to detrement of progress IMO.
Peter Krempa <pkrempa@redhat.com> writes: > On Fri, Nov 27, 2020 at 14:45:12 +0300, Roman Bolshakov wrote: >> On Fri, Nov 27, 2020 at 12:21:54PM +0100, Peter Krempa wrote: >> > On Fri, Nov 27, 2020 at 10:50:59 +0000, Daniel Berrange wrote: >> > > Copying libvir-list for the deprecation warning. >> > > >> > > >> > > On Mon, Nov 16, 2020 at 04:10:11PM +0300, Roman Bolshakov wrote: >> > > > 'query-accel' QMP command should be used instead. >> > > > >> > > > Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com> >> > > > --- > > [...] > >> > We try hard to stay on top of such changes by using the new interface as >> > soon as possible, but that is very hard if the replacement changes >> > during the dev cycle. >> > >> >> I see, thanks for the explanation! Perhaps I'll drop deprecation from >> the series to avoid the issue. >> >> Then as soon as libvirt gets support for queyring accels we might >> consider deprecation again. > > I don't want to imply that it's entirely necessary to postpone it, but > in such cases the new API which was added to replace the old one needs > to be considered a bit more strongly until the release. > > The main reason for this is that libvirt has tests whether it uses any > deprecated interface. If anything is marked as deprecated and our tests > flag it, we add an override. Usually the override is added in the same > patchset which actually implements the new approach. > > We obviously can add an override and then wait for the supported > interface, but once the override is added there's nothing to remind us > later on, so I generally like to have everything in one series. > > As you can see this has an issue when we have to add support for a > unreleased interface, which may change during the dev cycle or plainly > forget that something got deprecated as we've already added an override. > > This mainly comes from libvirt trying to keep on top of the changes so > we refresh the QMP schema during qemu's dev cycle multiple times. > > Obviously the argument that we try to depend on unreleased functionality > can be used, but that would be to detrement of progress IMO. I understand your concerns. We have a somewhat similar problem in QEMU: there's nothing to remind us later on that the old interface still needs to be deprecated. Here's an idea. Keep a list of things to deprecate in the repository. Instead of deprecating right away, we add to the list. When soft freeze comes, we go through the list and decide: either deprecate now (and delete from the list), or postpone deprecation to the next release (and keep it on the list). Would that work for libvirt? There's still a risk of us forgetting about the list. Perhaps keeping a reminder on the Planning/x.y wiki page could help. Peter (Maydell), do you have a check list for the various milestones?
On Fri, Nov 27, 2020 at 01:18:09PM +0100, Peter Krempa wrote: > On Fri, Nov 27, 2020 at 14:45:12 +0300, Roman Bolshakov wrote: > > On Fri, Nov 27, 2020 at 12:21:54PM +0100, Peter Krempa wrote: > > > On Fri, Nov 27, 2020 at 10:50:59 +0000, Daniel Berrange wrote: > > > > Copying libvir-list for the deprecation warning. > > > > > > > > > > > > On Mon, Nov 16, 2020 at 04:10:11PM +0300, Roman Bolshakov wrote: > > > > > 'query-accel' QMP command should be used instead. > > > > > > > > > > Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com> > > > > > --- > > [...] > > > > We try hard to stay on top of such changes by using the new interface as > > > soon as possible, but that is very hard if the replacement changes > > > during the dev cycle. > > > > > > > I see, thanks for the explanation! Perhaps I'll drop deprecation from > > the series to avoid the issue. > > > > Then as soon as libvirt gets support for queyring accels we might > > consider deprecation again. > > I don't want to imply that it's entirely necessary to postpone it, but > in such cases the new API which was added to replace the old one needs > to be considered a bit more strongly until the release. > > The main reason for this is that libvirt has tests whether it uses any > deprecated interface. If anything is marked as deprecated and our tests > flag it, we add an override. Usually the override is added in the same > patchset which actually implements the new approach. > > We obviously can add an override and then wait for the supported > interface, but once the override is added there's nothing to remind us > later on, so I generally like to have everything in one series. IIUC, this all relies on us importing a dump of the latest QEMU capabilities into the libvirt test suite. Most of the capabilities we import are the release version, but we also periodically import the git snapshot capabilities and will refresh them until GA of QEMU. Could we arrange it so that libvirt only reports an error for use of deprecated interfaces when testing against the GA capabilities dump. If testing against QEMU git snapshot capabilities, we can emit a warning only. That way, we'll have a grace period in which libvirt can see the warning from tests, and we would only need to add an override to silence it once we import the GA capabilities, at which time it is safe to implement the new solution too. Regards, Daniel
On Fri, Nov 27, 2020 at 16:44:05 +0100, Markus Armbruster wrote: > Peter Krempa <pkrempa@redhat.com> writes: > > > On Fri, Nov 27, 2020 at 14:45:12 +0300, Roman Bolshakov wrote: > >> On Fri, Nov 27, 2020 at 12:21:54PM +0100, Peter Krempa wrote: [...] > > As you can see this has an issue when we have to add support for a > > unreleased interface, which may change during the dev cycle or plainly > > forget that something got deprecated as we've already added an override. > > > > This mainly comes from libvirt trying to keep on top of the changes so > > we refresh the QMP schema during qemu's dev cycle multiple times. > > > > Obviously the argument that we try to depend on unreleased functionality > > can be used, but that would be to detrement of progress IMO. > > I understand your concerns. > > We have a somewhat similar problem in QEMU: there's nothing to remind us > later on that the old interface still needs to be deprecated. Oh, yes. That's basically the same thing. The thing is that changes to the new interface are very rare, but very real. Since I don't really want to increase the burden for any normal scenario. I'd also very much like to keep libvirt pulling in snapshots of qemu's capabilities/qapi schema etc throughout the development cycle. It allows us to adapt faster and develop features simultaneously. This unfortunately undermines my own arguments partially as libvirt regularly develops features on top of unreleased qemu features so we are regularly risking very similar circumstances. The small but important difference though is, that releasing a broken feature is not as bad as breaking an existing feature. As a conclusion of the above I'd basically prefer a sort of gentleman's agreement, that new APIs become 'somewhat' stable at the moment the commit deprecating the old interface hits upstream. The 'somewhat'-stable API would mean that any changes to the new API should be consulted with libvirt so that we can either give a go-ahead that we didn't adapt yet, disable the adaptation until the changes can be done, or another compromise depending on what's the state. I know it's hard to enforce, but probably the cheapest in terms of drawbacks any other solution would be. I'll probably keep notifying patchsets which implement and deprecate old api at the same time to keep in mind that we need to be kept in touch, but I really don't want to impose any kind of extra process to development on either side.
On Fri, Nov 27, 2020 at 15:53:16 +0000, Daniel Berrange wrote: > On Fri, Nov 27, 2020 at 01:18:09PM +0100, Peter Krempa wrote: > > On Fri, Nov 27, 2020 at 14:45:12 +0300, Roman Bolshakov wrote: [...] > IIUC, this all relies on us importing a dump of the latest QEMU > capabilities into the libvirt test suite. > > Most of the capabilities we import are the release version, but > we also periodically import the git snapshot capabilities and > will refresh them until GA of QEMU. > > Could we arrange it so that libvirt only reports an error for use > of deprecated interfaces when testing against the GA capabilities > dump. If testing against QEMU git snapshot capabilities, we can > emit a warning only. > > That way, we'll have a grace period in which libvirt can see the > warning from tests, and we would only need to add an override > to silence it once we import the GA capabilities, at which time > it is safe to implement the new solution too. As I've noted in my reply to Markus, in many cases we develop features on top of unreleased qemu features. I don't really want to throw a spanner into that process. While deprecating old interfaces which are changed would lead to regression in behaviour rather than releasing a broken feature if changes are done in the API of a new feature the situation is slightly worse, but it's also a very rare occurence. I want to basically just an agreement that after such a thing is done libvirt will be notified. We'll still try to develop replacements sooner which can actually catch problems in the new features sooner than they are released in qemu.
Peter Krempa <pkrempa@redhat.com> writes: > On Fri, Nov 27, 2020 at 16:44:05 +0100, Markus Armbruster wrote: >> Peter Krempa <pkrempa@redhat.com> writes: >> >> > On Fri, Nov 27, 2020 at 14:45:12 +0300, Roman Bolshakov wrote: >> >> On Fri, Nov 27, 2020 at 12:21:54PM +0100, Peter Krempa wrote: > > [...] > >> > As you can see this has an issue when we have to add support for a >> > unreleased interface, which may change during the dev cycle or plainly >> > forget that something got deprecated as we've already added an override. >> > >> > This mainly comes from libvirt trying to keep on top of the changes so >> > we refresh the QMP schema during qemu's dev cycle multiple times. >> > >> > Obviously the argument that we try to depend on unreleased functionality >> > can be used, but that would be to detrement of progress IMO. >> >> I understand your concerns. >> >> We have a somewhat similar problem in QEMU: there's nothing to remind us >> later on that the old interface still needs to be deprecated. > > Oh, yes. That's basically the same thing. > > The thing is that changes to the new interface are very rare, but very > real. Since I don't really want to increase the burden for any normal > scenario. > > I'd also very much like to keep libvirt pulling in snapshots of qemu's > capabilities/qapi schema etc throughout the development cycle. It allows > us to adapt faster and develop features simultaneously. > > This unfortunately undermines my own arguments partially as libvirt > regularly develops features on top of unreleased qemu features so we are > regularly risking very similar circumstances. The small but important > difference though is, that releasing a broken feature is not as bad as > breaking an existing feature. > > As a conclusion of the above I'd basically prefer a sort of gentleman's > agreement, that new APIs become 'somewhat' stable at the moment the > commit deprecating the old interface hits upstream. > > The 'somewhat'-stable API would mean that any changes to the new API > should be consulted with libvirt so that we can either give a go-ahead > that we didn't adapt yet, disable the adaptation until the changes can > be done, or another compromise depending on what's the state. > > I know it's hard to enforce, but probably the cheapest in terms of > drawbacks any other solution would be. We can and should try. Can we flag problematic interface changes automatically? Semantic changes, no. What about changes visible in query-qmp-schema? > I'll probably keep notifying patchsets which implement and deprecate old > api at the same time to keep in mind that we need to be kept in touch, > but I really don't want to impose any kind of extra process to > development on either side. Thanks!
On Mon, Nov 30, 2020 at 10:21:08 +0100, Markus Armbruster wrote: > Peter Krempa <pkrempa@redhat.com> writes: > > > On Fri, Nov 27, 2020 at 16:44:05 +0100, Markus Armbruster wrote: > >> Peter Krempa <pkrempa@redhat.com> writes: [...] > > I know it's hard to enforce, but probably the cheapest in terms of > > drawbacks any other solution would be. > > We can and should try. > > Can we flag problematic interface changes automatically? Semantic > changes, no. What about changes visible in query-qmp-schema? I don't know the internals of qemu good enough, but from the perspective of an user of 'query-qmp-schema' it might be possible but not without additional tooling. The output of query-qmp-schema is basically unreviewable when we are updating it. A small change in the schema may trigger a re-numbering of the internal type names so the result is a giant messy piece of JSON where it's impossible to differentiate changes from churn. I've played with generating/expanding all the possibilites and thus stripping the internal numbering for a tool which would simplify writing the query strings (a libvirtism for querying whether the QMP schema has something [1]). This tool could be used in this case to generate a fully expanded and sorted list which should in most cases be append only when new stuff is added. This could be then used to see whether something changed when we'd store the output and compare it against the new one. Unfortunately that would just make query-qmp-schema dumps more reviewable in libvirt though. A change in an interface would be noticed only after it hits qemu upstream. [1] https://gitlab.com/libvirt/libvirt/-/blob/08ae9e5f40f8bae0c3cf48f84181884ddd310fa0/src/qemu/qemu_qapi.c#L392 https://gitlab.com/libvirt/libvirt/-/blob/08ae9e5f40f8bae0c3cf48f84181884ddd310fa0/src/qemu/qemu_capabilities.c#L1512
On 11/27/20 10:30 AM, Peter Krempa wrote: > On Fri, Nov 27, 2020 at 16:44:05 +0100, Markus Armbruster wrote: >> Peter Krempa <pkrempa@redhat.com> writes: >> >>> On Fri, Nov 27, 2020 at 14:45:12 +0300, Roman Bolshakov wrote: >>>> On Fri, Nov 27, 2020 at 12:21:54PM +0100, Peter Krempa wrote: > > [...] > >>> As you can see this has an issue when we have to add support for a >>> unreleased interface, which may change during the dev cycle or plainly >>> forget that something got deprecated as we've already added an override. >>> >>> This mainly comes from libvirt trying to keep on top of the changes so >>> we refresh the QMP schema during qemu's dev cycle multiple times. >>> >>> Obviously the argument that we try to depend on unreleased functionality >>> can be used, but that would be to detrement of progress IMO. >> >> I understand your concerns. >> >> We have a somewhat similar problem in QEMU: there's nothing to remind us >> later on that the old interface still needs to be deprecated. > > Oh, yes. That's basically the same thing. > > The thing is that changes to the new interface are very rare, but very > real. Since I don't really want to increase the burden for any normal > scenario. Case in point: our last-minute changes to block-export-add in qemu commit cbad81ce. The original deprecation of nbd-server-add occurred much earlier in the 5.2 devel cycle, in qemu 443127e8, and also forgot to tell libvirt (https://www.redhat.com/archives/libvir-list/2020-October/msg00855.html); then in my efforts to improve qemu-nbd, I made more changes to block-export-add, but didn't cc libvirt on v1. We eventually got everything coordinated with libvirt in cc, but it did lead to some last minute churn in libvirt to avoid a parity mismatch between versions (https://www.redhat.com/archives/libvir-list/2020-October/msg01369.html). > > I'd also very much like to keep libvirt pulling in snapshots of qemu's > capabilities/qapi schema etc throughout the development cycle. It allows > us to adapt faster and develop features simultaneously. > > This unfortunately undermines my own arguments partially as libvirt > regularly develops features on top of unreleased qemu features so we are > regularly risking very similar circumstances. The small but important > difference though is, that releasing a broken feature is not as bad as > breaking an existing feature. > > As a conclusion of the above I'd basically prefer a sort of gentleman's > agreement, that new APIs become 'somewhat' stable at the moment the > commit deprecating the old interface hits upstream. Yes, moving towards this goal makes sense. And because we've called attention to the fact, I'll try harder to remember in my qapi reviews any time where a new interface exists _because_ it has replaced an interface we already marked as deprecated. > > The 'somewhat'-stable API would mean that any changes to the new API > should be consulted with libvirt so that we can either give a go-ahead > that we didn't adapt yet, disable the adaptation until the changes can > be done, or another compromise depending on what's the state. > > I know it's hard to enforce, but probably the cheapest in terms of > drawbacks any other solution would be. > > I'll probably keep notifying patchsets which implement and deprecate old > api at the same time to keep in mind that we need to be kept in touch, > but I really don't want to impose any kind of extra process to > development on either side. >
Peter Krempa <pkrempa@redhat.com> writes: > On Mon, Nov 30, 2020 at 10:21:08 +0100, Markus Armbruster wrote: >> Peter Krempa <pkrempa@redhat.com> writes: >> >> > On Fri, Nov 27, 2020 at 16:44:05 +0100, Markus Armbruster wrote: >> >> Peter Krempa <pkrempa@redhat.com> writes: > > [...] > >> > I know it's hard to enforce, but probably the cheapest in terms of >> > drawbacks any other solution would be. >> >> We can and should try. >> >> Can we flag problematic interface changes automatically? Semantic >> changes, no. What about changes visible in query-qmp-schema? > > I don't know the internals of qemu good enough, but from the perspective > of an user of 'query-qmp-schema' it might be possible but not without > additional tooling. > > The output of query-qmp-schema is basically unreviewable when we are > updating it. A small change in the schema may trigger a re-numbering of > the internal type names so the result is a giant messy piece of JSON > where it's impossible to differentiate changes from churn. A structural comparison could fare better. qapi-gen option -u might help: -u, --unmask-non-abi-names expose non-ABI names in introspection > I've played with generating/expanding all the possibilites and thus > stripping the internal numbering for a tool which would simplify writing > the query strings (a libvirtism for querying whether the QMP schema has > something [1]). This tool could be used in this case to generate a fully > expanded and sorted list which should in most cases be append only when > new stuff is added. This could be then used to see whether something > changed when we'd store the output and compare it against the new one. Such an expansion is one way to compare structurally. It reports differences for "command C, argument A.B...". Mapping to the QAPI schema is straightforward. > Unfortunately that would just make query-qmp-schema dumps more > reviewable in libvirt though. A change in an interface would be noticed > only after it hits qemu upstream. Yes, implementing the comparison in the QEMU repository would be more useful. > [1] https://gitlab.com/libvirt/libvirt/-/blob/08ae9e5f40f8bae0c3cf48f84181884ddd310fa0/src/qemu/qemu_qapi.c#L392 > https://gitlab.com/libvirt/libvirt/-/blob/08ae9e5f40f8bae0c3cf48f84181884ddd310fa0/src/qemu/qemu_capabilities.c#L1512
diff --git a/qapi/machine.json b/qapi/machine.json index 5648d8d24d..130b0dbebc 100644 --- a/qapi/machine.json +++ b/qapi/machine.json @@ -579,6 +579,9 @@ # # Returns information about KVM acceleration # +# Features: +# @deprecated: This command is deprecated, use 'query-accel' instead. +# # Returns: @AccelInfo # # Since: 0.14.0 @@ -589,7 +592,8 @@ # <- { "return": { "enabled": true, "present": true } } # ## -{ 'command': 'query-kvm', 'returns': 'AccelInfo' } +{ 'command': 'query-kvm', 'returns': 'AccelInfo', + 'features': [ 'deprecated' ] } ## # @query-accel:
'query-accel' QMP command should be used instead. Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com> --- qapi/machine.json | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)