Drive IO stats #891

r-scheele · 2024-01-14T13:16:17Z

Implemented new metrics in publishDriveStats:

Online/Offline status of drives.
Total bytes read and written.
Read and write latency calculations.
Read and write throughput.
Drive wait time.
Similar to the existing implementation publishVolumeStats

Praveenrajmani

can you fix the lint errors? @r-scheele

pkg/metrics/collector.go

Praveenrajmani · 2024-01-18T12:17:18Z

pkg/metrics/collector.go

+ return &stats, nil
+}
+
+func getSectorSize(device string) int64 {


let this function return an (error, int64)

Praveenrajmani · 2024-01-18T12:21:42Z

pkg/metrics/collector.go

+func (c *metricsCollector) publishDriveStats(ctx context.Context, drive *types.Drive, ch chan<- prometheus.Metric) {
+ device, err := c.getDeviceByFSUUID(drive.Status.FSUUID)
+ if err != nil {
+ klog.ErrorS(


If the device is not found by its FSUUID, this means the drive is not READY. we should also export this information as a metric.

Praveenrajmani · 2024-01-18T12:23:19Z

pkg/metrics/collector.go

+ // Drive is offline
+ ch <- prometheus.MustNewConstMetric(
+ prometheus.NewDesc(
+ prometheus.BuildFQName(consts.AppName, "stats", "drive_status"),


can we have _drive_ready instead of _drive_status?

if these directories are not present, then it means the drive is "not ok" (or) "not ready". we can have a label name accordingly.

balamurugana

Please add respective functional tests

balamurugana · 2024-01-27T01:18:41Z

.gitignore

@@ -11,3 +11,4 @@ dist/
 kustomize
 operator-sdk
 kubectl-directpv_*
+directpv.tar


As none of our build tool creates this file, could you explain why do we need to ignore this file?

it was added to the .gitignore because it is generated during testing.
https://github.com/minio/directpv/blob/master/docs/developer.md the image tar ball was created by podman

Add a new line

balamurugana · 2024-01-27T01:21:14Z

pkg/metrics/collector.go

+ ReadTicks int64
+ WriteSectors int64
+ WriteTicks int64
+ TimeInQueue int64


Add a function in pkg/device/sysfs_linux.go like below

func GetStat(name string) (stats []uint64, err error) { line, err := readFirstLine("/sys/class/block/" + name + "/stat") if err != nil { return nil, err } for _, token := range strings.Split(line, " ") { token = strings.TrimSpace(token) ui64, err := strconv.ParseUint(token, 10, 64) if err != nil { return nil, err } stats = append(stats, ui64) } return stats, nil }

and use it here like

func getDriveStats(driveName string) (*driveStats, error) { stats, err := device.GetStat(driveName) if err != nil { return nil, err } if len(stats) == 0 { // The drive is lost return nil, nil } if len(stats) < 10 { return nil, fmt.Errorf("unknown stats from sysfs for drive %v", driveName) } // Refer https://www.kernel.org/doc/Documentation/block/stat.txt // for meaning of each field. return &driveStats{ ReadSectors: stats[2], ReadTicks: stats[3], WriteSectors: stats[6], WriteTicks: stats[7], TimeInQueue: stats[9], }, nil }

unexport/make private all the fields in driveStats struct

balamurugana · 2024-01-27T01:22:33Z

pkg/metrics/collector.go

+ "Drive Online/Offline Status",
+ []string{"drive"}, nil),
+ prometheus.GaugeValue,
+ 0, // 0 for offline


Do you mean boolean value here?

If I get you correctly, I'm using 0 to show offline and 1 to show online. As it seems more Boolean-like. Prometheus does not support a native Boolean data type.

What is the other way to do to indicate boolean state? If none, default constants for drive online/offline and user it here.

balamurugana · 2024-01-27T01:24:36Z

pkg/metrics/collector.go

+
+ return
+ }
+ device = strings.TrimPrefix(device, "/dev")


Use https://github.com/minio/directpv/blob/master/pkg/utils/utils.go#L96

balamurugana · 2024-01-27T01:32:38Z

pkg/metrics/collector.go

+ }
+ device = strings.TrimPrefix(device, "/dev")
+ // Online/Offline Status
+ if _, err := os.Stat("/sys/block" + device); os.IsNotExist(err) {


As mentioned previously, this won't work for disk partition used as DirectPVDrive. You have to use /sys/class/block instead of /sys/block for both disk and partition

Use path.Join() instead of +

sysfs is unreliable to check drive existent on the system i.e. in some systems sysfs entry is present but actual drive is gone from the system

balamurugana · 2024-01-27T01:37:08Z

pkg/metrics/collector.go

+ )
+ }
+
+ filePath := "/sys/block" + device + "/stat"


Use path.Join()

balamurugana · 2024-01-27T02:31:34Z

pkg/metrics/collector.go

+
+func getSectorSize(device string) (int64, error) {
+ // Construct the file path to the sector size file
+ filePath := "/sys/block/" + device + "/queue/hw_sector_size"


This file doesn't exist for disk partitions. For partitions, find it is disk and fetch sector size from it and use it.

if /sys/class/block/<device>/queue/hw_sector_size is missing for disk, use 512 as default block size.

Move this function as GetHardwareSectorSize to pkg/device/sysfs_linux.go

I found a defaultBlockSize as 512 in the sysfs_linux.go, Since it's the same value, should I go ahead and use it? but I'm thinking block size and sector size aren't the same thing, I could create a new constant just for sector size.

Most of the system have default block/sector size as 512. you could define default sector size as default block size.

balamurugana · 2024-01-27T02:32:52Z

pkg/metrics/collector.go

 if result.Err != nil {
- return
+ continue


Why continue? do you mean break?

balamurugana · 2024-01-27T02:34:40Z

pkg/metrics/collector.go

+ List(ctx)
+ for result := range driveResultCh {
+ if result.Err != nil {
+ continue


Why continue? do you mean break?

I used continue to ensure we process all drives, even if an error occurs on one of them.

Once the error is thrown, we cannot get values further. break is logically correct thing to do.

r-scheele · 2024-01-27T08:03:25Z

Please add respective functional tests

Yes i will, I'm going through the previous one written already.

Praveenrajmani · 2024-06-04T04:20:05Z

PTAL @r-scheele

r-scheele · 2024-06-04T09:29:50Z

PTAL @r-scheele

Hi @Praveenrajmani yes, i'm working on this review now

r-scheele added 3 commits January 13, 2024 14:01

Add .sock and directpv.tar to ignore files

8c885e9

Add drive statistics collection to metrics collector

b291206

Update metricsCollector to use GaugeValue for drive stats

e77ef89

Praveenrajmani reviewed Jan 18, 2024

View reviewed changes

gofumpt formatting

2019998

harshavardhana requested a review from balamurugana January 27, 2024 00:01

r-scheele added 2 commits January 27, 2024 01:04

Add error handling and drive ready status metric to publishDriveStats

47ecfc0

Fixed linter errors

d410678

balamurugana requested changes Jan 27, 2024

View reviewed changes

Merge branch 'master' into drive-IOStats

dc10916

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drive IO stats #891

Drive IO stats #891

r-scheele commented Jan 14, 2024

Praveenrajmani left a comment

Praveenrajmani Jan 18, 2024

Praveenrajmani Jan 18, 2024

Praveenrajmani Jan 18, 2024

balamurugana left a comment

balamurugana Jan 27, 2024

r-scheele Jan 27, 2024

balamurugana Jan 27, 2024

balamurugana Jan 27, 2024

balamurugana Jan 27, 2024

r-scheele Jan 27, 2024

balamurugana Jan 27, 2024

balamurugana Jan 27, 2024

balamurugana Jan 27, 2024

balamurugana Jan 27, 2024

balamurugana Jan 27, 2024

r-scheele Jan 27, 2024

balamurugana Jan 27, 2024

balamurugana Jan 27, 2024

balamurugana Jan 27, 2024

r-scheele Jan 27, 2024

balamurugana Jan 27, 2024

r-scheele commented Jan 27, 2024

Praveenrajmani commented Jun 4, 2024

r-scheele commented Jun 4, 2024

Drive IO stats #891

Are you sure you want to change the base?

Drive IO stats #891

Conversation

r-scheele commented Jan 14, 2024

Praveenrajmani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

balamurugana left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r-scheele commented Jan 27, 2024

Praveenrajmani commented Jun 4, 2024

r-scheele commented Jun 4, 2024