Repository: threefoldtech/zos
Pull request #2023 (2023-08-14 12:10:31)

save device type instead of caching it
### Issues
 - https://github.com/threefoldtech/zos/issues/1970

xmonader: I noticed this in the seektime code https://github.com/maxux/seektime/issues/2 , not sure how it affects the accuracy or the calculations @maxux @muhamadazmy ?
maxux: Regarding https://github.com/maxux/seektime/issues/2, this doesn't affect any accuracy in our case.
But this https://github.com/threefoldtech/seektime/pull/3 was ready for a long time now and were never merged, but this could provide a way better help to troubleshoot in case of issue.

Does merging this PR would affect something in zos ? The json reply would change, maybe you need to support it ?

This pull requests will print `min` and `max` seektime, this helped me previously to diagnostic a bug where HDD were sleeping and disks needed to be woke up on the first check; `max` was set to few of seconds and `min` were real value, this helped to understand what happened. This should be merged IMO.
muhamadazmy: @maxux current seektime version in zos return this

```
seektime -j /dev/vda
{"device": "/dev/vda", "type": "HDD", "elapsed": 564}
```
if the format is going to change then zos also need to have some modification to support his.

Right now it seems that seektime is only provided by the base image. so after merging of your branch, and if zos image is rebuilt, then zos will break. Instead what we need to do is provide a runtime package update for seektime so both the seektime version and the code to handle it to be installed in one go.

Can you please provide a `seektime` pre-build in the seektime release. A static build is the best or in worst case make sure it uses the same libc version of the base zos image.

Once this build is available, an flist can be built for seektime that can be included in zos 


