Repository: threefoldtech/zos
Pull request #1626 (2022-03-15 12:46:18)

Use seektime again to find device type
Fixes https://github.com/threefoldtech/zos/issues/1601


Patch:
diff --git a/pkg/storage/filesystem/btrfs.go b/pkg/storage/filesystem/btrfs.go
index a457ffa2..18bb8fb5 100644
--- a/pkg/storage/filesystem/btrfs.go
+++ b/pkg/storage/filesystem/btrfs.go
@@ -11,8 +11,6 @@ import (
 	"github.com/google/uuid"
 	"github.com/pkg/errors"
 	"github.com/rs/zerolog/log"
-
-	"github.com/threefoldtech/zos/pkg"
 )
 
 var (
@@ -253,12 +251,6 @@ func (p *btrfsPool) Usage() (usage Usage, err error) {
 	return Usage{Size: p.device.Size, Used: du.Data.Used}, nil
 }
 
-// Type of the physical storage used for this pool
-func (p *btrfsPool) Type() pkg.DeviceType {
-	// We only create heterogenous pools for now
-	return p.device.Type()
-}
-
 // Reserved is reserved size of the devices in bytes
 func (p *btrfsPool) Reserved() (uint64, error) {
 
diff --git a/pkg/storage/filesystem/device.go b/pkg/storage/filesystem/device.go
index 4811fbbd..0f47ec98 100644
--- a/pkg/storage/filesystem/device.go
+++ b/pkg/storage/filesystem/device.go
@@ -4,14 +4,12 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
-	"os/exec"
 	"path/filepath"
 	"regexp"
-	"syscall"
-	"time"
+	"strings"
 
 	"github.com/pkg/errors"
-	log "github.com/rs/zerolog/log"
+	"github.com/rs/zerolog/log"
 	"github.com/threefoldtech/zos/pkg/gridtypes/zos"
 )
 
@@ -25,6 +23,8 @@ type DeviceManager interface {
 	ByLabel(ctx context.Context, label string) (Devices, error)
 	// Mountpoint returns mount point of a device
 	Mountpoint(ctx context.Context, device string) (string, error)
+	// Seektime checks device seektime
+	Seektime(ctx context.Context, device string) (zos.DeviceType, error)
 }
 
 // Devices represents a list of cached in memory devices
@@ -57,7 +57,6 @@ type DeviceInfo struct {
 	Filesystem FSType `json:"fstype"`
 	Rota       bool   `json:"rota"`
 	Subsystems string `json:"subsystems"`
-	Readtime   uint64 `json:"-"`
 }
 
 func (i *DeviceInfo) Name() string {
@@ -69,12 +68,8 @@ func (i *DeviceInfo) Used() bool {
 	return len(i.Label) != 0 || len(i.Filesystem) != 0
 }
 
-func (d *DeviceInfo) Type() zos.DeviceType {
-	if d.Rota {
-		return zos.HDDDevice
-	}
-
-	return zos.SSDDevice
+func (d *DeviceInfo) Type() (zos.DeviceType, error) {
+	return d.mgr.Seektime(context.Background(), d.Path)
 }
 
 func (d *DeviceInfo) Mountpoint(ctx context.Context) (string, error) {
@@ -104,6 +99,25 @@ func defaultDeviceManager(exec executer) DeviceManager {
 	return m
 }
 
+// Devices gets available block devices
+func (l *lsblkDeviceManager) Seektime(ctx context.Context, device string) (zos.DeviceType, error) {
+	log.Debug().Str("device", device).Msg("checking seektim for device")
+	out, err := l.executer.run(ctx, "seektime", "-j", device)
+	if err != nil {
+		return "", errors.Wrap(err, "failed to check device seektime")
+	}
+
+	var result struct {
+		Type string `json:"type"`
+	}
+
+	if err := json.Unmarshal(out, &result); err != nil {
+		return "", errors.Wrap(err, "failed to load seektime data")
+	}
+
+	return zos.DeviceType(strings.ToLower(result.Type)), nil
+}
+
 // Devices gets available block devices
 func (l *lsblkDeviceManager) Devices(ctx context.Context) (Devices, error) {
 	return l.scan(ctx)
@@ -239,67 +253,6 @@ func (l *lsblkDeviceManager) scan(ctx context.Context) ([]DeviceInfo, error) {
 		return nil, errors.Wrap(err, "failed to scan devices")
 	}
 
-	if err := l.setDeviceReadTimes(devs); err != nil {
-		return nil, err
-	}
-
 	l.cache = devs
 	return l.cache, nil
 }
-
-func isTimeout(err error) bool {
-	if err == nil {
-		return false
-	}
-	var exitErr *exec.ExitError
-	if errors.As(err, &exitErr) {
-		if status, ok := exitErr.ProcessState.Sys().(syscall.WaitStatus); ok {
-			return status.Signaled() && status.Signal() == syscall.SIGKILL
-		}
-	}
-
-	return false
-}
-
-// seektime uses the seektime binary to try and determine the type of a disk
-// This function returns the type of the device, as reported by seektime,
-// and the elapsed time in microseconds (also reported by seektime)
-func (l *lsblkDeviceManager) seektime(ctx context.Context, path string) (string, uint64, error) {
-	ctx, cancel := context.WithTimeout(ctx, time.Second*5)
-	defer cancel()
-
-	bytes, err := l.run(ctx, "seektime", "-j", path)
-	if isTimeout(err) {
-		// the seektime is taking too long that's defintely a HDD
-		return "HDD", 5 * 60, nil
-	} else if err != nil {
-		return "", 0, err
-	}
-
-	var seekTime struct {
-		Typ  string `json:"type"`
-		Time uint64 `json:"elapsed"`
-	}
-
-	err = json.Unmarshal(bytes, &seekTime)
-	log.Debug().Str("disk", path).Str("type", seekTime.Typ).Uint64("time", seekTime.Time).Msg("seektime")
-
-	return seekTime.Typ, seekTime.Time, err
-}
-
-func (l *lsblkDeviceManager) setDeviceReadTimes(devices []DeviceInfo) error {
-	for idx := range devices {
-		d := &devices[idx]
-
-		_, rt, err := l.seektime(context.Background(), d.Path)
-		if err != nil {
-			// don't include errored devices in the result
-			log.Error().Err(err).Msgf("failed to get disk read time")
-			return err
-		}
-
-		d.Readtime = rt
-	}
-
-	return nil
-}
diff --git a/pkg/storage/filesystem/filesystem.go b/pkg/storage/filesystem/filesystem.go
index 72944b4c..6e899292 100644
--- a/pkg/storage/filesystem/filesystem.go
+++ b/pkg/storage/filesystem/filesystem.go
@@ -4,7 +4,6 @@ import (
 	"context"
 
 	"github.com/pkg/errors"
-	"github.com/threefoldtech/zos/pkg"
 )
 
 // Usage struct (in bytes)
@@ -39,10 +38,6 @@ type Pool interface {
 	Mount() (string, error)
 	// UnMount the pool
 	UnMount() error
-	//AddDevice to the pool
-	// RemoveDevice from the pool
-	// Type of the physical storage in this pool
-	Type() pkg.DeviceType
 	// Reserved is reserved size of the devices in bytes
 	Reserved() (uint64, error)
 	// Volumes are all subvolumes of this volume
diff --git a/pkg/storage/storage.go b/pkg/storage/storage.go
index 44de1562..72b06773 100644
--- a/pkg/storage/storage.go
+++ b/pkg/storage/storage.go
@@ -165,7 +165,12 @@ func (s *Module) initialize() error {
 			log.Error().Err(err).Str("pool", pool.Name()).Str("device", device.Path).Msg("failed to get usage of pool")
 		}
 
-		typ := device.Type()
+		typ, err := device.Type()
+		if err != nil {
+			log.Error().Str("device", device.Path).Err(err).Msg("failed to check device type")
+			continue
+		}
+
 		if vm {
 			// force ssd device for vms
 			typ = zos.SSDDevice


Test patch:
diff --git a/pkg/storage/filesystem/btrfs_test.go b/pkg/storage/filesystem/btrfs_test.go
index c40cd92e..cbd2a7b5 100644
--- a/pkg/storage/filesystem/btrfs_test.go
+++ b/pkg/storage/filesystem/btrfs_test.go
@@ -6,6 +6,7 @@ import (
 
 	"github.com/stretchr/testify/mock"
 	"github.com/stretchr/testify/require"
+	"github.com/threefoldtech/zos/pkg/gridtypes/zos"
 )
 
 type TestDeviceManager struct {
@@ -36,6 +37,11 @@ func (m *TestDeviceManager) Mountpoint(ctx context.Context, device string) (stri
 	return args.String(0), args.Error(1)
 }
 
+func (m *TestDeviceManager) Seektime(ctx context.Context, device string) (zos.DeviceType, error) {
+	args := m.Called(ctx, device)
+	return zos.DeviceType(args.String(0)), args.Error(1)
+}
+
 func TestBtrfsCreatePoolExists(t *testing.T) {
 	require := require.New(t)
 	exe := &TestExecuter{}
@@ -47,7 +53,6 @@ func TestBtrfsCreatePoolExists(t *testing.T) {
 		mgr:        &mgr,
 		Path:       "/tmp/disk",
 		Rota:       false,
-		Readtime:   0,
 		Label:      "some-label",
 		Filesystem: BtrfsFSType,
 	}
@@ -66,7 +71,6 @@ func TestBtrfsCreatePoolExists(t *testing.T) {
 		mgr:        &mgr,
 		Path:       "/tmp/disk",
 		Rota:       false,
-		Readtime:   0,
 		Label:      "some-label",
 		Filesystem: BtrfsFSType,
 	}
@@ -85,9 +89,8 @@ func TestBtrfsCreatePoolNotExist(t *testing.T) {
 
 	exe := &TestExecuter{}
 	dev := DeviceInfo{
-		Path:     "/tmp/disk",
-		Rota:     false,
-		Readtime: 0,
+		Path: "/tmp/disk",
+		Rota: false,
 	}
 
 	// expected formating of the device
diff --git a/pkg/storage/filesystem/device_test.go b/pkg/storage/filesystem/device_test.go
index 6c7b1782..f96c07d0 100644
--- a/pkg/storage/filesystem/device_test.go
+++ b/pkg/storage/filesystem/device_test.go
@@ -6,6 +6,7 @@ import (
 
 	"github.com/stretchr/testify/mock"
 	"github.com/stretchr/testify/require"
+	"github.com/threefoldtech/zos/pkg/gridtypes/zos"
 )
 
 func TestDeviceManagerScan(t *testing.T) {
@@ -73,8 +74,11 @@ func TestDeviceManagerScan(t *testing.T) {
 
 	require.Len(cached, 2)
 	// make sure all types are set.
-	for _, dev := range cached {
-		require.NotEmpty(dev.Type(), "device: %s", dev.Path)
+	expected := []zos.DeviceType{zos.SSDDevice, zos.HDDDevice}
+	for i, dev := range cached {
+		typ, err := dev.Type()
+		require.NoError(err)
+		require.Equal(expected[i], typ)
 	}
 
 	filtered, err := mgr.ByLabel(ctx, "test")