Repository: threefoldtech/zos
Pull request #565 (2020-03-09 13:12:34)

Cache issue
Current flow:
1. Try to make cache on SSD, if not available on HDD
2. When failing to do both, create cache-limited file (used as flag) in /tmp/flags
3. Check if flag "limited-cache" is true, if so, do not provision
4. Retry the provision every 30 sec by looking if flag is still set

codecov[bot]: # [Codecov](https://codecov.io/gh/threefoldtech/zos/pull/565?src=pr&el=h1) Report
> Merging [#565](https://codecov.io/gh/threefoldtech/zos/pull/565?src=pr&el=desc) into [master](https://codecov.io/gh/threefoldtech/zos/commit/328df4540f655658acebe9a0d1451f5c046c9f3d&el=desc) will **decrease** coverage by `0.06%`.
> The diff coverage is `0.00%`.

[![Impacted file tree graph](https://codecov.io/gh/threefoldtech/zos/pull/565/graphs/tree.svg?width=650&height=150&src=pr&token=f7Vmq1OAEm)](https://codecov.io/gh/threefoldtech/zos/pull/565?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #565      +/-   ##
==========================================
- Coverage   36.45%   36.39%   -0.07%     
==========================================
  Files          76       76              
  Lines        5190     5199       +9     
==========================================
  Hits         1892     1892              
- Misses       3024     3033       +9     
  Partials      274      274              
```


| [Impacted Files](https://codecov.io/gh/threefoldtech/zos/pull/565?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/storage/storage.go](https://codecov.io/gh/threefoldtech/zos/pull/565/diff?src=pr&el=tree#diff-cGtnL3N0b3JhZ2Uvc3RvcmFnZS5nbw==) | `11.34% <0.00%> (-0.38%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/threefoldtech/zos/pull/565?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/threefoldtech/zos/pull/565?src=pr&el=footer). Last update [328df45...f1d20cf](https://codecov.io/gh/threefoldtech/zos/pull/565?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

maxux: For this specific PR, I would handle that in a different way.

Keeping track of system status using temporary files is IMO not adapted. Since you can check the real status as easily than checking file presence, adding this layer add possible inconsistency (file exists and should not, file doesn't exists and should) while you could simply avoid id completly.

In this specific case, I would have change the `filesystem.IsMountPoint` to not return a `bool` but returns a string, which contains the mountpoint type if found.

This is a simple change on the `filesystem.IsMountPoint` function since it already reads `/proc/mounts`, if entry is found, just returns the 3rd item on the line (which is the filesystem type).

The `filesystem.IsMountPoint` is only called 1 time, so it's not a big change on the whole code base. After that, you don't have to check the flag status, just check if `/var/cache` is mounted, if type is `tmpfs` then you know what to do.
maximevanhees: Following @maxux 's comment. If I now need to change it, just let me know. Thanks for the reply!
muhamadazmy: @maxux While i fully agree with your concern, but this raise another issue. The provision (or other daemons) must know be aware of storaged operation and also depends on its implementation. This means that changes to storaged can break provisiond (for example if we changed the limited cache underlying filesystem or decided not to mount anything at all)

With the file flags, this allow us to separate concerns, and make sure changes to storaged doesn't break provisiond.
maxux: > for example if we changed the limited cache underlying filesystem or decided not to mount anything at all

I don't see how a flag file can help that. And since all implementation share same code, you could reuse same function in the code to check status.

Patch:
diff --git a/cmds/provisiond/main.go b/cmds/provisiond/main.go
index c7ceb13c..4a1488b9 100644
--- a/cmds/provisiond/main.go
+++ b/cmds/provisiond/main.go
@@ -5,6 +5,7 @@ import (
 	"flag"
 	"os"
 	"path/filepath"
+	"time"
 
 	"github.com/pkg/errors"
 	"github.com/threefoldtech/zos/pkg"
@@ -52,6 +53,14 @@ func main() {
 		zerolog.SetGlobalLevel(zerolog.DebugLevel)
 	}
 
+	// keep checking if limited-cache flag is set
+	if app.CheckFlag(app.LimitedCache) {
+		log.Error().Msg("failed cache reservation! Retrying every 30 seconds...")
+		for app.CheckFlag(app.LimitedCache) {
+			time.Sleep(time.Second * 30)
+		}
+	}
+
 	flag.Parse()
 
 	if err := os.MkdirAll(storageDir, 0770); err != nil {
@@ -107,6 +116,7 @@ func main() {
 	)
 
 	engine := provision.New(nodeID.Identity(), source, localStore, remoteStore)
+
 	server.Register(zbus.ObjectID{Name: module, Version: "0.0.1"}, pkg.ProvisionMonitor(engine))
 
 	log.Info().
diff --git a/cmds/storaged/main.go b/cmds/storaged/main.go
index b9f6f731..082bf922 100644
--- a/cmds/storaged/main.go
+++ b/cmds/storaged/main.go
@@ -38,7 +38,7 @@ func main() {
 
 	storageModule, err := storage.New()
 	if err != nil {
-		log.Fatal().Err(err).Msg("Error initializing storage module")
+		log.Fatal().Err(err).Msg("failed to initialize storage module")
 	}
 
 	server, err := zbus.NewRedisServer(module, msgBrokerCon, workerNr)
@@ -46,14 +46,15 @@ func main() {
 		log.Fatal().Err(err).Msg("fail to connect to message broker server")
 	}
 
+	server.Register(zbus.ObjectID{Name: "storage", Version: "0.0.1"}, storageModule)
+
 	vdiskModule, err := storage.NewVDiskModule(storageModule)
 	if err != nil {
-		log.Fatal().Err(err)
+		log.Error().Err(err).Bool("limited-cache", app.CheckFlag(app.LimitedCache)).Msg("failed to initialize virtual disk module")
+	} else {
+		server.Register(zbus.ObjectID{Name: "vdisk", Version: "0.0.1"}, vdiskModule)
 	}
 
-	server.Register(zbus.ObjectID{Name: "storage", Version: "0.0.1"}, storageModule)
-	server.Register(zbus.ObjectID{Name: "vdisk", Version: "0.0.1"}, vdiskModule)
-
 	log.Info().
 		Str("broker", msgBrokerCon).
 		Uint("worker nr", workerNr).
diff --git a/pkg/app/flag.go b/pkg/app/flag.go
new file mode 100644
index 00000000..957e78b4
--- /dev/null
+++ b/pkg/app/flag.go
@@ -0,0 +1,53 @@
+package app
+
+import (
+	"fmt"
+	"os"
+	"path/filepath"
+
+	"github.com/pkg/errors"
+)
+
+const (
+	flagsDir = "/tmp/flags"
+	// LimitedCache represent the flag cache couldn't mount on ssd or hdd
+	LimitedCache = "limited-cache"
+)
+
+// SetFlag is used when the /var/cache cannot be mounted on a SSD or HDD,
+// it will mount the cache disk on a temporary file system in memory.
+func SetFlag(key string) error {
+	// creating a file that will be used as a flag
+	// the flag can 'warn' other deamons that the /var/cache is not on HDD or SDD
+	if err := os.MkdirAll(flagsDir, os.ModePerm); err != nil {
+		return errors.Wrap(err, "failed to create flags directory")
+	}
+
+	f, err := os.Create(filepath.Join(flagsDir, key))
+	if err != nil {
+		return errors.Wrap(err, "failed to create the flag file")
+	}
+
+	return f.Close()
+}
+
+// CheckFlag checks the status of a flag based on a key
+func CheckFlag(key string) bool {
+	_, err := os.Stat(filepath.Join(flagsDir, key))
+	return !os.IsNotExist(err)
+}
+
+// DeleteFlag deletes (unsets) a given flag based on a key
+func DeleteFlag(key string) error {
+	// to avoid "path injection"
+	path := filepath.Join(flagsDir, key)
+	if filepath.Dir(path) != flagsDir {
+		return fmt.Errorf("trying to delete a directory outside of the flags boundaries")
+	}
+
+	if err := os.RemoveAll(filepath.Join(flagsDir, key)); err != nil {
+		errors.Wrap(err, "failed to remove the flag file")
+		return err
+	}
+	return nil
+}
diff --git a/pkg/storage/storage.go b/pkg/storage/storage.go
index c3722d1d..3413b6d1 100644
--- a/pkg/storage/storage.go
+++ b/pkg/storage/storage.go
@@ -7,6 +7,7 @@ import (
 	"sort"
 	"strings"
 	"sync"
+	"syscall"
 	"time"
 
 	"github.com/google/uuid"
@@ -14,6 +15,7 @@ import (
 	log "github.com/rs/zerolog/log"
 	"github.com/shirou/gopsutil/disk"
 	"github.com/threefoldtech/zos/pkg"
+	"github.com/threefoldtech/zos/pkg/app"
 	"github.com/threefoldtech/zos/pkg/storage/filesystem"
 )
 
@@ -365,7 +367,12 @@ func (s *storageModule) ensureCache() error {
 
 	var cacheFs filesystem.Volume
 
-	// check if we already have a cache
+	if filesystem.IsMountPoint(CacheTarget) {
+		log.Debug().Msgf("Cache partition already mounted in %s", CacheTarget)
+		return nil
+	}
+
+	// check if cache volume available
 	for idx := range s.volumes {
 		filesystems, err := s.volumes[idx].Volumes()
 		if err != nil {
@@ -386,23 +393,41 @@ func (s *storageModule) ensureCache() error {
 	if cacheFs == nil {
 		log.Debug().Msgf("No cache found, try to create new cache")
 
+		log.Debug().Msgf("Trying to create new cache on SSD")
 		fs, err := s.createSubvol(cacheSize, cacheLabel, pkg.SSDDevice)
-		if errors.Is(err, pkg.ErrNotEnoughSpace{}) {
-			// No space on SSD (probably no SSD in the node at all), try HDD
-			fs, err = s.createSubvol(cacheSize, cacheLabel, pkg.HDDDevice)
+
+		if err != nil {
+			log.Warn().Err(err).Msg("failed to create new cache on SSD")
+		} else {
+			cacheFs = fs
 		}
+	}
+
+	if cacheFs == nil {
+		log.Debug().Msgf("Trying to create new cache on HDD")
+		fs, err := s.createSubvol(cacheSize, cacheLabel, pkg.HDDDevice)
+
 		if err != nil {
-			return err
+			log.Warn().Err(err).Msg("failed to create new cache on HDD")
+		} else {
+			cacheFs = fs
 		}
-		cacheFs = fs
 	}
 
-	if !filesystem.IsMountPoint(CacheTarget) {
-		log.Debug().Msgf("Mounting cache partition in %s", CacheTarget)
-		return filesystem.BindMount(cacheFs, CacheTarget)
+	if cacheFs == nil {
+		log.Warn().Msg("failed to create persisted cache disk. Running on limited cache")
+
+		// set limited cache flag
+		if err := app.SetFlag("limited-cache"); err != nil {
+			return err
+		}
+
+		// when everything failed, mount the Tmpfs
+		return syscall.Mount("", "/var/cache", "tmpfs", 0, "size=500M")
 	}
-	log.Debug().Msgf("Cache partition already mounted in %s", CacheTarget)
-	return nil
+
+	log.Debug().Msgf("Mounting cache partition in %s", CacheTarget)
+	return filesystem.BindMount(cacheFs, CacheTarget)
 }
 
 // createSubvol creates a subvolume with the given name and limits it to the given size


Test patch:
diff --git a/pkg/app/flag_test.go b/pkg/app/flag_test.go
new file mode 100644
index 00000000..bce547f7
--- /dev/null
+++ b/pkg/app/flag_test.go
@@ -0,0 +1,19 @@
+package app
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestDeleteFlag(t *testing.T) {
+	key := LimitedCache
+	err := DeleteFlag(key)
+	require.NoError(t, err)
+}
+
+func TestDeleteBadFlag(t *testing.T) {
+	key := "/"
+	err := DeleteFlag(key)
+	require.Error(t, err)
+}