Patch:
diff --git a/pkg/cache/cache.go b/pkg/cache/cache.go
index 0b26fd2a..68b9b758 100644
--- a/pkg/cache/cache.go
+++ b/pkg/cache/cache.go
@@ -13,7 +13,7 @@ const (
 
 // VolatileDir creates a new cache directory that is stored on a tmpfs.
 // This means data stored in this directory will NOT survive a reboot.
-// Use this when you need to store data that needs to survice deamon reboot but not between reboots
+// Use this when you need to store data that needs to survive deamon reboot but not between reboots
 // It is the caller's responsibility to remove the directory when no longer needed.
 // If the directory already exist error of type os.IsExist will be returned
 func VolatileDir(name string, size uint64) (string, error) {
diff --git a/pkg/storage/filesystem/btrfs.go b/pkg/storage/filesystem/btrfs.go
index 43fddc43..e2ef080b 100644
--- a/pkg/storage/filesystem/btrfs.go
+++ b/pkg/storage/filesystem/btrfs.go
@@ -11,6 +11,7 @@ import (
 	"github.com/google/uuid"
 	"github.com/pkg/errors"
 	"github.com/rs/zerolog/log"
+	"github.com/threefoldtech/zos/pkg/gridtypes/zos"
 )
 
 var (
@@ -308,6 +309,45 @@ func (p *btrfsPool) Shutdown() error {
 	return nil
 }
 
+// SetType sets the device type to the disk
+func (p *btrfsPool) SetType(typ zos.DeviceType) error {
+	path, err := p.Mounted()
+	if err != nil {
+		return err
+	}
+	diskTypePath := filepath.Join(path, ".seektime")
+	if err := os.WriteFile(diskTypePath, []byte(typ), 0644); err != nil {
+		return errors.Wrapf(err, "failed to store device type for '%s' in '%s'", p.Name(), diskTypePath)
+	}
+
+	return nil
+}
+
+// Type gets the device type from the disk
+// return the device type, if it's found and an error
+// that's based on .seektime file existing in the /mnt/DeviceName/.seektime, that contains the device type being SSD or HDD
+func (p *btrfsPool) Type() (zos.DeviceType, bool, error) {
+	path, err := p.Mounted()
+	if err != nil {
+		return "", false, err
+	}
+	diskTypePath := filepath.Join(path, ".seektime")
+	diskType, err := os.ReadFile(diskTypePath)
+	if os.IsNotExist(err) {
+		return "", false, nil
+	}
+
+	if err != nil {
+		return "", false, err
+	}
+
+	if len(diskType) == 0 {
+		return "", false, nil
+	}
+
+	return zos.DeviceType(diskType), true, nil
+}
+
 type btrfsVolume struct {
 	id    int
 	path  string
diff --git a/pkg/storage/filesystem/device.go b/pkg/storage/filesystem/device.go
index 0f47ec98..7eabff1f 100644
--- a/pkg/storage/filesystem/device.go
+++ b/pkg/storage/filesystem/device.go
@@ -68,7 +68,8 @@ func (i *DeviceInfo) Used() bool {
 	return len(i.Label) != 0 || len(i.Filesystem) != 0
 }
 
-func (d *DeviceInfo) Type() (zos.DeviceType, error) {
+// DetectType returns the device type according to seektime
+func (d *DeviceInfo) DetectType() (zos.DeviceType, error) {
 	return d.mgr.Seektime(context.Background(), d.Path)
 }
 
diff --git a/pkg/storage/filesystem/filesystem.go b/pkg/storage/filesystem/filesystem.go
index 27f61709..1e1378ba 100644
--- a/pkg/storage/filesystem/filesystem.go
+++ b/pkg/storage/filesystem/filesystem.go
@@ -4,6 +4,7 @@ import (
 	"context"
 
 	"github.com/pkg/errors"
+	"github.com/threefoldtech/zos/pkg/gridtypes/zos"
 )
 
 // Usage struct (in bytes)
@@ -72,6 +73,13 @@ type Pool interface {
 	Shutdown() error
 	// Device return device associated with pool
 	Device() DeviceInfo
+	// SetType sets a device type on the pool. this will make
+	// sure that the detected device type is reported
+	// correctly by calling the Type() method.
+	SetType(typ zos.DeviceType) error
+	// Type returns the device type set by a previous call
+	// to SetType.
+	Type() (zos.DeviceType, bool, error)
 }
 
 // Filter closure for Filesystem list
diff --git a/pkg/storage/storage.go b/pkg/storage/storage.go
index 503bab3f..54212f3c 100644
--- a/pkg/storage/storage.go
+++ b/pkg/storage/storage.go
@@ -58,6 +58,8 @@ type Module struct {
 	mu sync.RWMutex
 
 	// cache is a cache directory can be used with some files
+	// NOTED: this is deprecated, now type is stored on the device
+	// itself not in temp cache
 	cache TypeCache
 }
 
@@ -65,14 +67,6 @@ type TypeCache struct {
 	base string
 }
 
-func (t *TypeCache) Set(name string, typ pkg.DeviceType) error {
-	if err := os.WriteFile(filepath.Join(t.base, name), []byte(typ), 0644); err != nil {
-		return errors.Wrapf(err, "failed to store device type for '%s'", name)
-	}
-
-	return nil
-}
-
 func (t *TypeCache) Get(name string) (pkg.DeviceType, bool) {
 	data, err := os.ReadFile(filepath.Join(t.base, name))
 	if err != nil {
@@ -157,6 +151,50 @@ func (s *Module) dump() {
 
 }
 
+// poolType gets the device type of a disk
+func (s *Module) poolType(pool filesystem.Pool, vm bool) (zos.DeviceType, error) {
+	var typ zos.DeviceType
+	device := pool.Device()
+	// for development purposes only
+	if vm {
+		// force ssd device for vms
+		typ = zos.SSDDevice
+
+		if device.Path == "/dev/vdd" || device.Path == "/dev/vde" {
+			typ = zos.HDDDevice
+		}
+		return typ, nil
+	}
+
+	log.Debug().Str("device", device.Path).Msg("checking device type in disk")
+	typ, ok, err := pool.Type()
+	if err != nil {
+		return "", errors.Wrap(err, "failed to get device type")
+	}
+
+	if ok {
+		log.Debug().Str("device", device.Path).Str("type", typ.String()).Msg("device type loaded from disk")
+		return typ, nil
+	}
+
+	log.Debug().Str("device", device.Path).Msg("checking device type in cache")
+	typ, ok = s.cache.Get(device.Name())
+	if !ok {
+		log.Debug().Str("device", device.Path).Msg("detecting device type")
+		typ, err = device.DetectType()
+		if err != nil {
+			return "", errors.Wrap(err, "failed to detect device type")
+		}
+	}
+
+	log.Debug().Str("device", device.Path).Str("type", typ.String()).Msg("setting device type")
+	if err := pool.SetType(typ); err != nil {
+		return "", errors.Wrap(err, "failed to set device type")
+	}
+
+	return typ, nil
+}
+
 /*
 *
 initialize, must be called at least onetime each boot.
@@ -206,31 +244,11 @@ func (s *Module) initialize(ctx context.Context) error {
 			log.Error().Err(err).Str("pool", pool.Name()).Str("device", device.Path).Msg("failed to get usage of pool")
 		}
 
-		typ, ok := s.cache.Get(device.Name())
-		if !ok {
-			log.Debug().Str("device", device.Path).Msg("detecting device type")
-			typ, err = device.Type()
-			if err != nil {
-				log.Error().Str("device", device.Path).Err(err).Msg("failed to check device type")
-				continue
-			}
-
-			// for development purposes only
-			if vm {
-				// force ssd device for vms
-				typ = zos.SSDDevice
-
-				if device.Path == "/dev/vdd" || device.Path == "/dev/vde" {
-					typ = zos.HDDDevice
-				}
-			}
-
-			log.Debug().Str("device", device.Path).Str("type", typ.String()).Msg("caching device type")
-			if err := s.cache.Set(device.Name(), typ); err != nil {
-				log.Error().Str("device", device.Path).Err(err).Msg("failed to cache device type")
-			}
-		} else {
-			log.Debug().Str("device", device.Path).Str("type", typ.String()).Msg("device type loaded from cache")
+		typ, err := s.poolType(pool, vm)
+		if err != nil {
+			log.Error().Str("device", device.Path).Err(err).Msg("failed to get device type")
+			s.brokenDevices = append(s.brokenDevices, pkg.BrokenDevice{Path: device.Path, Err: err})
+			continue
 		}
 
 		switch typ {


Test patch:
diff --git a/pkg/storage/filesystem/device_test.go b/pkg/storage/filesystem/device_test.go
index f96c07d0..c8652af6 100644
--- a/pkg/storage/filesystem/device_test.go
+++ b/pkg/storage/filesystem/device_test.go
@@ -76,7 +76,7 @@ func TestDeviceManagerScan(t *testing.T) {
 	// make sure all types are set.
 	expected := []zos.DeviceType{zos.SSDDevice, zos.HDDDevice}
 	for i, dev := range cached {
-		typ, err := dev.Type()
+		typ, err := dev.DetectType()
 		require.NoError(err)
 		require.Equal(expected[i], typ)
 	}
diff --git a/pkg/storage/storage_test.go b/pkg/storage/storage_test.go
index c5df44af..c247dc63 100644
--- a/pkg/storage/storage_test.go
+++ b/pkg/storage/storage_test.go
@@ -91,8 +91,12 @@ func (p *testPool) UnMount() error {
 	return fmt.Errorf("UnMount not implemented")
 }
 
-func (p *testPool) Type() zos.DeviceType {
-	return p.ptype
+func (p *testPool) Type() (zos.DeviceType, bool, error) {
+	return p.ptype, false, nil
+}
+
+func (p *testPool) SetType(_ zos.DeviceType) error {
+	return nil
 }
 
 func (p *testPool) Volumes() ([]filesystem.Volume, error) {