From 537c0a1e8d2cbe12f3e510d6b51babd6486f4c3e Mon Sep 17 00:00:00 2001 From: Rene Peinthor Date: Tue, 3 Sep 2024 13:01:07 +0200 Subject: [PATCH 1/7] linstor: set/unset allow-two-primaries and protocol on rc level (#9560) --- plugins/storage/volume/linstor/CHANGELOG.md | 12 ++++ .../kvm/storage/LinstorStorageAdaptor.java | 58 +++++++++++++------ .../storage/datastore/util/LinstorUtil.java | 13 +++-- 3 files changed, 60 insertions(+), 23 deletions(-) create mode 100644 plugins/storage/volume/linstor/CHANGELOG.md diff --git a/plugins/storage/volume/linstor/CHANGELOG.md b/plugins/storage/volume/linstor/CHANGELOG.md new file mode 100644 index 00000000000..30f0225b45e --- /dev/null +++ b/plugins/storage/volume/linstor/CHANGELOG.md @@ -0,0 +1,12 @@ +# Changelog + +All notable changes to Linstor CloudStack plugin will be documented in this file. + +The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), +and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). + +## [2024-08-27] + +### Changed + +- Allow two primaries(+protocol c) is now set on resource-connection level instead of rd diff --git a/plugins/storage/volume/linstor/src/main/java/com/cloud/hypervisor/kvm/storage/LinstorStorageAdaptor.java b/plugins/storage/volume/linstor/src/main/java/com/cloud/hypervisor/kvm/storage/LinstorStorageAdaptor.java index b17c768b96a..0ae65573d2b 100644 --- a/plugins/storage/volume/linstor/src/main/java/com/cloud/hypervisor/kvm/storage/LinstorStorageAdaptor.java +++ b/plugins/storage/volume/linstor/src/main/java/com/cloud/hypervisor/kvm/storage/LinstorStorageAdaptor.java @@ -19,6 +19,8 @@ package com.cloud.hypervisor.kvm.storage; import java.io.BufferedReader; import java.io.IOException; import java.io.InputStreamReader; + +import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; import java.util.List; @@ -37,6 +39,7 @@ import org.libvirt.LibvirtException; import com.cloud.storage.Storage; import com.cloud.utils.exception.CloudRuntimeException; + import com.linbit.linstor.api.ApiClient; import com.linbit.linstor.api.ApiConsts; import com.linbit.linstor.api.ApiException; @@ -47,8 +50,8 @@ import com.linbit.linstor.api.model.ApiCallRcList; import com.linbit.linstor.api.model.Properties; import com.linbit.linstor.api.model.ProviderKind; import com.linbit.linstor.api.model.Resource; +import com.linbit.linstor.api.model.ResourceConnectionModify; import com.linbit.linstor.api.model.ResourceDefinition; -import com.linbit.linstor.api.model.ResourceDefinitionModify; import com.linbit.linstor.api.model.ResourceGroup; import com.linbit.linstor.api.model.ResourceGroupSpawn; import com.linbit.linstor.api.model.ResourceMakeAvailable; @@ -262,15 +265,19 @@ public class LinstorStorageAdaptor implements StorageAdaptor { * @throws ApiException if any problem connecting to the Linstor controller */ private void allow2PrimariesIfInUse(DevelopersApi api, String rscName) throws ApiException { - if (LinstorUtil.isResourceInUse(api, rscName)) { + String inUseNode = LinstorUtil.isResourceInUse(api, rscName); + if (inUseNode != null && !inUseNode.equalsIgnoreCase(localNodeName)) { // allow 2 primaries for live migration, should be removed by disconnect on the other end - ResourceDefinitionModify rdm = new ResourceDefinitionModify(); + ResourceConnectionModify rcm = new ResourceConnectionModify(); Properties props = new Properties(); props.put("DrbdOptions/Net/allow-two-primaries", "yes"); - rdm.setOverrideProps(props); - ApiCallRcList answers = api.resourceDefinitionModify(rscName, rdm); + props.put("DrbdOptions/Net/protocol", "C"); + rcm.setOverrideProps(props); + ApiCallRcList answers = api.resourceConnectionModify(rscName, inUseNode, localNodeName, rcm); if (answers.hasError()) { - s_logger.error("Unable to set 'allow-two-primaries' on " + rscName); + s_logger.error(String.format( + "Unable to set protocol C and 'allow-two-primaries' on %s/%s/%s", + inUseNode, localNodeName, rscName)); // do not fail here as adding allow-two-primaries property is only a problem while live migrating } } @@ -310,6 +317,23 @@ public class LinstorStorageAdaptor implements StorageAdaptor { return true; } + private void removeTwoPrimariesRcProps(DevelopersApi api, String inUseNode, String rscName) throws ApiException { + ResourceConnectionModify rcm = new ResourceConnectionModify(); + List deleteProps = new ArrayList<>(); + deleteProps.add("DrbdOptions/Net/allow-two-primaries"); + deleteProps.add("DrbdOptions/Net/protocol"); + rcm.deleteProps(deleteProps); + ApiCallRcList answers = api.resourceConnectionModify(rscName, localNodeName, inUseNode, rcm); + if (answers.hasError()) { + s_logger.error( + String.format("Failed to remove 'protocol' and 'allow-two-primaries' on %s/%s/%s: %s", + localNodeName, + inUseNode, + rscName, LinstorUtil.getBestErrorMessage(answers))); + // do not fail here as removing allow-two-primaries property isn't fatal + } + } + private boolean tryDisconnectLinstor(String volumePath, KVMStoragePool pool) { if (volumePath == null) { @@ -339,9 +363,18 @@ public class LinstorStorageAdaptor implements StorageAdaptor { if (optRsc.isPresent()) { + Resource rsc = optRsc.get(); try { - Resource rsc = optRsc.get(); + String inUseNode = LinstorUtil.isResourceInUse(api, rsc.getName()); + if (inUseNode != null && !inUseNode.equalsIgnoreCase(localNodeName)) { + removeTwoPrimariesRcProps(api, inUseNode, rsc.getName()); + } + } catch (ApiException apiEx) { + s_logger.error(apiEx.getBestMessage()); + // do not fail here as removing allow-two-primaries property or deleting diskless isn't fatal + } + try { // if diskless resource remove it, in the worst case it will be transformed to a tiebreaker if (rsc.getFlags() != null && rsc.getFlags().contains(ApiConsts.FLAG_DRBD_DISKLESS) && @@ -349,17 +382,6 @@ public class LinstorStorageAdaptor implements StorageAdaptor { ApiCallRcList delAnswers = api.resourceDelete(rsc.getName(), localNodeName); logLinstorAnswers(delAnswers); } - - // remove allow-two-primaries - ResourceDefinitionModify rdm = new ResourceDefinitionModify(); - rdm.deleteProps(Collections.singletonList("DrbdOptions/Net/allow-two-primaries")); - ApiCallRcList answers = api.resourceDefinitionModify(rsc.getName(), rdm); - if (answers.hasError()) { - s_logger.error( - String.format("Failed to remove 'allow-two-primaries' on %s: %s", - rsc.getName(), LinstorUtil.getBestErrorMessage(answers))); - // do not fail here as removing allow-two-primaries property isn't fatal - } } catch (ApiException apiEx) { s_logger.error(apiEx.getBestMessage()); // do not fail here as removing allow-two-primaries property or deleting diskless isn't fatal diff --git a/plugins/storage/volume/linstor/src/main/java/org/apache/cloudstack/storage/datastore/util/LinstorUtil.java b/plugins/storage/volume/linstor/src/main/java/org/apache/cloudstack/storage/datastore/util/LinstorUtil.java index 9f4dbae7835..db2b5d1ea0b 100644 --- a/plugins/storage/volume/linstor/src/main/java/org/apache/cloudstack/storage/datastore/util/LinstorUtil.java +++ b/plugins/storage/volume/linstor/src/main/java/org/apache/cloudstack/storage/datastore/util/LinstorUtil.java @@ -97,17 +97,20 @@ public class LinstorUtil { * * @param api developer api object to use * @param rscName resource name to check in use state. - * @return True if a resource found that is in use(primary) state, else false. + * @return NodeName where the resource is inUse, if not in use `null` * @throws ApiException forwards api errors */ - public static boolean isResourceInUse(DevelopersApi api, String rscName) throws ApiException { + public static String isResourceInUse(DevelopersApi api, String rscName) throws ApiException { List rscs = api.resourceList(rscName, null, null); if (rscs != null) { return rscs.stream() - .anyMatch(rsc -> rsc.getState() != null && Boolean.TRUE.equals(rsc.getState().isInUse())); - } + .filter(rsc -> rsc.getState() != null && Boolean.TRUE.equals(rsc.getState().isInUse())) + .map(Resource::getNodeName) + .findFirst() + .orElse(null); + } s_logger.error("isResourceInUse: null returned from resourceList"); - return false; + return null; } /** From b78aede2b705041636ffda99a25697b6f707ba43 Mon Sep 17 00:00:00 2001 From: Suresh Kumar Anaparti Date: Wed, 4 Sep 2024 11:54:33 +0530 Subject: [PATCH 2/7] Updated listStoragePools response - added new managed parameter (#9588) --- .../api/response/StoragePoolResponse.java | 12 ++++++++++++ .../cloud/api/query/dao/StoragePoolJoinDaoImpl.java | 13 +++++++++++++ 2 files changed, 25 insertions(+) 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 bd468a9201f..06d5103d731 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 @@ -141,6 +141,10 @@ public class StoragePoolResponse extends BaseResponseWithAnnotations { @Param(description = "the storage pool capabilities") private Map caps; + @SerializedName(ApiConstants.MANAGED) + @Param(description = "whether this pool is managed or not") + private Boolean managed; + public Map getCaps() { return caps; } @@ -383,4 +387,12 @@ public class StoragePoolResponse extends BaseResponseWithAnnotations { public void setTagARule(Boolean tagARule) { isTagARule = tagARule; } + + public Boolean getManaged() { + return managed; + } + + public void setManaged(Boolean managed) { + this.managed = managed; + } } diff --git a/server/src/main/java/com/cloud/api/query/dao/StoragePoolJoinDaoImpl.java b/server/src/main/java/com/cloud/api/query/dao/StoragePoolJoinDaoImpl.java index 8c828ba2067..186e121b62c 100644 --- a/server/src/main/java/com/cloud/api/query/dao/StoragePoolJoinDaoImpl.java +++ b/server/src/main/java/com/cloud/api/query/dao/StoragePoolJoinDaoImpl.java @@ -171,6 +171,7 @@ public class StoragePoolJoinDaoImpl extends GenericDaoBase Date: Wed, 4 Sep 2024 11:56:17 +0530 Subject: [PATCH 3/7] server: fix volume migration check for local volume attach on a stopped (#9578) vm Fixes #8645 When a local storage volume is being attached to a stopped VM, volume migration is only needed when it is not present on the last host as the current host ID will be null in the database. Signed-off-by: Abhishek Kumar --- .../main/java/com/cloud/storage/VolumeApiServiceImpl.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java index e6092223f01..f52cd155142 100644 --- a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java +++ b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java @@ -4312,7 +4312,11 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic } } else if (storeForNewStoreScope.getScopeType() == ScopeType.HOST && (storeForExistingStoreScope.getScopeType() == ScopeType.CLUSTER || storeForExistingStoreScope.getScopeType() == ScopeType.ZONE)) { - Long hostId = _vmInstanceDao.findById(existingVolume.getInstanceId()).getHostId(); + VMInstanceVO vm = _vmInstanceDao.findById(existingVolume.getInstanceId()); + Long hostId = vm.getHostId(); + if (hostId == null) { + hostId = vm.getLastHostId(); + } if (storeForNewStoreScope.getScopeId().equals(hostId)) { return false; } From 1ca9a10912d3a7014bb355a015cdf440f837b1ba Mon Sep 17 00:00:00 2001 From: Wei Zhou Date: Wed, 4 Sep 2024 08:27:28 +0200 Subject: [PATCH 4/7] VR: remove vpn user info when apply vpn users list (#9568) Prior to this PR ``` root@r-663-VM:/var/cache/cloud# gzip -dk vpn_user_list.json.aae73e2c-32ba-44f3-bf47-426933a67bcb.gz root@r-663-VM:/var/cache/cloud# /opt/cloud/bin/update_config.py vpn_user_list.json.aae73e2c-32ba-44f3-bf47-426933a67bcb {'id': 'vpnuserlist', 'test': {'add': True, 'password': 'test', 'user': 'test'}} {'vpn_users': [{'user': 'test', 'password': 'test', 'add': True}], 'type': 'vpnuserlist', 'delete_from_processed_cache': False} line = # Secrets for authentication using CHAP line = # client server secret IP addresses line = line = line = test * test * ``` with this PR ``` root@r-663-VM:/var/cache/cloud# gzip -dk vpn_user_list.json.aae73e2c-32ba-44f3-bf47-426933a67bcb.gz root@r-663-VM:/var/cache/cloud# /opt/cloud/bin/update_config.py vpn_user_list.json.aae73e2c-32ba-44f3-bf47-426933a67bcb root@r-663-VM:/var/cache/cloud# ``` --- systemvm/debian/opt/cloud/bin/cs/CsFile.py | 1 - systemvm/debian/opt/cloud/bin/cs_vpnusers.py | 2 -- 2 files changed, 3 deletions(-) diff --git a/systemvm/debian/opt/cloud/bin/cs/CsFile.py b/systemvm/debian/opt/cloud/bin/cs/CsFile.py index 2ee631a89d6..a60b6c6ad2e 100755 --- a/systemvm/debian/opt/cloud/bin/cs/CsFile.py +++ b/systemvm/debian/opt/cloud/bin/cs/CsFile.py @@ -153,7 +153,6 @@ class CsFile: logging.debug("Searching for %s string " % search) for index, line in enumerate(self.new_config): - print ' line = ' + line if line.lstrip().startswith(ignoreLinesStartWith): continue if search in line: diff --git a/systemvm/debian/opt/cloud/bin/cs_vpnusers.py b/systemvm/debian/opt/cloud/bin/cs_vpnusers.py index 3bef1fec239..7188741d518 100755 --- a/systemvm/debian/opt/cloud/bin/cs_vpnusers.py +++ b/systemvm/debian/opt/cloud/bin/cs_vpnusers.py @@ -22,8 +22,6 @@ import copy def merge(dbag, data): dbagc = copy.deepcopy(dbag) - print dbag - print data if "vpn_users" not in data: return dbagc From 0ba9a292d50613cb2ade2fa999d97a7cddb5f385 Mon Sep 17 00:00:00 2001 From: Vishesh Date: Wed, 4 Sep 2024 11:58:44 +0530 Subject: [PATCH 5/7] Add validation for secstorage.allowed.internal.sites (#9567) * Add validation for secstorage.allowed.internal.sites * Address comments * Apply suggestions from code review Co-authored-by: Suresh Kumar Anaparti * Address comments --------- Co-authored-by: Suresh Kumar Anaparti --- .../configuration/ConfigurationManagerImpl.java | 14 ++++++++++++++ .../SecondaryStorageManagerImpl.java | 5 +++++ ui/src/views/setting/ConfigurationValue.vue | 2 +- .../main/java/com/cloud/utils/net/NetUtils.java | 12 ++++++++++++ .../java/com/cloud/utils/net/NetUtilsTest.java | 11 +++++++++++ 5 files changed, 43 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java b/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java index 29579759c7f..9df33b47257 100644 --- a/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java +++ b/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java @@ -303,6 +303,8 @@ import com.google.common.base.Preconditions; import com.google.common.collect.Sets; import com.googlecode.ipv6.IPv6Network; +import static com.cloud.configuration.Config.SecStorageAllowedInternalDownloadSites; + public class ConfigurationManagerImpl extends ManagerBase implements ConfigurationManager, ConfigurationService, Configurable { public static final Logger s_logger = Logger.getLogger(ConfigurationManagerImpl.class); public static final String PERACCOUNT = "peraccount"; @@ -1314,6 +1316,18 @@ public class ConfigurationManagerImpl extends ManagerBase implements Configurati } } + if (type.equals(String.class)) { + if (name.equalsIgnoreCase(SecStorageAllowedInternalDownloadSites.key()) && StringUtils.isNotEmpty(value)) { + final String[] cidrs = value.split(","); + for (final String cidr : cidrs) { + if (!NetUtils.isValidIp4(cidr) && !NetUtils.isValidIp6(cidr) && !NetUtils.getCleanIp4Cidr(cidr).equals(cidr)) { + s_logger.error(String.format("Invalid CIDR %s value specified for the config %s", cidr, name)); + throw new InvalidParameterValueException(String.format("Invalid CIDR %s value specified for the config %s", cidr, name)); + } + } + } + } + if (configuration == null ) { //range validation has to be done per case basis, for now //return in case of Configkey parameters diff --git a/services/secondary-storage/controller/src/main/java/org/apache/cloudstack/secondarystorage/SecondaryStorageManagerImpl.java b/services/secondary-storage/controller/src/main/java/org/apache/cloudstack/secondarystorage/SecondaryStorageManagerImpl.java index f37caa712bc..cd6f23923e1 100644 --- a/services/secondary-storage/controller/src/main/java/org/apache/cloudstack/secondarystorage/SecondaryStorageManagerImpl.java +++ b/services/secondary-storage/controller/src/main/java/org/apache/cloudstack/secondarystorage/SecondaryStorageManagerImpl.java @@ -158,6 +158,8 @@ import com.cloud.vm.dao.SecondaryStorageVmDao; import com.cloud.vm.dao.UserVmDetailsDao; import com.cloud.vm.dao.VMInstanceDao; +import static com.cloud.configuration.Config.SecStorageAllowedInternalDownloadSites; + /** * Class to manage secondary storages.

* Possible secondary storage VM state transition cases:
@@ -401,6 +403,9 @@ public class SecondaryStorageManagerImpl extends ManagerBase implements Secondar String[] cidrs = _allowedInternalSites.split(","); for (String cidr : cidrs) { if (NetUtils.isValidIp4Cidr(cidr) || NetUtils.isValidIp4(cidr) || !cidr.startsWith("0.0.0.0")) { + if (NetUtils.getCleanIp4Cidr(cidr).equals(cidr)) { + s_logger.warn(String.format("Invalid CIDR %s in %s", cidr, SecStorageAllowedInternalDownloadSites.key())); + } allowedCidrs.add(cidr); } } diff --git a/ui/src/views/setting/ConfigurationValue.vue b/ui/src/views/setting/ConfigurationValue.vue index 0069896f7a5..836aed69dd3 100644 --- a/ui/src/views/setting/ConfigurationValue.vue +++ b/ui/src/views/setting/ConfigurationValue.vue @@ -266,7 +266,7 @@ export default { this.$message.error(this.$t('message.error.save.setting')) this.$notification.error({ message: this.$t('label.error'), - description: this.$t('message.error.save.setting') + description: error?.response?.data?.updateconfigurationresponse?.errortext || this.$t('message.error.save.setting') }) }).finally(() => { this.valueLoading = false diff --git a/utils/src/main/java/com/cloud/utils/net/NetUtils.java b/utils/src/main/java/com/cloud/utils/net/NetUtils.java index 91a2f4eb755..1b4ebcccf94 100644 --- a/utils/src/main/java/com/cloud/utils/net/NetUtils.java +++ b/utils/src/main/java/com/cloud/utils/net/NetUtils.java @@ -626,6 +626,18 @@ public class NetUtils { return long2Ip(firstPart) + "/" + size; } + public static String getCleanIp4Cidr(final String cidr) { + if (!isValidIp4Cidr(cidr)) { + throw new CloudRuntimeException("Invalid CIDR: " + cidr); + } + String gateway = cidr.split("/")[0]; + Long netmaskSize = Long.parseLong(cidr.split("/")[1]); + final long ip = ip2Long(gateway); + final long startNetMask = ip2Long(getCidrNetmask(netmaskSize)); + final long start = (ip & startNetMask); + return String.format("%s/%s", long2Ip(start), netmaskSize); + } + public static String[] getIpRangeFromCidr(final String cidr, final long size) { assert size < MAX_CIDR : "You do know this is not for ipv6 right? Keep it smaller than 32 but you have " + size; final String[] result = new String[2]; diff --git a/utils/src/test/java/com/cloud/utils/net/NetUtilsTest.java b/utils/src/test/java/com/cloud/utils/net/NetUtilsTest.java index defb440c2a5..0f19da38922 100644 --- a/utils/src/test/java/com/cloud/utils/net/NetUtilsTest.java +++ b/utils/src/test/java/com/cloud/utils/net/NetUtilsTest.java @@ -296,6 +296,17 @@ public class NetUtilsTest { assertTrue(NetUtils.isValidIp4Cidr(cidrThird));; } + @Test + public void testGetCleanIp4Cidr() throws Exception { + final String cidrFirst = "10.0.144.0/20"; + final String cidrSecond = "10.0.151.5/20"; + final String cidrThird = "10.0.144.10/21"; + + assertEquals(cidrFirst, NetUtils.getCleanIp4Cidr(cidrFirst)); + assertEquals("10.0.144.0/20", NetUtils.getCleanIp4Cidr(cidrSecond)); + assertEquals("10.0.144.0/21", NetUtils.getCleanIp4Cidr(cidrThird));; + } + @Test public void testIsValidCidrList() throws Exception { final String cidrFirst = "10.0.144.0/20,1.2.3.4/32,5.6.7.8/24"; From e06f80e899127abe45c10ff2d38420a770ceb076 Mon Sep 17 00:00:00 2001 From: Wei Zhou Date: Wed, 4 Sep 2024 08:32:03 +0200 Subject: [PATCH 6/7] storage: fix private templates are not copied to new image store (#9206) ``` 2024-06-10T12:24:50,711 INFO [o.a.c.s.i.TemplateServiceImpl] (qtp776484396-20:[ctx-eb090c22, ctx-5fa5579c]) (logid:7a783000) Template Sync did not find 211-2-d536fb03-5f89-3e77-8dea-323315bcbfab on image store 3, may request download based on available hypervisor types ... 2024-06-10T12:24:51,043 INFO [o.a.c.s.i.TemplateServiceImpl] (qtp776484396-20:[ctx-eb090c22, ctx-5fa5579c]) (logid:7a783000) Skip sync downloading private template 211-2-d536fb03-5f89-3e77-8dea-323315bcbfab to a new image store ``` --- .../apache/cloudstack/storage/image/TemplateServiceImpl.java | 5 ----- 1 file changed, 5 deletions(-) diff --git a/engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/TemplateServiceImpl.java b/engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/TemplateServiceImpl.java index 6c4fcab2f17..c040e1a7ad6 100644 --- a/engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/TemplateServiceImpl.java +++ b/engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/TemplateServiceImpl.java @@ -532,11 +532,6 @@ public class TemplateServiceImpl implements TemplateService { s_logger.info("Skip downloading template " + tmplt.getUniqueName() + " since no url is specified."); continue; } - // if this is private template, skip sync to a new image store - if (isSkipTemplateStoreDownload(tmplt, zoneId)) { - s_logger.info("Skip sync downloading private template " + tmplt.getUniqueName() + " to a new image store"); - continue; - } // if this is a region store, and there is already an DOWNLOADED entry there without install_path information, which // means that this is a duplicate entry from migration of previous NFS to staging. From 601e9b67eadc139b52d2f1b204f6b56c2d18c9ac Mon Sep 17 00:00:00 2001 From: Harikrishna Date: Wed, 4 Sep 2024 12:14:50 +0530 Subject: [PATCH 7/7] Fix snapshot deletion on template creation failure (#9239) * Don't delete the snapshot itself on the primary storage upon any failure * Change an if condition --- .../cloudstack/snapshot/SnapshotHelper.java | 20 ++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/server/src/main/java/org/apache/cloudstack/snapshot/SnapshotHelper.java b/server/src/main/java/org/apache/cloudstack/snapshot/SnapshotHelper.java index 7f0ee77deaf..b47f64de2bf 100644 --- a/server/src/main/java/org/apache/cloudstack/snapshot/SnapshotHelper.java +++ b/server/src/main/java/org/apache/cloudstack/snapshot/SnapshotHelper.java @@ -103,15 +103,21 @@ public class SnapshotHelper { return; } - logger.debug(String.format("Expunging snapshot [%s] due to it is a temporary backup to create a volume from snapshot. It is occurring because the global setting [%s]" - + " has the value [%s].", snapInfo.getId(), SnapshotInfo.BackupSnapshotAfterTakingSnapshot.key(), backupSnapshotAfterTakingSnapshot)); + if (!DataStoreRole.Image.equals(snapInfo.getDataStore().getRole())) { + logger.debug(String.format("Expunge template for Snapshot [%s] is called for primary storage role. Not expunging it, " + + "but we will still expunge the database reference of the snapshot for image storage role if any", snapInfo.getId())); + } else { + logger.debug(String.format("Expunging snapshot [%s] due to it is a temporary backup to create a volume from snapshot. It is occurring because the global setting [%s]" + + " has the value [%s].", snapInfo.getId(), SnapshotInfo.BackupSnapshotAfterTakingSnapshot.key(), backupSnapshotAfterTakingSnapshot)); - try { - snapshotService.deleteSnapshot(snapInfo); - } catch (CloudRuntimeException ex) { - logger.warn(String.format("Unable to delete the temporary snapshot [%s] on secondary storage due to [%s]. We still will expunge the database reference, consider" - + " manually deleting the file [%s].", snapInfo.getId(), ex.getMessage(), snapInfo.getPath()), ex); + try { + snapshotService.deleteSnapshot(snapInfo); + } catch (CloudRuntimeException ex) { + logger.warn(String.format("Unable to delete the temporary snapshot [%s] on secondary storage due to [%s]. We still will expunge the database reference, consider" + + " manually deleting the file [%s].", snapInfo.getId(), ex.getMessage(), snapInfo.getPath()), ex); + } } + long storeId = snapInfo.getDataStore().getId(); if (!DataStoreRole.Image.equals(snapInfo.getDataStore().getRole())) { long zoneId = dataStorageManager.getStoreZoneId(storeId, snapInfo.getDataStore().getRole());