Merge pull request #6019 from liubin/fix/6018-virtiofsd-cache-mod

Change cache mode from none to never

docs/how-to/how-to-set-sandbox-config-kata.md

@@ -87,7 +87,7 @@ There are several kinds of Kata configurations and they are listed below.
 | `io.katacontainers.config.hypervisor.use_vsock` | `boolean` | specify use of `vsock` for agent communication |
 | `io.katacontainers.config.hypervisor.vhost_user_store_path` (R) | `string` | specify the directory path where vhost-user devices related folders, sockets and device nodes should be (QEMU) |
 | `io.katacontainers.config.hypervisor.virtio_fs_cache_size` | uint32 | virtio-fs DAX cache size in `MiB` |
-| `io.katacontainers.config.hypervisor.virtio_fs_cache` | string | the cache mode for virtio-fs, valid values are `always`, `auto` and `none` |
+| `io.katacontainers.config.hypervisor.virtio_fs_cache` | string | the cache mode for virtio-fs, valid values are `always`, `auto` and `never` |
 | `io.katacontainers.config.hypervisor.virtio_fs_daemon` | string | virtio-fs `vhost-user` daemon path |
 | `io.katacontainers.config.hypervisor.virtio_fs_extra_args` | string | extra options passed to `virtiofs` daemon |
 | `io.katacontainers.config.hypervisor.enable_guest_swap` | `boolean` | enable swap in the guest |

src/libs/kata-types/src/annotations/mod.rs

@@ -266,7 +266,7 @@ pub const KATA_ANNO_CFG_HYPERVISOR_SHARED_FS: &str =
 /// A sandbox annotations to specify virtio-fs vhost-user daemon path.
 pub const KATA_ANNO_CFG_HYPERVISOR_VIRTIO_FS_DAEMON: &str =
     "io.katacontainers.config.hypervisor.virtio_fs_daemon";
-/// A sandbox annotation to specify the cache mode for fs version cache or "none".
+/// A sandbox annotation to specify the cache mode for fs version cache.
 pub const KATA_ANNO_CFG_HYPERVISOR_VIRTIO_FS_CACHE: &str =
     "io.katacontainers.config.hypervisor.virtio_fs_cache";
 /// A sandbox annotation to specify the DAX cache size in MiB.

src/libs/kata-types/src/config/hypervisor/mod.rs

