From 050f03bb3617b5a718e4963f32d23f7bcc039ec2 Mon Sep 17 00:00:00 2001 From: Wei Zhang Date: Tue, 5 Feb 2019 23:43:52 +0800 Subject: [PATCH 1/3] config: Add config flag "experimental" Fixes #1226 Add new flag "experimental" for supporting underworking features. Some features are under developing which are not ready for release, there're also some features which will break compatibility which is not suitable to be merged into a kata minor release(x version in x.y.z) For getting these features above merged earlier for more testing, we can mark them as "experimental" features, and move them to formal features when they are ready. Signed-off-by: Wei Zhang --- Makefile | 4 ++ cli/config/configuration-fc.toml.in | 8 +++ cli/config/configuration-qemu.toml.in | 8 +++ cli/kata-env.go | 3 + pkg/katautils/config.go | 70 ++++++++++++------- virtcontainers/experimental/experimental.go | 32 +++++++++ .../experimental/experimental_test.go | 26 +++++++ virtcontainers/pkg/oci/utils.go | 6 ++ virtcontainers/sandbox.go | 14 ++++ virtcontainers/sandbox_test.go | 16 +++++ 10 files changed, 163 insertions(+), 24 deletions(-) create mode 100644 virtcontainers/experimental/experimental.go create mode 100644 virtcontainers/experimental/experimental_test.go diff --git a/Makefile b/Makefile index 7340add51..1648e1622 100644 --- a/Makefile +++ b/Makefile @@ -157,6 +157,8 @@ DEFMEMSLOTS := 10 #Default number of bridges DEFBRIDGES := 1 DEFDISABLEGUESTSECCOMP := true +#Default experimental features enabled +DEFAULTEXPFEATURES := [] #Default entropy source DEFENTROPYSOURCE := /dev/urandom @@ -314,6 +316,7 @@ USER_VARS += DEFBRIDGES USER_VARS += DEFNETWORKMODEL_FC USER_VARS += DEFNETWORKMODEL_QEMU USER_VARS += DEFDISABLEGUESTSECCOMP +USER_VARS += DEFAULTEXPFEATURES USER_VARS += DEFDISABLEBLOCK USER_VARS += DEFBLOCKSTORAGEDRIVER_FC USER_VARS += DEFBLOCKSTORAGEDRIVER_QEMU @@ -503,6 +506,7 @@ $(GENERATED_FILES): %: %.in Makefile VERSION -e "s|@DEFNETWORKMODEL_FC@|$(DEFNETWORKMODEL_FC)|g" \ -e "s|@DEFNETWORKMODEL_QEMU@|$(DEFNETWORKMODEL_QEMU)|g" \ -e "s|@DEFDISABLEGUESTSECCOMP@|$(DEFDISABLEGUESTSECCOMP)|g" \ + -e "s|@DEFAULTEXPFEATURES@|$(DEFAULTEXPFEATURES)|g" \ -e "s|@DEFDISABLEBLOCK@|$(DEFDISABLEBLOCK)|g" \ -e "s|@DEFBLOCKSTORAGEDRIVER_FC@|$(DEFBLOCKSTORAGEDRIVER_FC)|g" \ -e "s|@DEFBLOCKSTORAGEDRIVER_QEMU@|$(DEFBLOCKSTORAGEDRIVER_QEMU)|g" \ diff --git a/cli/config/configuration-fc.toml.in b/cli/config/configuration-fc.toml.in index 6894b19c1..07dc190b6 100644 --- a/cli/config/configuration-fc.toml.in +++ b/cli/config/configuration-fc.toml.in @@ -289,3 +289,11 @@ disable_guest_seccomp=@DEFDISABLEGUESTSECCOMP@ # If you are using docker, `disable_new_netns` only works with `docker run --net=none` # (default: false) #disable_new_netns = true + +# Enabled experimental feature list, format: ["a", "b"]. +# Experimental features are features not stable enough for production, +# They may break compatibility, and are prepared for a big version bump. +# Supported experimental features: +# 1. "newstore": new persist storage driver +# (default: []) +experimental=@DEFAULTEXPFEATURES@ diff --git a/cli/config/configuration-qemu.toml.in b/cli/config/configuration-qemu.toml.in index f655136bf..52d9a974a 100644 --- a/cli/config/configuration-qemu.toml.in +++ b/cli/config/configuration-qemu.toml.in @@ -336,3 +336,11 @@ disable_guest_seccomp=@DEFDISABLEGUESTSECCOMP@ # If you are using docker, `disable_new_netns` only works with `docker run --net=none` # (default: false) #disable_new_netns = true + +# Enabled experimental feature list, format: ["a", "b"]. +# Experimental features are features not stable enough for production, +# They may break compatibility, and are prepared for a big version bump. +# Supported experimental features: +# 1. "newstore": new persist storage driver +# (default: []) +experimental=@DEFAULTEXPFEATURES@ diff --git a/cli/kata-env.go b/cli/kata-env.go index 8bf2d0576..4a66b1921 100644 --- a/cli/kata-env.go +++ b/cli/kata-env.go @@ -16,6 +16,7 @@ import ( "github.com/BurntSushi/toml" "github.com/kata-containers/runtime/pkg/katautils" vc "github.com/kata-containers/runtime/virtcontainers" + exp "github.com/kata-containers/runtime/virtcontainers/experimental" "github.com/kata-containers/runtime/virtcontainers/pkg/oci" vcUtils "github.com/kata-containers/runtime/virtcontainers/utils" specs "github.com/opencontainers/runtime-spec/specs-go" @@ -69,6 +70,7 @@ type RuntimeInfo struct { Trace bool DisableGuestSeccomp bool DisableNewNetNs bool + Experimental []exp.Feature Path string } @@ -181,6 +183,7 @@ func getRuntimeInfo(configFile string, config oci.RuntimeConfig) RuntimeInfo { Config: runtimeConfig, Path: runtimePath, DisableNewNetNs: config.DisableNewNetNs, + Experimental: config.Experimental, DisableGuestSeccomp: config.DisableGuestSeccomp, } } diff --git a/pkg/katautils/config.go b/pkg/katautils/config.go index a5145ec16..57b617f1b 100644 --- a/pkg/katautils/config.go +++ b/pkg/katautils/config.go @@ -16,6 +16,7 @@ import ( "github.com/BurntSushi/toml" vc "github.com/kata-containers/runtime/virtcontainers" "github.com/kata-containers/runtime/virtcontainers/device/config" + exp "github.com/kata-containers/runtime/virtcontainers/experimental" "github.com/kata-containers/runtime/virtcontainers/pkg/oci" "github.com/kata-containers/runtime/virtcontainers/utils" "github.com/sirupsen/logrus" @@ -122,11 +123,12 @@ type proxy struct { } type runtime struct { - Debug bool `toml:"enable_debug"` - Tracing bool `toml:"enable_tracing"` - DisableNewNetNs bool `toml:"disable_new_netns"` - DisableGuestSeccomp bool `toml:"disable_guest_seccomp"` - InterNetworkModel string `toml:"internetworking_model"` + Debug bool `toml:"enable_debug"` + Tracing bool `toml:"enable_tracing"` + DisableNewNetNs bool `toml:"disable_new_netns"` + DisableGuestSeccomp bool `toml:"disable_guest_seccomp"` + Experimental []string `toml:"experimental"` + InterNetworkModel string `toml:"internetworking_model"` } type shim struct { @@ -800,32 +802,15 @@ func initConfig() (config oci.RuntimeConfig, err error) { // All paths are resolved fully meaning if this function does not return an // error, all paths are valid at the time of the call. func LoadConfiguration(configPath string, ignoreLogging, builtIn bool) (resolvedConfigPath string, config oci.RuntimeConfig, err error) { - var resolved string config, err = initConfig() if err != nil { return "", oci.RuntimeConfig{}, err } - if configPath == "" { - resolved, err = getDefaultConfigFile() - } else { - resolved, err = ResolvePath(configPath) - } - + tomlConf, resolved, err := decodeConfig(configPath) if err != nil { - return "", config, fmt.Errorf("Cannot find usable config file (%v)", err) - } - - configData, err := ioutil.ReadFile(resolved) - if err != nil { - return "", config, err - } - - var tomlConf tomlConfig - _, err = toml.Decode(string(configData), &tomlConf) - if err != nil { - return "", config, err + return "", oci.RuntimeConfig{}, err } config.Debug = tomlConf.Runtime.Debug @@ -872,6 +857,13 @@ func LoadConfiguration(configPath string, ignoreLogging, builtIn bool) (resolved } config.DisableNewNetNs = tomlConf.Runtime.DisableNewNetNs + for _, f := range tomlConf.Runtime.Experimental { + feature := exp.Feature(f) + if !exp.Supported(feature) { + return "", config, fmt.Errorf("Unsupported experimental feature %q", f) + } + config.Experimental = append(config.Experimental, feature) + } if err := checkConfig(config); err != nil { return "", config, err @@ -880,6 +872,36 @@ func LoadConfiguration(configPath string, ignoreLogging, builtIn bool) (resolved return resolved, config, nil } +func decodeConfig(configPath string) (tomlConfig, string, error) { + var ( + resolved string + tomlConf tomlConfig + err error + ) + + if configPath == "" { + resolved, err = getDefaultConfigFile() + } else { + resolved, err = ResolvePath(configPath) + } + + if err != nil { + return tomlConf, "", fmt.Errorf("Cannot find usable config file (%v)", err) + } + + configData, err := ioutil.ReadFile(resolved) + if err != nil { + return tomlConf, resolved, err + } + + _, err = toml.Decode(string(configData), &tomlConf) + if err != nil { + return tomlConf, resolved, err + } + + return tomlConf, resolved, nil +} + // checkConfig checks the validity of the specified config. func checkConfig(config oci.RuntimeConfig) error { if err := checkNetNsConfig(config); err != nil { diff --git a/virtcontainers/experimental/experimental.go b/virtcontainers/experimental/experimental.go new file mode 100644 index 000000000..de208b71c --- /dev/null +++ b/virtcontainers/experimental/experimental.go @@ -0,0 +1,32 @@ +// Copyright (c) 2019 Huawei Corporation +// +// SPDX-License-Identifier: Apache-2.0 +// + +package experimental + +import ( + "fmt" +) + +// Feature to be experimental +type Feature string + +var ( + supportedFeatures = make(map[Feature]struct{}) +) + +// Register register a new experimental feature +func Register(feature Feature) error { + if _, ok := supportedFeatures[feature]; ok { + return fmt.Errorf("Feature %q had been registered before", feature) + } + supportedFeatures[feature] = struct{}{} + return nil +} + +// Supported check if the feature is supported +func Supported(feature Feature) bool { + _, ok := supportedFeatures[feature] + return ok +} diff --git a/virtcontainers/experimental/experimental_test.go b/virtcontainers/experimental/experimental_test.go new file mode 100644 index 000000000..1c24c0738 --- /dev/null +++ b/virtcontainers/experimental/experimental_test.go @@ -0,0 +1,26 @@ +// Copyright (c) 2019 Huawei Corporation +// +// SPDX-License-Identifier: Apache-2.0 +// + +package experimental + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestExperimental(t *testing.T) { + f := Feature("mock") + assert.False(t, Supported(f)) + + err := Register("mock") + assert.Nil(t, err) + + err = Register("mock") + assert.NotNil(t, err) + assert.Equal(t, len(supportedFeatures), 1) + + assert.True(t, Supported(f)) +} diff --git a/virtcontainers/pkg/oci/utils.go b/virtcontainers/pkg/oci/utils.go index 269edc962..cefa8f90f 100644 --- a/virtcontainers/pkg/oci/utils.go +++ b/virtcontainers/pkg/oci/utils.go @@ -23,6 +23,7 @@ import ( vc "github.com/kata-containers/runtime/virtcontainers" "github.com/kata-containers/runtime/virtcontainers/device/config" + exp "github.com/kata-containers/runtime/virtcontainers/experimental" vcAnnotations "github.com/kata-containers/runtime/virtcontainers/pkg/annotations" dockershimAnnotations "github.com/kata-containers/runtime/virtcontainers/pkg/annotations/dockershim" "github.com/kata-containers/runtime/virtcontainers/types" @@ -134,6 +135,9 @@ type RuntimeConfig struct { //Determines if create a netns for hypervisor process DisableNewNetNs bool + + //Experimental features enabled + Experimental []exp.Feature } // AddKernelParam allows the addition of new kernel parameters to an existing @@ -500,6 +504,8 @@ func SandboxConfig(ocispec CompatOCISpec, runtime RuntimeConfig, bundlePath, cid SystemdCgroup: systemdCgroup, DisableGuestSeccomp: runtime.DisableGuestSeccomp, + + Experimental: runtime.Experimental, } addAssetAnnotations(ocispec, &sandboxConfig) diff --git a/virtcontainers/sandbox.go b/virtcontainers/sandbox.go index 630f282f4..851145fb7 100644 --- a/virtcontainers/sandbox.go +++ b/virtcontainers/sandbox.go @@ -25,6 +25,7 @@ import ( "github.com/kata-containers/runtime/virtcontainers/device/config" "github.com/kata-containers/runtime/virtcontainers/device/drivers" deviceManager "github.com/kata-containers/runtime/virtcontainers/device/manager" + exp "github.com/kata-containers/runtime/virtcontainers/experimental" "github.com/kata-containers/runtime/virtcontainers/pkg/annotations" vcTypes "github.com/kata-containers/runtime/virtcontainers/pkg/types" "github.com/kata-containers/runtime/virtcontainers/store" @@ -100,6 +101,9 @@ type SandboxConfig struct { SystemdCgroup bool DisableGuestSeccomp bool + + // Experimental features enabled + Experimental []exp.Feature } func (s *Sandbox) trace(name string) (opentracing.Span, context.Context) { @@ -136,6 +140,12 @@ func (sandboxConfig *SandboxConfig) valid() bool { sandboxConfig.HypervisorType = QemuHypervisor } + // validate experimental features + for _, f := range sandboxConfig.Experimental { + if !exp.Supported(f) { + return false + } + } return true } @@ -439,6 +449,10 @@ func createSandbox(ctx context.Context, sandboxConfig SandboxConfig, factory Fac return nil, err } + if len(s.config.Experimental) != 0 { + s.Logger().WithField("features", s.config.Experimental).Infof("Enable experimental features") + } + // Fetch sandbox network to be able to access it from the sandbox structure. var networkNS NetworkNamespace if err := s.store.Load(store.Network, &networkNS); err == nil { diff --git a/virtcontainers/sandbox_test.go b/virtcontainers/sandbox_test.go index f9d78cce4..45b6f7a23 100644 --- a/virtcontainers/sandbox_test.go +++ b/virtcontainers/sandbox_test.go @@ -22,6 +22,7 @@ import ( "github.com/kata-containers/runtime/virtcontainers/device/config" "github.com/kata-containers/runtime/virtcontainers/device/drivers" "github.com/kata-containers/runtime/virtcontainers/device/manager" + exp "github.com/kata-containers/runtime/virtcontainers/experimental" "github.com/kata-containers/runtime/virtcontainers/pkg/annotations" "github.com/kata-containers/runtime/virtcontainers/store" "github.com/kata-containers/runtime/virtcontainers/types" @@ -1680,3 +1681,18 @@ func TestSandboxUpdateResources(t *testing.T) { t.Fatal(err) } } + +func TestSandboxExperimentalFeature(t *testing.T) { + testFeature := exp.Feature("mock") + sconfig := SandboxConfig{ + ID: testSandboxID, + Experimental: []exp.Feature{testFeature}, + } + + assert.False(t, exp.Supported(testFeature)) + assert.False(t, sconfig.valid()) + + exp.Register(testFeature) + assert.True(t, exp.Supported(testFeature)) + assert.True(t, sconfig.valid()) +} From 111774c859caa2691856df47d40dd1f9549ca9f0 Mon Sep 17 00:00:00 2001 From: Wei Zhang Date: Fri, 22 Feb 2019 14:48:15 +0800 Subject: [PATCH 2/3] config: add docs for experimental Fixes #1226 Add more docs for experimental features. Signed-off-by: Wei Zhang --- cli/config/configuration-fc.toml.in | 3 +- cli/config/configuration-qemu.toml.in | 3 +- virtcontainers/experimental/README.md | 60 +++++++++++++++++++++++++++ 3 files changed, 64 insertions(+), 2 deletions(-) create mode 100644 virtcontainers/experimental/README.md diff --git a/cli/config/configuration-fc.toml.in b/cli/config/configuration-fc.toml.in index 07dc190b6..2c2a14e79 100644 --- a/cli/config/configuration-fc.toml.in +++ b/cli/config/configuration-fc.toml.in @@ -294,6 +294,7 @@ disable_guest_seccomp=@DEFDISABLEGUESTSECCOMP@ # Experimental features are features not stable enough for production, # They may break compatibility, and are prepared for a big version bump. # Supported experimental features: -# 1. "newstore": new persist storage driver +# 1. "newstore": new persist storage driver which breaks backward compatibility, +# expected to move out of experimental in 2.0.0. # (default: []) experimental=@DEFAULTEXPFEATURES@ diff --git a/cli/config/configuration-qemu.toml.in b/cli/config/configuration-qemu.toml.in index 52d9a974a..8c8989b7a 100644 --- a/cli/config/configuration-qemu.toml.in +++ b/cli/config/configuration-qemu.toml.in @@ -341,6 +341,7 @@ disable_guest_seccomp=@DEFDISABLEGUESTSECCOMP@ # Experimental features are features not stable enough for production, # They may break compatibility, and are prepared for a big version bump. # Supported experimental features: -# 1. "newstore": new persist storage driver +# 1. "newstore": new persist storage driver which breaks backward compatibility, +# expected to move out of experimental in 2.0.0. # (default: []) experimental=@DEFAULTEXPFEATURES@ diff --git a/virtcontainers/experimental/README.md b/virtcontainers/experimental/README.md new file mode 100644 index 000000000..378b7e538 --- /dev/null +++ b/virtcontainers/experimental/README.md @@ -0,0 +1,60 @@ +# Experimental package description + +## 1. What are "experimental" features? + +"Experimental" features are features living in master branch, +but Kata community thinks they're not ready for production use. +They are **always disabled** by default in Kata components releases, +and can only be enabled by users when they want to have a try. + +We suggest you **NEVER** enable "experimental" features in production environment, +unless you know what breakage they can bring and have confidence to handle it by youself. + +Criteria of an experimental feature are: + +* the feature breaks backward compatibility + +compatibility is important to Kata Containers, +We will **NEVER** accept any new features/codes which break the backward compatibility of Kata components, +unless they are so important and there's no way to avoid the breakage. +If we decide to involve such changes, maintainers will help to make the decision that which Kata release +it should land. + +Before it's landed as a formal feature, we allow the codes to be merged first into our master with the tag "experimental", +so it can improve in next few releases to be stable enough. + +* the feature is not stable enough currently + +Some features could be big, it adds/changes lots of codes so may need more tests. +Our CI can help guarantee correctness of the feature, but it may not cover all scenarios. +Before we're confident that the feature is ready for production use, +the feature can be marked as "experimental" first, and users can test it manually in their own environment if intested in it. + +## 2. What's the difference between "WIP" and "experimental"? + +"WIP"(work in progress) are usually used to mark the PR as incomplete before the PR can be reviewed and merged, +after the PR finishes its designed purpose(fix bugs, add new features etc) and all CI jobs pass, the codes can be merged into master branch. +After merging, we can still mark this part as "experimental", and leave some space for its evolvement in next several releases. + +In one word, "experimental" can be unstable currently but it **MUST** be complete and functional, thus different from "WIP". + +## 3. When should "experimental" features be moved out from "experimental"? + +That depends. + +For the feature who breaks backward compatibility, we usually land it as formal feature in a major version bump(x in x.y.z, e.g. 2.0.0). +But for a new feature who becomes stable and ready, we can release it formally in any minor version bump. + +Check Kata Container [versioning rules](https://github.com/kata-containers/documentation/blob/c556f1853f2e3df69d336de01ad4bb38e64ecc1b/Releases.md#versioning). + +The experimental feature should state clearly in documentation the rationale for it to be experimental, +and when it is expected to be non-experimental, +so that maintainers can consider to make it formal in right release. + +## 4. Can "experimental" features fail the CI temporarily? + +No. + +"Experimental" features are part of Kata master codes, it should pass all CI jobs or we can't merge them, +that's different from "WIP", a "WIP" PR can fail the CI temporarily before it can be reviewed and merged. + From da80c70c0cee56744e0bf7d2d9ef9a08adfb626f Mon Sep 17 00:00:00 2001 From: Wei Zhang Date: Thu, 28 Feb 2019 16:44:24 +0800 Subject: [PATCH 3/3] config: enhance Feature structure Fixes #1226 Add more fields to better describe an experimental feature. Signed-off-by: Wei Zhang --- cli/kata-env.go | 2 +- pkg/katautils/config.go | 6 +-- virtcontainers/experimental/README.md | 12 +++-- virtcontainers/experimental/experimental.go | 49 +++++++++++++++---- .../experimental/experimental_test.go | 44 +++++++++++++++-- virtcontainers/sandbox.go | 2 +- virtcontainers/sandbox_test.go | 10 ++-- 7 files changed, 100 insertions(+), 25 deletions(-) diff --git a/cli/kata-env.go b/cli/kata-env.go index 4a66b1921..8608bf97d 100644 --- a/cli/kata-env.go +++ b/cli/kata-env.go @@ -27,7 +27,7 @@ import ( // // XXX: Increment for every change to the output format // (meaning any change to the EnvInfo type). -const formatVersion = "1.0.20" +const formatVersion = "1.0.21" // MetaInfo stores information on the format of the output itself type MetaInfo struct { diff --git a/pkg/katautils/config.go b/pkg/katautils/config.go index 57b617f1b..3b114f5c4 100644 --- a/pkg/katautils/config.go +++ b/pkg/katautils/config.go @@ -858,11 +858,11 @@ func LoadConfiguration(configPath string, ignoreLogging, builtIn bool) (resolved config.DisableNewNetNs = tomlConf.Runtime.DisableNewNetNs for _, f := range tomlConf.Runtime.Experimental { - feature := exp.Feature(f) - if !exp.Supported(feature) { + feature := exp.Get(f) + if feature == nil { return "", config, fmt.Errorf("Unsupported experimental feature %q", f) } - config.Experimental = append(config.Experimental, feature) + config.Experimental = append(config.Experimental, *feature) } if err := checkConfig(config); err != nil { diff --git a/virtcontainers/experimental/README.md b/virtcontainers/experimental/README.md index 378b7e538..075eece90 100644 --- a/virtcontainers/experimental/README.md +++ b/virtcontainers/experimental/README.md @@ -8,7 +8,7 @@ They are **always disabled** by default in Kata components releases, and can only be enabled by users when they want to have a try. We suggest you **NEVER** enable "experimental" features in production environment, -unless you know what breakage they can bring and have confidence to handle it by youself. +unless you know what breakage they can bring and have confidence to handle it by yourself. Criteria of an experimental feature are: @@ -28,7 +28,13 @@ so it can improve in next few releases to be stable enough. Some features could be big, it adds/changes lots of codes so may need more tests. Our CI can help guarantee correctness of the feature, but it may not cover all scenarios. Before we're confident that the feature is ready for production use, -the feature can be marked as "experimental" first, and users can test it manually in their own environment if intested in it. +the feature can be marked as "experimental" first, and users can test it manually in their own environment if interested in it. + +We make no guarantees about experimental features, they can be removed entirely at any point, +or become non-experimental at some release, so relative configuration options can change radically. + +An experimental feature **MUST** have a descriptive name containing only lower-case characters, numbers or '_', +e.g. new_hypervisor_2, the name **MUST** be unique and will never be re-used in future. ## 2. What's the difference between "WIP" and "experimental"? @@ -42,7 +48,7 @@ In one word, "experimental" can be unstable currently but it **MUST** be complet That depends. -For the feature who breaks backward compatibility, we usually land it as formal feature in a major version bump(x in x.y.z, e.g. 2.0.0). +For the feature that breaks backward compatibility, we usually land it as formal feature in a major version bump(x in x.y.z, e.g. 2.0.0). But for a new feature who becomes stable and ready, we can release it formally in any minor version bump. Check Kata Container [versioning rules](https://github.com/kata-containers/documentation/blob/c556f1853f2e3df69d336de01ad4bb38e64ecc1b/Releases.md#versioning). diff --git a/virtcontainers/experimental/experimental.go b/virtcontainers/experimental/experimental.go index de208b71c..4204e963e 100644 --- a/virtcontainers/experimental/experimental.go +++ b/virtcontainers/experimental/experimental.go @@ -7,26 +7,57 @@ package experimental import ( "fmt" + "regexp" +) + +const ( + nameRegStr = "^[a-z][a-z0-9_]*$" ) // Feature to be experimental -type Feature string +type Feature struct { + Name string + Description string + // the expected release version to move out from experimental + ExpRelease string +} var ( - supportedFeatures = make(map[Feature]struct{}) + supportedFeatures = make(map[string]Feature) ) // Register register a new experimental feature func Register(feature Feature) error { - if _, ok := supportedFeatures[feature]; ok { - return fmt.Errorf("Feature %q had been registered before", feature) + if err := validateFeature(feature); err != nil { + return err } - supportedFeatures[feature] = struct{}{} + + if _, ok := supportedFeatures[feature.Name]; ok { + return fmt.Errorf("Feature %q had been registered before", feature.Name) + } + supportedFeatures[feature.Name] = feature return nil } -// Supported check if the feature is supported -func Supported(feature Feature) bool { - _, ok := supportedFeatures[feature] - return ok +// Get returns Feature with requested name +func Get(name string) *Feature { + if f, ok := supportedFeatures[name]; ok { + return &f + } + return nil +} + +func validateFeature(feature Feature) error { + if len(feature.Name) == 0 || + len(feature.Description) == 0 || + len(feature.ExpRelease) == 0 { + return fmt.Errorf("experimental feature must have valid name, description and expected release") + } + + reg := regexp.MustCompile(nameRegStr) + if !reg.MatchString(feature.Name) { + return fmt.Errorf("feature name must in the format %q", nameRegStr) + } + + return nil } diff --git a/virtcontainers/experimental/experimental_test.go b/virtcontainers/experimental/experimental_test.go index 1c24c0738..ef6c98813 100644 --- a/virtcontainers/experimental/experimental_test.go +++ b/virtcontainers/experimental/experimental_test.go @@ -12,15 +12,49 @@ import ( ) func TestExperimental(t *testing.T) { - f := Feature("mock") - assert.False(t, Supported(f)) + f := Feature{ + Name: "mock", + Description: "mock experimental feature for test", + ExpRelease: "2.0", + } + assert.Nil(t, Get(f.Name)) - err := Register("mock") + err := Register(f) assert.Nil(t, err) - err = Register("mock") + err = Register(f) assert.NotNil(t, err) assert.Equal(t, len(supportedFeatures), 1) - assert.True(t, Supported(f)) + assert.NotNil(t, Get(f.Name)) +} + +func TestValidateFeature(t *testing.T) { + f := Feature{} + assert.NotNil(t, validateFeature(f)) + + for _, names := range []struct { + name string + valid bool + }{ + {"mock_test_1", true}, + {"m1234ock_test_1", true}, + {"1_mock_test", false}, + {"_mock_test_1", false}, + {"Mock", false}, + {"mock*&", false}, + } { + f := Feature{ + Name: names.name, + Description: "test", + ExpRelease: "2.0", + } + + err := validateFeature(f) + if names.valid { + assert.Nil(t, err) + } else { + assert.NotNil(t, err) + } + } } diff --git a/virtcontainers/sandbox.go b/virtcontainers/sandbox.go index 851145fb7..d39c46cd8 100644 --- a/virtcontainers/sandbox.go +++ b/virtcontainers/sandbox.go @@ -142,7 +142,7 @@ func (sandboxConfig *SandboxConfig) valid() bool { // validate experimental features for _, f := range sandboxConfig.Experimental { - if !exp.Supported(f) { + if exp.Get(f.Name) == nil { return false } } diff --git a/virtcontainers/sandbox_test.go b/virtcontainers/sandbox_test.go index 45b6f7a23..2fbb7908f 100644 --- a/virtcontainers/sandbox_test.go +++ b/virtcontainers/sandbox_test.go @@ -1683,16 +1683,20 @@ func TestSandboxUpdateResources(t *testing.T) { } func TestSandboxExperimentalFeature(t *testing.T) { - testFeature := exp.Feature("mock") + testFeature := exp.Feature{ + Name: "mock", + Description: "exp feature for test", + ExpRelease: "1.8.0", + } sconfig := SandboxConfig{ ID: testSandboxID, Experimental: []exp.Feature{testFeature}, } - assert.False(t, exp.Supported(testFeature)) + assert.Nil(t, exp.Get(testFeature.Name)) assert.False(t, sconfig.valid()) exp.Register(testFeature) - assert.True(t, exp.Supported(testFeature)) + assert.NotNil(t, exp.Get(testFeature.Name)) assert.True(t, sconfig.valid()) }