diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index 689275cff115..375912667d63 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -17,7 +17,7 @@ /plugins/storage/volume/linstor @rp- /plugins/storage/volume/storpool @slavkap -/plugins/storage/volume/ontap @rajiv1 @sandeeplocharla @piyush5 @suryag +/plugins/storage/volume/ontap @rajiv-jain-netapp @sandeeplocharla @piyush5netapp @suryag1201 .pre-commit-config.yaml @jbampton /.github/linters/ @jbampton diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/IscsiAdmStorageAdaptor.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/IscsiAdmStorageAdaptor.java index aa0088064ca7..d0a639715034 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/IscsiAdmStorageAdaptor.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/IscsiAdmStorageAdaptor.java @@ -350,7 +350,10 @@ private boolean hasOtherActiveLuns(String host, int port, String iqn, String lun } for (java.io.File entry : entries) { String name = entry.getName(); - if (name.startsWith(prefix) && !name.equals(prefix + lun)) { + // Skip partition entries (e.g. lun-0-part1, lun-0-part2) — these are not + // independent LUNs, they are partition symlinks for the same LUN disk. + // Only count actual LUN entries (no "-part" suffix after the lun number). + if (name.startsWith(prefix) && !name.equals(prefix + lun) && !name.contains("-part")) { logger.debug("Found other active LUN on same target: " + name); return true; } @@ -358,6 +361,45 @@ private boolean hasOtherActiveLuns(String host, int port, String iqn, String lun return false; } + /** + * Removes a single stale SCSI device from the kernel using the sysfs interface. + * + * When ONTAP unmaps a LUN from the host's igroup, the by-path symlink and the + * underlying SCSI device (/dev/sdX) remain present in the kernel until explicitly + * removed — the kernel does not auto-remove devices from live iSCSI sessions. + * + * This method resolves the by-path symlink to the real block device name (e.g. sdd), + * then writes "1" to /sys/block//device/delete — the standard Linux kernel SCSI + * API for removing a single device without tearing down the entire iSCSI session. + * Once the kernel processes the delete, it also removes the by-path symlink. + * + * This is used instead of iscsiadm --logout when other LUNs on the same IQN are still + * active (ONTAP single-IQN-per-SVM model), since logout would tear down ALL LUNs. + */ + private void removeStaleScsiDevice(String host, int port, String iqn, String lun) { + String byPath = getByPath(host, port, "/" + iqn + "/" + lun); + java.nio.file.Path byPathLink = java.nio.file.Paths.get(byPath); + if (!java.nio.file.Files.exists(byPathLink)) { + logger.debug("by-path entry for LUN " + lun + " already gone, nothing to remove"); + return; + } + try { + java.nio.file.Path realDevice = byPathLink.toRealPath(); + String devName = realDevice.getFileName().toString(); + java.io.File deleteFile = new java.io.File("/sys/block/" + devName + "/device/delete"); + if (!deleteFile.exists()) { + logger.warn("sysfs delete entry not found for device " + devName + " — cannot remove stale SCSI device"); + return; + } + try (java.io.FileWriter fw = new java.io.FileWriter(deleteFile)) { + fw.write("1"); + } + logger.info("Removed stale SCSI device " + devName + " for LUN /" + iqn + "/" + lun + " via sysfs"); + } catch (Exception e) { + logger.warn("Failed to remove stale SCSI device for LUN /" + iqn + "/" + lun + ": " + e.getMessage()); + } + } + private boolean disconnectPhysicalDisk(String host, int port, String iqn, String lun) { // Check if other LUNs on the same IQN target are still in use. // ONTAP (and similar) uses a single IQN per SVM with multiple LUNs. @@ -365,8 +407,15 @@ private boolean disconnectPhysicalDisk(String host, int port, String iqn, String // which would destroy access to ALL LUNs — not just the one being disconnected. if (hasOtherActiveLuns(host, port, iqn, lun)) { logger.info("Skipping iSCSI logout for /" + iqn + "/" + lun + - " — other LUNs on the same target are still active"); - return true; + " — other LUNs on the same target are still active. Removing stale SCSI device for this LUN only."); + removeStaleScsiDevice(host, port, iqn, lun); + // After removing this LUN's device, re-check: if no other LUNs remain active, + // If it is the last one then must logout to clean up the iSCSI session entirely. + if (hasOtherActiveLuns(host, port, iqn, lun)) { + logger.info("Other LUNs still active after removing /" + iqn + "/" + lun + " — session kept alive."); + return true; + } + logger.info("No more active LUNs on target after removing /" + iqn + "/" + lun + " — proceeding with iSCSI logout."); } // No other LUNs active on this target — safe to logout and delete the node record. diff --git a/plugins/storage/volume/ontap/pom.xml b/plugins/storage/volume/ontap/pom.xml index 94ca574e1788..12035f01d7f9 100644 --- a/plugins/storage/volume/ontap/pom.xml +++ b/plugins/storage/volume/ontap/pom.xml @@ -137,7 +137,7 @@ org.apache.cloudstack cloud-engine-storage-snapshot - 4.23.0.0-SNAPSHOT + ${project.version} compile diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/client/NASFeignClient.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/client/NASFeignClient.java index ecc31badf283..8cf21b94b2f1 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/client/NASFeignClient.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/client/NASFeignClient.java @@ -21,9 +21,7 @@ import feign.QueryMap; import org.apache.cloudstack.storage.feign.model.ExportPolicy; -import org.apache.cloudstack.storage.feign.model.FileClone; import org.apache.cloudstack.storage.feign.model.FileInfo; -import org.apache.cloudstack.storage.feign.model.response.JobResponse; import org.apache.cloudstack.storage.feign.model.response.OntapResponse; import feign.Headers; import feign.Param; @@ -60,11 +58,6 @@ void createFile(@Param("authHeader") String authHeader, @Param("path") String filePath, FileInfo file); - @RequestLine("POST /api/storage/file/clone") - @Headers({"Authorization: {authHeader}"}) - JobResponse cloneFile(@Param("authHeader") String authHeader, - FileClone fileClone); - // Export Policy Operations @RequestLine("POST /api/protocols/nfs/export-policies") @Headers({"Authorization: {authHeader}"}) diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/FileClone.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/FileClone.java deleted file mode 100644 index 08cccc42f905..000000000000 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/FileClone.java +++ /dev/null @@ -1,62 +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. - */ - -package org.apache.cloudstack.storage.feign.model; - -import com.fasterxml.jackson.annotation.JsonIgnoreProperties; -import com.fasterxml.jackson.annotation.JsonInclude; -import com.fasterxml.jackson.annotation.JsonProperty; - -@JsonIgnoreProperties(ignoreUnknown = true) -@JsonInclude(JsonInclude.Include.NON_NULL) -public class FileClone { - @JsonProperty("source_path") - private String sourcePath; - @JsonProperty("destination_path") - private String destinationPath; - @JsonProperty("volume") - private VolumeConcise volume; - @JsonProperty("overwrite_destination") - private Boolean overwriteDestination; - - public VolumeConcise getVolume() { - return volume; - } - public void setVolume(VolumeConcise volume) { - this.volume = volume; - } - public String getSourcePath() { - return sourcePath; - } - public void setSourcePath(String sourcePath) { - this.sourcePath = sourcePath; - } - public String getDestinationPath() { - return destinationPath; - } - public void setDestinationPath(String destinationPath) { - this.destinationPath = destinationPath; - } - public Boolean getOverwriteDestination() { - return overwriteDestination; - } - public void setOverwriteDestination(Boolean overwriteDestination) { - this.overwriteDestination = overwriteDestination; - } -} diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/VolumeConcise.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/VolumeConcise.java index eaa5b2ed2ae9..602c9ff73658 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/VolumeConcise.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/VolumeConcise.java @@ -39,5 +39,5 @@ public void setUuid(String uuid) { public String getName() { return name; } - public void setName(String name) {} + public void setName(String name) { this.name = name; } } diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycle.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycle.java index b055dad425a8..baf19f71b759 100755 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycle.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycle.java @@ -54,6 +54,7 @@ import org.apache.cloudstack.storage.utils.OntapStorageConstants; import org.apache.cloudstack.storage.utils.OntapStorageUtils; import org.apache.cloudstack.storage.volume.datastore.PrimaryDataStoreHelper; +import org.apache.commons.lang3.StringUtils; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -420,7 +421,7 @@ private boolean validateProtocolSupportAndFetchHostsIdentifier(List host for (HostVO host : hosts) { if (host != null) { ip = host.getStorageIpAddress() != null ? host.getStorageIpAddress().trim() : ""; - if (ip.isEmpty() && host.getPrivateIpAddress() != null || host.getPrivateIpAddress().trim().isEmpty()) { + if (ip.isEmpty() && StringUtils.isBlank(host.getPrivateIpAddress() )) { // TODO we will inform customer through alert for excluded host because of protocol enabled on host continue; } else { diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/listener/OntapHostListener.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/listener/OntapHostListener.java index fd527d285285..29aa89bff6c3 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/listener/OntapHostListener.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/listener/OntapHostListener.java @@ -100,6 +100,13 @@ public boolean hostConnect(long hostId, long poolId) { } // Get the mount path from the answer + + if (!(answer instanceof ModifyStoragePoolAnswer)) { + throw new CloudRuntimeException(String.format( + "Unexpected answer type %s returned for modify storage pool command for pool %s on host %d", + answer.getClass().getName(), pool, hostId)); + } + ModifyStoragePoolAnswer mspAnswer = (ModifyStoragePoolAnswer) answer; StoragePoolInfo poolInfo = mspAnswer.getPoolInfo(); if (poolInfo == null) { diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedSANStrategy.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedSANStrategy.java index a9664f4d4f24..af7410be10c4 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedSANStrategy.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedSANStrategy.java @@ -37,6 +37,7 @@ import org.apache.cloudstack.storage.service.model.ProtocolType; import org.apache.cloudstack.storage.utils.OntapStorageConstants; import org.apache.cloudstack.storage.utils.OntapStorageUtils; +import org.apache.commons.collections.CollectionUtils; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import javax.inject.Inject; @@ -274,47 +275,49 @@ public void deleteAccessGroup(AccessGroup accessGroup) { String authHeader = OntapStorageUtils.generateAuthHeader(storage.getUsername(), storage.getPassword()); String svmName = storage.getSvmName(); //Get iGroup name per host - for(HostVO host : accessGroup.getHostsToConnect()) { - String igroupName = OntapStorageUtils.getIgroupName(svmName, host.getName()); - logger.info("deleteAccessGroup: iGroup name '{}'", igroupName); - - // Get the iGroup to retrieve its UUID - Map igroupParams = Map.of( - OntapStorageConstants.SVM_DOT_NAME, svmName, - OntapStorageConstants.NAME, igroupName - ); - - try { - OntapResponse igroupResponse = sanFeignClient.getIgroupResponse(authHeader, igroupParams); - if (igroupResponse == null || igroupResponse.getRecords() == null || igroupResponse.getRecords().isEmpty()) { - logger.warn("deleteAccessGroup: iGroup '{}' not found, may have been already deleted", igroupName); - return; - } - - Igroup igroup = igroupResponse.getRecords().get(0); - String igroupUuid = igroup.getUuid(); - - if (igroupUuid == null || igroupUuid.isEmpty()) { - throw new CloudRuntimeException(" iGroup UUID is null or empty for iGroup: " + igroupName); - } - - logger.info("deleteAccessGroup: Deleting iGroup '{}' with UUID '{}'", igroupName, igroupUuid); - - // Delete the iGroup using the UUID - sanFeignClient.deleteIgroup(authHeader, igroupUuid); + if(!CollectionUtils.isEmpty(accessGroup.getHostsToConnect())) { + for (HostVO host : accessGroup.getHostsToConnect()) { + String igroupName = OntapStorageUtils.getIgroupName(svmName, host.getName()); + logger.info("deleteAccessGroup: iGroup name '{}'", igroupName); - logger.info("deleteAccessGroup: Successfully deleted iGroup '{}'", igroupName); - - } catch (FeignException e) { - if (e.status() == 404) { - logger.warn("deleteAccessGroup: iGroup '{}' does not exist (status 404), skipping deletion", igroupName); - } else { - logger.error("deleteAccessGroup: FeignException occurred: Status: {}, Exception: {}", e.status(), e.getMessage(), e); + // Get the iGroup to retrieve its UUID + Map igroupParams = Map.of( + OntapStorageConstants.SVM_DOT_NAME, svmName, + OntapStorageConstants.NAME, igroupName + ); + + try { + OntapResponse igroupResponse = sanFeignClient.getIgroupResponse(authHeader, igroupParams); + if (igroupResponse == null || igroupResponse.getRecords() == null || igroupResponse.getRecords().isEmpty()) { + logger.warn("deleteAccessGroup: iGroup '{}' not found, may have been already deleted", igroupName); + return; + } + + Igroup igroup = igroupResponse.getRecords().get(0); + String igroupUuid = igroup.getUuid(); + + if (igroupUuid == null || igroupUuid.isEmpty()) { + throw new CloudRuntimeException(" iGroup UUID is null or empty for iGroup: " + igroupName); + } + + logger.info("deleteAccessGroup: Deleting iGroup '{}' with UUID '{}'", igroupName, igroupUuid); + + // Delete the iGroup using the UUID + sanFeignClient.deleteIgroup(authHeader, igroupUuid); + + logger.info("deleteAccessGroup: Successfully deleted iGroup '{}'", igroupName); + + } catch (FeignException e) { + if (e.status() == 404) { + logger.warn("deleteAccessGroup: iGroup '{}' does not exist (status 404), skipping deletion", igroupName); + } else { + logger.error("deleteAccessGroup: FeignException occurred: Status: {}, Exception: {}", e.status(), e.getMessage(), e); + throw e; + } + } catch (Exception e) { + logger.error("deleteAccessGroup: Exception occurred: {}", e.getMessage(), e); throw e; } - } catch (Exception e) { - logger.error("deleteAccessGroup: Exception occurred: {}", e.getMessage(), e); - throw e; } } } catch (FeignException e) { diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageUtils.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageUtils.java index 22c30c1256ac..ae2663aa4620 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageUtils.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageUtils.java @@ -35,6 +35,8 @@ import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.springframework.util.Base64Utils; + +import java.nio.charset.StandardCharsets; import java.util.Map; public class OntapStorageUtils { @@ -51,7 +53,7 @@ public class OntapStorageUtils { * @return */ public static String generateAuthHeader (String username, String password) { - byte[] encodedBytes = Base64Utils.encode((username + AUTH_HEADER_COLON + password).getBytes()); + byte[] encodedBytes = Base64Utils.encode((username + AUTH_HEADER_COLON + password).getBytes(StandardCharsets.UTF_8)); return BASIC + StringUtils.SPACE + new String(encodedBytes); } diff --git a/server/src/main/java/com/cloud/vm/snapshot/VMSnapshotManagerImpl.java b/server/src/main/java/com/cloud/vm/snapshot/VMSnapshotManagerImpl.java index fb5becfe9d25..029c36a98fc0 100644 --- a/server/src/main/java/com/cloud/vm/snapshot/VMSnapshotManagerImpl.java +++ b/server/src/main/java/com/cloud/vm/snapshot/VMSnapshotManagerImpl.java @@ -135,6 +135,7 @@ public class VMSnapshotManagerImpl extends MutualExclusiveIdsManagerBase implements VMSnapshotManager, VMSnapshotService, VmWorkJobHandler, Configurable { public static final String VM_WORK_JOB_HANDLER = VMSnapshotManagerImpl.class.getSimpleName(); + public static final String ONTAP_PLUGIN_NAME = "NetApp ONTAP"; @Inject VMInstanceDao _vmInstanceDao; @@ -392,7 +393,7 @@ public VMSnapshot allocVMSnapshot(Long vmId, String vsDisplayName, String vsDesc if (snapshotStrategy == null) { // Check if this is ONTAP managed storage with memory snapshot request - provide specific error message if (snapshotMemory && rootVolumePool.isManaged() && - "ONTAP".equals(rootVolumePool.getStorageProviderName())) { + ONTAP_PLUGIN_NAME.equals(rootVolumePool.getStorageProviderName())) { String message = String.format("Memory snapshots (snapshotmemory=true) are not supported for VMs on ONTAP managed storage. " + "Instance [%s] uses ONTAP storage which only supports disk-only (crash-consistent) snapshots. " + "Please use snapshotmemory=false for disk-only snapshots.", userVmVo.getUuid());