Codecov Report

Attention: Patch coverage is 0% with 10 lines in your changes missing coverage. Please review.

Project coverage is 33.08%. Comparing base (c25264b) to head (4927f24).
Report is 164 commits behind head on 4.19.

@@             Coverage Diff              @@
##               4.19    #8887      +/-   ##
============================================
+ Coverage     30.98%   33.08%   +2.10%     
+ Complexity    34408    34302     -106     
============================================
  Files          5359     4998     -361     
  Lines        377017   348283   -28734     
  Branches      54856    49849    -5007     
============================================
- Hits         116804   115238    -1566     
+ Misses       244852   217867   -26985     
+ Partials      15361    15178     -183     

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@blueorangutan package

@sureshanaparti a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 9174

@blueorangutan test

@DaanHoogland a [SL] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

[SF] Trillian test result (tid-9748)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 48556 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8887-t9748-kvm-centos7.zip
Smoke tests completed. 126 look OK, 3 have errors, 0 did not run
Only failed and skipped tests results shown below:

So I got new logs and found the underlying issue.
This PR is probably obsolete, the real problem is that actions (I have to further analyze) created 2 same storage pool NFS
objects and the 1. one deleted the mount point of the other.

rg "(create|remove) storage pool 59eb8ccb-e7bb-3f5e-a0a7-9f7cf7dfadc0" agent.log
305216:2024-04-08 19:46:18,231 INFO  [kvm.storage.LibvirtStorageAdaptor] (agentRequest-Handler-5:null) (logid:0a2b21c6) Attempting to create storage pool 59eb8ccb-e7bb-3f5e-a0a7-9f7cf7dfadc0 (NetworkFilesystem) in libvirt
305645:2024-04-08 19:48:05,184 INFO  [kvm.storage.LibvirtStorageAdaptor] (agentRequest-Handler-2:null) (logid:57cbaf10) Attempting to create storage pool 59eb8ccb-e7bb-3f5e-a0a7-9f7cf7dfadc0 (NetworkFilesystem) in libvirt
305919:2024-04-08 19:49:07,622 INFO  [kvm.storage.LibvirtStorageAdaptor] (agentRequest-Handler-5:null) (logid:0a2b21c6) Attempting to remove storage pool 59eb8ccb-e7bb-3f5e-a0a7-9f7cf7dfadc0 from libvirt
306306:2024-04-08 19:50:46,508 INFO  [kvm.storage.LibvirtStorageAdaptor] (agentRequest-Handler-2:null) (logid:57cbaf10) Attempting to remove storage pool 59eb8ccb-e7bb-3f5e-a0a7-9f7cf7dfadc0 from libvirt

@DaanHoogland
Do you know if there is a policy regarding requesting the same multiple times?
Should this never happen and a safe guard exception in place?

Further details for this specific case:

1. Linstor is asked to createAsync Volume from Snapshot
2. Linstor is creating that from the management server call
3. CloudStack sends a CopyCommand to some host (src the just created Linstor resource, target the new template in secondary storage)

If 3. takes some time and you do the same action it happens that the 3. step happens on the same host, but the previous CopyCommand finishes and removes the same NFS secondary storage mount that the 2. CopyCommand would use.

@rp- , I am not sure. I think that the same command running twice should be prevented by the executeInSequence flag. As the host is handled by a single agent, this should prevent what I understand from your description.
createAsync is in contradiction with this.
If the same mount on the same mountpoint happening is an issue, it is new to me and it sounds like it must be guarded against in the agent.

any opinion @harikrishna-patnala , @sureshanaparti , @weizhouapache ?

I could reproduce this today in our lab and created a real issue #8899 out of it, with DEBUG logs.

It still might be that the Linstor driver is the only one, who triggers this kind of storagemotionstrategy, but I'm not experienced enough with CloudStack to know for sure.

I also think we can close this PR, as it would probably never really trigger anyway.
As the real problem is somewhere else, as I see now.

@rp- I see your point about the actual issue with copycommand, though we need to verify with other storages also. Regarding this PR, I feel this check is still valid, may be you can combine this and the fix for the issue #8899

