Skip to content

feature/CSTACKEX-122: Per host Igroup changes #37

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
piyush5netapp merged 13 commits into from
Mar 16, 2026
Merged

feature/CSTACKEX-122: Per host Igroup changes #37

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
4781b3f
feature/CSTACKEX-122: Per host Igroup changes
Mar 3, 2026
7f470fd
feature/CSTACKEX-122: Per host Igroup changes 2
Mar 5, 2026
c1535f3
feature/CSTACKEX-122: Per host Igroup changes 3
Mar 5, 2026
25cd46c
feature/CSTACKEX-122: Per host Igroup changes 4
Mar 5, 2026
121bbfe
feature/CSTACKEX-122: Comments resolution
Mar 11, 2026
36db8ae
Merge branch 'main' of https://github.com/NetApp/cloudstack into feat…
Mar 11, 2026
6f266fe
feature/CSTACKEX-122: remove method name from exception
Mar 11, 2026
c14c5cc
feature/CSTACKEX-122: UTs updated
Mar 11, 2026
305adf6
feature/CSTACKEX-122: UTs updated 2
Mar 11, 2026
f25cd63
feature/CSTACKEX-122: deleteonunmap added
Mar 11, 2026
0583966
feature/CSTACKEX-122: fix for boot disk
Mar 11, 2026
5c31c78
feature/CSTACKEX-122: fix for lun unmap
Mar 12, 2026
ef4ee96
feature/CSTACKEX-122: Comments resolution
Mar 16, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 17 additions & 2 deletions .../storage/volume/src/main/java/org/apache/cloudstack/storage/volume/VolumeServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,8 @@ public class VolumeServiceImpl implements VolumeService {
@Inject
protected DiskOfferingDao diskOfferingDao;

private static final String ONTAP_PROVIDER_NAME = "NetApp ONTAP";

public VolumeServiceImpl() {
}

Expand Down Expand Up @@ -730,8 +732,13 @@ protected Void managedCopyBaseImageCallback(AsyncCallbackDispatcher<VolumeServic
CopyCmdAnswer answer = (CopyCmdAnswer)result.getAnswer();
TemplateObjectTO templateObjectTo = (TemplateObjectTO)answer.getNewData();

volume.setPath(templateObjectTo.getPath());

// For NFS managed storage, preserve the volume UUID path to avoid file collision
Copy link
Copy Markdown

@rajiv-jain-netapp rajiv-jain-netapp Mar 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a common path. Please add the plugin check as well, since we want this applied to the ONTAP flow rather than all the vendors.

Copy link
Copy Markdown
Collaborator

@sandeeplocharla sandeeplocharla Mar 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if plugin name would be available in this method. If its not available, we can check if volume.getPath is null and set it if there's new info available.

Copy link
Copy Markdown
Author

@piyush5netapp piyush5netapp Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Introduced plugin check

PrimaryDataStore primaryDataStore = (PrimaryDataStore) volumeInfo.getDataStore();
if (primaryDataStore.isManaged() && ONTAP_PROVIDER_NAME.equals(primaryDataStore.getStorageProviderName()) && primaryDataStore.getPoolType() == StoragePoolType.NetworkFilesystem) {
logger.debug("NFS managed storage - preserving volume path: " + volume.getPath() + " (not overwriting with template path: " + templateObjectTo.getPath() + ")");
}else {
volume.setPath(templateObjectTo.getPath());
}
if (templateObjectTo.getFormat() != null) {
volume.setFormat(templateObjectTo.getFormat());
}
Expand Down Expand Up @@ -1337,6 +1344,14 @@ private void createManagedVolumeCopyTemplateAsync(VolumeInfo volumeInfo, Primary
primaryDataStore.setDetails(details);

grantAccess(volumeInfo, destHost, primaryDataStore);
volumeInfo = volFactory.getVolume(volumeInfo.getId(), primaryDataStore);
details.put(PrimaryDataStore.MANAGED_STORE_TARGET, volumeInfo.get_iScsiName());
Copy link
Copy Markdown
Author

@piyush5netapp piyush5netapp Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This particular changes are for ontap

Copy link
Copy Markdown

@rajiv-jain-netapp rajiv-jain-netapp Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add comments here for this change since it is going into commons code, it should not be reverted by any other vendor

primaryDataStore.setDetails(details);

// Update destTemplateInfo with the iSCSI path from volumeInfo
if (destTemplateInfo instanceof TemplateObject) {
Copy link
Copy Markdown

@rajiv-jain-netapp rajiv-jain-netapp Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the use of this change in the workflow?

Copy link
Copy Markdown
Author

@piyush5netapp piyush5netapp Mar 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It set install path for template

((TemplateObject)destTemplateInfo).setInstallPath(volumeInfo.getPath());
}

try {
motionSrv.copyAsync(srcTemplateInfo, destTemplateInfo, destHost, caller);
Expand Down
68 changes: 44 additions & 24 deletions ...ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import com.cloud.agent.api.to.DataTO;
import com.cloud.exception.InvalidParameterValueException;
import com.cloud.host.Host;
import com.cloud.host.HostVO;
import com.cloud.storage.Storage;
import com.cloud.storage.StoragePool;
import com.cloud.storage.Volume;
Expand Down Expand Up @@ -53,6 +54,7 @@
import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao;
import org.apache.cloudstack.storage.datastore.db.StoragePoolDetailsDao;
import org.apache.cloudstack.storage.datastore.db.StoragePoolVO;
import org.apache.cloudstack.storage.feign.model.Igroup;
import org.apache.cloudstack.storage.feign.client.SnapshotFeignClient;
import org.apache.cloudstack.storage.feign.model.FlexVolSnapshot;
import org.apache.cloudstack.storage.feign.model.Lun;
Expand All @@ -72,8 +74,10 @@
import org.apache.logging.log4j.Logger;

import javax.inject.Inject;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashMap;
import java.util.List;
import java.util.Map;

/**
Expand Down Expand Up @@ -110,6 +114,12 @@ public DataStoreTO getStoreTO(DataStore store) {
return null;
}

@Override
public boolean volumesRequireGrantAccessWhenUsed(){
Copy link
Copy Markdown
Collaborator

@sandeeplocharla sandeeplocharla Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, with this there would always be a grantAccess call before using a volume?
If yes, then any idea on what does "WhenUsed" mean? Is there a list of workflows?

Copy link
Copy Markdown
Author

@piyush5netapp piyush5netapp Mar 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as explained in previous comment, this is required when a vm is powered off and powered on again to grantAccess

s_logger.info("OntapPrimaryDatastoreDriver: volumesRequireGrantAccessWhenUsed: Called");
return true;
}

/**
* Creates a volume on the ONTAP storage system.
*/
Expand Down Expand Up @@ -154,7 +164,6 @@ public void createAsync(DataStore dataStore, DataObject dataObject, AsyncComplet
volumeVO.setPoolId(storagePool.getId());

if (ProtocolType.ISCSI.name().equalsIgnoreCase(details.get(Constants.PROTOCOL))) {
String svmName = details.get(Constants.SVM_NAME);
String lunName = created != null && created.getLun() != null ? created.getLun().getName() : null;
if (lunName == null) {
throw new CloudRuntimeException("Missing LUN name for volume " + volInfo.getId());
Expand All @@ -167,22 +176,13 @@ public void createAsync(DataStore dataStore, DataObject dataObject, AsyncComplet
volumeVO.setFolder(created.getLun().getUuid());
}

// Create LUN-to-igroup mapping and retrieve the assigned LUN ID
UnifiedSANStrategy sanStrategy = (UnifiedSANStrategy) Utility.getStrategyByStoragePoolDetails(details);
String accessGroupName = Utility.getIgroupName(svmName, storagePoolUuid);
String lunNumber = sanStrategy.ensureLunMapped(svmName, lunName, accessGroupName);

// Construct iSCSI path: /<iqn>/<lun_id> format for KVM/libvirt attachment
String iscsiPath = Constants.SLASH + storagePool.getPath() + Constants.SLASH + lunNumber;
volumeVO.set_iScsiName(iscsiPath);
volumeVO.setPath(iscsiPath);
s_logger.info("createAsync: Volume [{}] iSCSI path set to {}", volumeVO.getId(), iscsiPath);
createCmdResult = new CreateCmdResult(null, new Answer(null, true, null));

s_logger.info("createAsync: Created LUN [{}] for volume [{}]. LUN mapping will occur during grantAccess() to per-host igroup.",
lunName, volumeVO.getId());
createCmdResult = new CreateCmdResult(lunName, new Answer(null, true, null));
} else if (ProtocolType.NFS3.name().equalsIgnoreCase(details.get(Constants.PROTOCOL))) {
createCmdResult = new CreateCmdResult(volInfo.getUuid(), new Answer(null, true, null));
s_logger.info("createAsync: Managed NFS volume [{}] associated with pool {}",
volumeVO.getId(), storagePool.getId());
s_logger.info("createAsync: Managed NFS volume [{}] with path [{}] associated with pool {}",
volumeVO.getId(), volInfo.getUuid(), storagePool.getId());
}
volumeDao.update(volumeVO.getId(), volumeVO);
}
Expand Down Expand Up @@ -412,14 +412,35 @@ public boolean grantAccess(DataObject dataObject, Host host, DataStore dataStore
// Only retrieve LUN name for iSCSI volumes
String cloudStackVolumeName = volumeDetailsDao.findDetail(volumeVO.getId(), Constants.LUN_DOT_NAME).getValue();
UnifiedSANStrategy sanStrategy = (UnifiedSANStrategy) Utility.getStrategyByStoragePoolDetails(details);
String accessGroupName = Utility.getIgroupName(svmName, storagePoolUuid);

// Verify host initiator is registered in the igroup before allowing access
if (!sanStrategy.validateInitiatorInAccessGroup(host.getStorageUrl(), svmName, accessGroupName)) {
throw new CloudRuntimeException("Host initiator [" + host.getStorageUrl() +
"] is not present in iGroup [" + accessGroupName + "]");
String accessGroupName = Utility.getIgroupName(svmName, host.getName());

// Validate if Igroup exist ONTAP for this host as we may be using delete_on_unmap= true and igroup may be deleted by ONTAP automatically
Map<String, String> getAccessGroupMap = Map.of(
Constants.NAME, accessGroupName,
Constants.SVM_DOT_NAME, svmName
);
Igroup igroup = new Igroup();
AccessGroup accessGroup = sanStrategy.getAccessGroup(getAccessGroupMap);
if(accessGroup == null || accessGroup.getIgroup() == null) {
Copy link
Copy Markdown

@rajiv-jain-netapp rajiv-jain-netapp Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as we discussed, we would need a implementation even for export policy(Access group) for NFS protocol, to handle vm power-off from host1 and power-on to host2.

Copy link
Copy Markdown
Author

@piyush5netapp piyush5netapp Mar 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That will be taken care as backlog as discussed after discusion.

s_logger.info("grantAccess: Igroup {} does not exist for the host {} : Need to create Igroup for the host ", accessGroupName, host.getName());
// create the igroup for the host and perform lun-mapping
accessGroup = new AccessGroup();
List<HostVO> hosts = new ArrayList<>();
hosts.add((HostVO) host);
accessGroup.setHostsToConnect(hosts);
accessGroup.setStoragePoolId(storagePool.getId());
accessGroup = sanStrategy.createAccessGroup(accessGroup);
}else{
s_logger.info("grantAccess: Igroup {} already exist for the host {}: ", accessGroup.getIgroup().getName() ,host.getName());
igroup = accessGroup.getIgroup();
/* TODO Below cases will be covered later, for now they will be a pre-requisite on customer side
1. Igroup exist with the same name but host initiator has been rempved
Copy link
Copy Markdown
Collaborator

@sandeeplocharla sandeeplocharla Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo removed

2. Igroup exist with the same name but host initiator has been changed may be due to new NIC or new adapter
In both cases we need to verify current host initiator is registered in the igroup before allowing access
Incase it is not , add it and proceed for lun-mapping
*/
}

s_logger.info("grantAccess: Igroup {} is present now with initiators {} ", accessGroup.getIgroup().getName(), accessGroup.getIgroup().getInitiators());
// Create or retrieve existing LUN mapping
String lunNumber = sanStrategy.ensureLunMapped(svmName, cloudStackVolumeName, accessGroupName);
Copy link
Copy Markdown
Author

@piyush5netapp piyush5netapp Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a TODO, remove ensureLunMapped and do this as part of logical access operations.


Expand Down Expand Up @@ -546,7 +567,7 @@ private void revokeAccessForVolume(StoragePoolVO storagePool, VolumeVO volumeVO,

// Verify host initiator is in the igroup before attempting to remove mapping
SANStrategy sanStrategy = (UnifiedSANStrategy) storageStrategy;
if (!sanStrategy.validateInitiatorInAccessGroup(host.getStorageUrl(), svmName, accessGroup.getIgroup().getName())) {
if (!sanStrategy.validateInitiatorInAccessGroup(host.getStorageUrl(), svmName, accessGroup.getIgroup())) {
s_logger.warn("revokeAccessForVolume: Initiator [{}] is not in iGroup [{}], skipping revoke",
host.getStorageUrl(), accessGroupName);
return;
Expand Down Expand Up @@ -915,7 +936,6 @@ public boolean isVmInfoNeeded() {

@Override
public void provideVmInfo(long vmId, long volumeId) {

}

@Override
Expand Down
Loading
Loading