@@ -767,7 +767,7 @@ pub struct SharedFsInfo {
     pub virtio_fs_extra_args: Vec<String>,
 
     /// Cache mode:
-    /// - none: Metadata, data, and pathname lookup are not cached in guest. They are always
+    /// - never: Metadata, data, and pathname lookup are not cached in guest. They are always
     ///   fetched from host and any changes are immediately pushed to host.
     /// - auto: Metadata and pathname lookup cache expires after a configured amount of time
     ///   (default is 1 second). Data is cached while the file is open (close to open consistency).

src/runtime-rs/crates/resource/src/share_fs/share_virtio_fs_standalone.rs

@@ -35,7 +35,7 @@ pub struct ShareVirtioFsStandaloneConfig {
 
     // virtio_fs_daemon is the virtio-fs vhost-user daemon path
     pub virtio_fs_daemon: String,
-    // virtio_fs_cache cache mode for fs version cache or "none"
+    // virtio_fs_cache cache mode for fs version cache
     pub virtio_fs_cache: String,
     // virtio_fs_extra_args passes options to virtiofsd daemon
     pub virtio_fs_extra_args: Vec<String>,

src/runtime/config/configuration-clh.toml.in

@@ -147,7 +147,7 @@ virtio_fs_extra_args = @DEFVIRTIOFSEXTRAARGS@
 
 # Cache mode:
 #
-#  - none
+#  - never
 #    Metadata, data, and pathname lookup are not cached in guest. They are
 #    always fetched from host and any changes are immediately pushed to host.
 #

src/runtime/config/configuration-qemu.toml.in

@@ -205,7 +205,7 @@ virtio_fs_extra_args = @DEFVIRTIOFSEXTRAARGS@
 
 # Cache mode:
 #
-#  - none
+#  - never
 #    Metadata, data, and pathname lookup are not cached in guest. They are
 #    always fetched from host and any changes are immediately pushed to host.
 #

src/runtime/pkg/katautils/config_test.go

@@ -1242,9 +1242,9 @@ func TestDefaultVirtioFSCache(t *testing.T) {
 	cache = h.defaultVirtioFSCache()
 	assert.Equal("always", cache)
 
-	h.VirtioFSCache = "none"
+	h.VirtioFSCache = "never"
 	cache = h.defaultVirtioFSCache()
-	assert.Equal("none", cache)
+	assert.Equal("never", cache)
 }
 
 func TestDefaultFirmware(t *testing.T) {

src/runtime/pkg/oci/utils_test.go

@@ -650,7 +650,7 @@ func TestAddHypervisorAnnotations(t *testing.T) {
 	ocispec.Annotations[vcAnnotations.BlockDeviceCacheNoflush] = "true"
 	ocispec.Annotations[vcAnnotations.SharedFS] = "virtio-fs"
 	ocispec.Annotations[vcAnnotations.VirtioFSDaemon] = "/bin/false"
-	ocispec.Annotations[vcAnnotations.VirtioFSCache] = "/home/cache"
+	ocispec.Annotations[vcAnnotations.VirtioFSCache] = "auto"
 	ocispec.Annotations[vcAnnotations.VirtioFSExtraArgs] = "[ \"arg0\", \"arg1\" ]"
 	ocispec.Annotations[vcAnnotations.Msize9p] = "512"
 	ocispec.Annotations[vcAnnotations.MachineType] = "q35"
@@ -688,7 +688,7 @@ func TestAddHypervisorAnnotations(t *testing.T) {
 	assert.Equal(config.HypervisorConfig.BlockDeviceCacheNoflush, true)
 	assert.Equal(config.HypervisorConfig.SharedFS, "virtio-fs")
 	assert.Equal(config.HypervisorConfig.VirtioFSDaemon, "/bin/false")
-	assert.Equal(config.HypervisorConfig.VirtioFSCache, "/home/cache")
+	assert.Equal(config.HypervisorConfig.VirtioFSCache, "auto")
 	assert.ElementsMatch(config.HypervisorConfig.VirtioFSExtraArgs, [2]string{"arg0", "arg1"})
 	assert.Equal(config.HypervisorConfig.Msize9p, uint32(512))
 	assert.Equal(config.HypervisorConfig.HypervisorMachineType, "q35")

src/runtime/virtcontainers/documentation/api/1.0/api.md

@@ -219,7 +219,7 @@ type HypervisorConfig struct {
 	// VirtioFSDaemonList is the list of valid virtiofs names for annotations
 	VirtioFSDaemonList []string
 
-	// VirtioFSCache cache mode for fs version cache or "none"
+	// VirtioFSCache cache mode for fs version cache
 	VirtioFSCache string
 
 	// VirtioFSExtraArgs passes options to virtiofsd daemon

src/runtime/virtcontainers/hypervisor.go

@@ -325,7 +325,7 @@ type HypervisorConfig struct {
 	// VirtioFSDaemon is the virtio-fs vhost-user daemon path
 	VirtioFSDaemon string
 
-	// VirtioFSCache cache mode for fs version cache or "none"
+	// VirtioFSCache cache mode for fs version cache
 	VirtioFSCache string
 
 	// File based memory backend root directory

src/runtime/virtcontainers/kata_agent.go

@@ -34,6 +34,7 @@ import (
 	"github.com/kata-containers/kata-containers/src/runtime/virtcontainers/utils"
 
 	"context"
+
 	"github.com/gogo/protobuf/proto"
 	"github.com/opencontainers/runtime-spec/specs-go"
 	"github.com/opencontainers/selinux/go-selinux"
@@ -87,7 +88,7 @@ var (
 	type9pFs                     = "9p"
 	typeVirtioFS                 = "virtiofs"
 	typeOverlayFS                = "overlay"
-	typeVirtioFSNoCache          = "none"
+	typeVirtioFSNoCache          = "never"
 	kata9pDevType                = "9p"
 	kataMmioBlkDevType           = "mmioblk"
 	kataBlkDevType               = "blk"
@@ -801,7 +802,7 @@ func setupStorages(ctx context.Context, sandbox *Sandbox) []*grpc.Storage {
 		if sharedFS == config.VirtioFS || sharedFS == config.VirtioFSNydus {
 			// If virtio-fs uses either of the two cache options 'auto, always',
 			// the guest directory can be mounted with option 'dax' allowing it to
-			// directly map contents from the host. When set to 'none', the mount
+			// directly map contents from the host. When set to 'never', the mount
 			// options should not contain 'dax' lest the virtio-fs daemon crashing
 			// with an invalid address reference.
 			if sandbox.config.HypervisorConfig.VirtioFSCache != typeVirtioFSNoCache {

src/runtime/virtcontainers/persist/api/config.go

@@ -70,7 +70,7 @@ type HypervisorConfig struct {
 	// VirtioFSDaemon is the virtio-fs vhost-user daemon path
 	VirtioFSDaemon string
 
-	// VirtioFSCache cache mode for fs version cache or "none"
+	// VirtioFSCache cache mode for fs version cache
 	VirtioFSCache string
 
 	// File based memory backend root directory

src/runtime/virtcontainers/pkg/annotations/annotations.go

@@ -190,7 +190,7 @@ const (
 	// VirtioFSDaemon is a sandbox annotations to specify virtio-fs vhost-user daemon path
 	VirtioFSDaemon = kataAnnotHypervisorPrefix + "virtio_fs_daemon"
 
-	// VirtioFSCache is a sandbox annotation to specify the cache mode for fs version cache or "none"
+	// VirtioFSCache is a sandbox annotation to specify the cache mode for fs version cache
 	VirtioFSCache = kataAnnotHypervisorPrefix + "virtio_fs_cache"
 
 	// VirtioFSCacheSize is a sandbox annotation to specify the DAX cache size in MiB

src/runtime/virtcontainers/virtiofsd.go

@@ -29,11 +29,12 @@ var virtiofsdTracingTags = map[string]string{
 }
 
 var (
-	errVirtiofsdDaemonPathEmpty    = errors.New("virtiofsd daemon path is empty")
+	errVirtiofsdDaemonPathEmpty          = errors.New("virtiofsd daemon path is empty")
-	errVirtiofsdSocketPathEmpty    = errors.New("virtiofsd socket path is empty")
+	errVirtiofsdSocketPathEmpty          = errors.New("virtiofsd socket path is empty")
-	errVirtiofsdSourcePathEmpty    = errors.New("virtiofsd source path is empty")
+	errVirtiofsdSourcePathEmpty          = errors.New("virtiofsd source path is empty")
-	errVirtiofsdSourceNotAvailable = errors.New("virtiofsd source path not available")
+	errVirtiofsdInvalidVirtiofsCacheMode = func(mode string) error { return errors.Errorf("Invalid virtio-fs cache mode: %s", mode) }
-	errUnimplemented               = errors.New("unimplemented")
+	errVirtiofsdSourceNotAvailable       = errors.New("virtiofsd source path not available")
+	errUnimplemented                     = errors.New("unimplemented")
 )
 
 type VirtiofsDaemon interface {
@@ -63,7 +64,7 @@ type virtiofsd struct {
 	path string
 	// socketPath where daemon will serve
 	socketPath string
-	// cache size for virtiofsd
+	// cache mode for virtiofsd
 	cache string
 	// sourcePath path that daemon will help to share
 	sourcePath string
@@ -213,6 +214,16 @@ func (v *virtiofsd) valid() error {
 	if _, err := os.Stat(v.sourcePath); err != nil {
 		return errVirtiofsdSourceNotAvailable
 	}
+
+	if v.cache == "" {
+		v.cache = "auto"
+	} else if v.cache == "none" {
+		v.Logger().Warn("virtio-fs cache mode `none` is deprecated since Kata Containers 2.5.0 and will be removed in the future release, please use `never` instead. For more details please refer to https://github.com/kata-containers/kata-containers/issues/4234.")
+		v.cache = "never"
+	} else if v.cache != "auto" && v.cache != "always" && v.cache != "never" {
+		return errVirtiofsdInvalidVirtiofsCacheMode(v.cache)
+	}
+
 	return nil
 }
 

src/runtime/virtcontainers/virtiofsd_test.go

@@ -91,7 +91,7 @@ func TestVirtiofsdArgs(t *testing.T) {
 }
 
 func TestValid(t *testing.T) {
-	assert := assert.New(t)
+	a := assert.New(t)
 
 	sourcePath := t.TempDir()
 	socketDir := t.TempDir()
@@ -103,31 +103,61 @@ func TestValid(t *testing.T) {
 			path:       "/usr/bin/virtiofsd",
 			sourcePath: sourcePath,
 			socketPath: socketPath,
+			cache:      "auto",
 		}
 	}
 
-	// valid case
+	type fieldFunc func(v *virtiofsd)
-	v := newVirtiofsdFunc()
+	type assertFunc func(name string, assert *assert.Assertions, v *virtiofsd)
-	err := v.valid()
-	assert.NoError(err)
 
-	v = newVirtiofsdFunc()
+	// nolint: govet
-	v.path = ""
+	tests := []struct {
-	err = v.valid()
+		name         string
-	assert.Equal(errVirtiofsdDaemonPathEmpty, err)
+		f            fieldFunc
+		wantErr      error
+		customAssert assertFunc
+	}{
+		{"valid case", nil, nil, nil},
+		{"no path", func(v *virtiofsd) {
+			v.path = ""
+		}, errVirtiofsdDaemonPathEmpty, nil},
+		{"no sourcePath", func(v *virtiofsd) {
+			v.sourcePath = ""
+		}, errVirtiofsdSourcePathEmpty, nil},
+		{"no socketPath", func(v *virtiofsd) {
+			v.socketPath = ""
+		}, errVirtiofsdSocketPathEmpty, nil},
+		{"source is not available", func(v *virtiofsd) {
+			v.sourcePath = "/foo/bar"
+		}, errVirtiofsdSourceNotAvailable, nil},
+		{"replace cache mode none by never", func(v *virtiofsd) {
+			v.cache = "none"
+		}, nil, func(name string, a *assert.Assertions, v *virtiofsd) {
+			a.Equal("never", v.cache, "test case %+s, cache mode none should be replaced by never", name)
+		}},
+		{"invald cache mode: replace none by never", func(v *virtiofsd) {
+			v.cache = "foo"
+		}, errVirtiofsdInvalidVirtiofsCacheMode("foo"), nil},
+	}
 
-	v = newVirtiofsdFunc()
+	for _, tt := range tests {
-	v.sourcePath = ""
+		t.Run(tt.name, func(t *testing.T) {
-	err = v.valid()
+			v := newVirtiofsdFunc()
-	assert.Equal(errVirtiofsdSourcePathEmpty, err)
+			if tt.f != nil {
+				tt.f(v)
+			}
+			err := v.valid()
+			if tt.wantErr != nil && err == nil {
+				t.Errorf("test case %+s: virtiofsd.valid() should get error `%+v`, but got nil", tt.name, tt.wantErr)
+			} else if tt.wantErr == nil && err != nil {
+				t.Errorf("test case %+s: virtiofsd.valid() should get no erro, but got `%+v`", tt.name, err)
+			} else if tt.wantErr != nil && err != nil {
+				a.Equal(err.Error(), tt.wantErr.Error(), "test case %+s", tt.name)
+			}
 
-	v = newVirtiofsdFunc()
+			if tt.customAssert != nil {
-	v.socketPath = ""
+				tt.customAssert(tt.name, a, v)
-	err = v.valid()
+			}
-	assert.Equal(errVirtiofsdSocketPathEmpty, err)
+		})
-
+	}
-	v = newVirtiofsdFunc()
-	v.sourcePath = "/foo/bar"
-	err = v.valid()
-	assert.Equal(errVirtiofsdSourceNotAvailable, err)
 }