Make external driver name generation contain a more random suffix in case of double generation in the same framework context (twice in the same test) #77525

davidz627 · 2019-05-07T00:28:10Z

Fixes multivolume tests for external drivers.

/kind bug
/kind failing-test
/sig storage

NONE

davidz627 · 2019-05-07T00:54:31Z

Ran the multivolume tests with this fix with the external GCE PD CSI Driver and confirmed this fixes the issue

msau42 · 2019-05-07T00:59:38Z

/lgtm
/approve

fejta-bot · 2019-05-07T05:03:32Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

msau42 · 2019-05-07T06:07:15Z

/lgtm cancel

pohly · 2019-05-07T08:00:27Z

PatchItems was named this way because I wanted to have the ability to add other changes later without having to rename the function each time it gains a new feature. Renaming the function now is an API change that potentially can break users of the framework. I'd prefer to not rename it.

Regarding the actual change: the name space is already random. Can you explain in a bit more detail (ideally in the commit message) what problem you are trying to solve? It's not clear to me from the given information, and it looks like the fix isn't right either because other tests are now failing (or was that due to something unrelated?).

davidz627 · 2019-05-07T17:34:05Z

PatchItems was named this way because I wanted to have the ability to add other changes later without having to rename the function each time it gains a new feature. Renaming the function now is an API change that potentially can break users of the framework. I'd prefer to not rename it.

Thats fair; however, patch items means absolutely nothing and was one of the reasons why it took me longer than it should have to debug this issue. Unless you are already an expert in this code its almost impossible to know what it's doing and how to find the place where the name is being generated.

Also lets not design things for a potential future use case that we haven't thought of or needed. This name describes what the function currently does and it is not costly to be renamed if it needs to be refactored/rewritten later.

Regarding the actual change: the name space is already random. Can you explain in a bit more detail (ideally in the commit message) what problem you are trying to solve? It's not clear to me from the given information, and it looks like the fix isn't right either because other tests are now failing (or was that due to something unrelated?).

The multiVolume tests run createGenericTestResource twice, thus creating two storage classes in the same test framework. The namespace name is stable in the context of a test so the two storage classes are created with the same name and the tests fail. As for unrelated failing tests, I'm looking into them now but on first glance it looks like the CSI tests were inferring generated names for cluster role yamls and not using the actual generated names.

davidz627 · 2019-05-07T17:58:57Z

/retest

pohly · 2019-05-07T18:07:39Z

David Zhu <notifications@github.com> writes:

This name describes what the function currently does and it is not costly to be renamed if it needs to be refactored/rewritten later.

Changing the public API of the E2E framework is costly. Everyone using it has to make the same change. If you feel strongly about this, then please submit a separate cleanup PR.

> Regarding the actual change: the name space is already random. Can you explain in a bit more detail (ideally in the commit message) what problem you are trying to solve? It's not clear to me from the given information, and it looks like the fix isn't right either because other tests are now failing (or was that due to something unrelated?). The `multiVolume` tests run `createGenericTestResource` twice, thus creating two storage classes in the same test framework. The namespace name is stable in the context of a test so the two storage classes are created with the same name and the tests fail. As for unrelated failing tests, I'm looking into them now but on first glance it looks like the CSI tests were inferring generated names for cluster role yamls and not using the actual generated names.

It sounds a bit like it might better to handle this special case in the multiVolume test by explicitly renaming the storage classes, and just the storage classes. Anyway, I'll let you investigate this further.

davidz627 · 2019-05-07T18:09:09Z

/priority important-soon
resolved test failures

davidz627 · 2019-05-07T18:19:05Z

Changing the public API of the E2E framework is costly. Everyone using it has to make the same change. If you feel strongly about this, then please submit a separate cleanup PR.

I would argue that changing the function name when functionality changes forces you to think about whether this functionality change works for all function callers. If we are exporting this function as a public API, shouldn't the returned result (the modified object since this modifies objects in-place) be stable as well? If you are thinking about other callers using this function doesn't keeping an ambiguous function name PatchItem and "have the ability to add other changes later without having to rename the function each time it gains a new feature," explicitly break all API rules by being ambiguous and changing behavior under consumers feet.

It sounds a bit like it might better to handle this special case in the multiVolume test by explicitly renaming the storage classes, and just the storage classes. Anyway, I'll let you investigate this further.

Done

msau42 · 2019-05-07T18:39:40Z

Are we officially supporting the Kubernetes e2e test framework as a publicly consumable library outside of kubernetes/kubernetes?

pohly · 2019-05-07T19:26:21Z

Michelle Au <notifications@github.com> writes:

Are we officially supporting the Kubernetes e2e test framework as a publicly consumable library outside of kubernetes/kubernetes?

Yes, it is consumable externally. But as it is part of k/k, there's no API stability guarantee. I'm just pointing out that changing the APIs is not quite as cheap as it was said to be. What I do object against is making an API change together with a functional change in a single PR. These are two different changes and should be discussed separately.

…dom suffix in case of double generation in the same framework context (twice in the same test)

davidz627 · 2019-05-07T21:38:09Z

@pohly removed the function name change

k8s-ci-robot · 2019-05-07T21:38:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: davidz627, msau42

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~test/e2e/framework/OWNERS~~ [msau42]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

msau42 · 2019-05-07T21:41:29Z

/lgtm

msau42 · 2019-05-07T21:42:13Z

/test pull-kubernetes-e2e-gce-storage-slow
/test pull-kubernetes-e2e-gce-csi-serial

