Merge pull request kata-containers#6019 from liubin/fix/6018-virtiofs…

…d-cache-mod Change cache mode from none to never
itprguy · Jan 10, 2023 · 147c56b · 147c56b
2 parents 7094834 86a82ca
commit 147c56b
Show file tree

Hide file tree

Showing 15 changed files with 88 additions and 46 deletions.
diff --git a/docs/how-to/how-to-set-sandbox-config-kata.md b/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 |

diff --git a/src/libs/kata-types/src/annotations/mod.rs b/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.

diff --git a/src/libs/kata-types/src/config/hypervisor/mod.rs b/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).

diff --git a/src/runtime-rs/crates/resource/src/share_fs/share_virtio_fs_standalone.rs b/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>,

diff --git a/src/runtime/config/configuration-clh.toml.in b/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.
 #

diff --git a/src/runtime/config/configuration-qemu.toml.in b/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.
 #

diff --git a/src/runtime/pkg/katautils/config_test.go b/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) {

diff --git a/src/runtime/pkg/oci/utils_test.go b/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")

diff --git a/src/runtime/virtcontainers/documentation/api/1.0/api.md b/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

diff --git a/src/runtime/virtcontainers/hypervisor.go b/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

diff --git a/src/runtime/virtcontainers/kata_agent.go b/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 {

diff --git a/src/runtime/virtcontainers/persist/api/config.go b/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

diff --git a/src/runtime/virtcontainers/pkg/annotations/annotations.go b/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

diff --git a/src/runtime/virtcontainers/virtiofsd.go b/src/runtime/virtcontainers/virtiofsd.go
@@ -29,11  29,12 @@ var virtiofsdTracingTags = map[string]string{
 }
 
 var (
-	errVirtiofsdDaemonPathEmpty    = errors.New("virtiofsd daemon path is empty")
-	errVirtiofsdSocketPathEmpty    = errors.New("virtiofsd socket path is empty")
-	errVirtiofsdSourcePathEmpty    = errors.New("virtiofsd source path is empty")
-	errVirtiofsdSourceNotAvailable = errors.New("virtiofsd source path not available")
-	errUnimplemented               = errors.New("unimplemented")
 	errVirtiofsdDaemonPathEmpty          = errors.New("virtiofsd daemon path is empty")
 	errVirtiofsdSocketPathEmpty          = errors.New("virtiofsd socket path is empty")
 	errVirtiofsdSourcePathEmpty          = errors.New("virtiofsd source path is empty")
 	errVirtiofsdInvalidVirtiofsCacheMode = func(mode string) error { return errors.Errorf("Invalid virtio-fs cache mode: %s", mode) }
 	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
 }
 

diff --git a/src/runtime/virtcontainers/virtiofsd_test.go b/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
-	v := newVirtiofsdFunc()
-	err := v.valid()
-	assert.NoError(err)
 	type fieldFunc func(v *virtiofsd)
 	type assertFunc func(name string, assert *assert.Assertions, v *virtiofsd)
 
 	// nolint: govet
 	tests := []struct {
 		name         string
 		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()
-	v.path = ""
-	err = v.valid()
-	assert.Equal(errVirtiofsdDaemonPathEmpty, err)
-
-	v = newVirtiofsdFunc()
-	v.sourcePath = ""
-	err = v.valid()
-	assert.Equal(errVirtiofsdSourcePathEmpty, err)
-
-	v = newVirtiofsdFunc()
-	v.socketPath = ""
-	err = v.valid()
-	assert.Equal(errVirtiofsdSocketPathEmpty, err)
-
-	v = newVirtiofsdFunc()
-	v.sourcePath = "/foo/bar"
-	err = v.valid()
-	assert.Equal(errVirtiofsdSourceNotAvailable, err)
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			v := newVirtiofsdFunc()
 			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)
 			}
 
 			if tt.customAssert != nil {
 				tt.customAssert(tt.name, a, v)
 			}
 		})
 	}
 }