From 4eb43651e2d1f5eed6fdcbe57a88ea7f673dc3d8 Mon Sep 17 00:00:00 2001 From: Abhisar Sinha <63767682+abh1sar@users.noreply.github.com> Date: Tue, 25 Jun 2024 23:45:35 +0530 Subject: [PATCH] Ability to specify NFS mount options while adding a primary storage and modify them on a pre-existing primary storage (#8947) * Ability to specify NFS mount options while adding a primary storage and modify it later * Pull 8947: Rename all occurrence of nfsopt to nfsMountOpt and added nfsMountOpts to ApiConstants * Pull 8947: Refactor code - move into separate methods * Pull 8947: CollectionsUtils.isNotEmpty and switch statement in LibvirtStoragePoolDef.java * Pull 8947: UI - cancel maintainenace will remount the storage pool and apply the options * Pull 8947: UI - moved edit NFS mount options to edit Primary Storage form * Pull 8947: UI - moved 'NFS Mount Options' to below 'Type' in dataview * Pull 8947: Fixed message in AddPrimaryStorage.vue * Pull 8947: Convert _nfsmountOpts to Set in libvirtStoragePoolDef * Pull 8947: Throw exception and log error if mount fails due to incorrect mount option * Pull 8947: Added UT and moved integration test to component/maint * Pull 8947: Review comments * Pull 8947: Removed password from integration test * Pull 8947: move details allocation to inside the if loop in getStoragePoolNFSMountOpts * Pull 8947: Fixed a bug in AddPrimaryStorage.vue * Pull 8947: Pool should remain in maintenance mode if mount fails * Pull 8947: Removed password from integration test * Pull 8947: Added UT * Pull 8875: Fixed a bug in CloudStackPrimaryDataStoreLifeCycleImplTest * Pull 8875: Fixed a bug in LibvirtStoragePoolDefTest * Pull 8947: minor code restructuring * Pull 8947 : added some ut for coverage * Fix LibvirtStorageAdapterTest UT --- .../apache/cloudstack/api/ApiConstants.java | 2 + .../api/response/StoragePoolResponse.java | 12 ++ .../agent/api/ModifyStoragePoolCommand.java | 4 + .../com/cloud/storage/StorageManager.java | 5 + .../provider/DefaultHostListener.java | 10 +- .../kvm/resource/LibvirtStoragePoolDef.java | 131 ++++++++---- .../resource/LibvirtStoragePoolXMLParser.java | 21 +- .../kvm/storage/LibvirtStorageAdaptor.java | 55 ++++- .../resource/LibvirtStoragePoolDefTest.java | 43 +++- .../LibvirtStoragePoolXMLParserTest.java | 10 +- .../storage/LibvirtStorageAdaptorTest.java | 91 ++++++++ ...oudStackPrimaryDataStoreLifeCycleImpl.java | 10 +- ...tackPrimaryDataStoreLifeCycleImplTest.java | 21 +- .../com/cloud/api/query/QueryManagerImpl.java | 11 + .../com/cloud/storage/StorageManagerImpl.java | 87 +++++++- .../storage/StoragePoolAutomationImpl.java | 18 +- .../cloud/storage/StorageManagerImplTest.java | 159 ++++++++++++++ .../test_primary_storage_nfsmountopts_kvm.py | 184 +++++++++++++++++ ui/public/locales/en.json | 5 + .../config/section/infra/primaryStorages.js | 5 +- ui/src/views/infra/AddPrimaryStorage.vue | 14 ++ ui/src/views/infra/UpdatePrimaryStorage.vue | 195 ++++++++++++++++++ .../infra/zone/ZoneWizardAddResources.vue | 9 + .../views/infra/zone/ZoneWizardLaunchZone.vue | 1 + 24 files changed, 1037 insertions(+), 66 deletions(-) create mode 100644 plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptorTest.java create mode 100644 test/integration/component/maint/test_primary_storage_nfsmountopts_kvm.py create mode 100644 ui/src/views/infra/UpdatePrimaryStorage.vue diff --git a/api/src/main/java/org/apache/cloudstack/api/ApiConstants.java b/api/src/main/java/org/apache/cloudstack/api/ApiConstants.java index 7512120cc00..f5561a01191 100644 --- a/api/src/main/java/org/apache/cloudstack/api/ApiConstants.java +++ b/api/src/main/java/org/apache/cloudstack/api/ApiConstants.java @@ -1099,6 +1099,8 @@ public class ApiConstants { public static final String PARAMETER_DESCRIPTION_IS_TAG_A_RULE = "Whether the informed tag is a JS interpretable rule or not."; + public static final String NFS_MOUNT_OPTIONS = "nfsmountopts"; + /** * This enum specifies IO Drivers, each option controls specific policies on I/O. * Qemu guests support "threads" and "native" options Since 0.8.8 ; "io_uring" is supported Since 6.3.0 (QEMU 5.0). diff --git a/api/src/main/java/org/apache/cloudstack/api/response/StoragePoolResponse.java b/api/src/main/java/org/apache/cloudstack/api/response/StoragePoolResponse.java index 183290ec9eb..f514c8167ac 100644 --- a/api/src/main/java/org/apache/cloudstack/api/response/StoragePoolResponse.java +++ b/api/src/main/java/org/apache/cloudstack/api/response/StoragePoolResponse.java @@ -101,6 +101,10 @@ public class StoragePoolResponse extends BaseResponseWithAnnotations { @Param(description = "the tags for the storage pool") private String tags; + @SerializedName(ApiConstants.NFS_MOUNT_OPTIONS) + @Param(description = "the nfs mount options for the storage pool", since = "4.19.1") + private String nfsMountOpts; + @SerializedName(ApiConstants.IS_TAG_A_RULE) @Param(description = ApiConstants.PARAMETER_DESCRIPTION_IS_TAG_A_RULE) private Boolean isTagARule; @@ -347,4 +351,12 @@ public class StoragePoolResponse extends BaseResponseWithAnnotations { public void setProvider(String provider) { this.provider = provider; } + + public String getNfsMountOpts() { + return nfsMountOpts; + } + + public void setNfsMountOpts(String nfsMountOpts) { + this.nfsMountOpts = nfsMountOpts; + } } diff --git a/core/src/main/java/com/cloud/agent/api/ModifyStoragePoolCommand.java b/core/src/main/java/com/cloud/agent/api/ModifyStoragePoolCommand.java index ad05fe1d615..06940266b53 100644 --- a/core/src/main/java/com/cloud/agent/api/ModifyStoragePoolCommand.java +++ b/core/src/main/java/com/cloud/agent/api/ModifyStoragePoolCommand.java @@ -46,6 +46,10 @@ public class ModifyStoragePoolCommand extends Command { this.details = details; } + public ModifyStoragePoolCommand(boolean add, StoragePool pool, Map details) { + this(add, pool, LOCAL_PATH_PREFIX + File.separator + UUID.nameUUIDFromBytes((pool.getHostAddress() + pool.getPath()).getBytes()), details); + } + public ModifyStoragePoolCommand(boolean add, StoragePool pool) { this(add, pool, LOCAL_PATH_PREFIX + File.separator + UUID.nameUUIDFromBytes((pool.getHostAddress() + pool.getPath()).getBytes())); } diff --git a/engine/components-api/src/main/java/com/cloud/storage/StorageManager.java b/engine/components-api/src/main/java/com/cloud/storage/StorageManager.java index 13cea12853f..2def1dccced 100644 --- a/engine/components-api/src/main/java/com/cloud/storage/StorageManager.java +++ b/engine/components-api/src/main/java/com/cloud/storage/StorageManager.java @@ -18,6 +18,7 @@ package com.cloud.storage; import java.math.BigDecimal; import java.util.List; +import java.util.Map; import org.apache.cloudstack.engine.subsystem.api.storage.DataStore; import org.apache.cloudstack.engine.subsystem.api.storage.HypervisorHostListener; @@ -341,6 +342,10 @@ public interface StorageManager extends StorageService { boolean registerHostListener(String providerUuid, HypervisorHostListener listener); + Pair, Boolean> getStoragePoolNFSMountOpts(StoragePool pool, Map details); + + String getStoragePoolMountFailureReason(String error); + boolean connectHostToSharedPool(long hostId, long poolId) throws StorageUnavailableException, StorageConflictException; void disconnectHostFromSharedPool(long hostId, long poolId) throws StorageUnavailableException, StorageConflictException; diff --git a/engine/storage/volume/src/main/java/org/apache/cloudstack/storage/datastore/provider/DefaultHostListener.java b/engine/storage/volume/src/main/java/org/apache/cloudstack/storage/datastore/provider/DefaultHostListener.java index a453f2d48c2..ea1e524f5d6 100644 --- a/engine/storage/volume/src/main/java/org/apache/cloudstack/storage/datastore/provider/DefaultHostListener.java +++ b/engine/storage/volume/src/main/java/org/apache/cloudstack/storage/datastore/provider/DefaultHostListener.java @@ -42,7 +42,9 @@ import com.cloud.storage.StoragePool; import com.cloud.storage.StoragePoolHostVO; import com.cloud.storage.StorageService; import com.cloud.storage.dao.StoragePoolHostDao; +import com.cloud.utils.Pair; import com.cloud.utils.exception.CloudRuntimeException; + import org.apache.cloudstack.engine.subsystem.api.storage.DataStoreManager; import org.apache.cloudstack.engine.subsystem.api.storage.HypervisorHostListener; import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao; @@ -53,7 +55,9 @@ import org.apache.commons.lang3.StringUtils; import org.apache.log4j.Logger; import javax.inject.Inject; + import java.util.List; +import java.util.Map; public class DefaultHostListener implements HypervisorHostListener { private static final Logger s_logger = Logger.getLogger(DefaultHostListener.class); @@ -125,7 +129,9 @@ public class DefaultHostListener implements HypervisorHostListener { @Override public boolean hostConnect(long hostId, long poolId) throws StorageConflictException { StoragePool pool = (StoragePool) this.dataStoreMgr.getDataStore(poolId, DataStoreRole.Primary); - ModifyStoragePoolCommand cmd = new ModifyStoragePoolCommand(true, pool); + Pair, Boolean> nfsMountOpts = storageManager.getStoragePoolNFSMountOpts(pool, null); + + ModifyStoragePoolCommand cmd = new ModifyStoragePoolCommand(true, pool, nfsMountOpts.first()); cmd.setWait(modifyStoragePoolCommandWait); s_logger.debug(String.format("Sending modify storage pool command to agent: %d for storage pool: %d with timeout %d seconds", hostId, poolId, cmd.getWait())); @@ -138,7 +144,7 @@ public class DefaultHostListener implements HypervisorHostListener { if (!answer.getResult()) { String msg = "Unable to attach storage pool" + poolId + " to the host" + hostId; alertMgr.sendAlert(AlertManager.AlertType.ALERT_TYPE_HOST, pool.getDataCenterId(), pool.getPodId(), msg, msg); - throw new CloudRuntimeException("Unable establish connection from storage head to storage pool " + pool.getId() + " due to " + answer.getDetails() + + throw new CloudRuntimeException("Unable to establish connection from storage head to storage pool " + pool.getId() + " due to " + answer.getDetails() + pool.getId()); } diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtStoragePoolDef.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtStoragePoolDef.java index f0ec29f41f6..4f8b2843ec2 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtStoragePoolDef.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtStoragePoolDef.java @@ -16,6 +16,12 @@ // under the License. package com.cloud.hypervisor.kvm.resource; +import java.util.HashSet; +import java.util.List; +import java.util.Set; + +import org.apache.commons.collections.CollectionUtils; + public class LibvirtStoragePoolDef { public enum PoolType { ISCSI("iscsi"), NETFS("netfs"), LOGICAL("logical"), DIR("dir"), RBD("rbd"), GLUSTERFS("glusterfs"), POWERFLEX("powerflex"); @@ -55,6 +61,7 @@ public class LibvirtStoragePoolDef { private String _authUsername; private AuthenticationType _authType; private String _secretUuid; + private Set _nfsMountOpts = new HashSet<>(); public LibvirtStoragePoolDef(PoolType type, String poolName, String uuid, String host, int port, String dir, String targetPath) { _poolType = type; @@ -75,6 +82,15 @@ public class LibvirtStoragePoolDef { _targetPath = targetPath; } + public LibvirtStoragePoolDef(PoolType type, String poolName, String uuid, String host, String dir, String targetPath, List nfsMountOpts) { + this(type, poolName, uuid, host, dir, targetPath); + if (CollectionUtils.isNotEmpty(nfsMountOpts)) { + for (String nfsMountOpt : nfsMountOpts) { + this._nfsMountOpts.add(nfsMountOpt); + } + } + } + public LibvirtStoragePoolDef(PoolType type, String poolName, String uuid, String sourceHost, int sourcePort, String dir, String authUsername, AuthenticationType authType, String secretUuid) { _poolType = type; @@ -124,69 +140,98 @@ public class LibvirtStoragePoolDef { return _authType; } + public Set getNfsMountOpts() { + return _nfsMountOpts; + } + @Override public String toString() { StringBuilder storagePoolBuilder = new StringBuilder(); - if (_poolType == PoolType.GLUSTERFS) { - /* libvirt mounts a Gluster volume, similar to NFS */ - storagePoolBuilder.append("\n"); - } else { - storagePoolBuilder.append("\n"); + String poolTypeXML; + switch (_poolType) { + case NETFS: + if (_nfsMountOpts != null) { + poolTypeXML = "netfs' xmlns:fs='http://libvirt.org/schemas/storagepool/fs/1.0"; + } else { + poolTypeXML = _poolType.toString(); + } + break; + case GLUSTERFS: + /* libvirt mounts a Gluster volume, similar to NFS */ + poolTypeXML = "netfs"; + break; + default: + poolTypeXML = _poolType.toString(); } + storagePoolBuilder.append("\n"); + storagePoolBuilder.append("" + _poolName + "\n"); if (_uuid != null) storagePoolBuilder.append("" + _uuid + "\n"); - if (_poolType == PoolType.NETFS) { - storagePoolBuilder.append("\n"); - storagePoolBuilder.append("\n"); - storagePoolBuilder.append("\n"); - storagePoolBuilder.append("\n"); - } - if (_poolType == PoolType.RBD) { - storagePoolBuilder.append("\n"); - for (String sourceHost : _sourceHost.split(",")) { + + switch (_poolType) { + case NETFS: + storagePoolBuilder.append("\n"); + storagePoolBuilder.append("\n"); + storagePoolBuilder.append("\n"); + storagePoolBuilder.append("\n"); + break; + + case RBD: + storagePoolBuilder.append("\n"); + for (String sourceHost : _sourceHost.split(",")) { + storagePoolBuilder.append("\n"); + } + + storagePoolBuilder.append("" + _sourceDir + "\n"); + if (_authUsername != null) { + storagePoolBuilder.append("\n"); + storagePoolBuilder.append("\n"); + storagePoolBuilder.append("\n"); + } + storagePoolBuilder.append("\n"); + break; + + case GLUSTERFS: + storagePoolBuilder.append("\n"); storagePoolBuilder.append("\n"); - } + storagePoolBuilder.append("\n"); + storagePoolBuilder.append("\n"); + storagePoolBuilder.append("\n"); + break; + } - storagePoolBuilder.append("" + _sourceDir + "\n"); - if (_authUsername != null) { - storagePoolBuilder.append("\n"); - storagePoolBuilder.append("\n"); - storagePoolBuilder.append("\n"); - } - storagePoolBuilder.append("\n"); - } - if (_poolType == PoolType.GLUSTERFS) { - storagePoolBuilder.append("\n"); - storagePoolBuilder.append("\n"); - storagePoolBuilder.append("\n"); - storagePoolBuilder.append("\n"); - storagePoolBuilder.append("\n"); - } if (_poolType != PoolType.RBD && _poolType != PoolType.POWERFLEX) { storagePoolBuilder.append("\n"); storagePoolBuilder.append("" + _targetPath + "\n"); storagePoolBuilder.append("\n"); } + if (_poolType == PoolType.NETFS && _nfsMountOpts != null) { + storagePoolBuilder.append("\n"); + for (String options : _nfsMountOpts) { + storagePoolBuilder.append("\n"); + } + storagePoolBuilder.append("\n"); + } storagePoolBuilder.append("\n"); return storagePoolBuilder.toString(); } diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtStoragePoolXMLParser.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtStoragePoolXMLParser.java index d19c851d7dc..a39ff07708e 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtStoragePoolXMLParser.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtStoragePoolXMLParser.java @@ -37,6 +37,19 @@ import org.xml.sax.SAXException; public class LibvirtStoragePoolXMLParser { private static final Logger s_logger = Logger.getLogger(LibvirtStoragePoolXMLParser.class); + private List getNFSMountOptsFromRootElement(Element rootElement) { + List nfsMountOpts = new ArrayList<>(); + Element mountOpts = (Element) rootElement.getElementsByTagName("fs:mount_opts").item(0); + if (mountOpts != null) { + NodeList options = mountOpts.getElementsByTagName("fs:option"); + for (int i = 0; i < options.getLength(); i++) { + Element option = (Element) options.item(i); + nfsMountOpts.add(option.getAttribute("name")); + } + } + return nfsMountOpts; + } + public LibvirtStoragePoolDef parseStoragePoolXML(String poolXML) { DocumentBuilder builder; try { @@ -94,11 +107,15 @@ public class LibvirtStoragePoolXMLParser { poolName, uuid, host, port, path, targetPath); } else { String path = getAttrValue("dir", "path", source); - Element target = (Element)rootElement.getElementsByTagName("target").item(0); String targetPath = getTagValue("path", target); - return new LibvirtStoragePoolDef(LibvirtStoragePoolDef.PoolType.valueOf(type.toUpperCase()), poolName, uuid, host, path, targetPath); + if (type.equalsIgnoreCase("netfs")) { + List nfsMountOpts = getNFSMountOptsFromRootElement(rootElement); + return new LibvirtStoragePoolDef(LibvirtStoragePoolDef.PoolType.valueOf(type.toUpperCase()), poolName, uuid, host, path, targetPath, nfsMountOpts); + } else { + return new LibvirtStoragePoolDef(LibvirtStoragePoolDef.PoolType.valueOf(type.toUpperCase()), poolName, uuid, host, path, targetPath); + } } } catch (ParserConfigurationException e) { s_logger.debug(e.toString()); 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 31281615bce..e26b2c51790 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 @@ -25,6 +25,7 @@ import java.util.List; import java.util.Map; import java.util.UUID; +import org.apache.cloudstack.api.ApiConstants; import org.apache.cloudstack.utils.cryptsetup.KeyFile; import org.apache.cloudstack.utils.qemu.QemuImg; import org.apache.cloudstack.utils.qemu.QemuImg.PhysicalDiskFormat; @@ -32,6 +33,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.collections.CollectionUtils; import org.apache.log4j.Logger; import org.libvirt.Connect; import org.libvirt.LibvirtException; @@ -282,9 +284,9 @@ public class LibvirtStorageAdaptor implements StorageAdaptor { } } - private StoragePool createNetfsStoragePool(PoolType fsType, Connect conn, String uuid, String host, String path) throws LibvirtException { + private StoragePool createNetfsStoragePool(PoolType fsType, Connect conn, String uuid, String host, String path, List nfsMountOpts) throws LibvirtException { String targetPath = _mountPoint + File.separator + uuid; - LibvirtStoragePoolDef spd = new LibvirtStoragePoolDef(fsType, uuid, uuid, host, path, targetPath); + LibvirtStoragePoolDef spd = new LibvirtStoragePoolDef(fsType, uuid, uuid, host, path, targetPath, nfsMountOpts); _storageLayer.mkdir(targetPath); StoragePool sp = null; try { @@ -373,6 +375,42 @@ public class LibvirtStorageAdaptor implements StorageAdaptor { } + private List getNFSMountOptsFromDetails(StoragePoolType type, Map details) { + List nfsMountOpts = null; + if (!type.equals(StoragePoolType.NetworkFilesystem) || details == null) { + return nfsMountOpts; + } + if (details.containsKey(ApiConstants.NFS_MOUNT_OPTIONS)) { + nfsMountOpts = Arrays.asList(details.get(ApiConstants.NFS_MOUNT_OPTIONS).replaceAll("\\s", "").split(",")); + } + return nfsMountOpts; + } + + private boolean destroyStoragePoolOnNFSMountOptionsChange(StoragePool sp, Connect conn, List nfsMountOpts) { + try { + LibvirtStoragePoolDef poolDef = getStoragePoolDef(conn, sp); + Set poolNfsMountOpts = poolDef.getNfsMountOpts(); + boolean mountOptsDiffer = false; + if (poolNfsMountOpts.size() != nfsMountOpts.size()) { + mountOptsDiffer = true; + } else { + for (String nfsMountOpt : nfsMountOpts) { + if (!poolNfsMountOpts.contains(nfsMountOpt)) { + mountOptsDiffer = true; + break; + } + } + } + if (mountOptsDiffer) { + sp.destroy(); + return true; + } + } catch (LibvirtException e) { + s_logger.error("Failure in destroying the pre-existing storage pool for changing the NFS mount options" + e); + } + return false; + } + private StoragePool createRBDStoragePool(Connect conn, String uuid, String host, int port, String userInfo, String path) { LibvirtStoragePoolDef spd; @@ -664,12 +702,21 @@ public class LibvirtStorageAdaptor implements StorageAdaptor { } catch (LibvirtException e) { s_logger.error("Failure in attempting to see if an existing storage pool might be using the path of the pool to be created:" + e); } + } + + List nfsMountOpts = getNFSMountOptsFromDetails(type, details); + if (sp != null && CollectionUtils.isNotEmpty(nfsMountOpts) && + destroyStoragePoolOnNFSMountOptionsChange(sp, conn, nfsMountOpts)) { + sp = null; + } + + if (sp == null) { s_logger.debug("Attempting to create storage pool " + name); if (type == StoragePoolType.NetworkFilesystem) { try { - sp = createNetfsStoragePool(PoolType.NETFS, conn, name, host, path); + sp = createNetfsStoragePool(PoolType.NETFS, conn, name, host, path, nfsMountOpts); } catch (LibvirtException e) { s_logger.error("Failed to create netfs mount: " + host + ":" + path , e); s_logger.error(e.getStackTrace()); @@ -677,7 +724,7 @@ public class LibvirtStorageAdaptor implements StorageAdaptor { } } else if (type == StoragePoolType.Gluster) { try { - sp = createNetfsStoragePool(PoolType.GLUSTERFS, conn, name, host, path); + sp = createNetfsStoragePool(PoolType.GLUSTERFS, conn, name, host, path, null); } catch (LibvirtException e) { s_logger.error("Failed to create glusterfs mount: " + host + ":" + path , e); s_logger.error(e.getStackTrace()); diff --git a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtStoragePoolDefTest.java b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtStoragePoolDefTest.java index 51a47e9a025..712b38b0bb4 100644 --- a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtStoragePoolDefTest.java +++ b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtStoragePoolDefTest.java @@ -19,6 +19,9 @@ package com.cloud.hypervisor.kvm.resource; +import java.util.ArrayList; +import java.util.List; + import junit.framework.TestCase; import com.cloud.hypervisor.kvm.resource.LibvirtStoragePoolDef.PoolType; import com.cloud.hypervisor.kvm.resource.LibvirtStoragePoolDef.AuthenticationType; @@ -47,6 +50,14 @@ public class LibvirtStoragePoolDefTest extends TestCase { assertEquals(port, pool.getSourcePort()); assertEquals(dir, pool.getSourceDir()); assertEquals(targetPath, pool.getTargetPath()); + + List nfsMountOpts = new ArrayList<>(); + nfsMountOpts.add("vers=4.1"); + nfsMountOpts.add("nconnect=4"); + pool = new LibvirtStoragePoolDef(type, name, uuid, host, dir, targetPath, nfsMountOpts); + assertTrue(pool.getNfsMountOpts().contains("vers=4.1")); + assertTrue(pool.getNfsMountOpts().contains("nconnect=4")); + assertEquals(pool.getNfsMountOpts().size(), 2); } @Test @@ -57,12 +68,38 @@ public class LibvirtStoragePoolDefTest extends TestCase { String host = "127.0.0.1"; String dir = "/export/primary"; String targetPath = "/mnt/" + uuid; + List nfsMountOpts = new ArrayList<>(); + nfsMountOpts.add("vers=4.1"); + nfsMountOpts.add("nconnect=4"); - LibvirtStoragePoolDef pool = new LibvirtStoragePoolDef(type, name, uuid, host, dir, targetPath); + LibvirtStoragePoolDef pool = new LibvirtStoragePoolDef(type, name, uuid, host, dir, targetPath, nfsMountOpts); - String expectedXml = "\n" + name + "\n" + uuid + "\n" + + String expectedXml = "\n" + + "" +name + "\n" + uuid + "\n" + "\n\n\n\n\n" + - "" + targetPath + "\n\n\n"; + "" + targetPath + "\n\n" + + "\n\n\n\n\n"; + + assertEquals(expectedXml, pool.toString()); + } + + @Test + public void testGlusterFSStoragePool() { + PoolType type = PoolType.GLUSTERFS; + String name = "myGFSPool"; + String uuid = "89a605bc-d470-4637-b3df-27388be452f5"; + String host = "127.0.0.1"; + String dir = "/export/primary"; + String targetPath = "/mnt/" + uuid; + List nfsMountOpts = new ArrayList<>(); + + LibvirtStoragePoolDef pool = new LibvirtStoragePoolDef(type, name, uuid, host, dir, targetPath, nfsMountOpts); + + String expectedXml = "\n" + + "" +name + "\n" + uuid + "\n" + + "\n\n\n" + + "\n\n\n" + + "" + targetPath + "\n\n\n"; assertEquals(expectedXml, pool.toString()); } diff --git a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtStoragePoolXMLParserTest.java b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtStoragePoolXMLParserTest.java index 3637b7b1f9b..5854c21186f 100644 --- a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtStoragePoolXMLParserTest.java +++ b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtStoragePoolXMLParserTest.java @@ -30,7 +30,7 @@ public class LibvirtStoragePoolXMLParserTest extends TestCase { @Test public void testParseNfsStoragePoolXML() { - String poolXML = "\n" + + String poolXML = "\n" + " feff06b5-84b2-3258-b5f9-1953217295de\n" + " feff06b5-84b2-3258-b5f9-1953217295de\n" + " 111111111\n" + @@ -49,12 +49,18 @@ public class LibvirtStoragePoolXMLParserTest extends TestCase { " 0\n" + " \n" + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + ""; LibvirtStoragePoolXMLParser parser = new LibvirtStoragePoolXMLParser(); LibvirtStoragePoolDef pool = parser.parseStoragePoolXML(poolXML); - Assert.assertEquals("10.11.12.13", pool.getSourceHost()); + assertEquals("10.11.12.13", pool.getSourceHost()); + assertTrue(pool.getNfsMountOpts().contains("vers=4.1")); + assertTrue(pool.getNfsMountOpts().contains("nconnect=8")); } @Test diff --git a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptorTest.java b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptorTest.java new file mode 100644 index 00000000000..3a7491ec542 --- /dev/null +++ b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptorTest.java @@ -0,0 +1,91 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package com.cloud.hypervisor.kvm.storage; + +import java.util.HashMap; +import java.util.Map; +import java.util.UUID; + +import com.cloud.utils.exception.CloudRuntimeException; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.libvirt.Connect; +import org.libvirt.StoragePool; +import org.libvirt.StoragePoolInfo; +import org.mockito.MockedStatic; +import org.mockito.Mockito; +import org.mockito.MockitoAnnotations; +import org.mockito.Spy; +import org.mockito.junit.MockitoJUnitRunner; + +import com.cloud.hypervisor.kvm.resource.LibvirtConnection; +import com.cloud.hypervisor.kvm.resource.LibvirtStoragePoolDef; +import com.cloud.storage.Storage; + +@RunWith(MockitoJUnitRunner.class) +public class LibvirtStorageAdaptorTest { + + private MockedStatic libvirtConnectionMockedStatic; + + private AutoCloseable closeable; + + @Spy + static LibvirtStorageAdaptor libvirtStorageAdaptor = new LibvirtStorageAdaptor(null); + + @Before + public void initMocks() { + closeable = MockitoAnnotations.openMocks(this); + libvirtConnectionMockedStatic = Mockito.mockStatic(LibvirtConnection.class); + } + + @After + public void tearDown() throws Exception { + libvirtConnectionMockedStatic.close(); + closeable.close(); + } + + @Test(expected = CloudRuntimeException.class) + public void testCreateStoragePoolWithNFSMountOpts() throws Exception { + LibvirtStoragePoolDef.PoolType type = LibvirtStoragePoolDef.PoolType.NETFS; + String name = "Primary1"; + String uuid = String.valueOf(UUID.randomUUID()); + String host = "127.0.0.1"; + String dir = "/export/primary"; + String targetPath = "/mnt/" + uuid; + + String poolXml = "\n" + + "" +name + "\n" + uuid + "\n" + + "\n\n\n\n\n" + + "" + targetPath + "\n\n" + + "\n\n\n\n\n"; + + Connect conn = Mockito.mock(Connect.class); + StoragePool sp = Mockito.mock(StoragePool.class); + StoragePoolInfo spinfo = Mockito.mock(StoragePoolInfo.class); + Mockito.when(LibvirtConnection.getConnection()).thenReturn(conn); + Mockito.when(conn.storagePoolLookupByUUIDString(uuid)).thenReturn(sp); + Mockito.when(sp.isActive()).thenReturn(1); + Mockito.when(sp.getXMLDesc(0)).thenReturn(poolXml); + + Map details = new HashMap<>(); + details.put("nfsmountopts", "vers=4.1, nconnect=4"); + KVMStoragePool pool = libvirtStorageAdaptor.createStoragePool(uuid, null, 0, dir, null, Storage.StoragePoolType.NetworkFilesystem, details); + } +} diff --git a/plugins/storage/volume/default/src/main/java/org/apache/cloudstack/storage/datastore/lifecycle/CloudStackPrimaryDataStoreLifeCycleImpl.java b/plugins/storage/volume/default/src/main/java/org/apache/cloudstack/storage/datastore/lifecycle/CloudStackPrimaryDataStoreLifeCycleImpl.java index 419d4c0ce7c..bed5cac8648 100644 --- a/plugins/storage/volume/default/src/main/java/org/apache/cloudstack/storage/datastore/lifecycle/CloudStackPrimaryDataStoreLifeCycleImpl.java +++ b/plugins/storage/volume/default/src/main/java/org/apache/cloudstack/storage/datastore/lifecycle/CloudStackPrimaryDataStoreLifeCycleImpl.java @@ -411,6 +411,10 @@ public class CloudStackPrimaryDataStoreLifeCycleImpl implements PrimaryDataStore throw new CloudRuntimeException("Storage has already been added as local storage"); } catch (Exception e) { s_logger.warn("Unable to establish a connection between " + h + " and " + primarystore, e); + String reason = storageMgr.getStoragePoolMountFailureReason(e.getMessage()); + if (reason != null) { + throw new CloudRuntimeException(reason); + } } } @@ -438,6 +442,10 @@ public class CloudStackPrimaryDataStoreLifeCycleImpl implements PrimaryDataStore throw new CloudRuntimeException("Storage has already been added as local storage to host: " + host.getName()); } catch (Exception e) { s_logger.warn("Unable to establish a connection between " + host + " and " + dataStore, e); + String reason = storageMgr.getStoragePoolMountFailureReason(e.getMessage()); + if (reason != null) { + throw new CloudRuntimeException(reason); + } } } if (poolHosts.isEmpty()) { @@ -458,8 +466,8 @@ public class CloudStackPrimaryDataStoreLifeCycleImpl implements PrimaryDataStore @Override public boolean cancelMaintain(DataStore store) { - dataStoreHelper.cancelMaintain(store); storagePoolAutmation.cancelMaintain(store); + dataStoreHelper.cancelMaintain(store); return true; } diff --git a/plugins/storage/volume/default/src/test/java/org/apache/cloudstack/storage/datastore/lifecycle/CloudStackPrimaryDataStoreLifeCycleImplTest.java b/plugins/storage/volume/default/src/test/java/org/apache/cloudstack/storage/datastore/lifecycle/CloudStackPrimaryDataStoreLifeCycleImplTest.java index dbd13d8f0de..1939dc26db2 100644 --- a/plugins/storage/volume/default/src/test/java/org/apache/cloudstack/storage/datastore/lifecycle/CloudStackPrimaryDataStoreLifeCycleImplTest.java +++ b/plugins/storage/volume/default/src/test/java/org/apache/cloudstack/storage/datastore/lifecycle/CloudStackPrimaryDataStoreLifeCycleImplTest.java @@ -34,6 +34,7 @@ import com.cloud.storage.Storage; import com.cloud.storage.StorageManager; import com.cloud.storage.StorageManagerImpl; import com.cloud.storage.dao.StoragePoolHostDao; +import com.cloud.utils.exception.CloudRuntimeException; import junit.framework.TestCase; import org.apache.cloudstack.engine.subsystem.api.storage.ClusterScope; import org.apache.cloudstack.engine.subsystem.api.storage.DataStore; @@ -57,7 +58,6 @@ import org.mockito.Mockito; import org.mockito.MockitoAnnotations; import org.mockito.runners.MockitoJUnitRunner; import org.springframework.test.util.ReflectionTestUtils; - import java.util.ArrayList; import java.util.List; import java.util.UUID; @@ -171,4 +171,23 @@ public class CloudStackPrimaryDataStoreLifeCycleImplTest extends TestCase { public void testAttachCluster() throws Exception { Assert.assertTrue(_cloudStackPrimaryDataStoreLifeCycle.attachCluster(store, new ClusterScope(1L, 1L, 1L))); } + + @Test + public void testAttachClusterException() throws Exception { + String exceptionString = "Mount failed due to incorrect mount options."; + String mountFailureReason = "Incorrect mount option specified."; + + CloudRuntimeException exception = new CloudRuntimeException(exceptionString); + StorageManager storageManager = Mockito.mock(StorageManager.class); + Mockito.when(storageManager.connectHostToSharedPool(Mockito.anyLong(), Mockito.anyLong())).thenThrow(exception); + Mockito.when(storageManager.getStoragePoolMountFailureReason(exceptionString)).thenReturn(mountFailureReason); + ReflectionTestUtils.setField(_cloudStackPrimaryDataStoreLifeCycle, "storageMgr", storageManager); + + try { + _cloudStackPrimaryDataStoreLifeCycle.attachCluster(store, new ClusterScope(1L, 1L, 1L)); + Assert.fail(); + } catch (Exception e) { + Assert.assertEquals(e.getMessage(), mountFailureReason); + } + } } diff --git a/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java b/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java index 6475baec411..74c9b53a50b 100644 --- a/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java +++ b/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java @@ -2988,6 +2988,16 @@ public class QueryManagerImpl extends MutualExclusiveIdsManagerBase implements Q return new Pair<>(pools, pools.size()); } + private void setPoolResponseNFSMountOptions(StoragePoolResponse poolResponse, Long poolId) { + if (Storage.StoragePoolType.NetworkFilesystem.toString().equals(poolResponse.getType()) && + HypervisorType.KVM.toString().equals(poolResponse.getHypervisor())) { + StoragePoolDetailVO detail = _storagePoolDetailsDao.findDetail(poolId, ApiConstants.NFS_MOUNT_OPTIONS); + if (detail != null) { + poolResponse.setNfsMountOpts(detail.getValue()); + } + } + } + private ListResponse createStoragesPoolResponse(Pair, Integer> storagePools) { ListResponse response = new ListResponse<>(); @@ -3009,6 +3019,7 @@ public class QueryManagerImpl extends MutualExclusiveIdsManagerBase implements Q poolResponse.setCaps(caps); } } + setPoolResponseNFSMountOptions(poolResponse, poolUuidToIdMap.get(poolResponse.getId())); } response.setResponses(poolResponses, storagePools.second()); diff --git a/server/src/main/java/com/cloud/storage/StorageManagerImpl.java b/server/src/main/java/com/cloud/storage/StorageManagerImpl.java index 02edd02fa94..12b971425c8 100644 --- a/server/src/main/java/com/cloud/storage/StorageManagerImpl.java +++ b/server/src/main/java/com/cloud/storage/StorageManagerImpl.java @@ -412,6 +412,8 @@ public class StorageManagerImpl extends ManagerBase implements StorageManager, C private final Map hostListeners = new HashMap(); + private static final String NFS_MOUNT_OPTIONS_INCORRECT = "An incorrect mount option was specified"; + public boolean share(VMInstanceVO vm, List vols, HostVO host, boolean cancelPreviousShare) throws StorageUnavailableException { // if pool is in maintenance and it is the ONLY pool available; reject @@ -840,6 +842,53 @@ public class StorageManagerImpl extends ManagerBase implements StorageManager, C return String.format("%s-%s-%s", StringUtils.trim(host.getName()), "local", storagePoolInformation.getUuid().split("-")[0]); } + protected void checkNfsMountOptions(String nfsMountOpts) throws InvalidParameterValueException { + String[] options = nfsMountOpts.replaceAll("\\s", "").split(","); + Map optionsMap = new HashMap<>(); + for (String option : options) { + String[] keyValue = option.split("="); + if (keyValue.length > 2) { + throw new InvalidParameterValueException("Invalid value for NFS option " + keyValue[0]); + } + if (optionsMap.containsKey(keyValue[0])) { + throw new InvalidParameterValueException("Duplicate NFS option values found for option " + keyValue[0]); + } + optionsMap.put(keyValue[0], null); + } + } + + protected void checkNFSMountOptionsForCreate(Map details, HypervisorType hypervisorType, String scheme) throws InvalidParameterValueException { + if (!details.containsKey(ApiConstants.NFS_MOUNT_OPTIONS)) { + return; + } + if (!hypervisorType.equals(HypervisorType.KVM) && !hypervisorType.equals(HypervisorType.Simulator)) { + throw new InvalidParameterValueException("NFS options can not be set for the hypervisor type " + hypervisorType); + } + if (!"nfs".equals(scheme)) { + throw new InvalidParameterValueException("NFS options can only be set on pool type " + StoragePoolType.NetworkFilesystem); + } + checkNfsMountOptions(details.get(ApiConstants.NFS_MOUNT_OPTIONS)); + } + + protected void checkNFSMountOptionsForUpdate(Map details, StoragePoolVO pool, Long accountId) throws InvalidParameterValueException { + if (!details.containsKey(ApiConstants.NFS_MOUNT_OPTIONS)) { + return; + } + if (!_accountMgr.isRootAdmin(accountId)) { + throw new PermissionDeniedException("Only root admin can modify nfs options"); + } + if (!pool.getHypervisor().equals(HypervisorType.KVM) && !pool.getHypervisor().equals((HypervisorType.Simulator))) { + throw new InvalidParameterValueException("NFS options can only be set for the hypervisor type " + HypervisorType.KVM); + } + if (!pool.getPoolType().equals(StoragePoolType.NetworkFilesystem)) { + throw new InvalidParameterValueException("NFS options can only be set on pool type " + StoragePoolType.NetworkFilesystem); + } + if (!pool.isInMaintenance()) { + throw new InvalidParameterValueException("The storage pool should be in maintenance mode to edit nfs options"); + } + checkNfsMountOptions(details.get(ApiConstants.NFS_MOUNT_OPTIONS)); + } + @Override public PrimaryDataStoreInfo createPool(CreateStoragePoolCmd cmd) throws ResourceInUseException, IllegalArgumentException, UnknownHostException, ResourceUnavailableException { String providerName = cmd.getStorageProviderName(); @@ -904,6 +953,8 @@ public class StorageManagerImpl extends ManagerBase implements StorageManager, C } Map details = extractApiParamAsMap(cmd.getDetails()); + checkNFSMountOptionsForCreate(details, hypervisorType, uriParams.get("scheme")); + DataCenterVO zone = _dcDao.findById(cmd.getZoneId()); if (zone == null) { throw new InvalidParameterValueException("unable to find zone by id " + zoneId); @@ -1086,6 +1137,9 @@ public class StorageManagerImpl extends ManagerBase implements StorageManager, C throw new IllegalArgumentException("Unable to find storage pool with ID: " + id); } + Map inputDetails = extractApiParamAsMap(cmd.getDetails()); + checkNFSMountOptionsForUpdate(inputDetails, pool, cmd.getEntityOwnerId()); + String name = cmd.getName(); if(StringUtils.isNotBlank(name)) { s_logger.debug("Updating Storage Pool name to: " + name); @@ -1129,12 +1183,9 @@ public class StorageManagerImpl extends ManagerBase implements StorageManager, C } // retrieve current details and merge/overlay input to capture changes - Map inputDetails = extractApiParamAsMap(cmd.getDetails()); Map details = null; - if (inputDetails == null) { - details = _storagePoolDetailsDao.listDetailsKeyPairs(id); - } else { - details = _storagePoolDetailsDao.listDetailsKeyPairs(id); + details = _storagePoolDetailsDao.listDetailsKeyPairs(id); + if (inputDetails != null) { details.putAll(inputDetails); changes = true; } @@ -1233,6 +1284,32 @@ public class StorageManagerImpl extends ManagerBase implements StorageManager, C return deleteDataStoreInternal(sPool, forced); } + @Override + public Pair, Boolean> getStoragePoolNFSMountOpts(StoragePool pool, Map details) { + boolean details_added = false; + if (!pool.getPoolType().equals(Storage.StoragePoolType.NetworkFilesystem)) { + return new Pair<>(details, details_added); + } + + StoragePoolDetailVO nfsMountOpts = _storagePoolDetailsDao.findDetail(pool.getId(), ApiConstants.NFS_MOUNT_OPTIONS); + if (nfsMountOpts != null) { + if (details == null) { + details = new HashMap<>(); + } + details.put(ApiConstants.NFS_MOUNT_OPTIONS, nfsMountOpts.getValue()); + details_added = true; + } + return new Pair<>(details, details_added); + } + + public String getStoragePoolMountFailureReason(String reason) { + if (reason.toLowerCase().contains(NFS_MOUNT_OPTIONS_INCORRECT.toLowerCase())) { + return NFS_MOUNT_OPTIONS_INCORRECT; + } else { + return null; + } + } + private boolean checkIfDataStoreClusterCanbeDeleted(StoragePoolVO sPool, boolean forced) { List childStoragePools = _storagePoolDao.listChildStoragePoolsInDatastoreCluster(sPool.getId()); boolean canDelete = true; diff --git a/server/src/main/java/com/cloud/storage/StoragePoolAutomationImpl.java b/server/src/main/java/com/cloud/storage/StoragePoolAutomationImpl.java index 7b5ebc455c6..740e247723a 100644 --- a/server/src/main/java/com/cloud/storage/StoragePoolAutomationImpl.java +++ b/server/src/main/java/com/cloud/storage/StoragePoolAutomationImpl.java @@ -20,6 +20,7 @@ package com.cloud.storage; import java.util.ArrayList; import java.util.List; +import java.util.Map; import javax.inject.Inject; @@ -28,6 +29,7 @@ import org.apache.cloudstack.engine.subsystem.api.storage.DataStore; import org.apache.cloudstack.engine.subsystem.api.storage.DataStoreManager; import org.apache.cloudstack.engine.subsystem.api.storage.DataStoreProviderManager; import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao; +import org.apache.cloudstack.storage.datastore.db.StoragePoolDetailsDao; import org.apache.cloudstack.storage.datastore.db.StoragePoolVO; import org.apache.log4j.Logger; import org.springframework.stereotype.Component; @@ -48,6 +50,7 @@ import com.cloud.storage.dao.VolumeDao; import com.cloud.user.Account; import com.cloud.user.User; import com.cloud.user.dao.UserDao; +import com.cloud.utils.Pair; import com.cloud.utils.exception.CloudRuntimeException; import com.cloud.vm.ConsoleProxyVO; import com.cloud.vm.DomainRouterVO; @@ -88,6 +91,8 @@ public class StoragePoolAutomationImpl implements StoragePoolAutomation { @Inject PrimaryDataStoreDao primaryDataStoreDao; @Inject + StoragePoolDetailsDao storagePoolDetailsDao; + @Inject DataStoreManager dataStoreMgr; @Inject protected ResourceManager _resourceMgr; @@ -318,14 +323,25 @@ public class StoragePoolAutomationImpl implements StoragePoolAutomation { if (hosts == null || hosts.size() == 0) { return true; } + + Pair, Boolean> nfsMountOpts = storageManager.getStoragePoolNFSMountOpts(pool, null); // add heartbeat for (HostVO host : hosts) { - ModifyStoragePoolCommand msPoolCmd = new ModifyStoragePoolCommand(true, pool); + ModifyStoragePoolCommand msPoolCmd = new ModifyStoragePoolCommand(true, pool, nfsMountOpts.first()); final Answer answer = agentMgr.easySend(host.getId(), msPoolCmd); if (answer == null || !answer.getResult()) { if (s_logger.isDebugEnabled()) { s_logger.debug("ModifyStoragePool add failed due to " + ((answer == null) ? "answer null" : answer.getDetails())); } + if (answer != null && nfsMountOpts.second()) { + s_logger.error(String.format("Unable to attach storage pool to the host %s due to %s", host, answer.getDetails())); + StringBuilder exceptionSB = new StringBuilder("Unable to attach storage pool to the host ").append(host.getName()); + String reason = storageManager.getStoragePoolMountFailureReason(answer.getDetails()); + if (reason!= null) { + exceptionSB.append(". ").append(reason).append("."); + } + throw new CloudRuntimeException(exceptionSB.toString()); + } } else { if (s_logger.isDebugEnabled()) { s_logger.debug("ModifyStoragePool add succeeded"); diff --git a/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java b/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java index dbceac9efb1..2596f9facba 100644 --- a/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java +++ b/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java @@ -21,14 +21,21 @@ import com.cloud.dc.DataCenterVO; import com.cloud.dc.dao.DataCenterDao; import com.cloud.exception.ConnectionException; import com.cloud.exception.InvalidParameterValueException; +import com.cloud.exception.PermissionDeniedException; import com.cloud.host.Host; +import com.cloud.hypervisor.Hypervisor; import com.cloud.storage.dao.VolumeDao; +import com.cloud.user.AccountManager; +import com.cloud.utils.Pair; import com.cloud.vm.VMInstanceVO; import com.cloud.vm.dao.VMInstanceDao; +import org.apache.cloudstack.api.ApiConstants; import org.apache.cloudstack.framework.config.ConfigDepot; import org.apache.cloudstack.framework.config.dao.ConfigurationDao; import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao; +import org.apache.cloudstack.storage.datastore.db.StoragePoolDetailVO; +import org.apache.cloudstack.storage.datastore.db.StoragePoolDetailsDao; import org.apache.cloudstack.storage.datastore.db.StoragePoolVO; import org.apache.commons.collections.MapUtils; import org.junit.Assert; @@ -59,6 +66,10 @@ public class StorageManagerImplTest { ConfigurationDao configurationDao; @Mock DataCenterDao dataCenterDao; + @Mock + AccountManager accountManager; + @Mock + StoragePoolDetailsDao storagePoolDetailsDao; @Spy @InjectMocks @@ -249,4 +260,152 @@ public class StorageManagerImplTest { .update(StorageManager.DataStoreDownloadFollowRedirects.key(),StorageManager.DataStoreDownloadFollowRedirects.defaultValue()); } + @Test + public void testCheckNFSMountOptionsForCreateNoNFSMountOptions() { + Map details = new HashMap<>(); + try { + storageManagerImpl.checkNFSMountOptionsForCreate(details, Hypervisor.HypervisorType.XenServer, ""); + } catch (Exception e) { + Assert.fail(); + } + } + + @Test + public void testCheckNFSMountOptionsForCreateNotKVM() { + Map details = new HashMap<>(); + details.put(ApiConstants.NFS_MOUNT_OPTIONS, "vers=4.1"); + InvalidParameterValueException exception = Assert.assertThrows(InvalidParameterValueException.class, + () -> storageManagerImpl.checkNFSMountOptionsForCreate(details, Hypervisor.HypervisorType.XenServer, "")); + Assert.assertEquals(exception.getMessage(), "NFS options can not be set for the hypervisor type " + Hypervisor.HypervisorType.XenServer); + } + + @Test + public void testCheckNFSMountOptionsForCreateNotNFS() { + Map details = new HashMap<>(); + details.put(ApiConstants.NFS_MOUNT_OPTIONS, "vers=4.1"); + InvalidParameterValueException exception = Assert.assertThrows(InvalidParameterValueException.class, + () -> storageManagerImpl.checkNFSMountOptionsForCreate(details, Hypervisor.HypervisorType.KVM, "")); + Assert.assertEquals(exception.getMessage(), "NFS options can only be set on pool type " + Storage.StoragePoolType.NetworkFilesystem); + } + + @Test + public void testCheckNFSMountOptionsForUpdateNoNFSMountOptions() { + Map details = new HashMap<>(); + StoragePoolVO pool = new StoragePoolVO(); + Long accountId = 1L; + try { + storageManagerImpl.checkNFSMountOptionsForUpdate(details, pool, accountId); + } catch (Exception e) { + Assert.fail(); + } + } + + @Test + public void testCheckNFSMountOptionsForUpdateNotRootAdmin() { + Map details = new HashMap<>(); + StoragePoolVO pool = new StoragePoolVO(); + Long accountId = 1L; + details.put(ApiConstants.NFS_MOUNT_OPTIONS, "vers=4.1"); + Mockito.when(accountManager.isRootAdmin(accountId)).thenReturn(false); + PermissionDeniedException exception = Assert.assertThrows(PermissionDeniedException.class, + () -> storageManagerImpl.checkNFSMountOptionsForUpdate(details, pool, accountId)); + Assert.assertEquals(exception.getMessage(), "Only root admin can modify nfs options"); + } + + @Test + public void testCheckNFSMountOptionsForUpdateNotKVM() { + Map details = new HashMap<>(); + StoragePoolVO pool = new StoragePoolVO(); + Long accountId = 1L; + details.put(ApiConstants.NFS_MOUNT_OPTIONS, "vers=4.1"); + Mockito.when(accountManager.isRootAdmin(accountId)).thenReturn(true); + pool.setHypervisor(Hypervisor.HypervisorType.XenServer); + InvalidParameterValueException exception = Assert.assertThrows(InvalidParameterValueException.class, + () -> storageManagerImpl.checkNFSMountOptionsForUpdate(details, pool, accountId)); + Assert.assertEquals(exception.getMessage(), "NFS options can only be set for the hypervisor type " + Hypervisor.HypervisorType.KVM); + } + + @Test + public void testCheckNFSMountOptionsForUpdateNotNFS() { + Map details = new HashMap<>(); + StoragePoolVO pool = new StoragePoolVO(); + Long accountId = 1L; + details.put(ApiConstants.NFS_MOUNT_OPTIONS, "vers=4.1"); + Mockito.when(accountManager.isRootAdmin(accountId)).thenReturn(true); + pool.setHypervisor(Hypervisor.HypervisorType.KVM); + pool.setPoolType(Storage.StoragePoolType.FiberChannel); + InvalidParameterValueException exception = Assert.assertThrows(InvalidParameterValueException.class, + () -> storageManagerImpl.checkNFSMountOptionsForUpdate(details, pool, accountId)); + Assert.assertEquals(exception.getMessage(), "NFS options can only be set on pool type " + Storage.StoragePoolType.NetworkFilesystem); + } + + @Test + public void testCheckNFSMountOptionsForUpdateNotMaintenance() { + Map details = new HashMap<>(); + StoragePoolVO pool = new StoragePoolVO(); + Long accountId = 1L; + details.put(ApiConstants.NFS_MOUNT_OPTIONS, "vers=4.1"); + Mockito.when(accountManager.isRootAdmin(accountId)).thenReturn(true); + pool.setHypervisor(Hypervisor.HypervisorType.KVM); + pool.setPoolType(Storage.StoragePoolType.NetworkFilesystem); + pool.setStatus(StoragePoolStatus.Up); + InvalidParameterValueException exception = Assert.assertThrows(InvalidParameterValueException.class, + () -> storageManagerImpl.checkNFSMountOptionsForUpdate(details, pool, accountId)); + Assert.assertEquals(exception.getMessage(), "The storage pool should be in maintenance mode to edit nfs options"); + } + + @Test(expected = InvalidParameterValueException.class) + public void testDuplicateNFSMountOptions() { + String nfsMountOpts = "vers=4.1, nconnect=4,vers=4.2"; + Map details = new HashMap<>(); + details.put(ApiConstants.NFS_MOUNT_OPTIONS, nfsMountOpts); + storageManagerImpl.checkNFSMountOptionsForCreate(details, Hypervisor.HypervisorType.KVM, "nfs"); + } + + @Test(expected = InvalidParameterValueException.class) + public void testInvalidNFSMountOptions() { + String nfsMountOpts = "vers=4.1=2,"; + Map details = new HashMap<>(); + details.put(ApiConstants.NFS_MOUNT_OPTIONS, nfsMountOpts); + StoragePoolVO pool = new StoragePoolVO(); + pool.setHypervisor(Hypervisor.HypervisorType.KVM); + pool.setPoolType(Storage.StoragePoolType.NetworkFilesystem); + pool.setStatus(StoragePoolStatus.Maintenance); + Long accountId = 1L; + Mockito.when(accountManager.isRootAdmin(accountId)).thenReturn(true); + storageManagerImpl.checkNFSMountOptionsForUpdate(details, pool, accountId); + } + + @Test + public void testGetStoragePoolMountOptionsNotNFS() { + StoragePoolVO pool = new StoragePoolVO(); + + pool.setPoolType(Storage.StoragePoolType.FiberChannel); + Pair, Boolean> details = storageManagerImpl.getStoragePoolNFSMountOpts(pool, null); + Assert.assertEquals(details.second(), false); + Assert.assertEquals(details.first(), null); + } + + @Test + public void testGetStoragePoolMountOptions() { + Long poolId = 1L; + String key = "nfsmountopts"; + String value = "vers=4.1,nconnect=2"; + StoragePoolDetailVO nfsMountOpts = new StoragePoolDetailVO(poolId, key, value, true); + StoragePoolVO pool = new StoragePoolVO(); + pool.setId(poolId); + pool.setPoolType(Storage.StoragePoolType.NetworkFilesystem); + Mockito.when(storagePoolDetailsDao.findDetail(poolId, ApiConstants.NFS_MOUNT_OPTIONS)).thenReturn(nfsMountOpts); + + Pair, Boolean> details = storageManagerImpl.getStoragePoolNFSMountOpts(pool, null); + Assert.assertEquals(details.second(), true); + Assert.assertEquals(details.first().get(key), value); + } + + @Test + public void testGetStoragePoolMountFailureReason() { + String error = "Mount failed on kvm host. An incorrect mount option was specified.\nIncorrect mount option."; + String failureReason = storageManagerImpl.getStoragePoolMountFailureReason(error); + Assert.assertEquals(failureReason, "An incorrect mount option was specified"); + } } diff --git a/test/integration/component/maint/test_primary_storage_nfsmountopts_kvm.py b/test/integration/component/maint/test_primary_storage_nfsmountopts_kvm.py new file mode 100644 index 00000000000..f2ad18188ca --- /dev/null +++ b/test/integration/component/maint/test_primary_storage_nfsmountopts_kvm.py @@ -0,0 +1,184 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +from marvin.cloudstackTestCase import cloudstackTestCase +from marvin.cloudstackAPI import * +from marvin.lib.utils import * +from marvin.lib.base import * +from marvin.lib.common import (list_clusters, list_hosts, list_storage_pools) +import xml.etree.ElementTree as ET +from lxml import etree +from nose.plugins.attrib import attr + +class TestNFSMountOptsKVM(cloudstackTestCase): + """ Test cases for host HA using KVM host(s) + """ + + @classmethod + def setUpClass(cls): + testClient = super(TestNFSMountOptsKVM, cls).getClsTestClient() + cls.apiclient = testClient.getApiClient() + + cls.cluster = list_clusters(cls.apiclient)[0] + cls.host = cls.getHost(cls) + cls.storage_pool = cls.getPrimaryStorage(cls, cls.cluster.id) + cls.hostConfig = cls.config.__dict__["zones"][0].__dict__["pods"][0].__dict__["clusters"][0].__dict__["hosts"][0].__dict__ + cls.cluster_id = cls.host.clusterid + cls.sshClient = SshClient( + host=cls.host.ipaddress, + port=22, + user=cls.hostConfig['username'], + passwd=cls.hostConfig['password']) + cls.version = cls.getNFSMountOptionForPool(cls, "vers", cls.storage_pool.id) + if (cls.version == None): + raise cls.skipTest("Storage pool not associated with the host") + + def tearDown(self): + nfsMountOpts = "vers=" + self.version + details = [{'nfsmountopts': nfsMountOpts}] + self.changeNFSOptions(details) + pass + + def getHost(self): + response = list_hosts( + self.apiclient, + type='Routing', + hypervisor='kvm', + state='Up', + id=None + ) + + if response and len(response) > 0: + self.host = response[0] + return self.host + raise self.skipTest("Not enough KVM hosts found, skipping NFS options test") + + def getPrimaryStorage(self, clusterId): + response = list_storage_pools( + self.apiclient, + clusterid=clusterId, + type='NetworkFilesystem', + state='Up', + id=None, + ) + if response and len(response) > 0: + return response[0] + raise self.skipTest("Not enough KVM hosts found, skipping NFS options test") + + def getNFSMountOptionsFromVirsh(self, poolId): + virsh_cmd = "virsh pool-dumpxml %s" % poolId + xml_res = self.sshClient.execute(virsh_cmd) + xml_as_str = ''.join(xml_res) + self.debug(xml_as_str) + parser = etree.XMLParser(remove_blank_text=True) + root = ET.fromstring(xml_as_str, parser=parser) + mount_opts = root.findall("{http://libvirt.org/schemas/storagepool/fs/1.0}mount_opts")[0] + + options_map = {} + for child in mount_opts: + option = child.get('name').split("=") + options_map[option[0]] = option[1] + return options_map + + def getUnusedNFSVersions(self, filter): + nfsstat_cmd = "nfsstat -m | sed -n '/%s/{ n; p }'" % filter + nfsstats = self.sshClient.execute(nfsstat_cmd) + versions = {"4.1": 0, "4.2": 0, "3": 0} + for stat in nfsstats: + vers = stat[stat.find("vers"):].split("=")[1].split(",")[0] + versions[vers] += 1 + + for key in versions: + if versions[key] == 0: + return key + return None + + def getNFSMountOptionForPool(self, option, poolId): + nfsstat_cmd = "nfsstat -m | sed -n '/%s/{ n; p }'" % poolId + nfsstat = self.sshClient.execute(nfsstat_cmd) + if (nfsstat == None): + return None + stat = nfsstat[0] + vers = stat[stat.find(option):].split("=")[1].split(",")[0] + return vers + + def changeNFSOptions(self, details): + maint_cmd = enableStorageMaintenance.enableStorageMaintenanceCmd() + maint_cmd.id = self.storage_pool.id + + storage = StoragePool.list(self.apiclient, + id=self.storage_pool.id + )[0] + if storage.state != "Maintenance": + self.apiclient.enableStorageMaintenance(maint_cmd) + + StoragePool.update(self.apiclient, + id=self.storage_pool.id, + details=details + ) + + store_maint_cmd = cancelStorageMaintenance.cancelStorageMaintenanceCmd() + store_maint_cmd.id = self.storage_pool.id + resp = self.apiclient.cancelStorageMaintenance(store_maint_cmd) + return resp + + @attr(tags=["devcloud", "advanced", "advancedns", "smoke", "basic", "sg"], required_hardware="true") + def test_primary_storage_nfs_options_kvm(self): + """ + Tests that NFS mount options configured on the primary storage are set correctly on the KVM hypervisor host + """ + version = self.getUnusedNFSVersions(self.storage_pool.ipaddress) + nconnect = None + if version == None: + self.debug("skipping nconnect mount option as there are multiple mounts already present from the nfs server for all versions") + version = self.getUnusedNFSVersions(self.storage_pool.id) + nfsMountOpts = "vers=" + version + else: + nconnect='4' + nfsMountOpts = "vers=" + version + ",nconnect=" + nconnect + + details = [{'nfsmountopts': nfsMountOpts}] + + resp = self.changeNFSOptions(details) + + storage = StoragePool.list(self.apiclient, + id=self.storage_pool.id + )[0] + self.assertEqual(storage.nfsmountopts, nfsMountOpts) + + options = self.getNFSMountOptionsFromVirsh(self.storage_pool.id) + self.assertEqual(options["vers"], version) + if (nconnect != None): + self.assertEqual(options["nconnect"], nconnect) + + @attr(tags=["devcloud", "advanced", "advancedns", "smoke", "basic", "sg"], required_hardware="true") + def test_primary_storage_incorrect_nfs_options_kvm(self): + """ + Tests that incorrect NFS mount options leads to exception when maintenance mode is cancelled + """ + nfsMountOpts = "version=4.1" + details = [{'nfsmountopts': nfsMountOpts}] + + try: + resp = self.changeNFSOptions(details) + except Exception: + storage = StoragePool.list(self.apiclient, + id=self.storage_pool.id + )[0] + self.assertEqual(storage.state, "Maintenance") + else: + self.fail("Incorrect NFS mount option should throw error while mounting") diff --git a/ui/public/locales/en.json b/ui/public/locales/en.json index c49fef5e4fe..ce77d53f164 100644 --- a/ui/public/locales/en.json +++ b/ui/public/locales/en.json @@ -118,6 +118,7 @@ "label.action.edit.domain": "Edit domain", "label.action.edit.instance": "Edit Instance", "label.action.edit.iso": "Edit ISO", +"label.action.edit.nfs.options": "Edit NFS mount options", "label.action.edit.template": "Edit Template", "label.action.edit.zone": "Edit zone", "label.action.enable.two.factor.authentication": "Enabled Two factor authentication", @@ -1427,6 +1428,7 @@ "label.newname": "New name", "label.next": "Next", "label.nfs": "NFS", +"label.nfsmountopts": "NFS mount options", "label.nfsserver": "NFS server", "label.nic": "NIC", "label.nicadaptertype": "NIC adapter type", @@ -2457,6 +2459,7 @@ "message.action.disable.zone": "Please confirm that you want to disable this zone.", "message.action.download.iso": "Please confirm that you want to download this ISO.", "message.action.download.template": "Please confirm that you want to download this Template.", +"message.action.edit.nfs.mount.options": "Changes to NFS mount options will only take affect on cancelling maintenance mode which will cause the storage pool to be remounted on all KVM hosts with the new mount options.", "message.action.enable.cluster": "Please confirm that you want to enable this cluster.", "message.action.enable.disk.offering": "Please confirm that you want to enable this disk offering.", "message.action.enable.service.offering": "Please confirm that you want to enable this service offering.", @@ -3035,6 +3038,7 @@ "message.network.updateip": "Please confirm that you would like to change the IP address for this NIC on the Instance.", "message.network.usage.info.data.points": "Each data point represents the difference in data traffic since the last data point.", "message.network.usage.info.sum.of.vnics": "The Network usage shown is made up of the sum of data traffic from all the vNICs in the Instance.", +"message.nfs.mount.options.description": "Comma separated list of NFS mount options for KVM hosts. Supported options : vers=[3,4.0,4.1,4.2], nconnect=[1...16]", "message.no.data.to.show.for.period": "No data to show for the selected period.", "message.no.description": "No description entered.", "message.offering.internet.protocol.warning": "WARNING: IPv6 supported Networks use static routing and will require upstream routes to be configured manually.", @@ -3206,6 +3210,7 @@ "message.success.disable.saml.auth": "Successfully disabled SAML authorization", "message.success.disable.vpn": "Successfully disabled VPN", "message.success.edit.acl": "Successfully edited ACL rule", +"message.success.edit.primary.storage": "Successfully edited Primary Storage", "message.success.edit.rule": "Successfully edited rule", "message.success.enable.saml.auth": "Successfully enabled SAML Authorization", "message.success.import.instance": "Successfully imported Instance", diff --git a/ui/src/config/section/infra/primaryStorages.js b/ui/src/config/section/infra/primaryStorages.js index 74624b3e888..24333367f1c 100644 --- a/ui/src/config/section/infra/primaryStorages.js +++ b/ui/src/config/section/infra/primaryStorages.js @@ -35,7 +35,7 @@ export default { fields.push('zonename') return fields }, - details: ['name', 'id', 'ipaddress', 'type', 'scope', 'tags', 'path', 'provider', 'hypervisor', 'overprovisionfactor', 'disksizetotal', 'disksizeallocated', 'disksizeused', 'clustername', 'podname', 'zonename', 'created'], + details: ['name', 'id', 'ipaddress', 'type', 'nfsmountopts', 'scope', 'tags', 'path', 'provider', 'hypervisor', 'overprovisionfactor', 'disksizetotal', 'disksizeallocated', 'disksizeused', 'clustername', 'podname', 'zonename', 'created'], related: [{ name: 'volume', title: 'label.volumes', @@ -90,7 +90,8 @@ export default { icon: 'edit-outlined', label: 'label.edit', dataView: true, - args: ['name', 'tags', 'istagarule', 'capacitybytes', 'capacityiops'] + popup: true, + component: shallowRef(defineAsyncComponent(() => import('@/views/infra/UpdatePrimaryStorage.vue'))) }, { api: 'updateStoragePool', diff --git a/ui/src/views/infra/AddPrimaryStorage.vue b/ui/src/views/infra/AddPrimaryStorage.vue index 00fbe0dd438..0b9b7aec7c6 100644 --- a/ui/src/views/infra/AddPrimaryStorage.vue +++ b/ui/src/views/infra/AddPrimaryStorage.vue @@ -178,6 +178,17 @@ +
+ + + + +
@@ -794,6 +805,9 @@ export default { var url = '' if (values.protocol === 'nfs') { url = this.nfsURL(server, path) + if (values.nfsMountOpts) { + params['details[0].nfsmountopts'] = values.nfsMountOpts + } } else if (values.protocol === 'SMB') { url = this.smbURL(server, path) const smbParams = { diff --git a/ui/src/views/infra/UpdatePrimaryStorage.vue b/ui/src/views/infra/UpdatePrimaryStorage.vue new file mode 100644 index 00000000000..7c026630a99 --- /dev/null +++ b/ui/src/views/infra/UpdatePrimaryStorage.vue @@ -0,0 +1,195 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + + + + + + diff --git a/ui/src/views/infra/zone/ZoneWizardAddResources.vue b/ui/src/views/infra/zone/ZoneWizardAddResources.vue index 24ddd264486..ef2cba1c3d9 100644 --- a/ui/src/views/infra/zone/ZoneWizardAddResources.vue +++ b/ui/src/views/infra/zone/ZoneWizardAddResources.vue @@ -530,6 +530,15 @@ export default { primaryStorageProtocol: ['vmfs', 'datastorecluster'] } }, + { + title: 'label.nfsmountopts', + key: 'primaryStorageNFSMountOptions', + required: false, + display: { + primaryStorageProtocol: 'nfs', + hypervisor: ['KVM', 'Simulator'] + } + }, { title: 'label.resourcegroup', key: 'primaryStorageLinstorResourceGroup', diff --git a/ui/src/views/infra/zone/ZoneWizardLaunchZone.vue b/ui/src/views/infra/zone/ZoneWizardLaunchZone.vue index 929b0bf02cd..56997e76f83 100644 --- a/ui/src/views/infra/zone/ZoneWizardLaunchZone.vue +++ b/ui/src/views/infra/zone/ZoneWizardLaunchZone.vue @@ -1387,6 +1387,7 @@ export default { path = '/' + path } url = this.nfsURL(server, path) + params['details[0].nfsmountopts'] = this.prefillContent.primaryStorageNFSMountOptions } else if (protocol === 'SMB') { let path = this.prefillContent?.primaryStoragePath || '' if (path.substring(0, 1) !== '/') {