weizhouapache commented on PR #14454: URL: https://github.com/apache/cloudstack/pull/14454#issuecomment-6315426156
I tested this on KVM with the agent patched on one host, using a VM that has data disks on every storage type I could set up. This was on a 4.23.0 build with the same change; I have not run it on a 4.22 build. ## Cause The disk stats of a volume are matched on `getDiskPathFromDiskDef()`, which returned the 4th element of the path. That is the volume path for `/mnt/<pool>/<uuid>` (NFS, SharedMountPoint) and for RBD, but for local storage (`/var/lib/libvirt/images/<uuid>`) it is `libvirt`. The volume is never found, so its counters stay at 0. Also, in `VmDiskStatsTask` the first disk that does not match a volume ends the `for` loop (`break`), so the other disks of the VM can be skipped too. Now the last element of the path is used (same value as before for the other pool types), and an unmatched disk is skipped (`continue`). The second change, in `VolumeStatsTask`, only makes the "stats not implemented for this format" warning appear for every such volume and not only for the first. ## Test: before and after the fix One VM on one KVM host, a root disk plus these data disks, 600 s of a fixed-rate fio load (about 140 read + 60 write requests/s) on each data disk, `vm.disk.stats.interval` = 300. Values are `listVolumesMetrics` after the run, compared with `virsh domblkstat` on the host. | Volume | Storage | `diskioread` | `diskiowrite` | "Disk IOPS" | libvirt `rd_req + wr_req` | Without the fix | |---|---|---|---|---|---|---| | nfsdisk | NFS | 84,129 | 36,000 | 120,129 | 120,129 | same | | cephdisk | RBD | 84,128 | 36,000 | 120,128 | 120,128 | same | | localdisk | local | 168,214 | 72,000 | 240,214 | 240,214 | **0 / 0 / 0** | (The local volume was collected after the fix, so its totals include the second run of the same load. Before the fix, it stayed at 0 with libvirt showing the same load on it as on the NFS and RBD disks.) ## Test: different amounts on each disk, 5 data disks Then I wrote a different number of 4 KiB writes to each disk (fio, direct I/O) and compared the values 5 minutes later: | Volume | Storage | Writes added | `diskiowrite` | `diskkbswrite` | libvirt `wr_req` | Match | |---|---|---|---|---|---|---| | nfsdisk | NFS | 4,000 | 86,240 | 344,960 | 86,240 | yes | | cephdisk | RBD | 8,000 | 90,240 | 360,960 | 90,240 | yes | | localdisk | local | 12,000 | 94,240 | 376,960 | 94,240 | yes | | smpdisk1 | SharedMountPoint (NFS share) | 16,000 | 62,240 | 248,960 | 62,240 | yes | | smpdisk2 | SharedMountPoint (NFS share) | 20,000 | 66,240 | 264,960 | 66,240 | yes | `diskioread`, `diskkbsread`, `diskiopstotal` and `diskkbswrite` also match libvirt on all five disks (`diskkbswrite` = writes × 4 KiB). ## Unit tests - `LibvirtComputingResourceTest`: 316 tests pass, including the new `testGetDiskPathFromDiskDefForLocalStorage` (`/var/lib/libvirt/images/filepath` → `filepath`); the existing NFS, RBD and unsupported-path tests still pass. - `StatsCollectorTest` passes. ## Not tested - Linstor, StorPool and CLVM disks: I did not check the path format of their libvirt disks. - A root disk on local storage, and the case where an unmatched disk comes first in the list. - VMware and XenServer (the change only touches the KVM path parsing and the loop control). Related to #14302 (separate from the "Disk IOPS" display issue discussed there). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
