unit tests for operationGenerator.GenerateUnmapVolumeFunc #78267

mucahitkurt · 2019-05-23T21:31:06Z

What type of PR is this?
/kind cleanup
/sig storage

What this PR does / why we need it:
Unit tests for the volume plugin name that's used inside GeneratedUnmapVolumeFunc for csi migration on/off scenarios

Which issue(s) this PR fixes:
#77141

Special notes for your reviewer:
Added unit test for the just one function, If I'm on the right path I'll continue to write tests for the other operation generator functions.

Does this PR introduce a user-facing change?:

NONE

k8s-ci-robot · 2019-05-23T21:31:14Z

Hi @mucahitkurt. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

mucahitkurt · 2019-05-23T21:32:29Z

/assign @davidz627

hoyho · 2019-05-28T07:29:46Z

pkg/volume/util/operationexecutor/operation_generator_test.go

+
+	csiplugin, err := plugMgr.FindPluginByName(csi.CSIPluginName)
+	if err != nil {
+		t.Fatalf("can't find plugin %v", csi.CSIPluginName)


csi.CSIPluginName should be a string? maybe %s is more clear here

fixed, thanks!

hoyho · 2019-05-28T07:32:42Z

pkg/volume/util/operationexecutor/operation_generator_test.go

+
+	_, e := plugMgr.FindPluginByName(gcePdVolumePluginName)
+	if e != nil {
+		t.Errorf("Can't find the plugin by name: %v", gcePdVolumePluginName)


same as before，%s can be more clear for string gcePdVolumePluginName

fixed, thanks!

hoyho · 2019-05-28T07:44:44Z

pkg/volume/util/operationexecutor/operation_generator_test.go

+		var ee error
+		unmapVolumeFunc.CompleteFunc(&ee)
+
+		metric_family_name := "storage_operation_status_count"


recommend to use go style for variable declarations
https://golang.org/doc/effective_go.html#mixed-caps

fixed, thanks!

jsafrane · 2019-05-28T10:04:55Z

/ok-to-test
/assign @gnufied

mucahitkurt · 2019-05-28T19:40:56Z

/test pull-kubernetes-e2e-gce-storage-slow
/test pull-kubernetes-integration
/test pull-kubernetes-verify

mucahitkurt · 2019-05-28T19:55:50Z

I have a question about unit testing metrics, prometheus.DefaultGatherer is global metric registry so there is no guarantee for a metric value is changed with the code that's tested by my unit test. And tests can be flaky when they are run as parallel.

Is there any best practice to test metrics?

gnufied · 2019-05-28T19:56:18Z

pkg/volume/util/operationexecutor/operation_generator_test.go

+	name                  string
+	isCsiMigrationEnabled bool
+	expectedPluginName    string
+}


lets move this struct inside test function

fixed, thanks!

gnufied · 2019-05-28T20:04:00Z

pkg/volume/util/operationexecutor/operation_generator_test.go

+		{
+			name:                  "csi migration disabled",
+			isCsiMigrationEnabled: false,
+			expectedPluginName:    gcePdVolumePluginName,


just to be consistent can we use plugin defined in the csitranslation library?

fixed, thanks!

gnufied · 2019-05-28T20:04:43Z

pkg/volume/util/operationexecutor/operation_generator_test.go

+		{
+			name:                  "csi migration enabled",
+			isCsiMigrationEnabled: true,
+			expectedPluginName:    csi.CSIPluginName,


the expected name is not accurate. When csi migration is enable, it will be full csi plugin name. Lets use name defined in csi translation library.

I can't find a const for the full csi plugin name in csi translation library so I joined the csi plugin name and plugin driver name.

davidz627 · 2019-05-28T23:14:22Z

pkg/volume/util/operationexecutor/operation_generator_test.go

+		volumeToUnmount := getTestVolumeToUnmount(podName, pdName, pod)
+		expectedPluginName := tc.expectedPluginName
+
+		if tc.isCsiMigrationEnabled {


I'm not sure what this entire block does. It seems like there is no result from this. the errors we're checking for in here could be other unit tests but I don't think need to be captured in the unmap test.

AFAIK translateSpec and NewBlockVolumeMapper would be implementations details of GenerateUnmapVolumeFunc which we're actually testing later anyway

Actually I wrote this part to run the test, not to test the behavior of some functionality related with this part, because GenerateUnmapVolumeFunc call blockVolumePlugin.NewBlockVolumeUnmapper and when the plugin is csi, csi plugin looks a file that contains some information about the volume, and GenerateUnmapVolumeFuncfails if csi plugin can't find that file.

I call plugin.NewBlockVolumeMapper for csi enabled case to create that file.

Thanks for the explanation, I get it now! Could you add a comment in the code explaining this. It's not clear from reading the code why NewBlockVolumeMapper is being called

fixed, thanks!

davidz627 · 2019-05-28T23:16:29Z

pkg/volume/util/operationexecutor/operation_generator_test.go

+		{
+			name:                  "csi migration enabled",
+			isCsiMigrationEnabled: true,
+			expectedPluginName:    fmt.Sprintf("%s:%s", csi.CSIPluginName, plugins.GCEPDDriverName),


I think expectedPluginName can actually be generated from isCsiMigrationEnabled right? then instead of only doing this on GCE another test dimension could be the plugin we're using. So we can test GCE with migration enabled/disabled, AWS migration enabled/disabled. And we should be able to generated the expectedPluginName based on these dimensions

Fixed, I also added test cases for AWS EBS plugin.

davidz627 · 2019-05-28T23:19:21Z

pkg/volume/util/operationexecutor/operation_generator_test.go

+	return nil
+}
+
+func initTestPlugins(t *testing.T) (*volume.VolumePluginMgr, volume.VolumePlugin, string) {


theres a function in csi_plugin_test.go newTestPlugin that looks very similar. Is there anyway we could reuse that or refactor that into something that could be usable by both tests?

I renamed and moved the method csi.newTestPlugin to the testing_helper file to reuse it.

gnufied · 2019-05-29T02:12:21Z

@mucahitkurt to workaround problems associated with testing metrics, it might be better idea to test for relative increase in values you are looking for, rather than absolute numbers. That might help in eliminating flakes.

davidz627

mostly lgtm, just 2 final comments!

davidz627 · 2019-06-03T20:34:26Z

pkg/volume/util/operationexecutor/operation_generator_test.go

+			defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.CSIMigration, true)()
+			defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, tc.csiMigrationFeature, true)()
+			expectedPluginName = fmt.Sprintf("%s:%s", csi.CSIPluginName, tc.csiDriverName)
+		}


if !tc.isCsiMigrationEnabled we should explicitly turn off the feature gates in preparation for whenever the gates are on by default.

fixed, thanks!

davidz627 · 2019-06-03T20:37:59Z

pkg/volume/util/operationexecutor/operation_generator_test.go

+	}
+}
+
+func findMetricWithNameAndLabels(metricFamilyName string, labelFilter map[string]string) *io_prometheus_client.Metric {


are there no helper functions that exist in the codebase that do this exact thing? It's fine if the answer is no, but I'm just curious. Want to avoid duplicating code as much as possible

I found some functions that do the similar things but couldn't be sure about refactoring them to reuse, wdyt?

I think it's fine to not refactor. I was hoping there would be some common util functions that already exist but it seems like that's not the case.

davidz627 · 2019-06-04T21:49:47Z

/lgtm

davidz627 · 2019-06-05T21:36:17Z

@mucahitkurt Please resolve verify issues and ping me on github or slack when this PR is ready for lgtm again

…apVolumeFunc for csi migration on/off scenarios Signed-off-by: Mucahit Kurt <[email protected]>

mucahitkurt · 2019-06-11T05:49:56Z

@mucahitkurt Please resolve verify issues and ping me on github or slack when this PR is ready for lgtm again

@davidz627 I had some verification issue when I try to reuse the csi_plugin_test.newTestPlugin;

I created a different file and a the same package with csi to reuse the method NewTestPlugin but symbols and import verification failed because of usage of testing and testify in a non test file(cannot export a function from a test file).
I created a different file and a different package other than csi, like csi/testing, to reuse the method NewTestPlugin but I got cylic import fault for packages csi and csi/testing.

I couldn't get over these verification faults so I created the function NewTestPlugin under csi/testing but used only in operation_generator_test not in tests under csi package to pass those verification faults.

davidz627 · 2019-06-11T19:56:40Z

@mucahitkurt I see, no worries. Thanks for trying things out.

/lgtm

jsafrane · 2019-06-17T08:26:07Z

/approve

mucahitkurt · 2019-06-17T09:34:43Z

/assign @mikedanese

mucahitkurt · 2019-06-29T08:20:07Z

@kubernetes/sig-apps-pr-reviews PTAL, thanks!

k8s-ci-robot · 2019-06-29T08:20:15Z

@mucahitkurt: Reiterating the mentions to trigger a notification:
@kubernetes/sig-apps-pr-reviews

In response to this:

@kubernetes/sig-apps-pr-reviews PTAL, thanks!

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.

davidz627 · 2019-07-02T17:51:34Z

/assign @janetkuo
could you PTAL

janetkuo · 2019-07-09T17:09:53Z

/approve

k8s-ci-robot · 2019-07-09T17:10:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: janetkuo, jsafrane, mucahitkurt

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:

~~pkg/controller/OWNERS~~ [janetkuo]
~~pkg/volume/csi/OWNERS~~ [jsafrane]
~~pkg/volume/util/operationexecutor/OWNERS~~ [jsafrane]

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

fejta-bot · 2019-07-09T19:15: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.

davidz627 · 2019-07-09T22:02:21Z

/retest

k8s-ci-robot requested review from rootfs and screeley44 May 23, 2019 21:31

k8s-ci-robot assigned davidz627 May 23, 2019

mucahitkurt force-pushed the cleanup/operation-generator-migration-scenarios-unit-tests branch from f1d3605 to 3aa7f01 Compare May 24, 2019 20:49

hoyho reviewed May 28, 2019

View reviewed changes

mucahitkurt force-pushed the cleanup/operation-generator-migration-scenarios-unit-tests branch from 3aa7f01 to f44ba7e Compare May 28, 2019 09:52

k8s-ci-robot assigned gnufied May 28, 2019

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 28, 2019

gnufied reviewed May 28, 2019

View reviewed changes

mucahitkurt force-pushed the cleanup/operation-generator-migration-scenarios-unit-tests branch from f44ba7e to fade3b2 Compare May 28, 2019 21:44

davidz627 reviewed May 28, 2019

View reviewed changes

mucahitkurt force-pushed the cleanup/operation-generator-migration-scenarios-unit-tests branch from fade3b2 to 415702f Compare May 29, 2019 01:49

mucahitkurt force-pushed the cleanup/operation-generator-migration-scenarios-unit-tests branch 2 times, most recently from c289eb0 to 1f2026a Compare May 30, 2019 22:54

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 30, 2019

mucahitkurt force-pushed the cleanup/operation-generator-migration-scenarios-unit-tests branch from 1f2026a to 515169a Compare May 30, 2019 23:00

davidz627 reviewed Jun 3, 2019

View reviewed changes

mucahitkurt force-pushed the cleanup/operation-generator-migration-scenarios-unit-tests branch from 3f755bc to b8c19bb Compare June 4, 2019 19:41

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

mucahitkurt force-pushed the cleanup/operation-generator-migration-scenarios-unit-tests branch from b8c19bb to 4681bb8 Compare June 5, 2019 05:34

k8s-ci-robot added sig/apps Categorizes an issue or PR as relevant to SIG Apps. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Jun 5, 2019

mucahitkurt force-pushed the cleanup/operation-generator-migration-scenarios-unit-tests branch from 4681bb8 to 943fa0c Compare June 10, 2019 19:35

unit tests for the volume plugin name that's used inside GeneratedUnm…

db1c077

…apVolumeFunc for csi migration on/off scenarios Signed-off-by: Mucahit Kurt <[email protected]>

mucahitkurt force-pushed the cleanup/operation-generator-migration-scenarios-unit-tests branch from 943fa0c to db1c077 Compare June 11, 2019 05:33

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

k8s-ci-robot assigned mikedanese Jun 17, 2019

hoyho mentioned this pull request Jun 20, 2019

REQUEST: New membership for hoyho kubernetes/org#935

Closed

6 tasks

k8s-ci-robot assigned janetkuo Jul 2, 2019

janetkuo approved these changes Jul 9, 2019

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 9, 2019

k8s-ci-robot merged commit d59a603 into kubernetes:master Jul 10, 2019

mucahitkurt mentioned this pull request Sep 7, 2019

REQUEST: New membership for mucahitkurt kubernetes/org#1166

Closed

6 tasks

unit tests for operationGenerator.GenerateUnmapVolumeFunc #78267

unit tests for operationGenerator.GenerateUnmapVolumeFunc #78267

Conversation

mucahitkurt commented May 23, 2019

k8s-ci-robot commented May 23, 2019

mucahitkurt commented May 23, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsafrane commented May 28, 2019

mucahitkurt commented May 28, 2019

mucahitkurt commented May 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mucahitkurt May 30, 2019 • edited Loading

Choose a reason for hiding this comment

gnufied commented May 29, 2019

davidz627 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidz627 commented Jun 4, 2019

davidz627 commented Jun 5, 2019

mucahitkurt commented Jun 11, 2019

davidz627 commented Jun 11, 2019

jsafrane commented Jun 17, 2019

mucahitkurt commented Jun 17, 2019

mucahitkurt commented Jun 29, 2019

k8s-ci-robot commented Jun 29, 2019

davidz627 commented Jul 2, 2019

janetkuo commented Jul 9, 2019

k8s-ci-robot commented Jul 9, 2019

fejta-bot commented Jul 9, 2019

davidz627 commented Jul 9, 2019

mucahitkurt May 30, 2019 •

edited

Loading