docs/manpages/virsh.rst | 24 +++++---- include/libvirt/libvirt-domain.h | 1 - src/qemu/qemu_driver.c | 6 +-- src/qemu/qemu_migration_params.c | 34 ++++++------ tools/virsh-domain.c | 93 ++++++++++++++++---------------- 5 files changed, 80 insertions(+), 78 deletions(-)
Pavel Hrdina (6): tools: remove --parallel from virsh restore command tools: remote --parallel from virsh save command qemu: remove VIR_DOMAIN_SAVE_PARALLEL flag tools: use virDomainRestoreParams only when necessary tools: use virDomainSaveParams only when necessary virsh: add --image-format option to the save command docs/manpages/virsh.rst | 24 +++++---- include/libvirt/libvirt-domain.h | 1 - src/qemu/qemu_driver.c | 6 +-- src/qemu/qemu_migration_params.c | 34 ++++++------ tools/virsh-domain.c | 93 ++++++++++++++++---------------- 5 files changed, 80 insertions(+), 78 deletions(-) -- 2.48.1
On 3/20/25 17:07, Pavel Hrdina via Devel wrote: > Pavel Hrdina (6): > tools: remove --parallel from virsh restore command > tools: remote --parallel from virsh save command > qemu: remove VIR_DOMAIN_SAVE_PARALLEL flag Heh, I'm having one of those "why did I not realize that" moments :-). > tools: use virDomainRestoreParams only when necessary > tools: use virDomainSaveParams only when necessary Fair enough. > virsh: add --image-format option to the save command Thanks! Although I intended to do this, I forgot to add it to my todo list and would have likely forgot. > > docs/manpages/virsh.rst | 24 +++++---- > include/libvirt/libvirt-domain.h | 1 - > src/qemu/qemu_driver.c | 6 +-- > src/qemu/qemu_migration_params.c | 34 ++++++------ > tools/virsh-domain.c | 93 ++++++++++++++++---------------- > 5 files changed, 80 insertions(+), 78 deletions(-) > For series: Reviewed-by: Jim Fehlig <jfehlig@suse.com> Regards, Jim
On Thu, Mar 20, 2025 at 09:36:15PM -0600, Jim Fehlig via Devel wrote: > On 3/20/25 17:07, Pavel Hrdina via Devel wrote: > > Pavel Hrdina (6): > > tools: remove --parallel from virsh restore command > > tools: remote --parallel from virsh save command > > qemu: remove VIR_DOMAIN_SAVE_PARALLEL flag > > Heh, I'm having one of those "why did I not realize that" moments :-). IIRC, original we were using --parallel to decide whether to enable the new format, so --parallel with channels == 1 /was/ different from not setting --parallel at all. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Fri, Mar 21, 2025 at 08:29:30AM +0000, Daniel P. Berrangé via Devel wrote: >On Thu, Mar 20, 2025 at 09:36:15PM -0600, Jim Fehlig via Devel wrote: >> On 3/20/25 17:07, Pavel Hrdina via Devel wrote: >> > Pavel Hrdina (6): >> > tools: remove --parallel from virsh restore command >> > tools: remote --parallel from virsh save command >> > qemu: remove VIR_DOMAIN_SAVE_PARALLEL flag >> >> Heh, I'm having one of those "why did I not realize that" moments :-). > >IIRC, original we were using --parallel to decide whether to enable >the new format, so --parallel with channels == 1 /was/ different >from not setting --parallel at all. > Well, restore should figure out the format and save still has --image-format without which parallel save will fail (unless you have it set as default in a config). So I think this series is fine, at least it fixes the unit tests. Reviewed-by: Martin Kletzander <mkletzan@redhat.com> But there's more to it than meets the eye. There is also a check that the number of parallel channels is not lower than 1, for saving. Restoring happily takes parallel.channels=0 and (at least for non-sparse images) fails weirdly with: error: Failed to restore domain from test.img error: Failed to open file 'test.img': No such file or directory and the daemon reports: 2025-03-21 10:00:08.490+0000: 4076349: error : virFileIsSharedFSType:3598 : Invalid relative path 'test.img': Invalid argument 2025-03-21 10:00:08.490+0000: 4076349: error : virQEMUFileOpenAs:10448 : Failed to open file 'test.img': No such file or directory even though the file exists. And it works without --parallel-channels, this only happens with --parallel-channels 0. Last, but not least, our CI is broken with the patches here. And that is because now one cannot do save-image-edit, with both the new and the old format with: error: failed to write header to domain save file '/home/nert/test.img': Bad file descriptor And that's about what I've found out. I'll spend some time on this, trying to fix it up, but if anyone has a fix ready, then even better. What I'd like to know is whether we can > >With regards, >Daniel >-- >|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| >|: https://libvirt.org -o- https://fstop138.berrange.com :| >|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| >
On 3/21/25 04:21, Martin Kletzander wrote: > On Fri, Mar 21, 2025 at 08:29:30AM +0000, Daniel P. Berrangé via Devel wrote: >> On Thu, Mar 20, 2025 at 09:36:15PM -0600, Jim Fehlig via Devel wrote: >>> On 3/20/25 17:07, Pavel Hrdina via Devel wrote: >>> > Pavel Hrdina (6): >>> > tools: remove --parallel from virsh restore command >>> > tools: remote --parallel from virsh save command >>> > qemu: remove VIR_DOMAIN_SAVE_PARALLEL flag >>> >>> Heh, I'm having one of those "why did I not realize that" moments :-). >> >> IIRC, original we were using --parallel to decide whether to enable >> the new format, so --parallel with channels == 1 /was/ different >> from not setting --parallel at all. >> > > Well, restore should figure out the format and save still has > --image-format without which parallel save will fail (unless you have it > set as default in a config). So I think this series is fine, at least > it fixes the unit tests. Agreed. The new format is enabled with --image-format parameter or by setting it as default in qemu.conf. And trying to use --parallel-channels without also specifying sparse fails as expected # virsh save --parallel-channels 4 test-vm /data/test-vm.sav error: Failed to save domain 'test-vm' to /data/test-vm.sav error: invalid argument: Parallel save is only supported with the 'sparse' save image format > Reviewed-by: Martin Kletzander <mkletzan@redhat.com> > > But there's more to it than meets the eye. > > There is also a check that the number of parallel channels is not lower > than 1, for saving. Restoring happily takes parallel.channels=0 and (at > least for non-sparse images) fails weirdly with: > > error: Failed to restore domain from test.img > error: Failed to open file 'test.img': No such file or directory > > and the daemon reports: > > 2025-03-21 10:00:08.490+0000: 4076349: error : virFileIsSharedFSType:3598 : > Invalid relative path 'test.img': Invalid argument > 2025-03-21 10:00:08.490+0000: 4076349: error : virQEMUFileOpenAs:10448 : Failed > to open file 'test.img': No such file or directory > > even though the file exists. And it works without --parallel-channels, > this only happens with --parallel-channels 0. Hmm, I don't see this issue # virsh restore --parallel-channels 0 /data/test-vm.sav.sparse error: Failed to restore domain from /data/test-vm.sav.sparse error: invalid argument: number of parallel save channels cannot be less than 1 Could it be that you are using a relative path? Daniel and I just discussed this in patch1 of the V4 mapped-ram series. I'd provide a link, but https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/ is currently down for me. I dropped that patch since we prefer absolute paths. > Last, but not least, our CI is broken with the patches here. And that > is because now one cannot do save-image-edit, with both the new and the > old format with: > > error: failed to write header to domain save file '/home/nert/test.img': Bad > file descriptor I do see this issue and should have time to investigate it tomorrow. Regards, Jim
On 3/21/25 04:21, Martin Kletzander wrote: > On Fri, Mar 21, 2025 at 08:29:30AM +0000, Daniel P. Berrangé via Devel wrote: >> On Thu, Mar 20, 2025 at 09:36:15PM -0600, Jim Fehlig via Devel wrote: >>> On 3/20/25 17:07, Pavel Hrdina via Devel wrote: >>> > Pavel Hrdina (6): >>> > tools: remove --parallel from virsh restore command >>> > tools: remote --parallel from virsh save command >>> > qemu: remove VIR_DOMAIN_SAVE_PARALLEL flag >>> >>> Heh, I'm having one of those "why did I not realize that" moments :-). >> >> IIRC, original we were using --parallel to decide whether to enable >> the new format, so --parallel with channels == 1 /was/ different >> from not setting --parallel at all. >> > > Well, restore should figure out the format and save still has > --image-format without which parallel save will fail (unless you have it > set as default in a config). So I think this series is fine, at least > it fixes the unit tests. > > Reviewed-by: Martin Kletzander <mkletzan@redhat.com> > > But there's more to it than meets the eye. > > There is also a check that the number of parallel channels is not lower > than 1, for saving. Restoring happily takes parallel.channels=0 and (at > least for non-sparse images) fails weirdly with: > > error: Failed to restore domain from test.img > error: Failed to open file 'test.img': No such file or directory > > and the daemon reports: > > 2025-03-21 10:00:08.490+0000: 4076349: error : virFileIsSharedFSType:3598 : > Invalid relative path 'test.img': Invalid argument > 2025-03-21 10:00:08.490+0000: 4076349: error : virQEMUFileOpenAs:10448 : Failed > to open file 'test.img': No such file or directory > > even though the file exists. And it works without --parallel-channels, > this only happens with --parallel-channels 0. Opps, I didn't test that. I did verify save with '--parallel-channels 0' is handled properly. > > Last, but not least, our CI is broken with the patches here. And that > is because now one cannot do save-image-edit, with both the new and the > old format with: > > error: failed to write header to domain save file '/home/nert/test.img': Bad > file descriptor I didn't check save-image-edit :-/. > > And that's about what I've found out. I'll spend some time on this, > trying to fix it up, but if anyone has a fix ready, then even better. I've committed to a task that involves others today, so wont be able to look at these issues until later. > > What I'd like to know is whether we can Did you have more to say? :-) Regards, Jim
© 2016 - 2025 Red Hat, Inc.