Repository navigation
[CSTACKEX-339] ASUP Upstream review comments - #117
suryag1201 wants to merge 2 commits into
Conversation
🔴 Test Coverage Grade:
|
| Metric | Value |
|---|---|
| Line coverage | 24.82% |
| Branch coverage | 18.98% |
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
| logger.debug("ONTAP ASUP: pushing telemetry for {} pool(s) [CloudStack version={}]", | ||
| pools.size(), getCloudStackVersion()); | ||
| Map<String, AsupClusterClient> clientsByStorageIp = new HashMap<>(); | ||
| for (StoragePoolVO pool : pools) { |
There was a problem hiding this comment.
Would 5 secs wait timeout be enough? This loop itself could take a while if there are more no.of pools. If there's no retry mechanism, can you see if it's possible to add it?
There was a problem hiding this comment.
The loop duration and the 5-second timeout are different things. 5 seconds is how long to wait to get the lock.
In a deployment with more than one management server, each server has its own ASUP timer. The lock makes sure only one of them pushes telemetry at a time. lock(5) waits up to 5 seconds to acquire MySQL GET_LOCK. If another server already holds it, this server logs that and skips the cycle. If this server gets the lock, it keeps it for the whole pool loop and releases it only in unlock() after the last pool.
There was a problem hiding this comment.
Understood that. Was thinking of a scenario where the asup interval > time to push asup. Considering the fact that we push the entire data all the time, was thinking of a scenario where the loop runs long due to huge no.of pools and a sql db with high latency (as a loaded DB is highly likely to experience latency).
🔴 Test Coverage Grade:
|
| Metric | Value |
|---|---|
| Line coverage | 24.84% |
| Branch coverage | 19.01% |
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
2 participants
Description
1- This allocates an intermediate list and then traverses it three more times. Since this runs once per pool per interval, on very large pools this adds avoidable CPU/GC overhead. Consider computing rootDiskCount, dataDiskCount, and totalLogicalSizeBytes in a single pass over volumes (filtering by CS_VOLUME_STATES inline) to reduce allocations and repeated streaming.
2- This treats a missing/unknown is_all_flash_optimized (null) as false, which classifies the node as fas and can incorrectly set cluster platformType when ONTAP doesn’t return these fields (permissions/older versions/partial responses). Consider returning null (unknown) when the required flags are absent and only adding non-null classifications to the rollup set so ASUP doesn’t misreport hardware type.
3- The DAO method is documented as “non-destroyed” volumes, but the SQL currently includes volumes in transitional/pre-provisioning states (e.g., Allocated) because it only excludes Destroy/Expunged. That can incorrectly flip multiPrimaryStoragePoolVm=true for VMs whose data disk hasn’t actually been created/moved yet. Tighten the state predicates to align with the intended “physically exists” semantics (e.g., include only the same stable states you count elsewhere, or explicitly exclude Allocated/upload-related states).
4- lock.unlock() is executed even when lock.lock(...) returns false (early return), which can attempt to unlock a lock this thread never acquired. Additionally, GlobalLock.getInternLock(...) typically requires releasing the reference (e.g., releaseRef()) to avoid leaking the interned lock object. Track a boolean locked and only unlock() when true, and ensure the lock reference is released in finally (even when not acquired).
fixed the above issues
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?