From 62c5c6345e1fea1bdf7d24e0b28cdb1498f2fc48 Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Wed, 8 May 2019 17:28:13 +0200 Subject: [PATCH 1/2] Revert "Make external driver storage class name generation contain a more random suffix in case of double generation in the same framework context (twice in the same test)" This reverts commit c50e7fd30155d05fdde4174169945a43e26cf867 because it included API changes that shouldn't have been in that PR and fixing the storage class conflict inside the framework is probably the wrong place. --- test/e2e/framework/BUILD | 1 - test/e2e/framework/create.go | 52 +++++++++++++++--------------------- 2 files changed, 21 insertions(+), 32 deletions(-) diff --git a/test/e2e/framework/BUILD b/test/e2e/framework/BUILD index fe67cc58134..6d317ac5f7b 100644 --- a/test/e2e/framework/BUILD +++ b/test/e2e/framework/BUILD @@ -92,7 +92,6 @@ go_library( "//staging/src/k8s.io/apimachinery/pkg/util/yaml:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/watch:go_default_library", "//staging/src/k8s.io/apiserver/pkg/authentication/serviceaccount:go_default_library", - "//staging/src/k8s.io/apiserver/pkg/storage/names:go_default_library", "//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library", "//staging/src/k8s.io/client-go/discovery:go_default_library", "//staging/src/k8s.io/client-go/discovery/cached/memory:go_default_library", diff --git a/test/e2e/framework/create.go b/test/e2e/framework/create.go index c250bc1b3dc..bd22e0f646c 100644 --- a/test/e2e/framework/create.go +++ b/test/e2e/framework/create.go @@ -31,7 +31,6 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" - "k8s.io/apiserver/pkg/storage/names" "k8s.io/client-go/kubernetes/scheme" "k8s.io/client-go/tools/cache" "k8s.io/kubernetes/test/e2e/framework/testfiles" @@ -101,9 +100,9 @@ func visitManifests(cb func([]byte) error, files ...string) error { return nil } -// PatchItems modifies the given items in place such that each -// test gets its own instances, to avoid conflicts between different tests and -// between tests and normal deployments. +// PatchItems modifies the given items in place such that each test +// gets its own instances, to avoid conflicts between different tests +// and between tests and normal deployments. // // This is done by: // - creating namespaced items inside the test's namespace @@ -288,27 +287,18 @@ var factories = map[What]ItemFactory{ {"StorageClass"}: &storageClassFactory{}, } -// uniquifyName makes the name of some item unique per namespace by appending the -// generated unique name of the test namespace. -func (f *Framework) uniquifyName(item *string) { +// PatchName makes the name of some item unique by appending the +// generated unique name. +func (f *Framework) PatchName(item *string) { if *item != "" { *item = *item + "-" + f.UniqueName } } -// randomizeStorageClassName makes the name of the storage class unique per call -// by appending the generated unique name of the test namespace and a random 5 -// character string -func (f *Framework) randomizeStorageClassName(item *string) { - if *item != "" { - *item = names.SimpleNameGenerator.GenerateName(*item + "-" + f.UniqueName + "-") - } -} - -// patchNamespace moves the item into the test's namespace. Not +// PatchNamespace moves the item into the test's namespace. Not // all items can be namespaced. For those, the name also needs to be // patched. -func (f *Framework) patchNamespace(item *string) { +func (f *Framework) PatchNamespace(item *string) { if f.Namespace != nil { *item = f.Namespace.GetName() } @@ -317,31 +307,31 @@ func (f *Framework) patchNamespace(item *string) { func (f *Framework) patchItemRecursively(item interface{}) error { switch item := item.(type) { case *rbac.Subject: - f.patchNamespace(&item.Namespace) + f.PatchNamespace(&item.Namespace) case *rbac.RoleRef: // TODO: avoid hard-coding this special name. Perhaps add a Framework.PredefinedRoles // which contains all role names that are defined cluster-wide before the test starts? // All those names are excempt from renaming. That list could be populated by querying // and get extended by tests. if item.Name != "e2e-test-privileged-psp" { - f.uniquifyName(&item.Name) + f.PatchName(&item.Name) } case *rbac.ClusterRole: - f.uniquifyName(&item.Name) + f.PatchName(&item.Name) case *rbac.Role: - f.patchNamespace(&item.Namespace) + f.PatchNamespace(&item.Namespace) // Roles are namespaced, but because for RoleRef above we don't // know whether the referenced role is a ClusterRole or Role // and therefore always renames, we have to do the same here. - f.uniquifyName(&item.Name) + f.PatchName(&item.Name) case *storage.StorageClass: - f.randomizeStorageClassName(&item.Name) + f.PatchName(&item.Name) case *v1.ServiceAccount: - f.patchNamespace(&item.ObjectMeta.Namespace) + f.PatchNamespace(&item.ObjectMeta.Namespace) case *v1.Secret: - f.patchNamespace(&item.ObjectMeta.Namespace) + f.PatchNamespace(&item.ObjectMeta.Namespace) case *rbac.ClusterRoleBinding: - f.uniquifyName(&item.Name) + f.PatchName(&item.Name) for i := range item.Subjects { if err := f.patchItemRecursively(&item.Subjects[i]); err != nil { return errors.Wrapf(err, "%T", f) @@ -351,7 +341,7 @@ func (f *Framework) patchItemRecursively(item interface{}) error { return errors.Wrapf(err, "%T", f) } case *rbac.RoleBinding: - f.patchNamespace(&item.Namespace) + f.PatchNamespace(&item.Namespace) for i := range item.Subjects { if err := f.patchItemRecursively(&item.Subjects[i]); err != nil { return errors.Wrapf(err, "%T", f) @@ -361,11 +351,11 @@ func (f *Framework) patchItemRecursively(item interface{}) error { return errors.Wrapf(err, "%T", f) } case *v1.Service: - f.patchNamespace(&item.ObjectMeta.Namespace) + f.PatchNamespace(&item.ObjectMeta.Namespace) case *apps.StatefulSet: - f.patchNamespace(&item.ObjectMeta.Namespace) + f.PatchNamespace(&item.ObjectMeta.Namespace) case *apps.DaemonSet: - f.patchNamespace(&item.ObjectMeta.Namespace) + f.PatchNamespace(&item.ObjectMeta.Namespace) default: return errors.Errorf("missing support for patching item of type %T", item) } From 093027c8914b19754af3ece5a83899376efae4b8 Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Wed, 8 May 2019 17:58:58 +0200 Subject: [PATCH 2/2] e2e/storage: ensure that also external storage classes are unique One previously undocumented expectation is that GetDynamicProvisionStorageClass can be called more than once per test and then each time returns a new, unique storage class. The in-memory implementation in driveroperations.go:GetStorageClass ensured that, but loading from a .yaml file didn't. This caused the multivolume tests to fail when applied to an already installed GCE driver with the -storage.testdriver parameter. --- test/e2e/storage/external/BUILD | 1 + test/e2e/storage/external/external.go | 4 ++++ test/e2e/storage/testsuites/testdriver.go | 3 +++ 3 files changed, 8 insertions(+) diff --git a/test/e2e/storage/external/BUILD b/test/e2e/storage/external/BUILD index 603442833be..3112e6c98e4 100644 --- a/test/e2e/storage/external/BUILD +++ b/test/e2e/storage/external/BUILD @@ -11,6 +11,7 @@ go_library( "//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/runtime/schema:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/sets:go_default_library", + "//staging/src/k8s.io/apiserver/pkg/storage/names:go_default_library", "//staging/src/k8s.io/client-go/kubernetes/scheme:go_default_library", "//test/e2e/framework:go_default_library", "//test/e2e/storage/testpatterns:go_default_library", diff --git a/test/e2e/storage/external/external.go b/test/e2e/storage/external/external.go index d06c0ccf7a4..a539a54f347 100644 --- a/test/e2e/storage/external/external.go +++ b/test/e2e/storage/external/external.go @@ -27,6 +27,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/util/sets" + "k8s.io/apiserver/pkg/storage/names" "k8s.io/client-go/kubernetes/scheme" "k8s.io/kubernetes/test/e2e/framework" "k8s.io/kubernetes/test/e2e/storage/testpatterns" @@ -242,6 +243,9 @@ func (d *driverDefinition) GetDynamicProvisionStorageClass(config *testsuites.Pe sc, ok := items[0].(*storagev1.StorageClass) gomega.Expect(ok).To(gomega.BeTrue(), "storage class from %s", d.StorageClass.FromFile) + // Ensure that we can load more than once as required for + // GetDynamicProvisionStorageClass by adding a random suffix. + sc.Name = names.SimpleNameGenerator.GenerateName(sc.Name + "-") if fsType != "" { if sc.Parameters == nil { sc.Parameters = map[string]string{} diff --git a/test/e2e/storage/testsuites/testdriver.go b/test/e2e/storage/testsuites/testdriver.go index 3736390b747..b7dc23b2b19 100644 --- a/test/e2e/storage/testsuites/testdriver.go +++ b/test/e2e/storage/testsuites/testdriver.go @@ -88,6 +88,9 @@ type PreprovisionedPVTestDriver interface { type DynamicPVTestDriver interface { TestDriver // GetDynamicProvisionStorageClass returns a StorageClass dynamic provision Persistent Volume. + // The StorageClass must be created in the current test's namespace and have + // a unique name inside that namespace because GetDynamicProvisionStorageClass might + // be called more than once per test. // It will set fsType to the StorageClass, if TestDriver supports it. // It will return nil, if the TestDriver doesn't support it. GetDynamicProvisionStorageClass(config *PerTestConfig, fsType string) *storagev1.StorageClass