Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Multivolume storage class fix #77605

Merged
merged 2 commits into from
May 9, 2019
Merged

Multivolume storage class fix #77605

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
62c5c63
Revert "Make external driver storage class name generation contain a …
pohly May 8, 2019
093027c
e2e/storage: ensure that also external storage classes are unique
pohly May 8, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 0 additions & 1 deletion test/e2e/framework/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
52 changes: 21 additions & 31 deletions test/e2e/framework/create.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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()
}
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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)
}
Expand Down
1 change: 1 addition & 0 deletions test/e2e/storage/external/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
4 changes: 4 additions & 0 deletions test/e2e/storage/external/external.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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{}
Expand Down
3 changes: 3 additions & 0 deletions test/e2e/storage/testsuites/testdriver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, I got curious why we now have this requirement. It found that it was introduced two months ago when implementing the multi-attach test cases, but then wasn't treated as the API change for the TestDriver interface but rather got handled only in some of its implementations (those that use GetStorageClass):
b4c88ac

// 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
Expand Down