config: enhance Feature structure

Fixes kata-containers#1226

Add more fields to better describe an experimental feature.

Signed-off-by: Wei Zhang <[email protected]>

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 {

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 {

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).

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
+	}
+
+	if _, ok := supportedFeatures[feature.Name]; ok {
+		return fmt.Errorf("Feature %q had been registered before", feature.Name)
+	}
+	supportedFeatures[feature.Name] = feature
+	return nil
+}
+
+// Get returns Feature with requested name
+func Get(name string) *Feature {
+	if f, ok := supportedFeatures[name]; ok {
+		return &f
 	}
-	supportedFeatures[feature] = struct{}{}
 	return nil
 }
 
-// Supported check if the feature is supported
-func Supported(feature Feature) bool {
-	_, ok := supportedFeatures[feature]
-	return ok
+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
 }

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)
+		}
+	}
 }

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
 		}
 	}

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())
 }