Repository navigation
Conversation
🔴 Test Coverage Grade:
|
| Metric | Value |
|---|---|
| Line coverage | 24.82% |
| Branch coverage | 18.97% |
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
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
| if (sizeInBytes <= 0) { | ||
| throw new CloudRuntimeException("Failed to resize Lun, invalid size " + sizeInBytes); | ||
| } | ||
| String authHeader = OntapStorageUtils.generateAuthHeader(storage.getUsername(), storage.getPassword()); |
There was a problem hiding this comment.
instead of adding the retry logic here, during lun clone return the job response and poll for that job to get complete and perform the resize later
62c8ec7 to
182b07b
Compare
🔴 Test Coverage Grade:
|
| Metric | Value |
|---|---|
| Line coverage | 25.83% |
| Branch coverage | 20.12% |
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
| } | ||
| } | ||
| } | ||
| throw new CloudRuntimeException("Cloned Lun uuid was not visible for: " + lunUuid); |
There was a problem hiding this comment.
If lun uuid itself is not visible, what's the point of logging lunUuid? 😆
There was a problem hiding this comment.
Yes, there is no point. I have changed it.
| String authHeader = OntapStorageUtils.generateAuthHeader(storage.getUsername(), storage.getPassword()); | ||
| OntapResponse<Lun> clonedLun = sanFeignClient.createLun(authHeader, true, lunRequest); | ||
| if (clonedLun == null || CollectionUtils.isEmpty(clonedLun.getRecords())) { | ||
| OntapResponse<Lun> created = sanFeignClient.createLun(authHeader, true, lunRequest); |
There was a problem hiding this comment.
change the variable name to clonedLun
| OntapResponse<Lun> clonedLun = sanFeignClient.createLun(authHeader, true, lunRequest); | ||
| if (clonedLun == null || CollectionUtils.isEmpty(clonedLun.getRecords())) { | ||
| OntapResponse<Lun> created = sanFeignClient.createLun(authHeader, true, lunRequest); | ||
| if (created == null || created.getRecords() == null || created.getRecords().isEmpty() |
There was a problem hiding this comment.
use collectionUtils/Stringutils for null or empty check
| } | ||
| Lun lun = clonedLun.getRecords().get(0); | ||
| String lunUuid = created.getRecords().get(0).getUuid(); | ||
| Lun lun = getLunByUuidUntilVisible(authHeader, lunUuid, lunRequest.getName()); |
There was a problem hiding this comment.
what if the get lun call failed, cloned lun will. still exists and retry will see the duplicate lun
There was a problem hiding this comment.
Added it.The cloned LUN is deleted ,so a subsequent CloudStack retry does not encounter a duplicate LUN name.
| for (int attempt = 1; attempt <= OntapStorageConstants.ONTAP_LUN_CLONE_GET_MAX_RETRIES; attempt++) { | ||
| try { | ||
| Lun lun = sanFeignClient.getLunByUUID(authHeader, lunUuid); | ||
| if (lun != null) { |
There was a problem hiding this comment.
we can not decode based on lun != null condition that lun is visible.
return the get response record instead
There was a problem hiding this comment.
Changed it.The code now uses the collection GET with the LUN UUID, verifies that the response contains a record, and returns that record instead of relying on lun != null.
| public static final int ONTAP_FLEXVOL_JOB_POLL_INTERVAL_MS = 2000; | ||
| public static final int ONTAP_FLEXVOL_RESOLVE_MAX_RETRIES = 30; | ||
| /** GET-by-UUID retries after LUN clone response. */ | ||
| public static final int ONTAP_LUN_CLONE_GET_MAX_RETRIES = 5; |
There was a problem hiding this comment.
this interval and no of retries are less for clone
Description
After an iSCSI template clone, CloudStack grows the new LUN when the service offering disk is larger than the template. ONTAP returns the LUN UUID from the clone, but the immediate resize PATCH /api/storage/luns/{uuid} can return HTTP 404 (entry doesn't exist, target uuid) before that LUN is visible.
resizeCloudStackVolume now retries only that PATCH, using the same UUID. A 404 is retried for up to 30 attempts, one second apart. Any other error fails on the first attempt.
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
Before the fix:

How Has This Been Tested?
Live concurrent deployment:
-Created an ONTAP iSCSI primary storage pool.
-Deployed ten CentOS VMs concurrently using the ISCSI compute offering.
-Nine deployments reached LUN cloning; one failed earlier due to insufficient host capacity.
-Nine clone POST requests returned LUN UUIDs.
-Five cloned LUNs were visible on the first GET.
-Four cloned LUNs returned 404 on the first GET.
-For all four, CloudStack waited and retried; the LUN became visible on attempt 2/5.
-CloudStack then resized each LUN to 10 GiB using PATCH and continued VM deployment successfully.
-No clone failed with Cloned Lun uuid was not visible.
How did you try to break this feature and the system with this change?