davidz627 · 2019-05-08T00:46:51Z

/retest

fejta-bot · 2019-05-08T04:50:41Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

k8s-ci-robot · 2019-05-08T07:10:38Z

@davidz627: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-gce-storage-slow	`c50e7fd`	link	`/test pull-kubernetes-e2e-gce-storage-slow`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

pohly · 2019-05-08T07:11:31Z

test/e2e/framework/create.go

-func (f *Framework) PatchName(item *string) {
+// 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) {


This is still an API change. When this create.go was initially merged, I was explicitly asked to split it up so that someone can load items, patch them (either with the existing PatchItems or some custom code), and then create them.

Such custom code is meant to have access to individual building blocks like this PatchName.

feel free to submit a PR changing these back to external

pohly · 2019-05-08T07:16:53Z

test/e2e/framework/create.go

 	case *storage.StorageClass:
-		f.PatchName(&item.Name)
+		f.randomizeStorageClassName(&item.Name)


Now storage classes are "more unique" than other items and regardless what we call the function which does this, I'm worried that this will be confusing.

Where is this failing test code that must load storage classes twice into the same namespace?

pohly · 2019-05-08T07:18:02Z

test/e2e/framework/create.go

 // 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) {


Same API change here.

fejta-bot · 2019-05-08T10:05:39Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

pohly · 2019-05-08T14:52:27Z

Hrmm, I hadn't realized that this had both lgtm and approval. I don't think this should have been merged.

k8s-ci-robot assigned msau42 May 7, 2019

k8s-ci-robot added the release-note-none Denotes a PR that doesn't merit a release note. label May 7, 2019

k8s-ci-robot assigned pohly May 7, 2019

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label May 7, 2019

k8s-ci-robot requested a review from mkimuram May 7, 2019 00:28

davidz627 force-pushed the fix/externalMulti branch from fe8dafc to 55b3282 Compare May 7, 2019 00:35

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 7, 2019

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 7, 2019

davidz627 force-pushed the fix/externalMulti branch from 55b3282 to 014fb31 Compare May 7, 2019 17:49

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 7, 2019

k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels May 7, 2019

Make external driver storage class name generation contain a more ran…

c50e7fd

…dom suffix in case of double generation in the same framework context (twice in the same test)

davidz627 force-pushed the fix/externalMulti branch from 014fb31 to c50e7fd Compare May 7, 2019 21:37

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 7, 2019

pohly reviewed May 8, 2019

View reviewed changes

k8s-ci-robot merged commit 086a86b into kubernetes:master May 8, 2019

pohly mentioned this pull request May 8, 2019

Multivolume storage class fix #77605

Merged

davidz627 deleted the fix/externalMulti branch May 8, 2019 20:22

davidz627 mentioned this pull request Nov 22, 2019

e2e storage: public API for testsuites, support CSIInlineVolume type for generic resource #85540

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make external driver name generation contain a more random suffix in case of double generation in the same framework context (twice in the same test) #77525

Make external driver name generation contain a more random suffix in case of double generation in the same framework context (twice in the same test) #77525

davidz627 commented May 7, 2019

davidz627 commented May 7, 2019 •

edited

msau42 commented May 7, 2019

fejta-bot commented May 7, 2019

msau42 commented May 7, 2019

pohly commented May 7, 2019

davidz627 commented May 7, 2019

davidz627 commented May 7, 2019

pohly commented May 7, 2019 via email

davidz627 commented May 7, 2019

davidz627 commented May 7, 2019

msau42 commented May 7, 2019

pohly commented May 7, 2019 via email

davidz627 commented May 7, 2019

k8s-ci-robot commented May 7, 2019

msau42 commented May 7, 2019

msau42 commented May 7, 2019

davidz627 commented May 8, 2019

fejta-bot commented May 8, 2019

k8s-ci-robot commented May 8, 2019 •

edited

pohly May 8, 2019

davidz627 May 8, 2019

pohly May 8, 2019

pohly May 8, 2019

fejta-bot commented May 8, 2019

pohly commented May 8, 2019

Make external driver name generation contain a more random suffix in case of double generation in the same framework context (twice in the same test) #77525

Make external driver name generation contain a more random suffix in case of double generation in the same framework context (twice in the same test) #77525

Conversation

davidz627 commented May 7, 2019

davidz627 commented May 7, 2019 • edited

msau42 commented May 7, 2019

fejta-bot commented May 7, 2019

msau42 commented May 7, 2019

pohly commented May 7, 2019

davidz627 commented May 7, 2019

davidz627 commented May 7, 2019

pohly commented May 7, 2019 via email

davidz627 commented May 7, 2019

davidz627 commented May 7, 2019

msau42 commented May 7, 2019

pohly commented May 7, 2019 via email

davidz627 commented May 7, 2019

k8s-ci-robot commented May 7, 2019

msau42 commented May 7, 2019

msau42 commented May 7, 2019

davidz627 commented May 8, 2019

fejta-bot commented May 8, 2019

k8s-ci-robot commented May 8, 2019 • edited

pohly May 8, 2019

Choose a reason for hiding this comment

davidz627 May 8, 2019

Choose a reason for hiding this comment

pohly May 8, 2019

Choose a reason for hiding this comment

pohly May 8, 2019

Choose a reason for hiding this comment

fejta-bot commented May 8, 2019

pohly commented May 8, 2019

davidz627 commented May 7, 2019 •

edited

k8s-ci-robot commented May 8, 2019 •

edited