Skip to content

[CSTACKEX-328] QoS Support Part 2 #121

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

Open
suryag1201 wants to merge 6 commits into
base: main
Choose a base branch
from
Open

[CSTACKEX-328] QoS Support Part 2 #121

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
db102c6
[CSTACKEX-328] Change Disk Offering, allow min as 0 during volume cre…
Oct 13, 2026
96d97a5
[CSTACKEX-328] Fix Issues
Oct 13, 2026
83d07d8
[CSTACKEX-328] Fix Build Issue
Oct 14, 2026
6cd7718
[CSTACKEX-328] Fix Review Comments
Oct 14, 2026
d402fb6
[CSTACKEX-328] Fix Build Issue
Oct 14, 2026
4baa76f
[CSTACKEX-328] Resolve review comments
Oct 15, 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
159 changes: 128 additions & 31 deletions ...ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion .../volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedNASStrategy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ public CloudStackVolume createTemplateCache(StoragePoolVO storagePool, TemplateI
public CloudStackVolume updateCloudStackVolume(CloudStackVolume cloudstackVolume) {
if (cloudstackVolume == null || cloudstackVolume.getVolumeInfo() == null
|| cloudstackVolume.getFlexVolumeUuid() == null || cloudstackVolume.getFile() == null) {
throw new CloudRuntimeException("Invalid NFS volume QoS update request");
throw new CloudRuntimeException("Invalid NFS volume request");
}
FileInfo fileInfo = new FileInfo();
fileInfo.setQosPolicy(cloudstackVolume.getFile().getQosPolicy());
Expand Down
19 changes: 18 additions & 1 deletion .../volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedSANStrategy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,24 @@ private void bestEffortDeleteTemplateCacheLun(String svmName, String lunName, St

@Override
public CloudStackVolume updateCloudStackVolume(CloudStackVolume cloudstackVolume) {
return null;
if (cloudstackVolume == null || cloudstackVolume.getLun() == null
|| cloudstackVolume.getLun().getUuid() == null) {
throw new CloudRuntimeException("Invalid iSCSI volume request");
}
Lun lunUpdate = new Lun();
lunUpdate.setQosPolicy(cloudstackVolume.getLun().getQosPolicy());

@piyush5netapp piyush5netapp Oct 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think , qos update or other update should be a private mtethod and this method should be responsible to call ONTAP API for patch.

@suryag1201 suryag1201 Oct 15, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

For file case, we have to make diff call to attach a QoS but for lun, we can pass in the same request also as ONTAP accepts that.
otherwise it will be 2 diff call on for ISCSI, 1st of create/clone and 2nd for attach

try {
JobResponse response = sanFeignClient.updateLun(
getAuthHeader(), cloudstackVolume.getLun().getUuid(), lunUpdate);
pollJobIfPresent(response, "update QoS policy on LUN [" + cloudstackVolume.getLun().getUuid() + "]");
} catch (FeignException e) {
throw new CloudRuntimeException("Failed to apply QoS policy to LUN: " + e.getMessage(), e);
}
logger.info("Applied QoS policy [{}] to LUN [{}]",
cloudstackVolume.getLun().getQosPolicy() != null
? cloudstackVolume.getLun().getQosPolicy().getName() : null,
cloudstackVolume.getLun().getUuid());
return cloudstackVolume;
}

@Override
Expand Down
3 changes: 1 addition & 2 deletions ...volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageConstants.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,8 +112,7 @@ public class OntapStorageConstants {
public static final String LUN_DOT_UUID = "lun.uuid";
public static final String QOS_POLICY_UUID = "qosPolicyUuid";
public static final String IS_AFF = "isAFF";
public static final String MIN_IOPS_NOT_SUPPORTED_ON_FAS =
"Minimum IOPS is not supported on FAS/non-AFF ONTAP platforms; only maximum IOPS is supported";
public static final String MIN_IOPS_NOT_SUPPORTED_ON_FAS = "Minimum IOPS must be 0 for FAS. Only a Maximum IOPS limit is supported";
public static final String QOS_POLICY_NONE = "none";
public static final String QOS_POLICY_NAME_PREFIX = "cs_";
public static final String QOS_POLICY_NAME_TO = "to_";
Expand Down
280 changes: 278 additions & 2 deletions ...p/src/test/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriverTest.java
View file
Open in desktop

Large diffs are not rendered by default.

26 changes: 23 additions & 3 deletions ...ume/ontap/src/test/java/org/apache/cloudstack/storage/service/UnifiedSANStrategyTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1344,17 +1344,37 @@ void testSetOntapStorage() {
}

@Test
void testUpdateCloudStackVolume_ReturnsNull() {
void testUpdateCloudStackVolume_AppliesQosPolicy() {
Lun lun = new Lun();
lun.setUuid("lun-uuid-123");
VolumeQosPolicy qosPolicy = new VolumeQosPolicy();
qosPolicy.setName("cs_0_to_5000_iops_svm1");
qosPolicy.setUuid("qos-uuid");
lun.setQosPolicy(qosPolicy);
CloudStackVolume request = new CloudStackVolume();
request.setLun(lun);

assertNull(unifiedSANStrategy.updateCloudStackVolume(request));
assertNull(unifiedSANStrategy.updateCloudStackVolume(new CloudStackVolume()));
try (MockedStatic<OntapStorageUtils> utilityMock = mockStatic(OntapStorageUtils.class)) {
utilityMock.when(() -> OntapStorageUtils.generateAuthHeader("admin", "password"))
.thenReturn(authHeader);
when(sanFeignClient.updateLun(eq(authHeader), eq("lun-uuid-123"), any(Lun.class)))
.thenReturn(null);

CloudStackVolume result = unifiedSANStrategy.updateCloudStackVolume(request);

assertSame(request, result);
ArgumentCaptor<Lun> lunCaptor = ArgumentCaptor.forClass(Lun.class);
verify(sanFeignClient).updateLun(eq(authHeader), eq("lun-uuid-123"), lunCaptor.capture());
assertEquals("cs_0_to_5000_iops_svm1", lunCaptor.getValue().getQosPolicy().getName());
assertEquals("qos-uuid", lunCaptor.getValue().getQosPolicy().getUuid());
}
}

@Test
void testUpdateCloudStackVolume_InvalidRequest_ThrowsException() {
CloudRuntimeException ex = assertThrows(CloudRuntimeException.class,
() -> unifiedSANStrategy.updateCloudStackVolume(new CloudStackVolume()));
assertTrue(ex.getMessage().contains("Invalid iSCSI volume QoS update request"));
verify(sanFeignClient, never()).updateLun(any(), any(), any());
}

Expand Down
6 changes: 3 additions & 3 deletions server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -884,7 +884,7 @@ public VolumeVO allocVolume(long ownerId, Long zoneId, Long diskOfferingId, Long
minIops = 0L;
maxIops = 0L;
} else {
if (minIops == null || minIops <= 0) {
if (minIops == null || minIops < 0) {

@sandeeplocharla sandeeplocharla Oct 14, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why do we want to allow 0 for minIops?

@suryag1201 suryag1201 Oct 14, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

For FAS, we need to set 0 as a min IOPS
but UI is allowing this during VM create time but not volume create, resize and disk offering change time.

throw new InvalidParameterValueException("The min IOPS must be greater than 0.");
}

Expand Down Expand Up @@ -2327,7 +2327,7 @@ public Volume changeDiskOfferingForVolumeInternal(Long volumeId, Long newDiskOff
if (volumeMigrateRequired) {
logger.warn(String.format("Volume change offering operation succeeded for volume ID: %s but volume resize operation failed, so please try resize volume operation separately", volume.getUuid()));
} else {
throw new CloudRuntimeException(String.format("Volume disk offering change operation failed for volume ID [%s] because the volume resize operation failed.", volume.getUuid()));
throw new CloudRuntimeException(String.format("Volume disk offering change operation failed for volume ID [%s] because the volume resize operation failed: %s", volume.getUuid(), e.getMessage()));
}
}
}
Expand Down Expand Up @@ -2371,7 +2371,7 @@ public Volume changeDiskOfferingForVolumeInternal(Long volumeId, Long newDiskOff
if (volumeMigrateRequired) {
logger.warn(String.format("Volume change offering operation succeeded for volume ID: %s but volume resize operation failed, so please try resize volume operation separately", volume.getUuid()));
} else {
throw new CloudRuntimeException(String.format("Volume change offering operation failed for volume ID: %s due to resize volume operation failed", volume.getUuid()));
throw new CloudRuntimeException(String.format("Volume change offering operation failed for volume ID: %s due to resize volume operation failed: %s", volume.getUuid(), e.getMessage()), e);
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion ui/src/views/storage/ChangeOfferingForVolume.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ export default {
miniops: [{
type: 'number',
validator: async (rule, value) => {
if (value && (isNaN(value) || value <= 0)) {
if (value && (isNaN(value) || value < 0)) {
return Promise.reject(this.$t('message.error.number'))
}
return Promise.resolve()
Expand Down
2 changes: 1 addition & 1 deletion ui/src/views/storage/CreateVolume.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -311,7 +311,7 @@ export default {
size: [{ required: true, message: this.$t('message.error.custom.disk.size') }],
miniops: [{
validator: async (rule, value) => {
if (value && (isNaN(value) || value <= 0)) {
if (value && (isNaN(value) || value < 0)) {
return Promise.reject(this.$t('message.error.number'))
}
return Promise.resolve()
Expand Down
Loading