From 4c7712cb42f6e2e9aa00f8b1bd12645ffbc3fd81 Mon Sep 17 00:00:00 2001 From: abh1sar Date: Fri, 19 Apr 2024 13:05:11 +0530 Subject: [PATCH 01/23] Ability to specify NFS mount options while adding a primary storage and modify it later --- .../api/response/StoragePoolResponse.java | 12 ++ .../agent/api/ModifyStoragePoolCommand.java | 4 + .../provider/DefaultHostListener.java | 14 +- .../kvm/resource/LibvirtStoragePoolDef.java | 30 +++- .../resource/LibvirtStoragePoolXMLParser.java | 16 ++ .../kvm/storage/LibvirtStorageAdaptor.java | 42 ++++- .../resource/LibvirtStoragePoolDefTest.java | 22 ++- .../LibvirtStoragePoolXMLParserTest.java | 10 +- .../com/cloud/api/query/QueryManagerImpl.java | 5 + .../com/cloud/storage/StorageManagerImpl.java | 54 +++++- .../storage/StoragePoolAutomationImpl.java | 18 +- .../cloud/storage/StorageManagerImplTest.java | 11 ++ .../smoke/test_primary_storage_nfsopts_kvm.py | 162 ++++++++++++++++++ ui/public/locales/en.json | 5 + .../config/section/infra/primaryStorages.js | 12 +- ui/src/views/infra/AddPrimaryStorage.vue | 12 ++ ui/src/views/infra/NFSOptsPrimaryStorage.vue | 147 ++++++++++++++++ .../infra/zone/ZoneWizardAddResources.vue | 9 + .../views/infra/zone/ZoneWizardLaunchZone.vue | 1 + 19 files changed, 568 insertions(+), 18 deletions(-) create mode 100644 test/integration/smoke/test_primary_storage_nfsopts_kvm.py create mode 100644 ui/src/views/infra/NFSOptsPrimaryStorage.vue 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 183290ec9ebe..bb74ed85190c 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("nfsopts") + @Param(description = "the nfs options for the storage pool") + private String nfsopts; + @SerializedName(ApiConstants.IS_TAG_A_RULE) @Param(description = ApiConstants.PARAMETER_DESCRIPTION_IS_TAG_A_RULE) private Boolean isTagARule; @@ -347,4 +351,12 @@ public String getProvider() { public void setProvider(String provider) { this.provider = provider; } + + public String getNfsopts() { + return nfsopts; + } + + public void setNfsopts(String nfsopts) { + this.nfsopts = nfsopts; + } } 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 ad05fe1d615d..06940266b538 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 ModifyStoragePoolCommand(boolean add, StoragePool pool, String localPath, 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/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 a453f2d48c21..a75e2c2809ea 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 @@ -53,7 +53,10 @@ import org.apache.log4j.Logger; import javax.inject.Inject; + +import java.util.HashMap; 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 +128,16 @@ private NicTO createNicTOFromNetworkAndOffering(NetworkVO networkVO, NetworkOffe @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); + Map details = null; + if (pool.getPoolType().equals(Storage.StoragePoolType.NetworkFilesystem)) { + details = new HashMap<>(); + StoragePoolDetailVO nfsopts = storagePoolDetailsDao.findDetail(poolId, "nfsopts"); + if (nfsopts != null) { + details.put("nfsopts", nfsopts.getValue()); + } + } + + ModifyStoragePoolCommand cmd = new ModifyStoragePoolCommand(true, pool, details); 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())); 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 f0ec29f41f69..d78b1cae9e13 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,10 @@ // under the License. package com.cloud.hypervisor.kvm.resource; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + public class LibvirtStoragePoolDef { public enum PoolType { ISCSI("iscsi"), NETFS("netfs"), LOGICAL("logical"), DIR("dir"), RBD("rbd"), GLUSTERFS("glusterfs"), POWERFLEX("powerflex"); @@ -55,6 +59,7 @@ public String toString() { private String _authUsername; private AuthenticationType _authType; private String _secretUuid; + private Map _nfsopts = new HashMap<>(); public LibvirtStoragePoolDef(PoolType type, String poolName, String uuid, String host, int port, String dir, String targetPath) { _poolType = type; @@ -75,6 +80,15 @@ public LibvirtStoragePoolDef(PoolType type, String poolName, String uuid, String _targetPath = targetPath; } + public LibvirtStoragePoolDef(PoolType type, String poolName, String uuid, String host, String dir, String targetPath, List nfsopts) { + this(type, poolName, uuid, host, dir, targetPath); + if (nfsopts != null) { + for (String nfsopt : nfsopts) { + this._nfsopts.put(nfsopt, null); + } + } + } + public LibvirtStoragePoolDef(PoolType type, String poolName, String uuid, String sourceHost, int sourcePort, String dir, String authUsername, AuthenticationType authType, String secretUuid) { _poolType = type; @@ -124,10 +138,17 @@ public AuthenticationType getAuthType() { return _authType; } + public Map getNfsOpts() { + return _nfsopts; + } + @Override public String toString() { StringBuilder storagePoolBuilder = new StringBuilder(); - if (_poolType == PoolType.GLUSTERFS) { + if (_poolType == PoolType.NETFS && _nfsopts != null) { + // get from poolType + storagePoolBuilder.append("\n"); + } else if (_poolType == PoolType.GLUSTERFS) { /* libvirt mounts a Gluster volume, similar to NFS */ storagePoolBuilder.append("\n"); } else { @@ -187,6 +208,13 @@ public String toString() { storagePoolBuilder.append("" + _targetPath + "\n"); storagePoolBuilder.append("\n"); } + if (_poolType == PoolType.NETFS && _nfsopts != null) { + storagePoolBuilder.append("\n"); + for (Map.Entry options : _nfsopts.entrySet()) { + 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 d19c851d7dcc..13167958388d 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 @@ -92,6 +92,22 @@ public LibvirtStoragePoolDef parseStoragePoolXML(String poolXML) { return new LibvirtStoragePoolDef(LibvirtStoragePoolDef.PoolType.valueOf(format.toUpperCase()), poolName, uuid, host, port, path, targetPath); + } else if (type.equalsIgnoreCase("netfs")) { + List nfsopts = 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); + nfsopts.add(option.getAttribute("name")); + } + } + + 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, nfsopts); } else { String path = getAttrValue("dir", "path", source); 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 bdaa419c6983..2ff47d69f886 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 @@ -272,9 +272,9 @@ public void storagePoolRefresh(StoragePool pool) { } } - 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 nfsopts) 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, nfsopts); _storageLayer.mkdir(targetPath); StoragePool sp = null; try { @@ -654,12 +654,46 @@ public KVMStoragePool createStoragePool(String name, String host, int port, Stri } 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 nfsopts = null; + if (type == StoragePoolType.NetworkFilesystem) { + if (details != null && details.containsKey("nfsopts")) { + nfsopts = Arrays.asList(details.get("nfsopts").replaceAll("\\s", "").split(",")); + } + + if (sp != null && nfsopts != null) { + try { + LibvirtStoragePoolDef poolDef = getStoragePoolDef(conn, sp); + Map poolNfsOptsMap = poolDef.getNfsOpts(); + boolean optionsDiffer = false; + if (poolNfsOptsMap.size() != nfsopts.size()) { + optionsDiffer = true; + } else { + for (String nfsopt : nfsopts) { + if (!poolNfsOptsMap.containsKey(nfsopt)) { + optionsDiffer = true; + break; + } + } + } + if (optionsDiffer == true) { + sp.destroy(); + sp = null; + } + } catch (LibvirtException e) { + s_logger.error("Failure in destroying the pre-existing storage pool for changing the NFS options" + e); + } + } + } + + 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, nfsopts); } catch (LibvirtException e) { s_logger.error("Failed to create netfs mount: " + host + ":" + path , e); s_logger.error(e.getStackTrace()); @@ -667,7 +701,7 @@ public KVMStoragePool createStoragePool(String name, String host, int port, Stri } } 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 51a47e9a025f..3c4e8117b528 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 void testSetGetStoragePool() { assertEquals(port, pool.getSourcePort()); assertEquals(dir, pool.getSourceDir()); assertEquals(targetPath, pool.getTargetPath()); + + List nfsopts = new ArrayList<>(); + nfsopts.add("vers=4.1"); + nfsopts.add("nconnect=4"); + pool = new LibvirtStoragePoolDef(type, name, uuid, host, dir, targetPath, nfsopts); + assertTrue(pool.getNfsOpts().containsKey("vers=4.1")); + assertTrue(pool.getNfsOpts().containsKey("nconnect=4")); + assertEquals(pool.getNfsOpts().size(), 2); } @Test @@ -57,12 +68,17 @@ public void testNfsStoragePool() { String host = "127.0.0.1"; String dir = "/export/primary"; String targetPath = "/mnt/" + uuid; + List nfsopts = new ArrayList<>(); + nfsopts.add("vers=4.1"); + nfsopts.add("nconnect=4"); - LibvirtStoragePoolDef pool = new LibvirtStoragePoolDef(type, name, uuid, host, dir, targetPath); + LibvirtStoragePoolDef pool = new LibvirtStoragePoolDef(type, name, uuid, host, dir, targetPath, nfsopts); - 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()); } 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 3637b7b1f9b1..4d94c332bfae 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 void testParseNfsStoragePoolXML() { " 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.getNfsOpts().containsKey("vers=4.1")); + assertTrue(pool.getNfsOpts().containsKey("nconnect=8")); } @Test 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 d72e4760f572..8127594420bd 100644 --- a/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java +++ b/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java @@ -2994,6 +2994,11 @@ private ListResponse createStoragesPoolResponse(Pair 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); + } + } + @Override public PrimaryDataStoreInfo createPool(CreateStoragePoolCmd cmd) throws ResourceInUseException, IllegalArgumentException, UnknownHostException, ResourceUnavailableException { String providerName = cmd.getStorageProviderName(); @@ -903,6 +918,20 @@ public PrimaryDataStoreInfo createPool(CreateStoragePoolCmd cmd) throws Resource } Map details = extractApiParamAsMap(cmd.getDetails()); + if (details.containsKey("nfsopts")) { + Long accountId = cmd.getEntityOwnerId(); + if (!_accountMgr.isRootAdmin(accountId)) { + throw new PermissionDeniedException("Only root admin can set nfs options"); + } + 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(uriParams.get("scheme"))) { + throw new InvalidParameterValueException("NFS options can only be set on pool type " + StoragePoolType.NetworkFilesystem); + } + checkNfsOptions(details.get("nfsopts")); + } + DataCenterVO zone = _dcDao.findById(cmd.getZoneId()); if (zone == null) { throw new InvalidParameterValueException("unable to find zone by id " + zoneId); @@ -1085,6 +1114,24 @@ public PrimaryDataStoreInfo updateStoragePool(UpdateStoragePoolCmd cmd) throws I throw new IllegalArgumentException("Unable to find storage pool with ID: " + id); } + Map inputDetails = extractApiParamAsMap(cmd.getDetails()); + if (inputDetails.containsKey("nfsopts")) { + Long accountId = cmd.getEntityOwnerId(); + 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"); + } + checkNfsOptions(inputDetails.get("nfsopts")); + } + String name = cmd.getName(); if(StringUtils.isNotBlank(name)) { s_logger.debug("Updating Storage Pool name to: " + name); @@ -1128,12 +1175,9 @@ public PrimaryDataStoreInfo updateStoragePool(UpdateStoragePoolCmd cmd) throws I } // 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; } diff --git a/server/src/main/java/com/cloud/storage/StoragePoolAutomationImpl.java b/server/src/main/java/com/cloud/storage/StoragePoolAutomationImpl.java index 7b5ebc455c6d..bd0492031cd8 100644 --- a/server/src/main/java/com/cloud/storage/StoragePoolAutomationImpl.java +++ b/server/src/main/java/com/cloud/storage/StoragePoolAutomationImpl.java @@ -19,7 +19,9 @@ package com.cloud.storage; import java.util.ArrayList; +import java.util.HashMap; import java.util.List; +import java.util.Map; import javax.inject.Inject; @@ -28,6 +30,8 @@ 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.StoragePoolDetailVO; +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; @@ -88,6 +92,8 @@ public class StoragePoolAutomationImpl implements StoragePoolAutomation { @Inject PrimaryDataStoreDao primaryDataStoreDao; @Inject + StoragePoolDetailsDao storagePoolDetailsDao; + @Inject DataStoreManager dataStoreMgr; @Inject protected ResourceManager _resourceMgr; @@ -318,9 +324,19 @@ public boolean cancelMaintain(DataStore store) { if (hosts == null || hosts.size() == 0) { return true; } + + Map details = null; + if (pool.getPoolType().equals(Storage.StoragePoolType.NetworkFilesystem)) { + details = new HashMap<>(); + StoragePoolDetailVO nfsopts = storagePoolDetailsDao.findDetail(poolVO.getId(), "nfsopts"); + if (nfsopts != null) { + details.put("nfsopts", nfsopts.getValue()); + } + } + // add heartbeat for (HostVO host : hosts) { - ModifyStoragePoolCommand msPoolCmd = new ModifyStoragePoolCommand(true, pool); + ModifyStoragePoolCommand msPoolCmd = new ModifyStoragePoolCommand(true, pool, details); final Answer answer = agentMgr.easySend(host.getId(), msPoolCmd); if (answer == null || !answer.getResult()) { if (s_logger.isDebugEnabled()) { diff --git a/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java b/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java index dbceac9efb18..7ae0b4218b2c 100644 --- a/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java +++ b/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java @@ -249,4 +249,15 @@ public void testEnableDefaultDatastoreDownloadRedirectionForExistingInstallation .update(StorageManager.DataStoreDownloadFollowRedirects.key(),StorageManager.DataStoreDownloadFollowRedirects.defaultValue()); } + @Test(expected = InvalidParameterValueException.class) + public void testDuplicateNFSOptions() { + String nfsopts = "vers=4.1, nconnect=4,vers=4.2"; + storageManagerImpl.checkNfsOptions(nfsopts); + } + + @Test(expected = InvalidParameterValueException.class) + public void testInvalidNFSOptions() { + String nfsopts = "vers=4.1=2,"; + storageManagerImpl.checkNfsOptions(nfsopts); + } } diff --git a/test/integration/smoke/test_primary_storage_nfsopts_kvm.py b/test/integration/smoke/test_primary_storage_nfsopts_kvm.py new file mode 100644 index 000000000000..2b3f06169235 --- /dev/null +++ b/test/integration/smoke/test_primary_storage_nfsopts_kvm.py @@ -0,0 +1,162 @@ +# 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. + +import marvin +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 TestNFSOptsKVM(cloudstackTestCase): + """ Test cases for host HA using KVM host(s) + """ + + def setUp(self): + self.testClient = super(TestNFSOptsKVM, self).getClsTestClient() + self.apiclient = self.testClient.getApiClient() + self.dbclient = self.testClient.getDbConnection() + self.services = self.testClient.getParsedTestDataConfig() + self.logger = logging.getLogger('TestHAKVM') + + self.cluster = list_clusters(self.apiclient)[0] + self.hypervisor = self.testClient.getHypervisorInfo() + self.host = self.getHost() + self.storagePool = self.getPrimaryStorage(self.cluster.id) + self.hostConfig = self.config.__dict__["zones"][0].__dict__["pods"][0].__dict__["clusters"][0].__dict__["hosts"][0].__dict__ + self.cluster_id = self.host.clusterid + self.hostConfig["password"]="P@ssword123" + self.sshClient = SshClient( + host=self.host.ipaddress, + port=22, + user=self.hostConfig['username'], + passwd=self.hostConfig['password']) + + + 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: + self.storage_pool = response[0] + return self.storage_pool + raise self.skipTest("Not enough KVM hosts found, skipping NFS options test") + + def getNFSOptionsFromVirsh(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 getNFSOptionForPool(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 + + @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 + """ + + vers = self.getNFSOptionForPool("vers", self.storage_pool.id) + if (vers == None): + raise self.skipTest("Storage pool not associated with the 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) + nfsopts = "vers=" + version + else: + nconnect='4' + nfsopts = "vers=" + version + ",nconnect=" + nconnect + + details = [{'nfsopts': nfsopts}] + + maint_cmd = enableStorageMaintenance.enableStorageMaintenanceCmd() + maint_cmd.id = self.storage_pool.id + resp = self.apiclient.enableStorageMaintenance(maint_cmd) + + storage = 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) + + storage = StoragePool.list(self.apiclient, + id=self.storage_pool.id + )[0] + + self.assertEqual(storage.nfsopts, nfsopts) + + options = self.getNFSOptionsFromVirsh(self.storage_pool.id) + + self.assertEqual(options["vers"], version) + if (nconnect != None): + self.assertEqual(options["nconnect"], nconnect) diff --git a/ui/public/locales/en.json b/ui/public/locales/en.json index be9d4e720566..2a02c84e44c7 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", @@ -1416,6 +1417,7 @@ "label.newname": "New name", "label.next": "Next", "label.nfs": "NFS", +"label.nfsopts": "NFS mount options", "label.nfsserver": "NFS server", "label.nic": "NIC", "label.nicadaptertype": "NIC adapter type", @@ -2440,6 +2442,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.options": "Please confirm that you want to change NFS mount options.
This will cause the storage pool to be remounted on all KVM hosts.", "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.", @@ -3013,6 +3016,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.nfsopts.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.", @@ -3184,6 +3188,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.nfsopts": "Successfully updated NFS options", "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 f222edeaf70b..d2046872a5d3 100644 --- a/ui/src/config/section/infra/primaryStorages.js +++ b/ui/src/config/section/infra/primaryStorages.js @@ -34,7 +34,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', 'scope', 'tags', 'path', 'provider', 'hypervisor', 'overprovisionfactor', 'disksizetotal', 'disksizeallocated', 'disksizeused', 'clustername', 'podname', 'zonename', 'nfsopts', 'created'], related: [{ name: 'volume', title: 'label.volumes', @@ -109,6 +109,16 @@ export default { defaultArgs: { enabled: true }, show: (record) => { return record.state === 'Disabled' } }, + { + api: 'updateStoragePool', + icon: 'control-outlined', + label: 'label.action.edit.nfs.options', + message: 'message.action.edit.nfs.options', + dataView: true, + popup: true, + show: (record) => { return (record.type === 'NetworkFilesystem' && record.hypervisor === 'KVM' && record.state === 'Maintenance') }, + component: shallowRef(defineAsyncComponent(() => import('@/views/infra/NFSOptsPrimaryStorage.vue'))) + }, { api: 'syncStoragePool', icon: 'sync-outlined', diff --git a/ui/src/views/infra/AddPrimaryStorage.vue b/ui/src/views/infra/AddPrimaryStorage.vue index 730a806307c5..55ab064bf92f 100644 --- a/ui/src/views/infra/AddPrimaryStorage.vue +++ b/ui/src/views/infra/AddPrimaryStorage.vue @@ -178,6 +178,17 @@ +
+ + + + +
@@ -794,6 +805,7 @@ export default { var url = '' if (values.protocol === 'nfs') { url = this.nfsURL(server, path) + params['details[0].nfsopts'] = values.nfsopts } else if (values.protocol === 'SMB') { url = this.smbURL(server, path) const smbParams = { diff --git a/ui/src/views/infra/NFSOptsPrimaryStorage.vue b/ui/src/views/infra/NFSOptsPrimaryStorage.vue new file mode 100644 index 000000000000..3f72c9f25500 --- /dev/null +++ b/ui/src/views/infra/NFSOptsPrimaryStorage.vue @@ -0,0 +1,147 @@ +// 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 24ddd264486e..091047d9ace0 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.nfsopts', + key: 'primaryStorageNFSOptions', + 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 929b0bf02cdd..8ec7e582390c 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].nfsopts'] = this.prefillContent.primaryStorageNFSOptions } else if (protocol === 'SMB') { let path = this.prefillContent?.primaryStoragePath || '' if (path.substring(0, 1) !== '/') { From 6766115126a9d1e0ff7d3c5409b0eecf9d5352ec Mon Sep 17 00:00:00 2001 From: abh1sar Date: Tue, 23 Apr 2024 23:09:00 +0530 Subject: [PATCH 02/23] Pull 8947: Rename all occurrence of nfsopt to nfsMountOpt and added nfsMountOpts to ApiConstants --- .../apache/cloudstack/api/ApiConstants.java | 2 ++ .../api/response/StoragePoolResponse.java | 14 ++++---- .../provider/DefaultHostListener.java | 8 +++-- .../kvm/resource/LibvirtStoragePoolDef.java | 20 +++++------ .../resource/LibvirtStoragePoolXMLParser.java | 6 ++-- .../kvm/storage/LibvirtStorageAdaptor.java | 33 ++++++++++--------- .../resource/LibvirtStoragePoolDefTest.java | 22 ++++++------- .../LibvirtStoragePoolXMLParserTest.java | 4 +-- .../com/cloud/api/query/QueryManagerImpl.java | 4 +-- .../com/cloud/storage/StorageManagerImpl.java | 12 +++---- .../storage/StoragePoolAutomationImpl.java | 7 ++-- .../cloud/storage/StorageManagerImplTest.java | 12 +++---- ...est_primary_storage_nfs_mount_opts_kvm.py} | 20 +++++------ ui/public/locales/en.json | 6 ++-- .../config/section/infra/primaryStorages.js | 4 +-- ui/src/views/infra/AddPrimaryStorage.vue | 8 ++--- ...age.vue => NFSMountOptsPrimaryStorage.vue} | 10 +++--- .../infra/zone/ZoneWizardAddResources.vue | 4 +-- .../views/infra/zone/ZoneWizardLaunchZone.vue | 2 +- 19 files changed, 102 insertions(+), 96 deletions(-) rename test/integration/smoke/{test_primary_storage_nfsopts_kvm.py => test_primary_storage_nfs_mount_opts_kvm.py} (90%) rename ui/src/views/infra/{NFSOptsPrimaryStorage.vue => NFSMountOptsPrimaryStorage.vue} (89%) 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 18d25a0cfc3f..565ee88cdb95 100644 --- a/api/src/main/java/org/apache/cloudstack/api/ApiConstants.java +++ b/api/src/main/java/org/apache/cloudstack/api/ApiConstants.java @@ -1094,6 +1094,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 bb74ed85190c..058ef7b7baed 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,9 +101,9 @@ public class StoragePoolResponse extends BaseResponseWithAnnotations { @Param(description = "the tags for the storage pool") private String tags; - @SerializedName("nfsopts") - @Param(description = "the nfs options for the storage pool") - private String nfsopts; + @SerializedName(ApiConstants.NFS_MOUNT_OPTIONS) + @Param(description = "the nfs mount options for the storage pool") + private String nfsMountOpts; @SerializedName(ApiConstants.IS_TAG_A_RULE) @Param(description = ApiConstants.PARAMETER_DESCRIPTION_IS_TAG_A_RULE) @@ -352,11 +352,11 @@ public void setProvider(String provider) { this.provider = provider; } - public String getNfsopts() { - return nfsopts; + public String getNfsMountOpts() { + return nfsMountOpts; } - public void setNfsopts(String nfsopts) { - this.nfsopts = nfsopts; + public void setNfsMountOpts(String nfsMountOpts) { + this.nfsMountOpts = nfsMountOpts; } } 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 a75e2c2809ea..5b1adcc7bb51 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 @@ -43,6 +43,8 @@ import com.cloud.storage.StorageService; import com.cloud.storage.dao.StoragePoolHostDao; import com.cloud.utils.exception.CloudRuntimeException; + +import org.apache.cloudstack.api.ApiConstants; 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; @@ -131,9 +133,9 @@ public boolean hostConnect(long hostId, long poolId) throws StorageConflictExcep Map details = null; if (pool.getPoolType().equals(Storage.StoragePoolType.NetworkFilesystem)) { details = new HashMap<>(); - StoragePoolDetailVO nfsopts = storagePoolDetailsDao.findDetail(poolId, "nfsopts"); - if (nfsopts != null) { - details.put("nfsopts", nfsopts.getValue()); + StoragePoolDetailVO nfsMountOpts = storagePoolDetailsDao.findDetail(poolId, ApiConstants.NFS_MOUNT_OPTIONS); + if (nfsMountOpts != null) { + details.put(ApiConstants.NFS_MOUNT_OPTIONS, nfsMountOpts.getValue()); } } 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 d78b1cae9e13..c6f8e6e010d1 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 @@ -59,7 +59,7 @@ public String toString() { private String _authUsername; private AuthenticationType _authType; private String _secretUuid; - private Map _nfsopts = new HashMap<>(); + private Map _nfsMountOpts = new HashMap<>(); public LibvirtStoragePoolDef(PoolType type, String poolName, String uuid, String host, int port, String dir, String targetPath) { _poolType = type; @@ -80,11 +80,11 @@ public LibvirtStoragePoolDef(PoolType type, String poolName, String uuid, String _targetPath = targetPath; } - public LibvirtStoragePoolDef(PoolType type, String poolName, String uuid, String host, String dir, String targetPath, List nfsopts) { + public LibvirtStoragePoolDef(PoolType type, String poolName, String uuid, String host, String dir, String targetPath, List nfsMountOpts) { this(type, poolName, uuid, host, dir, targetPath); - if (nfsopts != null) { - for (String nfsopt : nfsopts) { - this._nfsopts.put(nfsopt, null); + if (nfsMountOpts != null) { + for (String nfsMountOpt : nfsMountOpts) { + this._nfsMountOpts.put(nfsMountOpt, null); } } } @@ -138,14 +138,14 @@ public AuthenticationType getAuthType() { return _authType; } - public Map getNfsOpts() { - return _nfsopts; + public Map getNfsMountOpts() { + return _nfsMountOpts; } @Override public String toString() { StringBuilder storagePoolBuilder = new StringBuilder(); - if (_poolType == PoolType.NETFS && _nfsopts != null) { + if (_poolType == PoolType.NETFS && _nfsMountOpts != null) { // get from poolType storagePoolBuilder.append("\n"); } else if (_poolType == PoolType.GLUSTERFS) { @@ -208,9 +208,9 @@ public String toString() { storagePoolBuilder.append("" + _targetPath + "\n"); storagePoolBuilder.append("\n"); } - if (_poolType == PoolType.NETFS && _nfsopts != null) { + if (_poolType == PoolType.NETFS && _nfsMountOpts != null) { storagePoolBuilder.append("\n"); - for (Map.Entry options : _nfsopts.entrySet()) { + for (Map.Entry options : _nfsMountOpts.entrySet()) { storagePoolBuilder.append("\n"); } storagePoolBuilder.append("\n"); 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 13167958388d..a8a3c69763be 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 @@ -93,13 +93,13 @@ public LibvirtStoragePoolDef parseStoragePoolXML(String poolXML) { return new LibvirtStoragePoolDef(LibvirtStoragePoolDef.PoolType.valueOf(format.toUpperCase()), poolName, uuid, host, port, path, targetPath); } else if (type.equalsIgnoreCase("netfs")) { - List nfsopts = new ArrayList<>(); + 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); - nfsopts.add(option.getAttribute("name")); + nfsMountOpts.add(option.getAttribute("name")); } } @@ -107,7 +107,7 @@ public LibvirtStoragePoolDef parseStoragePoolXML(String poolXML) { 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, nfsopts); + return new LibvirtStoragePoolDef(LibvirtStoragePoolDef.PoolType.valueOf(type.toUpperCase()), poolName, uuid, host, path, targetPath, nfsMountOpts); } else { String path = getAttrValue("dir", "path", source); 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 2ff47d69f886..eeb8fb26103b 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.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; @@ -272,9 +273,9 @@ public void storagePoolRefresh(StoragePool pool) { } } - private StoragePool createNetfsStoragePool(PoolType fsType, Connect conn, String uuid, String host, String path, List nfsopts) 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, nfsopts); + LibvirtStoragePoolDef spd = new LibvirtStoragePoolDef(fsType, uuid, uuid, host, path, targetPath, nfsMountOpts); _storageLayer.mkdir(targetPath); StoragePool sp = null; try { @@ -656,33 +657,33 @@ public KVMStoragePool createStoragePool(String name, String host, int port, Stri } } - List nfsopts = null; + List nfsMountOpts = null; if (type == StoragePoolType.NetworkFilesystem) { - if (details != null && details.containsKey("nfsopts")) { - nfsopts = Arrays.asList(details.get("nfsopts").replaceAll("\\s", "").split(",")); + if (details != null && details.containsKey(ApiConstants.NFS_MOUNT_OPTIONS)) { + nfsMountOpts = Arrays.asList(details.get(ApiConstants.NFS_MOUNT_OPTIONS).replaceAll("\\s", "").split(",")); } - if (sp != null && nfsopts != null) { + if (sp != null && nfsMountOpts != null) { try { LibvirtStoragePoolDef poolDef = getStoragePoolDef(conn, sp); - Map poolNfsOptsMap = poolDef.getNfsOpts(); - boolean optionsDiffer = false; - if (poolNfsOptsMap.size() != nfsopts.size()) { - optionsDiffer = true; + Map poolNfsMountOptsMap = poolDef.getNfsMountOpts(); + boolean mountOptsDiffer = false; + if (poolNfsMountOptsMap.size() != nfsMountOpts.size()) { + mountOptsDiffer = true; } else { - for (String nfsopt : nfsopts) { - if (!poolNfsOptsMap.containsKey(nfsopt)) { - optionsDiffer = true; + for (String nfsMountOpt : nfsMountOpts) { + if (!poolNfsMountOptsMap.containsKey(nfsMountOpt)) { + mountOptsDiffer = true; break; } } } - if (optionsDiffer == true) { + if (mountOptsDiffer == true) { sp.destroy(); sp = null; } } catch (LibvirtException e) { - s_logger.error("Failure in destroying the pre-existing storage pool for changing the NFS options" + e); + s_logger.error("Failure in destroying the pre-existing storage pool for changing the NFS mount options" + e); } } } @@ -693,7 +694,7 @@ public KVMStoragePool createStoragePool(String name, String host, int port, Stri if (type == StoragePoolType.NetworkFilesystem) { try { - sp = createNetfsStoragePool(PoolType.NETFS, conn, name, host, path, nfsopts); + 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()); 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 3c4e8117b528..818f4bdc253e 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 @@ -51,13 +51,13 @@ public void testSetGetStoragePool() { assertEquals(dir, pool.getSourceDir()); assertEquals(targetPath, pool.getTargetPath()); - List nfsopts = new ArrayList<>(); - nfsopts.add("vers=4.1"); - nfsopts.add("nconnect=4"); - pool = new LibvirtStoragePoolDef(type, name, uuid, host, dir, targetPath, nfsopts); - assertTrue(pool.getNfsOpts().containsKey("vers=4.1")); - assertTrue(pool.getNfsOpts().containsKey("nconnect=4")); - assertEquals(pool.getNfsOpts().size(), 2); + 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().containsKey("vers=4.1")); + assertTrue(pool.getNfsMountOpts().containsKey("nconnect=4")); + assertEquals(pool.getNfsMountOpts().size(), 2); } @Test @@ -68,11 +68,11 @@ public void testNfsStoragePool() { String host = "127.0.0.1"; String dir = "/export/primary"; String targetPath = "/mnt/" + uuid; - List nfsopts = new ArrayList<>(); - nfsopts.add("vers=4.1"); - nfsopts.add("nconnect=4"); + List nfsMountOpts = new ArrayList<>(); + nfsMountOpts.add("vers=4.1"); + nfsMountOpts.add("nconnect=4"); - LibvirtStoragePoolDef pool = new LibvirtStoragePoolDef(type, name, uuid, host, dir, targetPath, nfsopts); + LibvirtStoragePoolDef pool = new LibvirtStoragePoolDef(type, name, uuid, host, dir, targetPath, nfsMountOpts); String expectedXml = "\n" + "" +name + "\n" + uuid + "\n" + 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 4d94c332bfae..b9215259205a 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 @@ -59,8 +59,8 @@ public void testParseNfsStoragePoolXML() { LibvirtStoragePoolDef pool = parser.parseStoragePoolXML(poolXML); assertEquals("10.11.12.13", pool.getSourceHost()); - assertTrue(pool.getNfsOpts().containsKey("vers=4.1")); - assertTrue(pool.getNfsOpts().containsKey("nconnect=8")); + assertTrue(pool.getNfsMountOpts().containsKey("vers=4.1")); + assertTrue(pool.getNfsMountOpts().containsKey("nconnect=8")); } @Test 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 8127594420bd..9c10749ef1d0 100644 --- a/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java +++ b/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java @@ -2996,8 +2996,8 @@ private ListResponse createStoragesPoolResponse(Pair optionsMap = new HashMap<>(); for (String option : options) { String[] keyValue = option.split("="); @@ -918,7 +918,7 @@ public PrimaryDataStoreInfo createPool(CreateStoragePoolCmd cmd) throws Resource } Map details = extractApiParamAsMap(cmd.getDetails()); - if (details.containsKey("nfsopts")) { + if (details.containsKey(ApiConstants.NFS_MOUNT_OPTIONS)) { Long accountId = cmd.getEntityOwnerId(); if (!_accountMgr.isRootAdmin(accountId)) { throw new PermissionDeniedException("Only root admin can set nfs options"); @@ -929,7 +929,7 @@ public PrimaryDataStoreInfo createPool(CreateStoragePoolCmd cmd) throws Resource if (!"nfs".equals(uriParams.get("scheme"))) { throw new InvalidParameterValueException("NFS options can only be set on pool type " + StoragePoolType.NetworkFilesystem); } - checkNfsOptions(details.get("nfsopts")); + checkNfsMountOptions(details.get(ApiConstants.NFS_MOUNT_OPTIONS)); } DataCenterVO zone = _dcDao.findById(cmd.getZoneId()); @@ -1115,7 +1115,7 @@ public PrimaryDataStoreInfo updateStoragePool(UpdateStoragePoolCmd cmd) throws I } Map inputDetails = extractApiParamAsMap(cmd.getDetails()); - if (inputDetails.containsKey("nfsopts")) { + if (inputDetails.containsKey(ApiConstants.NFS_MOUNT_OPTIONS)) { Long accountId = cmd.getEntityOwnerId(); if (!_accountMgr.isRootAdmin(accountId)) { throw new PermissionDeniedException("Only root admin can modify nfs options"); @@ -1129,7 +1129,7 @@ public PrimaryDataStoreInfo updateStoragePool(UpdateStoragePoolCmd cmd) throws I if (!pool.isInMaintenance()) { throw new InvalidParameterValueException("The storage pool should be in maintenance mode to edit nfs options"); } - checkNfsOptions(inputDetails.get("nfsopts")); + checkNfsMountOptions(inputDetails.get(ApiConstants.NFS_MOUNT_OPTIONS)); } String name = cmd.getName(); diff --git a/server/src/main/java/com/cloud/storage/StoragePoolAutomationImpl.java b/server/src/main/java/com/cloud/storage/StoragePoolAutomationImpl.java index bd0492031cd8..f9634bffb4c4 100644 --- a/server/src/main/java/com/cloud/storage/StoragePoolAutomationImpl.java +++ b/server/src/main/java/com/cloud/storage/StoragePoolAutomationImpl.java @@ -25,6 +25,7 @@ import javax.inject.Inject; +import org.apache.cloudstack.api.ApiConstants; import org.apache.cloudstack.context.CallContext; import org.apache.cloudstack.engine.subsystem.api.storage.DataStore; import org.apache.cloudstack.engine.subsystem.api.storage.DataStoreManager; @@ -328,9 +329,9 @@ public boolean cancelMaintain(DataStore store) { Map details = null; if (pool.getPoolType().equals(Storage.StoragePoolType.NetworkFilesystem)) { details = new HashMap<>(); - StoragePoolDetailVO nfsopts = storagePoolDetailsDao.findDetail(poolVO.getId(), "nfsopts"); - if (nfsopts != null) { - details.put("nfsopts", nfsopts.getValue()); + StoragePoolDetailVO nfsMountOpts = storagePoolDetailsDao.findDetail(poolVO.getId(), ApiConstants.NFS_MOUNT_OPTIONS); + if (nfsMountOpts != null) { + details.put(ApiConstants.NFS_MOUNT_OPTIONS, nfsMountOpts.getValue()); } } diff --git a/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java b/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java index 7ae0b4218b2c..5b86d8482c1c 100644 --- a/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java +++ b/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java @@ -250,14 +250,14 @@ public void testEnableDefaultDatastoreDownloadRedirectionForExistingInstallation } @Test(expected = InvalidParameterValueException.class) - public void testDuplicateNFSOptions() { - String nfsopts = "vers=4.1, nconnect=4,vers=4.2"; - storageManagerImpl.checkNfsOptions(nfsopts); + public void testDuplicateNFSMountOptions() { + String nfsMountOpts = "vers=4.1, nconnect=4,vers=4.2"; + storageManagerImpl.checkNfsMountOptions(nfsMountOpts); } @Test(expected = InvalidParameterValueException.class) - public void testInvalidNFSOptions() { - String nfsopts = "vers=4.1=2,"; - storageManagerImpl.checkNfsOptions(nfsopts); + public void testInvalidNFSMountOptions() { + String nfsMountOpts = "vers=4.1=2,"; + storageManagerImpl.checkNfsMountOptions(nfsMountOpts); } } diff --git a/test/integration/smoke/test_primary_storage_nfsopts_kvm.py b/test/integration/smoke/test_primary_storage_nfs_mount_opts_kvm.py similarity index 90% rename from test/integration/smoke/test_primary_storage_nfsopts_kvm.py rename to test/integration/smoke/test_primary_storage_nfs_mount_opts_kvm.py index 2b3f06169235..f994b9aeb0e7 100644 --- a/test/integration/smoke/test_primary_storage_nfsopts_kvm.py +++ b/test/integration/smoke/test_primary_storage_nfs_mount_opts_kvm.py @@ -25,12 +25,12 @@ from lxml import etree from nose.plugins.attrib import attr -class TestNFSOptsKVM(cloudstackTestCase): +class TestNFSMountOptsKVM(cloudstackTestCase): """ Test cases for host HA using KVM host(s) """ def setUp(self): - self.testClient = super(TestNFSOptsKVM, self).getClsTestClient() + self.testClient = super(TestNFSMountOptsKVM, self).getClsTestClient() self.apiclient = self.testClient.getApiClient() self.dbclient = self.testClient.getDbConnection() self.services = self.testClient.getParsedTestDataConfig() @@ -77,7 +77,7 @@ def getPrimaryStorage(self, clusterId): return self.storage_pool raise self.skipTest("Not enough KVM hosts found, skipping NFS options test") - def getNFSOptionsFromVirsh(self, poolId): + def getNFSMountOptionsFromVirsh(self, poolId): virsh_cmd = "virsh pool-dumpxml %s" % poolId xml_res = self.sshClient.execute(virsh_cmd) xml_as_str = ''.join(xml_res) @@ -105,7 +105,7 @@ def getUnusedNFSVersions(self, filter): return key return None - def getNFSOptionForPool(self, option, poolId): + def getNFSMountOptionForPool(self, option, poolId): nfsstat_cmd = "nfsstat -m | sed -n '/%s/{ n; p }'" % poolId nfsstat = self.sshClient.execute(nfsstat_cmd) if (nfsstat == None): @@ -120,7 +120,7 @@ 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 """ - vers = self.getNFSOptionForPool("vers", self.storage_pool.id) + vers = self.getNFSMountOptionForPool("vers", self.storage_pool.id) if (vers == None): raise self.skipTest("Storage pool not associated with the host") @@ -129,12 +129,12 @@ def test_primary_storage_nfs_options_kvm(self): 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) - nfsopts = "vers=" + version + nfsMountOpts = "vers=" + version else: nconnect='4' - nfsopts = "vers=" + version + ",nconnect=" + nconnect + nfsMountOpts = "vers=" + version + ",nconnect=" + nconnect - details = [{'nfsopts': nfsopts}] + details = [{'nfsmountopts': nfsMountOpts}] maint_cmd = enableStorageMaintenance.enableStorageMaintenanceCmd() maint_cmd.id = self.storage_pool.id @@ -153,9 +153,9 @@ def test_primary_storage_nfs_options_kvm(self): id=self.storage_pool.id )[0] - self.assertEqual(storage.nfsopts, nfsopts) + self.assertEqual(storage.nfsmountopts, nfsMountOpts) - options = self.getNFSOptionsFromVirsh(self.storage_pool.id) + options = self.getNFSMountOptionsFromVirsh(self.storage_pool.id) self.assertEqual(options["vers"], version) if (nconnect != None): diff --git a/ui/public/locales/en.json b/ui/public/locales/en.json index 2a02c84e44c7..448a471dc396 100644 --- a/ui/public/locales/en.json +++ b/ui/public/locales/en.json @@ -1417,7 +1417,7 @@ "label.newname": "New name", "label.next": "Next", "label.nfs": "NFS", -"label.nfsopts": "NFS mount options", +"label.nfsmountopts": "NFS mount options", "label.nfsserver": "NFS server", "label.nic": "NIC", "label.nicadaptertype": "NIC adapter type", @@ -3016,7 +3016,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.nfsopts.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.nfs.nfsmountopts.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.", @@ -3188,7 +3188,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.nfsopts": "Successfully updated NFS options", +"message.success.edit.nfsmountopts": "Successfully updated NFS mount options", "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 d2046872a5d3..f445fa57d515 100644 --- a/ui/src/config/section/infra/primaryStorages.js +++ b/ui/src/config/section/infra/primaryStorages.js @@ -34,7 +34,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', 'nfsopts', 'created'], + details: ['name', 'id', 'ipaddress', 'type', 'scope', 'tags', 'path', 'provider', 'hypervisor', 'overprovisionfactor', 'disksizetotal', 'disksizeallocated', 'disksizeused', 'clustername', 'podname', 'zonename', 'nfsmountopts', 'created'], related: [{ name: 'volume', title: 'label.volumes', @@ -117,7 +117,7 @@ export default { dataView: true, popup: true, show: (record) => { return (record.type === 'NetworkFilesystem' && record.hypervisor === 'KVM' && record.state === 'Maintenance') }, - component: shallowRef(defineAsyncComponent(() => import('@/views/infra/NFSOptsPrimaryStorage.vue'))) + component: shallowRef(defineAsyncComponent(() => import('@/views/infra/NFSMountOptsPrimaryStorage.vue'))) }, { api: 'syncStoragePool', diff --git a/ui/src/views/infra/AddPrimaryStorage.vue b/ui/src/views/infra/AddPrimaryStorage.vue index 55ab064bf92f..2905aacf9443 100644 --- a/ui/src/views/infra/AddPrimaryStorage.vue +++ b/ui/src/views/infra/AddPrimaryStorage.vue @@ -182,11 +182,11 @@ v-if="form.protocol === 'nfs' && ((form.scope === 'zone' && (form.hypervisor === 'KVM' || form.hypervisor === 'Simulator')) || (form.scope === 'cluster' && (hypervisorType === 'KVM' || hypervisorType === 'Simulator')))"> - + - +
@@ -805,7 +805,7 @@ export default { var url = '' if (values.protocol === 'nfs') { url = this.nfsURL(server, path) - params['details[0].nfsopts'] = values.nfsopts + 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/NFSOptsPrimaryStorage.vue b/ui/src/views/infra/NFSMountOptsPrimaryStorage.vue similarity index 89% rename from ui/src/views/infra/NFSOptsPrimaryStorage.vue rename to ui/src/views/infra/NFSMountOptsPrimaryStorage.vue index 3f72c9f25500..98c080cb2d41 100644 --- a/ui/src/views/infra/NFSOptsPrimaryStorage.vue +++ b/ui/src/views/infra/NFSMountOptsPrimaryStorage.vue @@ -31,11 +31,11 @@

- + - +
{{ $t('label.cancel') }} @@ -92,7 +92,7 @@ export default { var params = { id: this.resource.id } - params['details[0].nfsopts'] = values.nfsopts + params['details[0].nfsmountopts'] = values.nfsMountOpts this.updateStoragePool(params) }).catch(error => { @@ -104,7 +104,7 @@ export default { }, updateStoragePool (args) { api('updateStoragePool', args).then(json => { - this.$message.success(`${this.$t('message.success.edit.nfsopts')}: ${this.resource.name}`) + this.$message.success(`${this.$t('message.success.edit.nfsmountopts')}: ${this.resource.name}`) this.$emit('refresh-data') this.closeAction() }).catch(error => { diff --git a/ui/src/views/infra/zone/ZoneWizardAddResources.vue b/ui/src/views/infra/zone/ZoneWizardAddResources.vue index 091047d9ace0..ef2cba1c3d9b 100644 --- a/ui/src/views/infra/zone/ZoneWizardAddResources.vue +++ b/ui/src/views/infra/zone/ZoneWizardAddResources.vue @@ -531,8 +531,8 @@ export default { } }, { - title: 'label.nfsopts', - key: 'primaryStorageNFSOptions', + title: 'label.nfsmountopts', + key: 'primaryStorageNFSMountOptions', required: false, display: { primaryStorageProtocol: 'nfs', diff --git a/ui/src/views/infra/zone/ZoneWizardLaunchZone.vue b/ui/src/views/infra/zone/ZoneWizardLaunchZone.vue index 8ec7e582390c..56997e76f83f 100644 --- a/ui/src/views/infra/zone/ZoneWizardLaunchZone.vue +++ b/ui/src/views/infra/zone/ZoneWizardLaunchZone.vue @@ -1387,7 +1387,7 @@ export default { path = '/' + path } url = this.nfsURL(server, path) - params['details[0].nfsopts'] = this.prefillContent.primaryStorageNFSOptions + params['details[0].nfsmountopts'] = this.prefillContent.primaryStorageNFSMountOptions } else if (protocol === 'SMB') { let path = this.prefillContent?.primaryStoragePath || '' if (path.substring(0, 1) !== '/') { From d213df3d6c241fe40127641f7c76eae55887dd2f Mon Sep 17 00:00:00 2001 From: abh1sar Date: Wed, 24 Apr 2024 01:23:12 +0530 Subject: [PATCH 03/23] Pull 8947: Refactor code - move into separate methods --- .../com/cloud/storage/StorageManager.java | 3 + .../provider/DefaultHostListener.java | 11 +-- .../resource/LibvirtStoragePoolXMLParser.java | 37 +++++----- .../kvm/storage/LibvirtStorageAdaptor.java | 67 +++++++++-------- .../com/cloud/api/query/QueryManagerImpl.java | 16 +++-- .../com/cloud/storage/StorageManagerImpl.java | 71 +++++++++++-------- .../storage/StoragePoolAutomationImpl.java | 13 +--- 7 files changed, 118 insertions(+), 100 deletions(-) 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 b1594e315bb3..c4a8b5c8a3c3 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 @@ 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,8 @@ static Boolean getFullCloneConfiguration(Long storeId) { boolean registerHostListener(String providerUuid, HypervisorHostListener listener); + void addStoragePoolNFSMountOptsToDetailsMap(StoragePool pool, Map details); + 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 5b1adcc7bb51..692afc34500d 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 @@ -44,7 +44,6 @@ import com.cloud.storage.dao.StoragePoolHostDao; import com.cloud.utils.exception.CloudRuntimeException; -import org.apache.cloudstack.api.ApiConstants; 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; @@ -130,14 +129,8 @@ private NicTO createNicTOFromNetworkAndOffering(NetworkVO networkVO, NetworkOffe @Override public boolean hostConnect(long hostId, long poolId) throws StorageConflictException { StoragePool pool = (StoragePool) this.dataStoreMgr.getDataStore(poolId, DataStoreRole.Primary); - Map details = null; - if (pool.getPoolType().equals(Storage.StoragePoolType.NetworkFilesystem)) { - details = new HashMap<>(); - StoragePoolDetailVO nfsMountOpts = storagePoolDetailsDao.findDetail(poolId, ApiConstants.NFS_MOUNT_OPTIONS); - if (nfsMountOpts != null) { - details.put(ApiConstants.NFS_MOUNT_OPTIONS, nfsMountOpts.getValue()); - } - } + Map details = new HashMap<>(); + storageManager.addStoragePoolNFSMountOptsToDetailsMap(pool, details); ModifyStoragePoolCommand cmd = new ModifyStoragePoolCommand(true, pool, details); cmd.setWait(modifyStoragePoolCommandWait); 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 a8a3c69763be..a39ff07708e8 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 @@ 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 { @@ -92,29 +105,17 @@ public LibvirtStoragePoolDef parseStoragePoolXML(String poolXML) { return new LibvirtStoragePoolDef(LibvirtStoragePoolDef.PoolType.valueOf(format.toUpperCase()), poolName, uuid, host, port, path, targetPath); - } else if (type.equalsIgnoreCase("netfs")) { - 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")); - } - } - - 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, nfsMountOpts); } 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 eeb8fb26103b..331e2146fd9b 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 @@ -364,6 +364,41 @@ private StoragePool createCLVMStoragePool(Connect conn, String uuid, String host } + private List getNFSMountOptsFromDetails(StoragePoolType type, Map details) { + List nfsMountOpts = null; + if (type == StoragePoolType.NetworkFilesystem) { + if (details != null && details.containsKey(ApiConstants.NFS_MOUNT_OPTIONS)) { + nfsMountOpts = Arrays.asList(details.get(ApiConstants.NFS_MOUNT_OPTIONS).replaceAll("\\s", "").split(",")); + } + } + return nfsMountOpts; + } + + private boolean destroyStoragePoolForNFSMountOptions(StoragePool sp, Connect conn, List nfsMountOpts) { + try { + LibvirtStoragePoolDef poolDef = getStoragePoolDef(conn, sp); + Map poolNfsMountOptsMap = poolDef.getNfsMountOpts(); + boolean mountOptsDiffer = false; + if (poolNfsMountOptsMap.size() != nfsMountOpts.size()) { + mountOptsDiffer = true; + } else { + for (String nfsMountOpt : nfsMountOpts) { + if (!poolNfsMountOptsMap.containsKey(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; @@ -657,34 +692,10 @@ public KVMStoragePool createStoragePool(String name, String host, int port, Stri } } - List nfsMountOpts = null; - if (type == StoragePoolType.NetworkFilesystem) { - if (details != null && details.containsKey(ApiConstants.NFS_MOUNT_OPTIONS)) { - nfsMountOpts = Arrays.asList(details.get(ApiConstants.NFS_MOUNT_OPTIONS).replaceAll("\\s", "").split(",")); - } - - if (sp != null && nfsMountOpts != null) { - try { - LibvirtStoragePoolDef poolDef = getStoragePoolDef(conn, sp); - Map poolNfsMountOptsMap = poolDef.getNfsMountOpts(); - boolean mountOptsDiffer = false; - if (poolNfsMountOptsMap.size() != nfsMountOpts.size()) { - mountOptsDiffer = true; - } else { - for (String nfsMountOpt : nfsMountOpts) { - if (!poolNfsMountOptsMap.containsKey(nfsMountOpt)) { - mountOptsDiffer = true; - break; - } - } - } - if (mountOptsDiffer == true) { - sp.destroy(); - sp = null; - } - } catch (LibvirtException e) { - s_logger.error("Failure in destroying the pre-existing storage pool for changing the NFS mount options" + e); - } + List nfsMountOpts = getNFSMountOptsFromDetails(type, details); + if (sp != null && nfsMountOpts != null) { + if (destroyStoragePoolForNFSMountOptions(sp, conn, nfsMountOpts)) { + sp = null; } } 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 9c10749ef1d0..ea6fb1abe366 100644 --- a/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java +++ b/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java @@ -2973,6 +2973,16 @@ private Pair, Integer> searchForLocalStorages(ListStorag 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<>(); @@ -2994,11 +3004,7 @@ private ListResponse createStoragesPoolResponse(Pair details, HypervisorType hypervisorType, String scheme) throws InvalidParameterValueException { + if (details.containsKey(ApiConstants.NFS_MOUNT_OPTIONS)) { + 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)) { + 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(); @@ -918,19 +948,7 @@ public PrimaryDataStoreInfo createPool(CreateStoragePoolCmd cmd) throws Resource } Map details = extractApiParamAsMap(cmd.getDetails()); - if (details.containsKey(ApiConstants.NFS_MOUNT_OPTIONS)) { - Long accountId = cmd.getEntityOwnerId(); - if (!_accountMgr.isRootAdmin(accountId)) { - throw new PermissionDeniedException("Only root admin can set nfs options"); - } - 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(uriParams.get("scheme"))) { - throw new InvalidParameterValueException("NFS options can only be set on pool type " + StoragePoolType.NetworkFilesystem); - } - checkNfsMountOptions(details.get(ApiConstants.NFS_MOUNT_OPTIONS)); - } + checkNFSMountOptionsForCreate(details, hypervisorType, uriParams.get("scheme")); DataCenterVO zone = _dcDao.findById(cmd.getZoneId()); if (zone == null) { @@ -1115,22 +1133,7 @@ public PrimaryDataStoreInfo updateStoragePool(UpdateStoragePoolCmd cmd) throws I } Map inputDetails = extractApiParamAsMap(cmd.getDetails()); - if (inputDetails.containsKey(ApiConstants.NFS_MOUNT_OPTIONS)) { - Long accountId = cmd.getEntityOwnerId(); - 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(inputDetails.get(ApiConstants.NFS_MOUNT_OPTIONS)); - } + checkNFSMountOptionsForUpdate(inputDetails, pool, cmd.getEntityOwnerId()); String name = cmd.getName(); if(StringUtils.isNotBlank(name)) { @@ -1276,6 +1279,16 @@ public void doInTransactionWithoutResult(TransactionStatus status) { return deleteDataStoreInternal(sPool, forced); } + @Override + public void addStoragePoolNFSMountOptsToDetailsMap(StoragePool pool, Map details) { + if (pool.getPoolType().equals(Storage.StoragePoolType.NetworkFilesystem)) { + StoragePoolDetailVO nfsMountOpts = _storagePoolDetailsDao.findDetail(pool.getId(), ApiConstants.NFS_MOUNT_OPTIONS); + if (nfsMountOpts != null) { + details.put(ApiConstants.NFS_MOUNT_OPTIONS, nfsMountOpts.getValue()); + } + } + } + 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 f9634bffb4c4..4ccdfbb152f9 100644 --- a/server/src/main/java/com/cloud/storage/StoragePoolAutomationImpl.java +++ b/server/src/main/java/com/cloud/storage/StoragePoolAutomationImpl.java @@ -25,13 +25,11 @@ import javax.inject.Inject; -import org.apache.cloudstack.api.ApiConstants; import org.apache.cloudstack.context.CallContext; 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.StoragePoolDetailVO; import org.apache.cloudstack.storage.datastore.db.StoragePoolDetailsDao; import org.apache.cloudstack.storage.datastore.db.StoragePoolVO; import org.apache.log4j.Logger; @@ -326,15 +324,8 @@ public boolean cancelMaintain(DataStore store) { return true; } - Map details = null; - if (pool.getPoolType().equals(Storage.StoragePoolType.NetworkFilesystem)) { - details = new HashMap<>(); - StoragePoolDetailVO nfsMountOpts = storagePoolDetailsDao.findDetail(poolVO.getId(), ApiConstants.NFS_MOUNT_OPTIONS); - if (nfsMountOpts != null) { - details.put(ApiConstants.NFS_MOUNT_OPTIONS, nfsMountOpts.getValue()); - } - } - + Map details = new HashMap<>(); + storageManager.addStoragePoolNFSMountOptsToDetailsMap(pool, details); // add heartbeat for (HostVO host : hosts) { ModifyStoragePoolCommand msPoolCmd = new ModifyStoragePoolCommand(true, pool, details); From 8fd6b12313373e538c283824e9dc48cbdd17a5bd Mon Sep 17 00:00:00 2001 From: abh1sar Date: Wed, 24 Apr 2024 03:00:55 +0530 Subject: [PATCH 04/23] Pull 8947: CollectionsUtils.isNotEmpty and switch statement in LibvirtStoragePoolDef.java --- .../api/response/StoragePoolResponse.java | 2 +- .../kvm/resource/LibvirtStoragePoolDef.java | 111 ++++++++++-------- .../kvm/storage/LibvirtStorageAdaptor.java | 5 +- 3 files changed, 68 insertions(+), 50 deletions(-) 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 058ef7b7baed..f514c8167ac8 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 @@ -102,7 +102,7 @@ public class StoragePoolResponse extends BaseResponseWithAnnotations { private String tags; @SerializedName(ApiConstants.NFS_MOUNT_OPTIONS) - @Param(description = "the nfs mount options for the storage pool") + @Param(description = "the nfs mount options for the storage pool", since = "4.19.1") private String nfsMountOpts; @SerializedName(ApiConstants.IS_TAG_A_RULE) 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 c6f8e6e010d1..a7833ba1854f 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 @@ -20,6 +20,8 @@ import java.util.List; import java.util.Map; +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"); @@ -82,7 +84,7 @@ public LibvirtStoragePoolDef(PoolType type, String poolName, String uuid, String public LibvirtStoragePoolDef(PoolType type, String poolName, String uuid, String host, String dir, String targetPath, List nfsMountOpts) { this(type, poolName, uuid, host, dir, targetPath); - if (nfsMountOpts != null) { + if (CollectionUtils.isNotEmpty(nfsMountOpts)) { for (String nfsMountOpt : nfsMountOpts) { this._nfsMountOpts.put(nfsMountOpt, null); } @@ -145,64 +147,79 @@ public Map getNfsMountOpts() { @Override public String toString() { StringBuilder storagePoolBuilder = new StringBuilder(); - if (_poolType == PoolType.NETFS && _nfsMountOpts != null) { - // get from poolType - storagePoolBuilder.append("\n"); - } else 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("" + _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"); + storagePoolBuilder.append("\n"); + storagePoolBuilder.append("\n"); + storagePoolBuilder.append("\n"); + break; } + if (_poolType != PoolType.RBD && _poolType != PoolType.POWERFLEX) { storagePoolBuilder.append("\n"); storagePoolBuilder.append("" + _targetPath + "\n"); 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 331e2146fd9b..abbd729649a2 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 @@ -33,6 +33,7 @@ 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; @@ -366,7 +367,7 @@ private StoragePool createCLVMStoragePool(Connect conn, String uuid, String host private List getNFSMountOptsFromDetails(StoragePoolType type, Map details) { List nfsMountOpts = null; - if (type == StoragePoolType.NetworkFilesystem) { + if (type.equals(StoragePoolType.NetworkFilesystem)) { if (details != null && details.containsKey(ApiConstants.NFS_MOUNT_OPTIONS)) { nfsMountOpts = Arrays.asList(details.get(ApiConstants.NFS_MOUNT_OPTIONS).replaceAll("\\s", "").split(",")); } @@ -693,7 +694,7 @@ public KVMStoragePool createStoragePool(String name, String host, int port, Stri } List nfsMountOpts = getNFSMountOptsFromDetails(type, details); - if (sp != null && nfsMountOpts != null) { + if (sp != null && CollectionUtils.isNotEmpty(nfsMountOpts)) { if (destroyStoragePoolForNFSMountOptions(sp, conn, nfsMountOpts)) { sp = null; } From 434496b1d25666b19cd193d513a7ca09e92456e2 Mon Sep 17 00:00:00 2001 From: abh1sar Date: Wed, 24 Apr 2024 07:52:16 +0530 Subject: [PATCH 05/23] Pull 8947: UI - cancel maintainenace will remount the storage pool and apply the options --- ui/public/locales/en.json | 4 ++-- ui/src/config/section/infra/primaryStorages.js | 2 +- ui/src/views/infra/NFSMountOptsPrimaryStorage.vue | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/ui/public/locales/en.json b/ui/public/locales/en.json index 448a471dc396..020eb588178b 100644 --- a/ui/public/locales/en.json +++ b/ui/public/locales/en.json @@ -2442,7 +2442,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.options": "Please confirm that you want to change NFS mount options.
This will cause the storage pool to be remounted on all KVM hosts.", +"message.action.edit.nfs.options": "Please confirm that you want to change NFS mount options.
Cancelling maintenance mode will cause the storage pool to be remounted on all KVM hosts with the new 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.", @@ -3188,7 +3188,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.nfsmountopts": "Successfully updated NFS mount options", +"message.success.edit.nfsmountopts": "Successfully recorded the change in NFS mount options. Cancel maintenance mode to apply the new options", "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 f445fa57d515..351562726383 100644 --- a/ui/src/config/section/infra/primaryStorages.js +++ b/ui/src/config/section/infra/primaryStorages.js @@ -116,7 +116,7 @@ export default { message: 'message.action.edit.nfs.options', dataView: true, popup: true, - show: (record) => { return (record.type === 'NetworkFilesystem' && record.hypervisor === 'KVM' && record.state === 'Maintenance') }, + show: (record) => { return (record.type === 'NetworkFilesystem' && record.state === 'Maintenance' && (record.hypervisor === 'KVM' || record.hypervisor === 'Simulator')) }, component: shallowRef(defineAsyncComponent(() => import('@/views/infra/NFSMountOptsPrimaryStorage.vue'))) }, { diff --git a/ui/src/views/infra/NFSMountOptsPrimaryStorage.vue b/ui/src/views/infra/NFSMountOptsPrimaryStorage.vue index 98c080cb2d41..90aa7db5201f 100644 --- a/ui/src/views/infra/NFSMountOptsPrimaryStorage.vue +++ b/ui/src/views/infra/NFSMountOptsPrimaryStorage.vue @@ -31,7 +31,7 @@

- + From db4eb24b3f3c98ab17201bbf434cfeb5984d87af Mon Sep 17 00:00:00 2001 From: abh1sar Date: Wed, 24 Apr 2024 11:35:28 +0530 Subject: [PATCH 06/23] Pull 8947: UI - moved edit NFS mount options to edit Primary Storage form --- ui/public/locales/en.json | 6 +- .../config/section/infra/primaryStorages.js | 13 +- .../infra/NFSMountOptsPrimaryStorage.vue | 147 ------------- ui/src/views/infra/UpdatePrimaryStorage.vue | 195 ++++++++++++++++++ 4 files changed, 200 insertions(+), 161 deletions(-) delete mode 100644 ui/src/views/infra/NFSMountOptsPrimaryStorage.vue create mode 100644 ui/src/views/infra/UpdatePrimaryStorage.vue diff --git a/ui/public/locales/en.json b/ui/public/locales/en.json index 020eb588178b..3ad4bbea6e5f 100644 --- a/ui/public/locales/en.json +++ b/ui/public/locales/en.json @@ -2442,7 +2442,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.options": "Please confirm that you want to change NFS mount options.
Cancelling maintenance mode will cause the storage pool to be remounted on all KVM hosts with the new options.", +"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.", @@ -3016,7 +3016,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.nfsmountopts.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.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.", @@ -3188,7 +3188,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.nfsmountopts": "Successfully recorded the change in NFS mount options. Cancel maintenance mode to apply the new options", +"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 351562726383..64670e6a6a28 100644 --- a/ui/src/config/section/infra/primaryStorages.js +++ b/ui/src/config/section/infra/primaryStorages.js @@ -89,7 +89,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', @@ -109,16 +110,6 @@ export default { defaultArgs: { enabled: true }, show: (record) => { return record.state === 'Disabled' } }, - { - api: 'updateStoragePool', - icon: 'control-outlined', - label: 'label.action.edit.nfs.options', - message: 'message.action.edit.nfs.options', - dataView: true, - popup: true, - show: (record) => { return (record.type === 'NetworkFilesystem' && record.state === 'Maintenance' && (record.hypervisor === 'KVM' || record.hypervisor === 'Simulator')) }, - component: shallowRef(defineAsyncComponent(() => import('@/views/infra/NFSMountOptsPrimaryStorage.vue'))) - }, { api: 'syncStoragePool', icon: 'sync-outlined', diff --git a/ui/src/views/infra/NFSMountOptsPrimaryStorage.vue b/ui/src/views/infra/NFSMountOptsPrimaryStorage.vue deleted file mode 100644 index 90aa7db5201f..000000000000 --- a/ui/src/views/infra/NFSMountOptsPrimaryStorage.vue +++ /dev/null @@ -1,147 +0,0 @@ -// 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/UpdatePrimaryStorage.vue b/ui/src/views/infra/UpdatePrimaryStorage.vue new file mode 100644 index 000000000000..7c026630a999 --- /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. + + + + + + From e7c7365040c32c27d26453f6a359f29ea1ab6767 Mon Sep 17 00:00:00 2001 From: abh1sar Date: Wed, 24 Apr 2024 11:49:02 +0530 Subject: [PATCH 07/23] Pull 8947: UI - moved 'NFS Mount Options' to below 'Type' in dataview --- ui/src/config/section/infra/primaryStorages.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ui/src/config/section/infra/primaryStorages.js b/ui/src/config/section/infra/primaryStorages.js index 64670e6a6a28..c2ca76b4c715 100644 --- a/ui/src/config/section/infra/primaryStorages.js +++ b/ui/src/config/section/infra/primaryStorages.js @@ -34,7 +34,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', 'nfsmountopts', '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', From 4ff2f0a169c7894023098c2077737cf5d8eb7147 Mon Sep 17 00:00:00 2001 From: abh1sar Date: Tue, 30 Apr 2024 09:03:37 +0530 Subject: [PATCH 08/23] Pull 8947: Fixed message in AddPrimaryStorage.vue --- ui/src/views/infra/AddPrimaryStorage.vue | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ui/src/views/infra/AddPrimaryStorage.vue b/ui/src/views/infra/AddPrimaryStorage.vue index 2905aacf9443..ea0296580ce0 100644 --- a/ui/src/views/infra/AddPrimaryStorage.vue +++ b/ui/src/views/infra/AddPrimaryStorage.vue @@ -184,9 +184,9 @@ (form.scope === 'cluster' && (hypervisorType === 'KVM' || hypervisorType === 'Simulator')))"> - +
From 228b6d3b54be850bd8098f33d98db474f08e2cfd Mon Sep 17 00:00:00 2001 From: abh1sar Date: Tue, 30 Apr 2024 09:05:19 +0530 Subject: [PATCH 09/23] Pull 8947: Convert _nfsmountOpts to Set in libvirtStoragePoolDef --- .../kvm/resource/LibvirtStoragePoolDef.java | 14 +++++++------- .../kvm/storage/LibvirtStorageAdaptor.java | 4 ++-- .../kvm/resource/LibvirtStoragePoolDefTest.java | 4 ++-- .../resource/LibvirtStoragePoolXMLParserTest.java | 4 ++-- 4 files changed, 13 insertions(+), 13 deletions(-) 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 a7833ba1854f..4f8b2843ec20 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,9 +16,9 @@ // under the License. package com.cloud.hypervisor.kvm.resource; -import java.util.HashMap; +import java.util.HashSet; import java.util.List; -import java.util.Map; +import java.util.Set; import org.apache.commons.collections.CollectionUtils; @@ -61,7 +61,7 @@ public String toString() { private String _authUsername; private AuthenticationType _authType; private String _secretUuid; - private Map _nfsMountOpts = new HashMap<>(); + private Set _nfsMountOpts = new HashSet<>(); public LibvirtStoragePoolDef(PoolType type, String poolName, String uuid, String host, int port, String dir, String targetPath) { _poolType = type; @@ -86,7 +86,7 @@ public LibvirtStoragePoolDef(PoolType type, String poolName, String uuid, String this(type, poolName, uuid, host, dir, targetPath); if (CollectionUtils.isNotEmpty(nfsMountOpts)) { for (String nfsMountOpt : nfsMountOpts) { - this._nfsMountOpts.put(nfsMountOpt, null); + this._nfsMountOpts.add(nfsMountOpt); } } } @@ -140,7 +140,7 @@ public AuthenticationType getAuthType() { return _authType; } - public Map getNfsMountOpts() { + public Set getNfsMountOpts() { return _nfsMountOpts; } @@ -227,8 +227,8 @@ public String toString() { } if (_poolType == PoolType.NETFS && _nfsMountOpts != null) { storagePoolBuilder.append("\n"); - for (Map.Entry options : _nfsMountOpts.entrySet()) { - storagePoolBuilder.append("\n"); + for (String options : _nfsMountOpts) { + storagePoolBuilder.append("\n"); } storagePoolBuilder.append("\n"); } 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 abbd729649a2..da213be0eea7 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 @@ -378,13 +378,13 @@ private List getNFSMountOptsFromDetails(StoragePoolType type, Map nfsMountOpts) { try { LibvirtStoragePoolDef poolDef = getStoragePoolDef(conn, sp); - Map poolNfsMountOptsMap = poolDef.getNfsMountOpts(); + Set poolNfsMountOptsMap = poolDef.getNfsMountOpts(); boolean mountOptsDiffer = false; if (poolNfsMountOptsMap.size() != nfsMountOpts.size()) { mountOptsDiffer = true; } else { for (String nfsMountOpt : nfsMountOpts) { - if (!poolNfsMountOptsMap.containsKey(nfsMountOpt)) { + if (!poolNfsMountOptsMap.contains(nfsMountOpt)) { mountOptsDiffer = true; break; } 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 818f4bdc253e..975845393502 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 @@ -55,8 +55,8 @@ public void testSetGetStoragePool() { nfsMountOpts.add("vers=4.1"); nfsMountOpts.add("nconnect=4"); pool = new LibvirtStoragePoolDef(type, name, uuid, host, dir, targetPath, nfsMountOpts); - assertTrue(pool.getNfsMountOpts().containsKey("vers=4.1")); - assertTrue(pool.getNfsMountOpts().containsKey("nconnect=4")); + assertTrue(pool.getNfsMountOpts().contains("vers=4.1")); + assertTrue(pool.getNfsMountOpts().contains("nconnect=4")); assertEquals(pool.getNfsMountOpts().size(), 2); } 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 b9215259205a..5854c21186f9 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 @@ -59,8 +59,8 @@ public void testParseNfsStoragePoolXML() { LibvirtStoragePoolDef pool = parser.parseStoragePoolXML(poolXML); assertEquals("10.11.12.13", pool.getSourceHost()); - assertTrue(pool.getNfsMountOpts().containsKey("vers=4.1")); - assertTrue(pool.getNfsMountOpts().containsKey("nconnect=8")); + assertTrue(pool.getNfsMountOpts().contains("vers=4.1")); + assertTrue(pool.getNfsMountOpts().contains("nconnect=8")); } @Test From 249ffe93b635afe15748e4a87cf7986b9f3d4118 Mon Sep 17 00:00:00 2001 From: abh1sar Date: Tue, 30 Apr 2024 09:08:34 +0530 Subject: [PATCH 10/23] Pull 8947: Throw exception and log error if mount fails due to incorrect mount option --- .../src/main/java/com/cloud/storage/StorageManager.java | 2 +- .../storage/datastore/provider/DefaultHostListener.java | 2 +- .../src/main/java/com/cloud/storage/StorageManagerImpl.java | 5 ++++- .../java/com/cloud/storage/StoragePoolAutomationImpl.java | 6 +++++- 4 files changed, 11 insertions(+), 4 deletions(-) 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 c4a8b5c8a3c3..d3dd4c8ba00e 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 @@ -342,7 +342,7 @@ static Boolean getFullCloneConfiguration(Long storeId) { boolean registerHostListener(String providerUuid, HypervisorHostListener listener); - void addStoragePoolNFSMountOptsToDetailsMap(StoragePool pool, Map details); + boolean addStoragePoolNFSMountOptsToDetailsMap(StoragePool pool, Map details); boolean connectHostToSharedPool(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 692afc34500d..1a9bc42b1ab0 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 @@ -145,7 +145,7 @@ public boolean hostConnect(long hostId, long poolId) throws StorageConflictExcep 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/server/src/main/java/com/cloud/storage/StorageManagerImpl.java b/server/src/main/java/com/cloud/storage/StorageManagerImpl.java index 8b7874cefc9e..6665f314e5e7 100644 --- a/server/src/main/java/com/cloud/storage/StorageManagerImpl.java +++ b/server/src/main/java/com/cloud/storage/StorageManagerImpl.java @@ -1280,13 +1280,16 @@ public void doInTransactionWithoutResult(TransactionStatus status) { } @Override - public void addStoragePoolNFSMountOptsToDetailsMap(StoragePool pool, Map details) { + public boolean addStoragePoolNFSMountOptsToDetailsMap(StoragePool pool, Map details) { + boolean details_added = false; if (pool.getPoolType().equals(Storage.StoragePoolType.NetworkFilesystem)) { StoragePoolDetailVO nfsMountOpts = _storagePoolDetailsDao.findDetail(pool.getId(), ApiConstants.NFS_MOUNT_OPTIONS); if (nfsMountOpts != null) { details.put(ApiConstants.NFS_MOUNT_OPTIONS, nfsMountOpts.getValue()); + details_added = true; } } + return details_added; } private boolean checkIfDataStoreClusterCanbeDeleted(StoragePoolVO sPool, boolean forced) { diff --git a/server/src/main/java/com/cloud/storage/StoragePoolAutomationImpl.java b/server/src/main/java/com/cloud/storage/StoragePoolAutomationImpl.java index 4ccdfbb152f9..46a6ef69efd8 100644 --- a/server/src/main/java/com/cloud/storage/StoragePoolAutomationImpl.java +++ b/server/src/main/java/com/cloud/storage/StoragePoolAutomationImpl.java @@ -325,7 +325,7 @@ public boolean cancelMaintain(DataStore store) { } Map details = new HashMap<>(); - storageManager.addStoragePoolNFSMountOptsToDetailsMap(pool, details); + boolean nfsMountOptsAdded = storageManager.addStoragePoolNFSMountOptsToDetailsMap(pool, details); // add heartbeat for (HostVO host : hosts) { ModifyStoragePoolCommand msPoolCmd = new ModifyStoragePoolCommand(true, pool, details); @@ -334,6 +334,10 @@ public boolean cancelMaintain(DataStore store) { if (s_logger.isDebugEnabled()) { s_logger.debug("ModifyStoragePool add failed due to " + ((answer == null) ? "answer null" : answer.getDetails())); } + if (answer != null && nfsMountOptsAdded) { + s_logger.error(String.format("Unable to attach storage pool to the host %s due to %s", host, answer.getDetails())); + throw new CloudRuntimeException("Unable to attach storage pool to the host " + host.getName()); + } } else { if (s_logger.isDebugEnabled()) { s_logger.debug("ModifyStoragePool add succeeded"); From 830fb0f0ca783d89486273f0eadf721c8a4ba267 Mon Sep 17 00:00:00 2001 From: abh1sar Date: Thu, 2 May 2024 11:48:03 +0530 Subject: [PATCH 11/23] Pull 8947: Added UT and moved integration test to component/maint --- .../storage/LibvirtStorageAdaptorTest.java | 94 +++++++++++++++++++ .../cloud/storage/StorageManagerImplTest.java | 19 +++- .../test_primary_storage_nfsmountopts_kvm.py} | 94 +++++++++++-------- 3 files changed, 164 insertions(+), 43 deletions(-) create mode 100644 plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptorTest.java rename test/integration/{smoke/test_primary_storage_nfs_mount_opts_kvm.py => component/maint/test_primary_storage_nfsmountopts_kvm.py} (75%) 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 000000000000..976ecbdd8d88 --- /dev/null +++ b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptorTest.java @@ -0,0 +1,94 @@ +// 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 org.junit.After; +import org.junit.Assert; +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 + 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); + Mockito.when(sp.getInfo()).thenReturn(spinfo); + + 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); + Assert.assertEquals(pool.getUuid(), uuid); + } +} diff --git a/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java b/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java index 5b86d8482c1c..0766009d8001 100644 --- a/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java +++ b/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java @@ -22,10 +22,13 @@ import com.cloud.exception.ConnectionException; import com.cloud.exception.InvalidParameterValueException; import com.cloud.host.Host; +import com.cloud.hypervisor.Hypervisor; import com.cloud.storage.dao.VolumeDao; +import com.cloud.user.AccountManager; 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; @@ -59,6 +62,8 @@ public class StorageManagerImplTest { ConfigurationDao configurationDao; @Mock DataCenterDao dataCenterDao; + @Mock + AccountManager accountManager; @Spy @InjectMocks @@ -252,12 +257,22 @@ public void testEnableDefaultDatastoreDownloadRedirectionForExistingInstallation @Test(expected = InvalidParameterValueException.class) public void testDuplicateNFSMountOptions() { String nfsMountOpts = "vers=4.1, nconnect=4,vers=4.2"; - storageManagerImpl.checkNfsMountOptions(nfsMountOpts); + 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,"; - storageManagerImpl.checkNfsMountOptions(nfsMountOpts); + 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); } } diff --git a/test/integration/smoke/test_primary_storage_nfs_mount_opts_kvm.py b/test/integration/component/maint/test_primary_storage_nfsmountopts_kvm.py similarity index 75% rename from test/integration/smoke/test_primary_storage_nfs_mount_opts_kvm.py rename to test/integration/component/maint/test_primary_storage_nfsmountopts_kvm.py index f994b9aeb0e7..c7d38e807a21 100644 --- a/test/integration/smoke/test_primary_storage_nfs_mount_opts_kvm.py +++ b/test/integration/component/maint/test_primary_storage_nfsmountopts_kvm.py @@ -15,7 +15,6 @@ # specific language governing permissions and limitations # under the License. -import marvin from marvin.cloudstackTestCase import cloudstackTestCase from marvin.cloudstackAPI import * from marvin.lib.utils import * @@ -29,26 +28,31 @@ class TestNFSMountOptsKVM(cloudstackTestCase): """ Test cases for host HA using KVM host(s) """ - def setUp(self): - self.testClient = super(TestNFSMountOptsKVM, self).getClsTestClient() - self.apiclient = self.testClient.getApiClient() - self.dbclient = self.testClient.getDbConnection() - self.services = self.testClient.getParsedTestDataConfig() - self.logger = logging.getLogger('TestHAKVM') - - self.cluster = list_clusters(self.apiclient)[0] - self.hypervisor = self.testClient.getHypervisorInfo() - self.host = self.getHost() - self.storagePool = self.getPrimaryStorage(self.cluster.id) - self.hostConfig = self.config.__dict__["zones"][0].__dict__["pods"][0].__dict__["clusters"][0].__dict__["hosts"][0].__dict__ - self.cluster_id = self.host.clusterid - self.hostConfig["password"]="P@ssword123" - self.sshClient = SshClient( - host=self.host.ipaddress, + @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.hostConfig["password"]="P@ssword123" + cls.sshClient = SshClient( + host=cls.host.ipaddress, port=22, - user=self.hostConfig['username'], - passwd=self.hostConfig['password']) - + 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( @@ -73,8 +77,7 @@ def getPrimaryStorage(self, clusterId): id=None, ) if response and len(response) > 0: - self.storage_pool = response[0] - return self.storage_pool + return response[0] raise self.skipTest("Not enough KVM hosts found, skipping NFS options test") def getNFSMountOptionsFromVirsh(self, poolId): @@ -114,16 +117,26 @@ def getNFSMountOptionForPool(self, option, poolId): 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 + resp = self.apiclient.enableStorageMaintenance(maint_cmd) + + storage = 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 """ - - vers = self.getNFSMountOptionForPool("vers", self.storage_pool.id) - if (vers == None): - raise self.skipTest("Storage pool not associated with the host") - version = self.getUnusedNFSVersions(self.storage_pool.ipaddress) nconnect = None if version == None: @@ -136,27 +149,26 @@ def test_primary_storage_nfs_options_kvm(self): details = [{'nfsmountopts': nfsMountOpts}] - maint_cmd = enableStorageMaintenance.enableStorageMaintenanceCmd() - maint_cmd.id = self.storage_pool.id - resp = self.apiclient.enableStorageMaintenance(maint_cmd) - - storage = 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) + 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): + nfsMountOpts = "version=4.1" + details = [{'nfsmountopts': nfsMountOpts}] + + try: + resp = self.changeNFSOptions(details) + except Exception: + pass + else: + self.fail("Incorrect NFS mount option should throw error while mounting") From 0850bbec3ce495a1dbe492e3c7c88460415b7935 Mon Sep 17 00:00:00 2001 From: abh1sar Date: Thu, 2 May 2024 11:50:15 +0530 Subject: [PATCH 12/23] Pull 8947: Review comments --- .../java/com/cloud/storage/StorageManager.java | 4 +++- .../datastore/provider/DefaultHostListener.java | 7 +++---- .../kvm/storage/LibvirtStorageAdaptor.java | 10 +++++----- ...CloudStackPrimaryDataStoreLifeCycleImpl.java | 8 ++++++++ .../com/cloud/storage/StorageManagerImpl.java | 17 +++++++++++++++-- .../storage/StoragePoolAutomationImpl.java | 16 ++++++++++------ 6 files changed, 44 insertions(+), 18 deletions(-) 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 d3dd4c8ba00e..4df0e90034ca 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 @@ -342,7 +342,9 @@ static Boolean getFullCloneConfiguration(Long storeId) { boolean registerHostListener(String providerUuid, HypervisorHostListener listener); - boolean addStoragePoolNFSMountOptsToDetailsMap(StoragePool pool, Map details); + Pair, Boolean> getStoragePoolNFSMountOpts(StoragePool pool, Map details); + + String getStoragePoolMountFailureReason(String error); boolean connectHostToSharedPool(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 1a9bc42b1ab0..ea1e524f5d60 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,6 +42,7 @@ 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; @@ -55,7 +56,6 @@ import javax.inject.Inject; -import java.util.HashMap; import java.util.List; import java.util.Map; @@ -129,10 +129,9 @@ private NicTO createNicTOFromNetworkAndOffering(NetworkVO networkVO, NetworkOffe @Override public boolean hostConnect(long hostId, long poolId) throws StorageConflictException { StoragePool pool = (StoragePool) this.dataStoreMgr.getDataStore(poolId, DataStoreRole.Primary); - Map details = new HashMap<>(); - storageManager.addStoragePoolNFSMountOptsToDetailsMap(pool, details); + Pair, Boolean> nfsMountOpts = storageManager.getStoragePoolNFSMountOpts(pool, null); - ModifyStoragePoolCommand cmd = new ModifyStoragePoolCommand(true, pool, details); + 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())); 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 da213be0eea7..d075a10cfde3 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 @@ -375,16 +375,16 @@ private List getNFSMountOptsFromDetails(StoragePoolType type, Map nfsMountOpts) { + private boolean destroyStoragePoolOnNFSMountOptionsChange(StoragePool sp, Connect conn, List nfsMountOpts) { try { LibvirtStoragePoolDef poolDef = getStoragePoolDef(conn, sp); - Set poolNfsMountOptsMap = poolDef.getNfsMountOpts(); + Set poolNfsMountOpts = poolDef.getNfsMountOpts(); boolean mountOptsDiffer = false; - if (poolNfsMountOptsMap.size() != nfsMountOpts.size()) { + if (poolNfsMountOpts.size() != nfsMountOpts.size()) { mountOptsDiffer = true; } else { for (String nfsMountOpt : nfsMountOpts) { - if (!poolNfsMountOptsMap.contains(nfsMountOpt)) { + if (!poolNfsMountOpts.contains(nfsMountOpt)) { mountOptsDiffer = true; break; } @@ -695,7 +695,7 @@ public KVMStoragePool createStoragePool(String name, String host, int port, Stri List nfsMountOpts = getNFSMountOptsFromDetails(type, details); if (sp != null && CollectionUtils.isNotEmpty(nfsMountOpts)) { - if (destroyStoragePoolForNFSMountOptions(sp, conn, nfsMountOpts)) { + if (destroyStoragePoolOnNFSMountOptionsChange(sp, conn, nfsMountOpts)) { sp = null; } } 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 419d4c0ce7c6..ecfb7d4ab8cc 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 boolean attachCluster(DataStore store, ClusterScope scope) { 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 boolean attachZone(DataStore dataStore, ZoneScope scope, HypervisorType h 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()) { diff --git a/server/src/main/java/com/cloud/storage/StorageManagerImpl.java b/server/src/main/java/com/cloud/storage/StorageManagerImpl.java index 6665f314e5e7..c2c174420e5b 100644 --- a/server/src/main/java/com/cloud/storage/StorageManagerImpl.java +++ b/server/src/main/java/com/cloud/storage/StorageManagerImpl.java @@ -411,6 +411,8 @@ public void setDiscoverers(List discoverers) { 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 @@ -1280,8 +1282,11 @@ public void doInTransactionWithoutResult(TransactionStatus status) { } @Override - public boolean addStoragePoolNFSMountOptsToDetailsMap(StoragePool pool, Map details) { + public Pair, Boolean> getStoragePoolNFSMountOpts(StoragePool pool, Map details) { boolean details_added = false; + if (details == null) { + details = new HashMap<>(); + } if (pool.getPoolType().equals(Storage.StoragePoolType.NetworkFilesystem)) { StoragePoolDetailVO nfsMountOpts = _storagePoolDetailsDao.findDetail(pool.getId(), ApiConstants.NFS_MOUNT_OPTIONS); if (nfsMountOpts != null) { @@ -1289,7 +1294,15 @@ public boolean addStoragePoolNFSMountOptsToDetailsMap(StoragePool pool, Map(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) { diff --git a/server/src/main/java/com/cloud/storage/StoragePoolAutomationImpl.java b/server/src/main/java/com/cloud/storage/StoragePoolAutomationImpl.java index 46a6ef69efd8..740e247723a2 100644 --- a/server/src/main/java/com/cloud/storage/StoragePoolAutomationImpl.java +++ b/server/src/main/java/com/cloud/storage/StoragePoolAutomationImpl.java @@ -19,7 +19,6 @@ package com.cloud.storage; import java.util.ArrayList; -import java.util.HashMap; import java.util.List; import java.util.Map; @@ -51,6 +50,7 @@ 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; @@ -324,19 +324,23 @@ public boolean cancelMaintain(DataStore store) { return true; } - Map details = new HashMap<>(); - boolean nfsMountOptsAdded = storageManager.addStoragePoolNFSMountOptsToDetailsMap(pool, details); + Pair, Boolean> nfsMountOpts = storageManager.getStoragePoolNFSMountOpts(pool, null); // add heartbeat for (HostVO host : hosts) { - ModifyStoragePoolCommand msPoolCmd = new ModifyStoragePoolCommand(true, pool, details); + 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 && nfsMountOptsAdded) { + 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())); - throw new CloudRuntimeException("Unable to attach storage pool to the host " + host.getName()); + 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()) { From d55a4f143b15d46705d596d1ca7807199e5a3079 Mon Sep 17 00:00:00 2001 From: abh1sar Date: Thu, 2 May 2024 11:52:34 +0530 Subject: [PATCH 13/23] Pull 8947: Removed password from integration test --- .../component/maint/test_primary_storage_nfsmountopts_kvm.py | 1 - 1 file changed, 1 deletion(-) diff --git a/test/integration/component/maint/test_primary_storage_nfsmountopts_kvm.py b/test/integration/component/maint/test_primary_storage_nfsmountopts_kvm.py index c7d38e807a21..b604867aed60 100644 --- a/test/integration/component/maint/test_primary_storage_nfsmountopts_kvm.py +++ b/test/integration/component/maint/test_primary_storage_nfsmountopts_kvm.py @@ -38,7 +38,6 @@ def setUpClass(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.hostConfig["password"]="P@ssword123" cls.sshClient = SshClient( host=cls.host.ipaddress, port=22, From 9eaabc74bb3a7de8a461ce1b55a7d7be5a56604c Mon Sep 17 00:00:00 2001 From: abh1sar Date: Fri, 3 May 2024 09:13:09 +0530 Subject: [PATCH 14/23] Pull 8947: move details allocation to inside the if loop in getStoragePoolNFSMountOpts --- .../src/main/java/com/cloud/storage/StorageManagerImpl.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/server/src/main/java/com/cloud/storage/StorageManagerImpl.java b/server/src/main/java/com/cloud/storage/StorageManagerImpl.java index c2c174420e5b..88641a2cdc2b 100644 --- a/server/src/main/java/com/cloud/storage/StorageManagerImpl.java +++ b/server/src/main/java/com/cloud/storage/StorageManagerImpl.java @@ -1284,12 +1284,12 @@ public void doInTransactionWithoutResult(TransactionStatus status) { @Override public Pair, Boolean> getStoragePoolNFSMountOpts(StoragePool pool, Map details) { boolean details_added = false; - if (details == null) { - details = new HashMap<>(); - } if (pool.getPoolType().equals(Storage.StoragePoolType.NetworkFilesystem)) { 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; } From e73d6dce3c9cc64add17a730a4198b5d84064af9 Mon Sep 17 00:00:00 2001 From: abh1sar Date: Fri, 3 May 2024 15:09:47 +0530 Subject: [PATCH 15/23] Pull 8947: Fixed a bug in AddPrimaryStorage.vue --- ui/src/views/infra/AddPrimaryStorage.vue | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/ui/src/views/infra/AddPrimaryStorage.vue b/ui/src/views/infra/AddPrimaryStorage.vue index ea0296580ce0..55f18aec2cc7 100644 --- a/ui/src/views/infra/AddPrimaryStorage.vue +++ b/ui/src/views/infra/AddPrimaryStorage.vue @@ -805,7 +805,9 @@ export default { var url = '' if (values.protocol === 'nfs') { url = this.nfsURL(server, path) - params['details[0].nfsmountopts'] = values.nfsMountOpts + if (values.nfsMountOpts) { + params['details[0].nfsmountopts'] = values.nfsMountOpts + } } else if (values.protocol === 'SMB') { url = this.smbURL(server, path) const smbParams = { From 96938e0c683b37c750b94488a57b75b82a041cdc Mon Sep 17 00:00:00 2001 From: abh1sar Date: Fri, 3 May 2024 18:00:42 +0530 Subject: [PATCH 16/23] Pull 8947: Pool should remain in maintenance mode if mount fails --- ...oudStackPrimaryDataStoreLifeCycleImpl.java | 2 +- .../test_primary_storage_nfsmountopts_kvm.py | 24 ++++++++++++++----- 2 files changed, 19 insertions(+), 7 deletions(-) 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 ecfb7d4ab8cc..bed5cac8648c 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 @@ -466,8 +466,8 @@ public boolean maintain(DataStore dataStore) { @Override public boolean cancelMaintain(DataStore store) { - dataStoreHelper.cancelMaintain(store); storagePoolAutmation.cancelMaintain(store); + dataStoreHelper.cancelMaintain(store); return true; } diff --git a/test/integration/component/maint/test_primary_storage_nfsmountopts_kvm.py b/test/integration/component/maint/test_primary_storage_nfsmountopts_kvm.py index b604867aed60..861065b38ae6 100644 --- a/test/integration/component/maint/test_primary_storage_nfsmountopts_kvm.py +++ b/test/integration/component/maint/test_primary_storage_nfsmountopts_kvm.py @@ -38,6 +38,7 @@ def setUpClass(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.hostConfig['password'] = "P@ssword123" cls.sshClient = SshClient( host=cls.host.ipaddress, port=22, @@ -119,12 +120,17 @@ def getNFSMountOptionForPool(self, option, poolId): def changeNFSOptions(self, details): maint_cmd = enableStorageMaintenance.enableStorageMaintenanceCmd() maint_cmd.id = self.storage_pool.id - resp = self.apiclient.enableStorageMaintenance(maint_cmd) - storage = StoragePool.update(self.apiclient, - id=self.storage_pool.id, - details=details - ) + 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 @@ -162,12 +168,18 @@ def test_primary_storage_nfs_options_kvm(self): @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: - pass + 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") From 662aec61ee56dd852d8ecff417d67c4fddcd20d6 Mon Sep 17 00:00:00 2001 From: abh1sar Date: Fri, 3 May 2024 18:03:41 +0530 Subject: [PATCH 17/23] Pull 8947: Removed password from integration test --- .../component/maint/test_primary_storage_nfsmountopts_kvm.py | 1 - 1 file changed, 1 deletion(-) diff --git a/test/integration/component/maint/test_primary_storage_nfsmountopts_kvm.py b/test/integration/component/maint/test_primary_storage_nfsmountopts_kvm.py index 861065b38ae6..f2ad18188ca7 100644 --- a/test/integration/component/maint/test_primary_storage_nfsmountopts_kvm.py +++ b/test/integration/component/maint/test_primary_storage_nfsmountopts_kvm.py @@ -38,7 +38,6 @@ def setUpClass(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.hostConfig['password'] = "P@ssword123" cls.sshClient = SshClient( host=cls.host.ipaddress, port=22, From e826c94a0720ae0a18680e875b9dd69d875ec637 Mon Sep 17 00:00:00 2001 From: abh1sar Date: Thu, 9 May 2024 11:13:29 +0530 Subject: [PATCH 18/23] Pull 8947: Added UT --- .../resource/LibvirtStoragePoolDefTest.java | 20 +++++++++++++ ...tackPrimaryDataStoreLifeCycleImplTest.java | 28 ++++++++++++++++++- .../cloud/storage/StorageManagerImplTest.java | 28 +++++++++++++++++++ 3 files changed, 75 insertions(+), 1 deletion(-) 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 975845393502..f60a130ca79a 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 @@ -83,6 +83,26 @@ public void testNfsStoragePool() { 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\n\n\n" + + "" + targetPath + "\n\n"; + + assertEquals(expectedXml, pool.toString()); + } + @Test public void testRbdStoragePool() { PoolType type = PoolType.RBD; 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 dbd13d8f0de1..816ec5090374 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.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.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,30 @@ public void tearDown() throws Exception { public void testAttachCluster() throws Exception { Assert.assertTrue(_cloudStackPrimaryDataStoreLifeCycle.attachCluster(store, new ClusterScope(1L, 1L, 1L))); } + + @Test + public void testAttachClusterException() throws Exception { + Long clusterId = 1L; + Long poolId = 2L; + Long zoneId = 3L; + Long podId = 4L; + 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); + + HostVO hostMock = Mockito.mock(HostVO.class); + List hosts = List.of(hostMock); + Mockito.when(_resourceMgr.listAllUpHosts(Host.Type.Routing, clusterId, poolId, zoneId)).thenReturn(hosts); + + try { + _cloudStackPrimaryDataStoreLifeCycle.attachCluster(store, new ClusterScope(clusterId, podId, zoneId)); + } catch (Exception e) { + Assert.assertEquals(e.getMessage(), mountFailureReason); + } + } } diff --git a/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java b/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java index 0766009d8001..597f0288b520 100644 --- a/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java +++ b/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java @@ -25,6 +25,7 @@ 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; @@ -32,6 +33,8 @@ 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; @@ -64,6 +67,8 @@ public class StorageManagerImplTest { DataCenterDao dataCenterDao; @Mock AccountManager accountManager; + @Mock + StoragePoolDetailsDao storagePoolDetailsDao; @Spy @InjectMocks @@ -275,4 +280,27 @@ public void testInvalidNFSMountOptions() { Mockito.when(accountManager.isRootAdmin(accountId)).thenReturn(true); storageManagerImpl.checkNFSMountOptionsForUpdate(details, pool, accountId); } + + @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"); + } } From 68873c54bff0588c89300d6fe64b732f54b933f5 Mon Sep 17 00:00:00 2001 From: abh1sar Date: Thu, 9 May 2024 12:16:07 +0530 Subject: [PATCH 19/23] Pull 8875: Fixed a bug in CloudStackPrimaryDataStoreLifeCycleImplTest --- .../CloudStackPrimaryDataStoreLifeCycleImplTest.java | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) 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 816ec5090374..1939dc26db2a 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 @@ -174,10 +174,6 @@ public void testAttachCluster() throws Exception { @Test public void testAttachClusterException() throws Exception { - Long clusterId = 1L; - Long poolId = 2L; - Long zoneId = 3L; - Long podId = 4L; String exceptionString = "Mount failed due to incorrect mount options."; String mountFailureReason = "Incorrect mount option specified."; @@ -187,12 +183,9 @@ public void testAttachClusterException() throws Exception { Mockito.when(storageManager.getStoragePoolMountFailureReason(exceptionString)).thenReturn(mountFailureReason); ReflectionTestUtils.setField(_cloudStackPrimaryDataStoreLifeCycle, "storageMgr", storageManager); - HostVO hostMock = Mockito.mock(HostVO.class); - List hosts = List.of(hostMock); - Mockito.when(_resourceMgr.listAllUpHosts(Host.Type.Routing, clusterId, poolId, zoneId)).thenReturn(hosts); - try { - _cloudStackPrimaryDataStoreLifeCycle.attachCluster(store, new ClusterScope(clusterId, podId, zoneId)); + _cloudStackPrimaryDataStoreLifeCycle.attachCluster(store, new ClusterScope(1L, 1L, 1L)); + Assert.fail(); } catch (Exception e) { Assert.assertEquals(e.getMessage(), mountFailureReason); } From d4c8bfdd5a4dd299ebc855fbb582f3b45ab6ef49 Mon Sep 17 00:00:00 2001 From: abh1sar Date: Thu, 9 May 2024 13:10:27 +0530 Subject: [PATCH 20/23] Pull 8875: Fixed a bug in LibvirtStoragePoolDefTest --- .../kvm/resource/LibvirtStoragePoolDefTest.java | 9 +++++---- .../java/com/cloud/storage/StorageManagerImplTest.java | 2 +- 2 files changed, 6 insertions(+), 5 deletions(-) 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 f60a130ca79a..712b38b0bb4b 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 @@ -95,10 +95,11 @@ public void testGlusterFSStoragePool() { LibvirtStoragePoolDef pool = new LibvirtStoragePoolDef(type, name, uuid, host, dir, targetPath, nfsMountOpts); - String expectedXml = "\n\n\n\n" + - "" + targetPath + "\n\n"; + String expectedXml = "\n" + + "" +name + "\n" + uuid + "\n" + + "\n\n\n" + + "\n\n\n" + + "" + targetPath + "\n\n\n"; assertEquals(expectedXml, pool.toString()); } diff --git a/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java b/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java index 597f0288b520..38b81485baa2 100644 --- a/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java +++ b/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java @@ -298,7 +298,7 @@ public void testGetStoragePoolMountOptions() { } @Test - public void testgetStoragePoolMountFailureReason() { + 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"); From 88fa638946d574ceb2978036c91f29875bdb80a6 Mon Sep 17 00:00:00 2001 From: abh1sar Date: Thu, 20 Jun 2024 12:08:01 +0530 Subject: [PATCH 21/23] Pull 8947: minor code restructuring --- .../kvm/storage/LibvirtStorageAdaptor.java | 16 ++--- .../com/cloud/storage/StorageManagerImpl.java | 64 ++++++++++--------- 2 files changed, 42 insertions(+), 38 deletions(-) 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 65a44ccf892f..42bb23b71352 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 @@ -367,10 +367,11 @@ private StoragePool createCLVMStoragePool(Connect conn, String uuid, String host private List getNFSMountOptsFromDetails(StoragePoolType type, Map details) { List nfsMountOpts = null; - if (type.equals(StoragePoolType.NetworkFilesystem)) { - if (details != null && details.containsKey(ApiConstants.NFS_MOUNT_OPTIONS)) { - nfsMountOpts = Arrays.asList(details.get(ApiConstants.NFS_MOUNT_OPTIONS).replaceAll("\\s", "").split(",")); - } + 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; } @@ -694,10 +695,9 @@ public KVMStoragePool createStoragePool(String name, String host, int port, Stri } List nfsMountOpts = getNFSMountOptsFromDetails(type, details); - if (sp != null && CollectionUtils.isNotEmpty(nfsMountOpts)) { - if (destroyStoragePoolOnNFSMountOptionsChange(sp, conn, nfsMountOpts)) { - sp = null; - } + if (sp != null && CollectionUtils.isNotEmpty(nfsMountOpts) && + destroyStoragePoolOnNFSMountOptionsChange(sp, conn, nfsMountOpts)) { + sp = null; } if (sp == null) { diff --git a/server/src/main/java/com/cloud/storage/StorageManagerImpl.java b/server/src/main/java/com/cloud/storage/StorageManagerImpl.java index adabd507c6fc..c415cc71c848 100644 --- a/server/src/main/java/com/cloud/storage/StorageManagerImpl.java +++ b/server/src/main/java/com/cloud/storage/StorageManagerImpl.java @@ -857,33 +857,35 @@ protected void checkNfsMountOptions(String nfsMountOpts) throws InvalidParameter } protected void checkNFSMountOptionsForCreate(Map details, HypervisorType hypervisorType, String scheme) throws InvalidParameterValueException { - if (details.containsKey(ApiConstants.NFS_MOUNT_OPTIONS)) { - 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)); + 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)) { - 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)); + 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 @@ -1284,15 +1286,17 @@ public void doInTransactionWithoutResult(TransactionStatus status) { @Override public Pair, Boolean> getStoragePoolNFSMountOpts(StoragePool pool, Map details) { boolean details_added = false; - if (pool.getPoolType().equals(Storage.StoragePoolType.NetworkFilesystem)) { - 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; + 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); } From 00b9c587dcdd9a977816884a89d48c8fc2b3e461 Mon Sep 17 00:00:00 2001 From: abh1sar Date: Fri, 21 Jun 2024 02:27:32 +0530 Subject: [PATCH 22/23] Pull 8947 : added some ut for coverage --- .../cloud/storage/StorageManagerImplTest.java | 105 ++++++++++++++++++ 1 file changed, 105 insertions(+) diff --git a/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java b/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java index 38b81485baa2..2596f9facbab 100644 --- a/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java +++ b/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java @@ -21,6 +21,7 @@ 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; @@ -259,6 +260,100 @@ public void testEnableDefaultDatastoreDownloadRedirectionForExistingInstallation .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"; @@ -281,6 +376,16 @@ public void testInvalidNFSMountOptions() { 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; From f683fc98c61872c99c8ddf724f7255af20a60866 Mon Sep 17 00:00:00 2001 From: abh1sar Date: Tue, 25 Jun 2024 03:05:11 +0530 Subject: [PATCH 23/23] Fix LibvirtStorageAdapterTest UT --- .../hypervisor/kvm/storage/LibvirtStorageAdaptorTest.java | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) 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 index 976ecbdd8d88..3a7491ec542e 100644 --- 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 @@ -21,8 +21,8 @@ import java.util.Map; import java.util.UUID; +import com.cloud.utils.exception.CloudRuntimeException; import org.junit.After; -import org.junit.Assert; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; @@ -61,7 +61,7 @@ public void tearDown() throws Exception { closeable.close(); } - @Test + @Test(expected = CloudRuntimeException.class) public void testCreateStoragePoolWithNFSMountOpts() throws Exception { LibvirtStoragePoolDef.PoolType type = LibvirtStoragePoolDef.PoolType.NETFS; String name = "Primary1"; @@ -79,16 +79,13 @@ public void testCreateStoragePoolWithNFSMountOpts() throws Exception { 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); - Mockito.when(sp.getInfo()).thenReturn(spinfo); 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); - Assert.assertEquals(pool.getUuid(), uuid); } }