Congratulations on your first Pull Request and welcome to the Apache CloudStack community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md)
Here are some useful points:

• In case of a new feature add useful documentation (raise doc PR at https://github.com/apache/cloudstack-documentation)
• Be patient and persistent. It might take some time to get a review or get the final approval from the committers.
• Pay attention to the quality of your code, ensure tests are passing and your PR doesn't have conflicts.
• Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Issues, Mailing list and Slack.
• Be sure to read the CloudStack Coding Conventions.
  Apache CloudStack is a community-driven project and together we are making it better 🚀.
  In case of doubts contact the developers at:
  Mailing List: dev@cloudstack.apache.org (https://cloudstack.apache.org/mailing-lists.html)
  Slack: https://apachecloudstack.slack.com/

Codecov Report

Merging #7615 (38f47fb) into 4.18 (939ee9e) will increase coverage by 0.02%.
Report is 71 commits behind head on 4.18.
The diff coverage is 98.27%.

@@             Coverage Diff              @@
##               4.18    #7615      +/-   ##
============================================
+ Coverage     13.02%   13.04%   +0.02%     
- Complexity     9026     9054      +28     
============================================
  Files          2719     2720       +1     
  Lines        256867   257067     +200     
  Branches      40051    40088      +37     
============================================
+ Hits          33445    33534      +89     
- Misses       219233   219339     +106     
- Partials       4189     4194       +5     

... and 34 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

thanks @gpordeus for the fix (or enhancement even) .
Do you think you can codify your testing by adding an integration test for these fixes to make sure we have no regression on those in the future?

is it applicable for 4.18 ?

thanks @gpordeus for the fix (or enhancement even) . Do you think you can codify your testing by adding an integration test for these fixes to make sure we have no regression on those in the future?

@DaanHoogland I have added the integration test.

is it applicable for 4.18 ?

@weizhouapache Yes

thanks @gpordeus for the fix (or enhancement even) . Do you think you can codify your testing by adding an integration test for these fixes to make sure we have no regression on those in the future?

@DaanHoogland I have added the integration test.

good work, i left some comments, but nothing serious.

is it applicable for 4.18 ?

@weizhouapache Yes

can you rebase the change on the 4.18 branch than?

@gpordeus all of a sudden all kinds of unrelated files are changed in your PR. Can you have a look?

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

@weizhouapache a [SF] Trillian-Jenkins test job (rocky8 mgmt + kvm-rocky8) has been kicked to run smoke tests

[SF] Trillian test result (tid-7536)
Environment: kvm-rocky8 (x2), Advanced Networking with Mgmt server r8
Total time taken: 44187 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7615-t7536-kvm-rocky8.zip
Smoke tests completed. 108 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:

[SF] Trillian test result (tid-7537)
Environment: kvm-rocky8 (x2), Advanced Networking with Mgmt server r8
Total time taken: 44249 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7615-t7537-kvm-rocky8.zip
Smoke tests completed. 107 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:

@gpordeus
I can reproduce the issue @vladimirpetrov faced in his testing

• create a env with 2 primary pools, and disable pool primary2
• register a template
• create two vms (A and B)
• force-delete the template
• enable pool primary2
• migrate running vm A to primary2, it succeeds
• stop vm B, and migrate it to primary2, it succeeds
• start vm B, all looks good.
• stop vm A, and start it again, vm A is stuck at booting
  

the qcow2 image does not have a backing file

# qemu-img info -U /mnt/c2876c2e-5312-3cf9-b332-64d597086dc9/1a6593ad-8d02-4b10-a520-3eeefb336b13
image: /mnt/c2876c2e-5312-3cf9-b332-64d597086dc9/1a6593ad-8d02-4b10-a520-3eeefb336b13
file format: qcow2
virtual size: 50 MiB (52428800 bytes)
disk size: 1.06 MiB
cluster_size: 65536
Format specific information:
    compat: 1.1
    compression type: zlib
    lazy refcounts: false
    refcount bits: 16
    corrupt: false
    extended l2: false

image of vm B does not have backing file either, but it works (the disk size is much bigger than vm A)

# qemu-img info -U /mnt/c2876c2e-5312-3cf9-b332-64d597086dc9/408948d1-cedd-4de4-87a9-311501343000
image: /mnt/c2876c2e-5312-3cf9-b332-64d597086dc9/408948d1-cedd-4de4-87a9-311501343000
file format: qcow2
virtual size: 50 MiB (52428800 bytes)
disk size: 41.6 MiB
cluster_size: 65536
Format specific information:
    compat: 1.1
    compression type: zlib
    lazy refcounts: false
    refcount bits: 16
    corrupt: false
    extended l2: false

@DaanHoogland @rohityadavcloud @gpordeus

my summary: if template is removed,

Before this PR,

• when migrate volume of stopped vm, it succeeds. vm also works after starting
• when migrate a running vm with volumes, got an NPE
  Failed to migrate VM [VM instance {"id":3,"instanceName":"i-2-3-VM","type":"User","uuid":"1d3541d7-187e-417f-9cd4-cd18d0fc1e3d"}] along with its volumes due to [java.lang.NullPointerException].

With this PR:

• the NPE issue in live storage migration is fixed
• However, vm is corrupted after live storage migration

I would rather get an NPE exception than vm corruption , as data loss is a more critical issue for users.

moved to 4.18.2.0

@gpordeus I can reproduce the issue @vladimirpetrov faced in his testing

image of vm B does not have backing file either, but it works (the disk size is much bigger than vm A)

@weizhouapache Great, thank you for testing and for pointing out the disk size. I hadn't been able to replicate it before last week because I was testing from and to iSCSI and NFS storages, while this error only happens in NFS to NFS.

@DaanHoogland @rohityadavcloud @gpordeus

my summary: if template is removed,

Before this PR,

* when migrate volume of stopped vm, it succeeds. vm also works after starting

* when migrate a running vm with volumes, got an NPE
  Failed to migrate VM [VM instance {"id":3,"instanceName":"i-2-3-VM","type":"User","uuid":"1d3541d7-187e-417f-9cd4-cd18d0fc1e3d"}] along with its volumes due to [java.lang.NullPointerException].

With this PR:

* the NPE issue in live storage migration is fixed

* However, vm is corrupted after live storage migration

I would rather get an NPE exception than vm corruption , as data loss is a more critical issue for users.

@gpordeus can you please fix the vm corruption issue in our testing ?

Yes, I agree and I'm already working on it. The problem is a flag that is passed to libvirt migration only when it's NFS to NFS, for some reason, which makes it so that only the last layer of the vm is copied and, since the template was deleted and wasn't copied first, it corrupts them. Getting rid of the flag makes it work, but I'm testing if there are any other side effects or if there is a better way of going about it.

@gpordeus any progress? can we add this to 4.19?

@gpordeus any progress? can we add this to 4.19?

@DaanHoogland Sorry for the delay, it's being a little trickier than I anticipated. It won't take much longer, but not enough to make it to 4.19.

@gpordeus when you get to this, will it include a fix for #5759 ?

@gpordeus should we still try to merge this? (as 4.18 is coming to end of maintemnance)

@gpordeus should we still try to merge this? (as 4.18 is coming to end of maintemnance)

@DaanHoogland I suppose not, especially with #8911. I'm closing this, but I'll still be working on #5759. I was sidetracked, but I'll give it priority now.