From 12990dec4057847bf6e7e8037c27baa16e58648b Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Mon, 25 Apr 2022 14:41:30 +0200 Subject: [PATCH 1/2] e2e: remove useless SkipUnlessFeatureGateEnabled These SkipUnlessFeatureGateEnabled are useless because: - the tests run in test/e2e where feature gates always have their default state - CSIMigration, SizeMemoryBackedVolumes and ExecProbeTimeout are all enabled by default (beta resp. GA) --- test/e2e/common/node/container_probe.go | 14 -------------- test/e2e/common/storage/empty_dir.go | 4 ---- test/e2e/storage/volume_limits.go | 5 +---- 3 files changed, 1 insertion(+), 22 deletions(-) diff --git a/test/e2e/common/node/container_probe.go b/test/e2e/common/node/container_probe.go index dfb49ce32af..1c1fae5a589 100644 --- a/test/e2e/common/node/container_probe.go +++ b/test/e2e/common/node/container_probe.go @@ -29,12 +29,10 @@ import ( "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/apimachinery/pkg/util/uuid" podutil "k8s.io/kubernetes/pkg/api/v1/pod" - kubefeatures "k8s.io/kubernetes/pkg/features" "k8s.io/kubernetes/pkg/kubelet/events" "k8s.io/kubernetes/test/e2e/framework" e2eevents "k8s.io/kubernetes/test/e2e/framework/events" e2epod "k8s.io/kubernetes/test/e2e/framework/pod" - e2eskipper "k8s.io/kubernetes/test/e2e/framework/skipper" testutils "k8s.io/kubernetes/test/utils" imageutils "k8s.io/kubernetes/test/utils/image" admissionapi "k8s.io/pod-security-admission/api" @@ -220,9 +218,6 @@ var _ = SIGDescribe("Probing container", func() { Description: A Pod is created with liveness probe with a Exec action on the Pod. If the liveness probe call does not return within the timeout specified, liveness probe MUST restart the Pod. */ ginkgo.It("should be restarted with an exec liveness probe with timeout [MinimumKubeletVersion:1.20] [NodeConformance]", func() { - // The ExecProbeTimeout feature gate exists to allow backwards-compatibility with pre-1.20 cluster behaviors, where livenessProbe timeouts were ignored - // If ExecProbeTimeout feature gate is disabled, timeout enforcement for exec livenessProbes is disabled, so we should skip this test - e2eskipper.SkipUnlessFeatureGateEnabled(kubefeatures.ExecProbeTimeout) cmd := []string{"/bin/sh", "-c", "sleep 600"} livenessProbe := &v1.Probe{ ProbeHandler: execHandler([]string{"/bin/sh", "-c", "sleep 10"}), @@ -240,10 +235,6 @@ var _ = SIGDescribe("Probing container", func() { Description: A Pod is created with readiness probe with a Exec action on the Pod. If the readiness probe call does not return within the timeout specified, readiness probe MUST not be Ready. */ ginkgo.It("should not be ready with an exec readiness probe timeout [MinimumKubeletVersion:1.20] [NodeConformance]", func() { - // The ExecProbeTimeout feature gate exists to allow backwards-compatibility with pre-1.20 cluster behaviors, where readiness probe timeouts were ignored - // If ExecProbeTimeout feature gate is disabled, timeout enforcement for exec readiness probe is disabled, so we should skip this test - e2eskipper.SkipUnlessFeatureGateEnabled(kubefeatures.ExecProbeTimeout) - cmd := []string{"/bin/sh", "-c", "sleep 600"} readinessProbe := &v1.Probe{ ProbeHandler: execHandler([]string{"/bin/sh", "-c", "sleep 10"}), @@ -261,11 +252,6 @@ var _ = SIGDescribe("Probing container", func() { Description: A Pod is created with liveness probe with a Exec action on the Pod. If the liveness probe call does not return within the timeout specified, liveness probe MUST restart the Pod. When ExecProbeTimeout feature gate is disabled and cluster is using dockershim, the timeout is ignored BUT a failing liveness probe MUST restart the Pod. */ ginkgo.It("should be restarted with a failing exec liveness probe that took longer than the timeout", func() { - // The ExecProbeTimeout feature gate exists to allow backwards-compatibility with pre-1.20 cluster behaviors using dockershim, where livenessProbe timeouts were ignored - // If ExecProbeTimeout feature gate is disabled on a dockershim cluster, timeout enforcement for exec livenessProbes is disabled, but a failing liveness probe MUST still trigger a restart - // Note ExecProbeTimeout=false is not recommended for non-dockershim clusters (e.g., containerd), and this test will fail if run against such a configuration - e2eskipper.SkipUnlessFeatureGateEnabled(kubefeatures.ExecProbeTimeout) - cmd := []string{"/bin/sh", "-c", "sleep 600"} livenessProbe := &v1.Probe{ ProbeHandler: execHandler([]string{"/bin/sh", "-c", "sleep 10 & exit 1"}), diff --git a/test/e2e/common/storage/empty_dir.go b/test/e2e/common/storage/empty_dir.go index e0cdcea2406..5d90524398c 100644 --- a/test/e2e/common/storage/empty_dir.go +++ b/test/e2e/common/storage/empty_dir.go @@ -26,7 +26,6 @@ import ( "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/uuid" - "k8s.io/kubernetes/pkg/features" "k8s.io/kubernetes/test/e2e/framework" e2epod "k8s.io/kubernetes/test/e2e/framework/pod" e2eskipper "k8s.io/kubernetes/test/e2e/framework/skipper" @@ -297,9 +296,6 @@ var _ = SIGDescribe("EmptyDir volumes", func() { Description: A Pod created with an 'emptyDir' Volume backed by memory should be sized to user provided value. */ ginkgo.It("pod should support memory backed volumes of specified size", func() { - // skip if feature gate is not enabled, this could be elevated to conformance in future if on Linux. - e2eskipper.SkipUnlessFeatureGateEnabled(features.SizeMemoryBackedVolumes) - var ( volumeName = "shared-data" busyBoxMainVolumeMountPath = "/usr/share/volumeshare" diff --git a/test/e2e/storage/volume_limits.go b/test/e2e/storage/volume_limits.go index 8d87c2c182b..4614507d56e 100644 --- a/test/e2e/storage/volume_limits.go +++ b/test/e2e/storage/volume_limits.go @@ -18,10 +18,9 @@ package storage import ( "github.com/onsi/ginkgo" - "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" clientset "k8s.io/client-go/kubernetes" v1helper "k8s.io/kubernetes/pkg/apis/core/v1/helper" - kubefeatures "k8s.io/kubernetes/pkg/features" "k8s.io/kubernetes/test/e2e/framework" e2enode "k8s.io/kubernetes/test/e2e/framework/node" e2eskipper "k8s.io/kubernetes/test/e2e/framework/skipper" @@ -37,8 +36,6 @@ var _ = utils.SIGDescribe("Volume limits", func() { f.NamespacePodSecurityEnforceLevel = admissionapi.LevelPrivileged ginkgo.BeforeEach(func() { e2eskipper.SkipUnlessProviderIs("aws", "gce", "gke") - // If CSIMigration is enabled, then the limits should be on CSINodes, not Nodes, and another test checks this - e2eskipper.SkipIfFeatureGateEnabled(kubefeatures.CSIMigration) c = f.ClientSet framework.ExpectNoError(framework.WaitForAllNodesSchedulable(c, framework.TestContext.NodeSchedulableTimeout)) }) From 2664740043ac30845d6062d4c69eb6a29ee52375 Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Mon, 25 Apr 2022 13:21:57 +0200 Subject: [PATCH 2/2] e2e: move feature gate support from test/e2e to test/e2e_node The test/e2e suite has never supported feature gates: - it cannot discover at runtime how the cluster is configured - its --feature-gates parameter had no effect Despite that, tests were written that used e2eskipper.SkipUnlessFeatureGateEnabled even though that function then only checked the default feature gate state. To catch such mistakes, e2e tests suites now must explicitly enable feature gate checking via e2eskipper.InitFeatureGates. They also must register their own command line flag. When that is not done, then using SkipUnlessFeatureGateEnabled or SkipIfFeatureGateEnabled leads to a test failure. test/e2e_node does both and therefore continues to work as before. --- test/e2e/framework/skipper/skipper.go | 39 +++++++++++++++++++++++---- test/e2e/framework/test_context.go | 3 --- test/e2e_node/e2e_node_suite_test.go | 13 +++++++-- test/e2e_node/services/kubelet.go | 18 +++++-------- test/e2e_node/services/services.go | 10 ++----- 5 files changed, 53 insertions(+), 30 deletions(-) diff --git a/test/e2e/framework/skipper/skipper.go b/test/e2e/framework/skipper/skipper.go index 00805f9e97f..2cb50bb49be 100644 --- a/test/e2e/framework/skipper/skipper.go +++ b/test/e2e/framework/skipper/skipper.go @@ -33,7 +33,6 @@ import ( "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime/schema" utilversion "k8s.io/apimachinery/pkg/util/version" - utilfeature "k8s.io/apiserver/pkg/util/feature" "k8s.io/client-go/discovery" "k8s.io/client-go/dynamic" clientset "k8s.io/client-go/kubernetes" @@ -128,16 +127,46 @@ func SkipUnlessAtLeast(value int, minValue int, message string) { } } -// SkipUnlessFeatureGateEnabled skips if the feature is disabled +var featureGate featuregate.FeatureGate + +// InitFeatureGates must be called in test suites that have a --feature-gates parameter. +// If not called, SkipUnlessFeatureGateEnabled and SkipIfFeatureGateEnabled will +// record a test failure. +func InitFeatureGates(defaults featuregate.FeatureGate, overrides map[string]bool) error { + clone := defaults.DeepCopy() + if err := clone.SetFromMap(overrides); err != nil { + return err + } + featureGate = clone + return nil +} + +// SkipUnlessFeatureGateEnabled skips if the feature is disabled. +// +// Beware that this only works in test suites that have a --feature-gate +// parameter and call InitFeatureGates. In test/e2e, the `Feature: XYZ` tag +// has to be used instead and invocations have to make sure that they +// only run tests that work with the given test cluster. func SkipUnlessFeatureGateEnabled(gate featuregate.Feature) { - if !utilfeature.DefaultFeatureGate.Enabled(gate) { + if featureGate == nil { + framework.Failf("Feature gate checking is not enabled, don't use SkipUnlessFeatureGateEnabled(%v). Instead use the Feature tag.", gate) + } + if !featureGate.Enabled(gate) { skipInternalf(1, "Only supported when %v feature is enabled", gate) } } -// SkipIfFeatureGateEnabled skips if the feature is enabled +// SkipIfFeatureGateEnabled skips if the feature is enabled. +// +// Beware that this only works in test suites that have a --feature-gate +// parameter and call InitFeatureGates. In test/e2e, the `Feature: XYZ` tag +// has to be used instead and invocations have to make sure that they +// only run tests that work with the given test cluster. func SkipIfFeatureGateEnabled(gate featuregate.Feature) { - if utilfeature.DefaultFeatureGate.Enabled(gate) { + if featureGate == nil { + framework.Failf("Feature gate checking is not enabled, don't use SkipFeatureGateEnabled(%v). Instead use the Feature tag.", gate) + } + if featureGate.Enabled(gate) { skipInternalf(1, "Only supported when %v feature is disabled", gate) } } diff --git a/test/e2e/framework/test_context.go b/test/e2e/framework/test_context.go index b4c4743b6ea..114efbf990c 100644 --- a/test/e2e/framework/test_context.go +++ b/test/e2e/framework/test_context.go @@ -150,8 +150,6 @@ type TestContextType struct { DisableLogDump bool // Path to the GCS artifacts directory to dump logs from nodes. Logexporter gets enabled if this is non-empty. LogexporterGCSPath string - // featureGates is a map of feature names to bools that enable or disable alpha/experimental features. - FeatureGates map[string]bool // Node e2e specific test context NodeTestContextType @@ -304,7 +302,6 @@ func RegisterCommonFlags(flags *flag.FlagSet) { flags.StringVar(&TestContext.Host, "host", "", fmt.Sprintf("The host, or apiserver, to connect to. Will default to %s if this argument and --kubeconfig are not set.", defaultHost)) flags.StringVar(&TestContext.ReportPrefix, "report-prefix", "", "Optional prefix for JUnit XML reports. Default is empty, which doesn't prepend anything to the default name.") flags.StringVar(&TestContext.ReportDir, "report-dir", "", "Path to the directory where the JUnit XML reports should be saved. Default is empty, which doesn't generate these reports.") - flags.Var(cliflag.NewMapStringBool(&TestContext.FeatureGates), "feature-gates", "A set of key=value pairs that describe feature gates for alpha/experimental features.") flags.StringVar(&TestContext.ContainerRuntimeEndpoint, "container-runtime-endpoint", "unix:///var/run/containerd/containerd.sock", "The container runtime endpoint of cluster VM instances.") flags.StringVar(&TestContext.ContainerRuntimeProcessName, "container-runtime-process-name", "dockerd", "The name of the container runtime process.") flags.StringVar(&TestContext.ContainerRuntimePidFile, "container-runtime-pid-file", "/var/run/docker.pid", "The pid file of the container runtime.") diff --git a/test/e2e_node/e2e_node_suite_test.go b/test/e2e_node/e2e_node_suite_test.go index 2a68fd4e306..b8ebf3667e6 100644 --- a/test/e2e_node/e2e_node_suite_test.go +++ b/test/e2e_node/e2e_node_suite_test.go @@ -39,6 +39,7 @@ import ( v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" utilyaml "k8s.io/apimachinery/pkg/util/yaml" + utilfeature "k8s.io/apiserver/pkg/util/feature" clientset "k8s.io/client-go/kubernetes" cliflag "k8s.io/component-base/cli/flag" "k8s.io/component-base/logs" @@ -46,6 +47,7 @@ import ( commontest "k8s.io/kubernetes/test/e2e/common" "k8s.io/kubernetes/test/e2e/framework" e2econfig "k8s.io/kubernetes/test/e2e/framework/config" + e2eskipper "k8s.io/kubernetes/test/e2e/framework/skipper" e2etestfiles "k8s.io/kubernetes/test/e2e/framework/testfiles" e2etestingmanifests "k8s.io/kubernetes/test/e2e/testing-manifests" "k8s.io/kubernetes/test/e2e_node/services" @@ -62,6 +64,8 @@ import ( var ( e2es *services.E2EServices + // featureGates is a map of feature names to bools that enable or disable alpha/experimental features. + featureGates map[string]bool // TODO(random-liu): Change the following modes to sub-command. runServicesMode = flag.Bool("run-services-mode", false, "If true, only run services (etcd, apiserver) in current process, and not run test.") @@ -92,6 +96,7 @@ func registerNodeFlags(flags *flag.FlagSet) { flag.StringVar(&framework.TestContext.ClusterDNSDomain, "dns-domain", "", "The DNS Domain of the cluster.") flag.Var(cliflag.NewMapStringString(&framework.TestContext.RuntimeConfig), "runtime-config", "The runtime configuration used on node e2e tests.") flags.BoolVar(&framework.TestContext.RequireDevices, "require-devices", false, "If true, require device plugins to be installed in the running environment.") + flags.Var(cliflag.NewMapStringBool(&featureGates), "feature-gates", "A set of key=value pairs that describe feature gates for alpha/experimental features.") } func init() { @@ -118,6 +123,10 @@ func TestMain(m *testing.M) { rand.Seed(time.Now().UnixNano()) pflag.Parse() framework.AfterReadingAllFlags(&framework.TestContext) + if err := e2eskipper.InitFeatureGates(utilfeature.DefaultFeatureGate, featureGates); err != nil { + fmt.Fprintf(os.Stderr, "ERROR: initialize feature gates: %v", err) + os.Exit(1) + } setExtraEnvs() os.Exit(m.Run()) } @@ -140,7 +149,7 @@ func TestE2eNode(t *testing.T) { } if *runKubeletMode { // If run-kubelet-mode is specified, only start kubelet. - services.RunKubelet() + services.RunKubelet(featureGates) return } if *systemValidateMode { @@ -209,7 +218,7 @@ var _ = ginkgo.SynchronizedBeforeSuite(func() []byte { // If the services are expected to stop after test, they should monitor the test process. // If the services are expected to keep running after test, they should not monitor the test process. e2es = services.NewE2EServices(*stopServices) - gomega.Expect(e2es.Start()).To(gomega.Succeed(), "should be able to start node services.") + gomega.Expect(e2es.Start(featureGates)).To(gomega.Succeed(), "should be able to start node services.") } else { klog.Infof("Running tests without starting services.") } diff --git a/test/e2e_node/services/kubelet.go b/test/e2e_node/services/kubelet.go index 8bcd854748e..69f6602829c 100644 --- a/test/e2e_node/services/kubelet.go +++ b/test/e2e_node/services/kubelet.go @@ -27,7 +27,6 @@ import ( "strings" "time" - utilfeature "k8s.io/apiserver/pkg/util/feature" cliflag "k8s.io/component-base/cli/flag" "k8s.io/klog/v2" kubeletconfigv1beta1 "k8s.io/kubelet/config/v1beta1" @@ -73,13 +72,13 @@ func init() { // RunKubelet starts kubelet and waits for termination signal. Once receives the // termination signal, it will stop the kubelet gracefully. -func RunKubelet() { +func RunKubelet(featureGates map[string]bool) { var err error // Enable monitorParent to make sure kubelet will receive termination signal // when test process exits. e := NewE2EServices(true /* monitorParent */) defer e.Stop() - e.kubelet, err = e.startKubelet() + e.kubelet, err = e.startKubelet(featureGates) if err != nil { klog.Fatalf("Failed to start kubelet: %v", err) } @@ -152,14 +151,9 @@ func baseKubeConfiguration(cfgPath string) (*kubeletconfig.KubeletConfiguration, // startKubelet starts the Kubelet in a separate process or returns an error // if the Kubelet fails to start. -func (e *E2EServices) startKubelet() (*server, error) { +func (e *E2EServices) startKubelet(featureGates map[string]bool) (*server, error) { klog.Info("Starting kubelet") - // set feature gates so we can check which features are enabled and pass the appropriate flags - if err := utilfeature.DefaultMutableFeatureGate.SetFromMap(framework.TestContext.FeatureGates); err != nil { - return nil, err - } - // Build kubeconfig kubeconfigPath, err := createKubeconfigCWD() if err != nil { @@ -264,9 +258,9 @@ func (e *E2EServices) startKubelet() (*server, error) { // Apply test framework feature gates by default. This could also be overridden // by kubelet-flags. - if len(framework.TestContext.FeatureGates) > 0 { - cmdArgs = append(cmdArgs, "--feature-gates", cliflag.NewMapStringBool(&framework.TestContext.FeatureGates).String()) - kc.FeatureGates = framework.TestContext.FeatureGates + if len(featureGates) > 0 { + cmdArgs = append(cmdArgs, "--feature-gates", cliflag.NewMapStringBool(&featureGates).String()) + kc.FeatureGates = featureGates } // Keep hostname override for convenience. diff --git a/test/e2e_node/services/services.go b/test/e2e_node/services/services.go index a4aa5949393..f2a111137d7 100644 --- a/test/e2e_node/services/services.go +++ b/test/e2e_node/services/services.go @@ -23,7 +23,6 @@ import ( "path" "testing" - utilfeature "k8s.io/apiserver/pkg/util/feature" "k8s.io/klog/v2" "k8s.io/kubernetes/test/e2e/framework" @@ -61,7 +60,7 @@ func NewE2EServices(monitorParent bool) *E2EServices { // namespace controller. // * kubelet: kubelet binary is outside. (We plan to move main kubelet start logic out when we have // standard kubelet launcher) -func (e *E2EServices) Start() error { +func (e *E2EServices) Start(featureGates map[string]bool) error { var err error if e.services, err = e.startInternalServices(); err != nil { return fmt.Errorf("failed to start internal services: %v", err) @@ -72,7 +71,7 @@ func (e *E2EServices) Start() error { klog.Info("nothing to do in node-e2e-services, running conformance suite") } else { // Start kubelet - e.kubelet, err = e.startKubelet() + e.kubelet, err = e.startKubelet(featureGates) if err != nil { return fmt.Errorf("failed to start kubelet: %v", err) } @@ -110,11 +109,6 @@ func (e *E2EServices) Stop() { // RunE2EServices actually start the e2e services. This function is used to // start e2e services in current process. This is only used in run-services-mode. func RunE2EServices(t *testing.T) { - // Populate global DefaultFeatureGate with value from TestContext.FeatureGates. - // This way, statically-linked components see the same feature gate config as the test context. - if err := utilfeature.DefaultMutableFeatureGate.SetFromMap(framework.TestContext.FeatureGates); err != nil { - t.Fatal(err) - } e := newE2EServices() if err := e.run(t); err != nil { klog.Fatalf("Failed to run e2e services: %v", err)