Repository navigation
libvirtstorage: Make sure netfs storage was really mounted - #8887
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ 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. |
|
@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)
|
|
So I got new logs and found the underlying issue. @DaanHoogland |
|
Further details for this specific case:
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 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. |
|
Would it be acceptable to "mark" a mount point as such, and make it immutable? 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 |
|
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)
|
|
Merging this, based on reviews & test results. |
Description
I encountered a situation, where libvirt/CloudStack believed a nfs/secondary storage was correctly mounted
and the agent wrote images without any problem into it.
It turned out, that it just wrote everything into a a local /mnt/ directory and later was only found, because
we couldn't create instances(on a other agent) of the just created template.
I don't know yet how the agent/node got into this state, thinking everything was mounted ok, but instead
writing into a local directory.
This should check after creating the mount that it is really mounted and fail if not.
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?