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 7 commits into
base: main
Choose a base branch
from
+478 −46
Open

[CSTACKEX-328] QoS Support Part 2 #121

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
bdd9162
[CSTACKEX-328] Change Disk Offering, allow min as 0 during volume cre…
Oct 13, 2026
8f2175f
[CSTACKEX-328] Fix Issues
Oct 13, 2026
b12dc90
[CSTACKEX-328] Fix Build Issue
Oct 14, 2026
09bf05e
[CSTACKEX-328] Fix Review Comments
Oct 14, 2026
27e863e
[CSTACKEX-328] Fix Build Issue
Oct 14, 2026
8a24278
[CSTACKEX-328] Resolve review comments
Oct 15, 2026
90e193a
[CSTACKEX-328] Fix build failure and resolve new comments
Oct 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
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 @@ -129,8 +129,7 @@ public class OntapStorageConstants {
public static final String LUN_DOT_SERIAL_NUMBER = "lun.serial_number";
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.

15 changes: 14 additions & 1 deletion ...ume/ontap/src/test/java/org/apache/cloudstack/storage/service/UnifiedNASStrategyTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -330,10 +330,23 @@ public void testUpdateCloudStackVolume_AppliesQosPolicy() {

@Test
public void testUpdateCloudStackVolume_InvalidRequest_ThrowsException() {
assertThrows(CloudRuntimeException.class, () -> strategy.updateCloudStackVolume(new CloudStackVolume()));
CloudStackVolume missingFile = new CloudStackVolume();
missingFile.setVolumeInfo(mock(VolumeInfo.class));
missingFile.setFlexVolumeUuid("flex-uuid");

assertInvalidNfsVolumeUpdate(null);
assertInvalidNfsVolumeUpdate(new CloudStackVolume());
assertInvalidNfsVolumeUpdate(missingFile);

verify(nasFeignClient, never()).updateFile(anyString(), anyString(), anyString(), any(FileInfo.class));
}

private void assertInvalidNfsVolumeUpdate(CloudStackVolume request) {
CloudRuntimeException ex = assertThrows(CloudRuntimeException.class,
() -> strategy.updateCloudStackVolume(request));
assertEquals("Invalid NFS volume request", ex.getMessage());
}

// Test createCloudStackVolume - Volume Not Found
@Test
public void testCreateCloudStackVolume_VolumeNotFound() {
Expand Down
36 changes: 33 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,20 +1344,50 @@ 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() {
CloudStackVolume lunWithoutUuid = new CloudStackVolume();
lunWithoutUuid.setLun(new Lun());

assertInvalidIscsiVolumeUpdate(null);
assertInvalidIscsiVolumeUpdate(new CloudStackVolume());
assertInvalidIscsiVolumeUpdate(lunWithoutUuid);

verify(sanFeignClient, never()).updateLun(any(), any(), any());
}

private void assertInvalidIscsiVolumeUpdate(CloudStackVolume request) {
CloudRuntimeException ex = assertThrows(CloudRuntimeException.class,
() -> unifiedSANStrategy.updateCloudStackVolume(request));
assertEquals("Invalid iSCSI volume request", ex.getMessage());
}

@Test
void testUpdateAccessGroup_ReturnsNull() {
AccessGroup accessGroup = new AccessGroup();
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 @@ -2328,7 +2328,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 @@ -2372,7 +2372,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)) {

@sandeeplocharla sandeeplocharla Oct 16, 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.

Can you add this change in other DiskOffering forms also?

@suryag1201 suryag1201 Oct 16, 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.

bcz cloudstack does not support min as 0, so i have not included the disk offering and included the resize, create Volume screens only but this point is valid, we can check on this

return Promise.reject(this.$t('message.error.number'))
}
return Promise.resolve()
Expand Down
Loading