storage: avoid removing the xva/vdi before going in the debugger

[glehmann]

In case of exception, the cleanup code in try…finally was removing the
xva image before getting in the debugger, making impossible to look at
the xva content.

This PR is part of a tree containing 19 PRs:

1. master
2. "storage: avoid removing the xva/vdi before going in the debugger" (this PR) → master
3. storage: test large volumes #437 → storage: avoid removing the xva/vdi before going in the debugger #436
4. host: include mdadm RAID devices in disk detection #447 → storage: test large volumes #437
5. Update randstream to 0.5.0 #446 → host: include mdadm RAID devices in disk detection #447
6. storage: Avoid writing the whole device in coalesce tests #449 → Update randstream to 0.5.0 #446
7. storage: avoid writing the whole device in migration tests #450 → storage: Avoid writing the whole device in coalesce tests #449
8. xva/vdi: only write a small data amount in large volumes for faster tests #452 → storage: avoid writing the whole device in migration tests #450
9. storage: test full device write #453 → xva/vdi: only write a small data amount in large volumes for faster tests #452
10. storage: test that we can't create a vdi over its max allowed size #454 → storage: test full device write #453
11. storage: add jobs for large volume tests #461 → storage: test that we can't create a vdi over its max allowed size #454
12. skip large volume tests for zvol and nfsv4 #464 → storage: add jobs for large volume tests #461
13. storage: add comprehensive tests for lvmohba storage repositories #470 → skip large volume tests for zvol and nfsv4 #464
14. storage: free space for XVA import by destroying source VM first #471 → storage: add comprehensive tests for lvmohba storage repositories #470
15. storage: limit data written per VDI with --write-volume-cap #481 → storage: free space for XVA import by destroying source VM first #471
16. Add Packer configuration to build a minimal Alpine 3.23 UEFI VM for XCP-ng tests #523 → storage: limit data written per VDI with --write-volume-cap #481
17. migration: create a xfs sr on the second host for intra/cross-pool migration #497 → storage: limit data written per VDI with --write-volume-cap #481
18. Enhance block device management #498 → migration: create a xfs sr on the second host for intra/cross-pool migration #497
19. Add VHD_MAX and QCOW2_MAX symbolic size constants #500 → Enhance block device management #498
20. partially_populate_device: align span positions to block size for better performance #509 → Add VHD_MAX and QCOW2_MAX symbolic size constants #500

[rzr]

nitpick: some newlines would help readability

[glehmann]

I could agree, but I won't start arguing about coding style :)

[stormi]

Nitpick 2 : so much reformatting in commit 2 that I had difficulties finding the actual changes.

[stormi]

Meld does a better job than github, thankfully

[glehmann]

Ok, so we are talking about coding style.

nitpick: some newlines would help readability

In this code base, I think the coding style tends to keep things vertically compact. Newlines would go against that.

Nitpick 2 : so much reformatting in commit 2 that I had difficulties finding the actual changes.

Indeed github is not very good at that.
The lines became longer than our 120-character limit. Would you have preferred to keep the first part of the parameters on the same line? The line would still have been modified

    def test_coalesce(self, storage_test_vm: VM, vdi_on_lvm_sr: VDI, vdi_op: CoalesceOperation):

would become

    def test_coalesce(self, storage_test_vm: VM, vdi_on_lvm_sr: VDI, vdi_op: CoalesceOperation,
                      defer: Defer):

[stormi]

With a good diff tool, the current commit is fine for me. But the "onboarding" when opening commit 2 in github was not the best, and it's not necessarily on you :)

[glehmann]

I reordered the commit to use defer directly and avoid reaching the 120-character limit in most cases, and minimized changes as much as possible.

[stormi]

It would be nice to describe the defer fixture in the commit message. Here all the details are in the documentation of the fixture, which is nice in itself, but at first I was confused by the diff, and people from the future looking for the origin of this change might be do. Something as simple as inviting to check the doc of the defer fixture in the global conftest.py would be enough.

[glehmann]

done

[stormi]

Move this up with the other type definitions?

[glehmann]

done

[stormi]

Minor comments. Overall it's a clear improvement vs try... finally structures in tests.

Let's however continue to strive to keep this an exception for things that we can't nicely handle via fixtures.

[Lankou66]

LGTM