Would it be acceptable to "mark" a mount point as such, and make it immutable?
The goal is to make it impossible to "accidentally" create stuff inside the mount point that should have been created inside the supposedly mounted file system.
The idea is to place an indicator file into the mount point directory (not the mounted file system), remove all permissions from the mount point (that will be overridden by the actual mount, once that happens), and make it immutable just in case (because root tends to ignore permissions).
If you like it, it should probably be done more generically for all mkdird mount points, and not only for libvirt nfs storage pools, but I don't know my way around cloud stack code good enough yet to find the "proper" location for this.
Suggestion below. I'm not Java native, so consider this "pseudo code" if it is not sufficiently java-ish...

diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java
index 3002fea41d..0afc232828 100644
--- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java
+++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java
@@ -32,6 +32,7 @@ import org.apache.cloudstack.utils.qemu.QemuImgException;
 import org.apache.cloudstack.utils.qemu.QemuImgFile;
 import org.apache.cloudstack.utils.qemu.QemuObject;
 import org.apache.commons.codec.binary.Base64;
+import org.apache.commons.lang.SystemUtils;
 import org.apache.logging.log4j.Logger;
 import org.apache.logging.log4j.LogManager;
 import org.libvirt.Connect;
@@ -276,7 +277,29 @@ public class LibvirtStorageAdaptor implements StorageAdaptor {
     private StoragePool createNetfsStoragePool(PoolType fsType, Connect conn, String uuid, String host, String path) throws LibvirtException {
         String targetPath = _mountPoint + File.separator + uuid;
         LibvirtStoragePoolDef spd = new LibvirtStoragePoolDef(fsType, uuid, uuid, host, path, targetPath);
-        _storageLayer.mkdir(targetPath);
+        if (SystemUtils.IS_OS_LINUX) {
+		// targetPath better not contain funny characters!
+		// maybe s/'/'\''/g first?
+                int createMntPointResult = Script.runSimpleBashScriptForExitValue(
+			// That is a directory already? Good enough.
+                        "! test -L '" + targetPath + "' && test -d '"  + targetPath + "' && exit 0;" +
+                        // we really want that mount point
+                        "mkdir -p '"  + targetPath + "' || exit;" +
+                        // optionally /try/ to mark it as mount point and make it immutable,
+                        // so we won't "accidentally" create stuff inside the mount point
+                        // that should have been created in the mounted file system
+                        "touch '"     + targetPath + "/__MOUNT_POINT_ONLY__'; " +
+                        "chmod 0 '"   + targetPath + "'; " +
+                        "chattr +i '" + targetPath + "'; " +
+                        // mkdir was successful: propagate exit code 0
+                        "exit 0");
+		if (createMntPointResult != 0) {
+                    logger.error("Unable to create mount point " + targetPath);
+		    // What now? Just let Libvirt try itself and fail below.
+		}
+        } else {
+                _storageLayer.mkdir(targetPath);
+        }
         StoragePool sp = null;
         try {
             logger.debug(spd.toString());

Is libvirt on non IS_OS_LINUX even a thing?

Possibly check for existence of the directory in a java native way first, or use JNA, if calling the shell looks too heavy.

Bonus points to avoid the race between the first one to "mkdir", someone else winning the race to mount some file system on that mount point, and then the chattr +i happening on the top level directory of the mounted file system instead of the mount point ;-)

Is this good to merge @rp- @sureshanaparti @DaanHoogland @shwstppr @weizhouapache ?

Is this good to merge @rp- @sureshanaparti @DaanHoogland @shwstppr @weizhouapache ?

this is purely linstor ; if @rp- says so and smoke tests pass it should be good to go. however, this does not address @rp- 's point about multiple jobs requiring the same mount and umounting it while the other is still busy.

As @DaanHoogland said, it doesn't fix the real issue, but somewhat helps to identify it and also fail instead of silently write into a local folder and sometimes "creating templates" will fail...

@blueorangutan package

@sureshanaparti a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 10067

@blueorangutan test

@sureshanaparti a [SL] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

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

Merging this, based on reviews & test results.