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

^^ @r-scheele

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 · 2024-08-20T16:05:40Z

Hi @balamurugana @Praveenrajmani PTAL?

Praveenrajmani

Initial review

Praveenrajmani · 2024-08-21T05:11:09Z

pkg/admin/installer/directpv.min.io_directpvdrives.yaml

@@ -3,7 +3,7 @@ apiVersion: apiextensions.k8s.io/v1
 kind: CustomResourceDefinition
 metadata:
  annotations:
-    controller-gen.kubebuilder.io/version: v0.15.0
+    controller-gen.kubebuilder.io/version: v0.14.0


please update your controller-gen version and run build.sh again

Okay, i did that already.

Praveenrajmani · 2024-08-21T05:11:40Z

pkg/installer/directpv.min.io_directpvvolumes.yaml

@@ -0,0 +1,149 @@
+---
+apiVersion: apiextensions.k8s.io/v1


this file looks unrelated, can you remove?

it was present in the file before https://github.com/minio/directpv/blob/master/pkg/admin/installer/directpv.min.io_directpvvolumes.yaml

these files are not present in the existing source. can you remove these?

cmd/kubectl-directpv/discover.go

…into drive-IOStats

Praveenrajmani · 2024-09-09T15:44:04Z

pkg/admin/installer/directpv.min.io_directpvdrives.yaml

@@ -3,7 +3,7 @@ apiVersion: apiextensions.k8s.io/v1
 kind: CustomResourceDefinition
 metadata:
  annotations:
-    controller-gen.kubebuilder.io/version: v0.15.0
+    controller-gen.kubebuilder.io/version: v0.16.1


The controller-gen version should be v0.15.0. You can remove $GOPATH/bin/controller-gen and then run ./build.sh - It will install the correct version @r-scheele

Praveenrajmani · 2024-09-09T15:50:15Z

pkg/device/sysfs_linux.go

+
+// GetStat retrieves and returns statistics for a given device name.
+func GetStat(name string) ([]uint64, int, error) {
+	filePath := path.Join("/sys/class/block/", name, "/stat")


/sys/class/block can be defined as a constant as it is referred in more than one place

also, use path.Join and keep this as a global var

Praveenrajmani · 2024-09-09T15:53:29Z

pkg/device/sysfs_linux.go

+func GetStat(name string) ([]uint64, int, error) {
+	filePath := path.Join("/sys/class/block/", name, "/stat")
+
+	driveStatus := 1


we actually don't need a variable here, the function can be simply return the StatInfo, err.

ie, func Stat(name string) (*StatInfo, error)

driveStatus is used to track the status of the drive, so it can also be exported as a metric. Should i still remove it?

yes, we can remove it.

err == nil implies the drive is online and err != nil implies drive is offline. we don't need the extra param to indicate this

@r-scheele ^^

Praveenrajmani · 2024-09-09T15:54:09Z

pkg/device/sysfs_linux.go

+		return nil, driveStatus, nil
+	}
+
+	klog.Infof("Reading drive statistics from: %s", filePath)


we can use verbose levels for loggers, we dont want these logs to be printed all the time when prometheus scrapes the metrics.

like, klog.V(4).Infof

Praveenrajmani · 2024-09-09T15:58:05Z

pkg/device/sysfs_linux.go

+
+		ui64, err := strconv.ParseUint(token, 10, 64)
+		if err != nil {
+			klog.Warningf("Failed to parse token '%s': %v", token, err)


shouldn't we return err here as the parsing fails

Praveenrajmani · 2024-09-09T17:25:57Z

pkg/device/sysfs_linux.go

+// GetHardwareSectorSize retrieves the hardware sector size for a given device.
+// It works for both whole disks and partitions.
+// The device name should be without the "/dev/" prefix (e.g., "sda" or "sda1").
+func GetHardwareSectorSize(deviceName string) (uint64, error) {


Suggested change

func GetHardwareSectorSize(deviceName string) (uint64, error) {

func HardwareSectorSize(deviceName string) (uint64, error) {

Praveenrajmani · 2024-09-09T17:26:19Z

pkg/device/sysfs_linux.go

+		return 0, fmt.Errorf("failed to parse hardware sector size for %s: %v", deviceName, err)
+	}
+
+	klog.Infof("Hardware sector size for %s: %d bytes", deviceName, sectorSize)


Same: Use verbose levels in logging

Praveenrajmani · 2024-09-09T17:27:29Z

sorry for the late review @r-scheele

r-scheele · 2024-09-11T12:32:50Z

sorry for the late review @r-scheele

No problem @Praveenrajmani, would look into it soon.

kannappanr · 2024-09-17T07:50:05Z

@balamurugana Can you PTAL

balamurugana · 2024-09-18T05:49:58Z

@r-scheele I sent a PR r-scheele#1 to your branch in order to avoid many review cycles. Please check and merge it accordingly.

Praveenrajmani · 2024-10-15T13:21:26Z

closing this as #955 PR got merged

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

Praveenrajmani mentioned this pull request Aug 5, 2024

Disk Usage Metrics for DirectPV Volumes #921

Closed

r-scheele added 6 commits August 13, 2024 17:33

update code for review

3176938

merged master to drive-IOStats

2eb0b11

update for code review

242396e

Adjust tests for initial review

76bc542

Go formatting

ef44fb2

Fix broken ci - linter/formatting

6e5dea8

Praveenrajmani reviewed Aug 21, 2024

View reviewed changes

r-scheele and others added 6 commits August 23, 2024 13:56

updated controller-gen to latest

6e015c2

Merge branch 'master' into drive-IOStats

5e59d3d

test drive stats

54f0dd0

Merge branch 'drive-IOStats' of https://github.com/r-scheele/directpv …

3a000d4

…into drive-IOStats

fixed linter error

7d53b56

Merge branch 'master' into drive-IOStats

ef4d5eb

Praveenrajmani reviewed Sep 9, 2024

View reviewed changes

Merge branch 'master' into drive-IOStats

7efd6f1

Merge branch 'master' into drive-IOStats

8463c16

Praveenrajmani closed this Oct 15, 2024

	func GetHardwareSectorSize(deviceName string) (uint64, error) {
	func HardwareSectorSize(deviceName string) (uint64, error) {

Drive IO stats #891

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

Choose a reason for hiding this comment

r-scheele commented Jan 27, 2024

Praveenrajmani commented Jun 4, 2024

r-scheele commented Jun 4, 2024

r-scheele commented Aug 20, 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

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

Praveenrajmani commented Sep 9, 2024

r-scheele commented Sep 11, 2024

kannappanr commented Sep 17, 2024

balamurugana commented Sep 18, 2024

Praveenrajmani commented Oct 15, 2024