From fee4e7c7c494a5dfc94a94f51489e8191b2589c6 Mon Sep 17 00:00:00 2001 From: Bin Liu Date: Tue, 10 Jan 2023 14:19:25 +0800 Subject: [PATCH 1/4] docs: change cache mode from none to never New Rust virtiofsd's `cache` mode doesn't support `none` mode, we should use `never` to replace it. Fixes: #6018 Signed-off-by: Bin Liu --- docs/how-to/how-to-set-sandbox-config-kata.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/how-to/how-to-set-sandbox-config-kata.md b/docs/how-to/how-to-set-sandbox-config-kata.md index 9f612831c2d0..27edbf76d84c 100644 --- 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 | From 7b309b578deec95b35f5cf66df267352a94b313b Mon Sep 17 00:00:00 2001 From: Bin Liu Date: Tue, 10 Jan 2023 14:21:30 +0800 Subject: [PATCH 2/4] kata-types: change cache mode from none to never New Rust virtiofsd's `cache` mode doesn't support `none` mode, we should use `never` to replace it. Fixes: #6018 Signed-off-by: Bin Liu --- src/libs/kata-types/src/annotations/mod.rs | 2 +- src/libs/kata-types/src/config/hypervisor/mod.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libs/kata-types/src/annotations/mod.rs b/src/libs/kata-types/src/annotations/mod.rs index 83c36d2f7e9d..c8d63125b977 100644 --- 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 index b7dd9f68c58f..aba5c751b63b 100644 --- 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, /// 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). From 82c59efd65c721f59425460cb6fd875701e25cac Mon Sep 17 00:00:00 2001 From: Bin Liu Date: Tue, 10 Jan 2023 14:22:04 +0800 Subject: [PATCH 3/4] runtime-rs: change cache mode from none to never New Rust virtiofsd's `cache` mode doesn't support `none` mode, we should use `never` to replace it. Fixes: #6018 Signed-off-by: Bin Liu --- .../crates/resource/src/share_fs/share_virtio_fs_standalone.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 index e663cc029b88..db421ada36ae 100644 --- 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, From 86a82cace9c9995de1e986fff7f408a979a69e4a Mon Sep 17 00:00:00 2001 From: Bin Liu Date: Tue, 10 Jan 2023 14:22:44 +0800 Subject: [PATCH 4/4] runtime: change cache mode from none to never New Rust virtiofsd's `cache` mode doesn't support `none` mode, we should use `never` to replace it. Fixes: #6018 Signed-off-by: Bin Liu --- src/runtime/config/configuration-clh.toml.in | 2 +- src/runtime/config/configuration-qemu.toml.in | 2 +- src/runtime/pkg/katautils/config_test.go | 4 +- src/runtime/pkg/oci/utils_test.go | 4 +- .../documentation/api/1.0/api.md | 2 +- src/runtime/virtcontainers/hypervisor.go | 2 +- src/runtime/virtcontainers/kata_agent.go | 5 +- .../virtcontainers/persist/api/config.go | 2 +- .../pkg/annotations/annotations.go | 2 +- src/runtime/virtcontainers/virtiofsd.go | 23 ++++-- src/runtime/virtcontainers/virtiofsd_test.go | 78 +++++++++++++------ 11 files changed, 84 insertions(+), 42 deletions(-) diff --git a/src/runtime/config/configuration-clh.toml.in b/src/runtime/config/configuration-clh.toml.in index cedf2303ad8b..fbf8385721c9 100644 --- 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 index f7e70a6d53ff..26b7e2f08797 100644 --- 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 index 335f077fbb4a..156c312cfc48 100644 --- 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 index b5b15ad871f4..7b2c10f2f091 100644 --- 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 index c7215cc97c5a..9255a96ca912 100644 --- 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 index b8482e7b4bef..eb6ed3fd364e 100644 --- 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 index 57467595429b..9de1661efde4 100644 --- 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 index 44ba820643a8..9b5af5668da0 100644 --- 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 index 67c81cb1f876..5d36926c5e3a 100644 --- 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 index 0d8ba1c19029..08f7dfd6f87a 100644 --- 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 index 3705a559b929..a4252a2ba9c5 100644 --- 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) + } + }) + } }