Skip to content

QoS Support (Capacity IOPS on the pool, Create Offering with QoS and Resize Changes for IOPS) - #102

Open
suryag1201 wants to merge 17 commits into
mainfrom
feature/CSTACKEX-279
Open

suryag1201 wants to merge 17 commits into
mainfrom
feature/CSTACKEX-279

Conversation

@suryag1201

@suryag1201 suryag1201 commented Sep 15, 2026

Copy link
Copy Markdown

Description

QoS Support includes:
1- Capacity IOPS on the pool,
2- Create Disk/Compute offering with QoS
3- Resize changes for QoS

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)
  • Build/CI
  • Test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

  • Major
  • Minor

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

Screenshots (if appropriate):

How Has This Been Tested?

Performed the below testing for ISCSI and NFS both
1- Created the compute/disk offering with CustomIOPS and Fix IOPS both (min 2000, max 10000)
2- Created a Storage Pool with Total IOPS 5000 - It will just store the value in DB
3- Create a VM with Fix IOPS (ROOT + 1 DATA) - Created Lun/File, QoS policy got created and attached to file/lun
4- Create a VM with Custom IOPS (min 2000, max 5000) (ROOT + 1 DATA) - Created Lun/File, new QoS policy got created and attached to file/lun
5- Create a VM with Fix IOPS (ROOT + 1 DATA) again - Failed with error saying there is no more IOPS on a pool
6- Change the Total IOPS on pool - worked and new VM creation also went fine.
7- Resize the Volume, QoS existing field got populated and changed the min and max value - that also worked by creating new QoS if not found and attached to the file/Lun
8- Tested on FAS platform where Min QoS is set 1000 and it throw the error in logs, not on UI as UI is not using the thrown message from vendor
9- Tested on FAS platform where Min QoS is set 0 and VM created successfully

How did you try to break this feature and the system with this change?

@github-actions

Copy link
Copy Markdown

🔴 Test Coverage Grade: D — Marginal

Metric Value
Line coverage 24.78%
Branch coverage 18.92%

Grade Scale

Grade Line Coverage Meaning
🟢 A ≥ 80% Excellent - this code sleeps well at night 😴
🟡 B 60-79% Good - almost there, don't stop now 😉
🟠 C 40-59% Acceptable - your code is wearing a seatbelt, but no airbags 😬
🔴 D 20-39% Marginal - boldly shipping where no test has gone before 🖖
⛔ F < 20% Failing - tests? what tests? 🔥

Branch coverage is shown as a secondary signal. Grade is determined by line coverage.
View full Actions run

@github-actions

Copy link
Copy Markdown

🔴 Test Coverage Grade: D — Marginal

Metric Value
Line coverage 24.80%
Branch coverage 18.94%

Grade Scale

Grade Line Coverage Meaning
🟢 A ≥ 80% Excellent - this code sleeps well at night 😴
🟡 B 60-79% Good - almost there, don't stop now 😉
🟠 C 40-59% Acceptable - your code is wearing a seatbelt, but no airbags 😬
🔴 D 20-39% Marginal - boldly shipping where no test has gone before 🖖
⛔ F < 20% Failing - tests? what tests? 🔥

Branch coverage is shown as a secondary signal. Grade is determined by line coverage.
View full Actions run

@github-actions

Copy link
Copy Markdown

🔴 Test Coverage Grade: D — Marginal

Metric Value
Line coverage 24.79%
Branch coverage 18.93%

Grade Scale

Grade Line Coverage Meaning
🟢 A ≥ 80% Excellent - this code sleeps well at night 😴
🟡 B 60-79% Good - almost there, don't stop now 😉
🟠 C 40-59% Acceptable - your code is wearing a seatbelt, but no airbags 😬
🔴 D 20-39% Marginal - boldly shipping where no test has gone before 🖖
⛔ F < 20% Failing - tests? what tests? 🔥

Branch coverage is shown as a secondary signal. Grade is determined by line coverage.
View full Actions run


CloudStackVolume request = iscsi
? createCloneLunRequest(storagePool, details, volumeInfo, templatePoolRef, templateId)
? createCloneLunRequest(storagePool, details, volumeInfo, templatePoolRef, templateId, qosPolicy)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

can you help reminding me the reason for keeping this implementation like this instead strategy specific implementation ?

@github-actions

Copy link
Copy Markdown

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

if (userVmDetails != null) {
String minIops = userVmDetails.get(MIN_IOPS);
String maxIops = userVmDetails.get(MAX_IOPS);
String minIops = userVmDetails.get(VmDetailConstants.MIN_IOPS);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

can you check for setters also for iops specific statements?

return;
}
VolumeQosPolicy policy = getVolumeQosPolicyByUuid(policyUuid);
if (policy.getObjectCount() != null && policy.getObjectCount() > 0) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

what if policy is retuned as null? it can be potentials candidate for NPE.


@Override
public void resize(DataObject data, AsyncCompletionCallback<CreateCmdResult> callback) {}
public void resize(DataObject data, AsyncCompletionCallback<CreateCmdResult> callback) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Lets add todo for actual size specific impl until satvika's changes are not in

@RequestLine("PATCH /api/storage/luns/{uuid}")
@Headers({"Authorization: {authHeader}", "Content-Type: application/json"})
void updateLun(@Param("authHeader") String authHeader, @Param("uuid") String uuid, Lun lun);
JobResponse updateLun(@Param("authHeader") String authHeader, @Param("uuid") String uuid, Lun lun);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

can you watch for usage of this method, have you updated all of them for this signature change ?

"Unable to determine whether the ONTAP cluster is AFF or FAS");
}
for (ClusterNode node : response.getRecords()) {
if (node == null || Boolean.FALSE.equals(node.getAllFlashOptimized())) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

null for node also treat this as non-AFF, this is not intended behaviour, right ?

fetchData () {
this.loading = true
if (this.resource.size != null) {
this.form.size = Math.round(this.resource.size / (1024 * 1024 * 1024))

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

math.round is not required as we discussed

this.form.diskofferingid = this.offerings[0].id || ''
this.customDiskOffering = this.offerings[0].iscustomized || false
this.customDiskOfferingIops = this.offerings[0].iscustomizediops || false
const currentOffering = (json.listdiskofferingsresponse.diskoffering || [])[0]

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

how can we ensure it will not regress any existing workflow?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants