| Message ID | 249a9ca557108397b313625593bc83f161f32a16.1568735079.git.pkrempa@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qapi: Add detection for the 'savevm' fix for blockdev | expand |
On 9/17/19 10:49 AM, Peter Krempa wrote: > savevm was buggy as it considered all monitor owned block device nodes > for snapshot. With introduction of -blockdev the common usage made all > nodes including protocol nodes monitor owned and thus considered for > snapshot. This was fixed but clients need to be able to detect whether > this fix is present. > > Since savevm does not have an QMP alternative add the feature for the > 'human-monitor-command' backdoor which is used to call this command in > modern use. > > Signed-off-by: Peter Krempa <pkrempa@redhat.com> > --- > qapi/misc.json | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/qapi/misc.json b/qapi/misc.json > index 6bd11f50e6..e2b33c3f8a 100644 > --- a/qapi/misc.json > +++ b/qapi/misc.json > @@ -1020,6 +1020,11 @@ > # > # @cpu-index: The CPU to use for commands that require an implicit CPU > # > +# Features: > +# @savevm-blockdev-monitor-nodes: If present, the savevm monitor command > +# correctly handles monitor owned block nodes > +# when taking a snapshot. Is it worth adding a '(since 4.2)' on when features are added? > +# > # Returns: the output of the command as a string > # > # Since: 0.14.0 > @@ -1047,7 +1052,8 @@ > ## > { 'command': 'human-monitor-command', > 'data': {'command-line': 'str', '*cpu-index': 'int'}, > - 'returns': 'str' } > + 'returns': 'str', > + 'features' : [ { 'name': 'savevm-blockdev-monitor-nodes' } ] } We could, of course, actually implement a QMP 'savevm' and use _that_ as the introspection. But that's a bigger can of worms, so this is reasonable enough for the 4.2 timeframe.
Am 17.09.2019 um 18:33 hat Eric Blake geschrieben: > On 9/17/19 10:49 AM, Peter Krempa wrote: > > savevm was buggy as it considered all monitor owned block device nodes > > for snapshot. With introduction of -blockdev the common usage made all > > nodes including protocol nodes monitor owned and thus considered for > > snapshot. This was fixed but clients need to be able to detect whether > > this fix is present. > > > > Since savevm does not have an QMP alternative add the feature for the > > 'human-monitor-command' backdoor which is used to call this command in > > modern use. > > > > Signed-off-by: Peter Krempa <pkrempa@redhat.com> > > --- > > qapi/misc.json | 8 +++++++- > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/qapi/misc.json b/qapi/misc.json > > index 6bd11f50e6..e2b33c3f8a 100644 > > --- a/qapi/misc.json > > +++ b/qapi/misc.json > > @@ -1020,6 +1020,11 @@ > > # > > # @cpu-index: The CPU to use for commands that require an implicit CPU > > # > > +# Features: > > +# @savevm-blockdev-monitor-nodes: If present, the savevm monitor command > > +# correctly handles monitor owned block nodes > > +# when taking a snapshot. > > Is it worth adding a '(since 4.2)' on when features are added? I would also rather describe the change in behaviour ("only includes monitor owned block nodes if they have no parents") than saying that the behaviour is now correct. (Not the least because we know that the current way still isn't quite correct, just hopefully correct enough: Block job BlockBackends are currently snapshotted, which is unintended and will be changed in the future. However, in practice people probably won't use block jobs on non-root nodes and internal snapshots together.) > > +# > > # Returns: the output of the command as a string > > # > > # Since: 0.14.0 > > @@ -1047,7 +1052,8 @@ > > ## > > { 'command': 'human-monitor-command', > > 'data': {'command-line': 'str', '*cpu-index': 'int'}, > > - 'returns': 'str' } > > + 'returns': 'str', > > + 'features' : [ { 'name': 'savevm-blockdev-monitor-nodes' } ] } > > We could, of course, actually implement a QMP 'savevm' and use _that_ as > the introspection. But that's a bigger can of worms, so this is > reasonable enough for the 4.2 timeframe. I think a QMP savevm would sidestep the whole issue by taking an explicit list of nodes to snapshot, and an explicit option to tell which node to store the vmstate on. Kevin
On Wed, Sep 18, 2019 at 10:22:13 +0200, Kevin Wolf wrote: > Am 17.09.2019 um 18:33 hat Eric Blake geschrieben: > > On 9/17/19 10:49 AM, Peter Krempa wrote: > > > savevm was buggy as it considered all monitor owned block device nodes > > > for snapshot. With introduction of -blockdev the common usage made all > > > nodes including protocol nodes monitor owned and thus considered for > > > snapshot. This was fixed but clients need to be able to detect whether > > > this fix is present. > > > > > > Since savevm does not have an QMP alternative add the feature for the > > > 'human-monitor-command' backdoor which is used to call this command in > > > modern use. > > > > > > Signed-off-by: Peter Krempa <pkrempa@redhat.com> > > > --- > > > qapi/misc.json | 8 +++++++- > > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > > > diff --git a/qapi/misc.json b/qapi/misc.json > > > index 6bd11f50e6..e2b33c3f8a 100644 > > > --- a/qapi/misc.json > > > +++ b/qapi/misc.json > > > @@ -1020,6 +1020,11 @@ > > > # > > > # @cpu-index: The CPU to use for commands that require an implicit CPU > > > # > > > +# Features: > > > +# @savevm-blockdev-monitor-nodes: If present, the savevm monitor command > > > +# correctly handles monitor owned block nodes > > > +# when taking a snapshot. > > > > Is it worth adding a '(since 4.2)' on when features are added? > > I would also rather describe the change in behaviour ("only includes > monitor owned block nodes if they have no parents") than saying that the > behaviour is now correct. That's a good idea. I'll post it in a v2 if required. > (Not the least because we know that the current way still isn't quite > correct, just hopefully correct enough: Block job BlockBackends are > currently snapshotted, which is unintended and will be changed in the > future. However, in practice people probably won't use block jobs on > non-root nodes and internal snapshots together.) > > > > +# > > > # Returns: the output of the command as a string > > > # > > > # Since: 0.14.0 > > > @@ -1047,7 +1052,8 @@ > > > ## > > > { 'command': 'human-monitor-command', > > > 'data': {'command-line': 'str', '*cpu-index': 'int'}, > > > - 'returns': 'str' } > > > + 'returns': 'str', > > > + 'features' : [ { 'name': 'savevm-blockdev-monitor-nodes' } ] } > > > > We could, of course, actually implement a QMP 'savevm' and use _that_ as > > the introspection. But that's a bigger can of worms, so this is > > reasonable enough for the 4.2 timeframe. > > I think a QMP savevm would sidestep the whole issue by taking an > explicit list of nodes to snapshot, and an explicit option to tell which > node to store the vmstate on. A straight replacement for savevm would be quite pointless and also such discussion took already place multiple times in the past. The result always was that we need something better than just a qmp version of savevm. I'll be happy to implement the libvirt support for the "new" savevm if it ever appears.
diff --git a/qapi/misc.json b/qapi/misc.json index 6bd11f50e6..e2b33c3f8a 100644 --- a/qapi/misc.json +++ b/qapi/misc.json @@ -1020,6 +1020,11 @@ # # @cpu-index: The CPU to use for commands that require an implicit CPU # +# Features: +# @savevm-blockdev-monitor-nodes: If present, the savevm monitor command +# correctly handles monitor owned block nodes +# when taking a snapshot. +# # Returns: the output of the command as a string # # Since: 0.14.0 @@ -1047,7 +1052,8 @@ ## { 'command': 'human-monitor-command', 'data': {'command-line': 'str', '*cpu-index': 'int'}, - 'returns': 'str' } + 'returns': 'str', + 'features' : [ { 'name': 'savevm-blockdev-monitor-nodes' } ] } ## # @change:
savevm was buggy as it considered all monitor owned block device nodes for snapshot. With introduction of -blockdev the common usage made all nodes including protocol nodes monitor owned and thus considered for snapshot. This was fixed but clients need to be able to detect whether this fix is present. Since savevm does not have an QMP alternative add the feature for the 'human-monitor-command' backdoor which is used to call this command in modern use. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- qapi/misc.json | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)