Add testing infra for checking whether an in-tree plugin is using backend that is shimmed to CSI #77101

davidz627 · 2019-04-25T23:32:15Z

In order to test the CSI Migration feature we implement infrastructure that will gather volume operation metrics at the start and end of each volume test. We will then compare the two and make sure that only CSI Driver metrics are emitted when the in-tree plugin is migrated, or that only in-tree operation metrics are emitted when the in-tree plugin is NOT migrated.

I am currently running all gce pd tests to confirm that this PR is working as expected, will update the PR comments as I get results.

At least for a simple dynamic provisioning exec test [sig-storage] In-tree Volumes [Driver: gcepd] [Testpattern: Dynamic PV (default fs)] volumes should allow exec of files on the volume the implementation will pass for all expected success cases, and fail with reasonable error messages for the expected failure cases.

There is one open issue about the validation of metrics happening before volume unmount and detach. Suggestions welcome but I am actively investigating how to get the validation to happen after volumes are removed from the pod.

/sig storage
/kind feature

/assign @ddebroy @leakingtapan @msau42 @jsafrane @saad-ali

NONE

… code

davidz627 · 2019-04-25T23:33:23Z

/priority important-soon

msau42 · 2019-04-25T23:58:33Z

test/e2e/storage/testsuites/testdriver.go

@@ -128,7 +128,10 @@ const (

 // DriverInfo represents static information about a TestDriver.
 type DriverInfo struct {
-	Name       string // Name of the driver, aka the provisioner name.
+	Name string // Internal name of the driver, this is NOT the provisioner name


maybe just say this is a display name for the driver to use in the test case name and test objects

msau42 · 2019-04-25T23:59:01Z

test/e2e/storage/testsuites/testdriver.go

+	Name string // Internal name of the driver, this is NOT the provisioner name
+	// Name of the in-tree plugin if it exists
+	// empty if this DriverInfo represents a CSI Driver
+	PluginName string


Clarify that this is the fully qualified plugin name as registered in Kubernetes

msau42 · 2019-04-26T00:08:20Z

test/e2e/storage/testsuites/base.go

+	return totOps
+}
+
+func getVolumeOpsFromKubeletMetricsForPlugin(ms metrics.KubeletMetrics, pluginName string) opCounts {


The two methods look exactly the same, can we merge them (or part of them) somehow?

msau42 · 2019-04-26T00:08:44Z

test/e2e/storage/testsuites/base.go

+					continue
+				}
+				opName := string(sample.Metric["operation_name"])
+				if opName == "verify_controller_attached_volume" {


is this emitted for controller metrics?

no, but since i've merged the two metrics functions I skip verify_controller_attached_volume for both controller and Kubelet metrics. I see no issue with this

msau42 · 2019-04-26T00:12:27Z

test/e2e/storage/testsuites/base.go

+	for op, count := range o1 {
+		seen.Insert(op)
+		totOps[op] = totOps[op] + count
+		totOps[op] = totOps[op] + o2[op]


o2[op] can go in the line before?

brain fart lol. for some reason I thought I could only add exactly two numbers together

msau42 · 2019-04-26T00:14:28Z

test/e2e/storage/testsuites/base.go

+}
+
+func validateMigrationVolumeOpCounts(cs clientset.Interface, pluginName string, oldInTreeOps, oldMigratedOps opCounts) {
+	if len(pluginName) == 0 {


Maybe call the field in the driver config InTreePluginName then if you don't want it to be used for csi drivers

msau42 · 2019-04-26T00:16:05Z

test/e2e/storage/testsuites/base.go

+
+	newInTreeOps, newMigratedOps := getMigrationVolumeOpCounts(cs, pluginName)
+
+	if sets.NewString(strings.Split(*migratedPlugins, ",")...).Has(pluginName) {


Should we check this first? It saves querying the components for the metrics

even if this condition is not true, we want to get the newInTreeOps. However, with #77101 (comment) this may no longer be true

msau42 · 2019-04-26T00:17:01Z

test/e2e/storage/testsuites/base.go

+
+	nodes, err := c.CoreV1().Nodes().List(metav1.ListOptions{})
+	framework.ExpectNoError(err, "Error listing nodes: %v", err)
+	for _, node := range nodes.Items {


Is the intention that this validation will be checked by default when migration is on by default?

Iterating over all nodes may not work so well for scale testing clusters that are 1000s of nodes.

Good catch! I believe most tests specify a "ClientNodeName" which we can use to only query the specific node the volume/pod is scheduled on. However for tests that don't specify what do we want the behavior to be?

Maybe we check how many nodes there are and above a certain threshhold we just decide to not check node metrics? That seems pretty hacky to me, however it still achieves the goal of validating migration functionality when we run the tests on small-ish clusters

I've made a 10 node threshhold. However, I've noticed that only certain drivers set a nodeName (local, hostpath, iscsi) while the other drivers do not. I wonder if it is maybe worth having each of the drivers set a nodeName for their tests (which is kind of weird for multi-node tests). However, this seems like an issue with the test infra itself as I believe nodeName should probably be a per-test level field, not a per-driver field.

clientNodeName essentially bypasses the scheduler and forces the pod to go to a specific node. It should only be used for hostpath. I'm not sure why local needs it since local supports topology cc @cofyc

Anyway, we should not use node name, especially as we add more topology and multi-node, multi-writer tests in the future.

In that case there is no real generic way (that I can think of) to scrape the metrics from the "correct" node that the test is running on and I'm thinking custom logic for each test doesn't seem great. It would be nice if the operation metric also emitted the node that it was done on, then we could pull which node it got attached to from the controller metric but that increases the carnality of the metric and is only useful for this specific functionality.

Maybe we only look at the controller metrics unless the cluster is "small enough"? That is far from ideal, but it gives us some signal to whether the feature is working or not.. In the near future we were thinking of "disabling" the entire in-tree plugin if the migration feature for that plugin is on anyway, so that combined with some negative metrics checking, combined with tests passing, is a pretty strong signal. Even if the metrics infra part is not the strongest signal on its own

msau42 · 2019-04-26T00:23:08Z

test/e2e/storage/testsuites/base.go

+		}
+		totMigrated := getTotOps(newMigratedOps)
+		oldTotMigrated := getTotOps(oldMigratedOps)
+		if totMigrated-oldTotMigrated <= 0 {


Some negative test cases may not cause the operation metrics to be incremented

this is a good point. We could loosen the guarantees and only check:
if migrated -> no in-tree operations are done
if not migrated -> no migrated CSI Driver ops are done (we currently can't do this unless we add a "migrated" dimension to the metrics).

For the migrated case we at least know:

No in-tree operations were done in any test

The test passes so the e2e things we needed to do were achieved
these seem sufficient to determine that migration is "working". To determine that we're not "accidentally" using migration it is suffiecient to run all the tests with migration off without the driver installed.

So maybe this is enough. lmk what you think

Yeah I think this is ok.

msau42 · 2019-04-26T01:18:07Z

test/e2e/storage/testsuites/base.go

+	if sets.NewString(strings.Split(*migratedPlugins, ",")...).Has(pluginName) {
+		// If this plugin is migrated based on the test flag storage.migratedPlugins
+		for op, count := range newInTreeOps {
+			if count != oldInTreeOps[op] {


Can this be 0? Why would the intree metric be emitted?

yes, they can both be 0. The in-tree metric would be emitted if CSI Migration was not working and the operation was actually done with the in-tree plugin

davidz627 · 2019-04-26T20:54:20Z

@msau42 I've made the recommended changes/fixes. However, there are still some open questions. Please let me know what you think.

msau42 · 2019-04-30T00:04:37Z

pkg/volume/util/operationexecutor/operation_generator.go

-		// useCSIPlugin will check both CSIMigration and the plugin specific feature gate
-		if useCSIPlugin(og.volumePluginMgr, volumeToAttach.VolumeSpec) && nu {
+		// useCSIPlugin will check both CSIMigration and the plugin specific feature gates
+		ucp := useCSIPlugin(og.volumePluginMgr, volumeToAttach.VolumeSpec)


do you need to define ucp? It's only used once in this function?

msau42 · 2019-04-30T00:58:40Z

pkg/volume/util/operationexecutor/operation_generator.go

 	volumePlugin, err :=
 		og.volumePluginMgr.FindPluginBySpec(volumeToMount.VolumeSpec)
 	if err == nil && volumePlugin != nil {
 		volumePluginName = volumePlugin.GetPluginName()
 	}

 	mountVolumeFunc := func() (error, error) {
-		originalSpec := volumeToMount.VolumeSpec
+
 		// Get mounter plugin
 		if useCSIPlugin(og.volumePluginMgr, volumeToMount.VolumeSpec) {


Do we still need to translate the spec here too since it's done earlier now?

I think it is safer to as the one done earlier just swallows any errors it encounters. This issue can be resolved along with this: https://github.com/kubernetes/kubernetes/pull/77101/files#diff-450e811a4953f760ff1594ede8b2037eR388

I think the idea would be to determine which plugin to emit the metric for when the operation is being run (since this now actually depends on some runtime state), instead of prior in the operation generator func.

msau42 · 2019-05-01T18:14:12Z

test/e2e/storage/testsuites/volumes.go

@@ -131,6 +136,10 @@ func (t *volumesTestSuite) defineTests(driver TestDriver, pattern testpatterns.T
 			l.testCleanup()
 			l.testCleanup = nil
 		}
+
+		// TODO(dyzz): At this point the volume still has not unmounted or detached...


If the test cleanup checks that the Pod object is fully deleted, then that means unmount has been successful.

Also if cleanupResource() also handles deleting the PVC and waiting for the PV to disappear, then that means it needs to have been detached and deleted too.

msau42 · 2019-05-01T18:15:23Z

/lgtm
/approve

k8s-ci-robot · 2019-05-01T18:19:31Z

[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:

~~pkg/volume/util/operationexecutor/OWNERS~~ [msau42]
~~test/e2e/storage/OWNERS~~ [msau42]

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

davidz627 · 2019-05-01T18:25:04Z

@msau42 removed that TODOm cleaned up commits, fixed gofmt issue, should be ready for re-LGTM

msau42 · 2019-05-01T18:31:56Z

/lgtm

Random-Liu · 2019-05-02T08:08:31Z

It seems that this broke containerd test.

https://k8s-testgrid.appspot.com/sig-node-containerd#e2e-gci

/cc @davidz627 @msau42

k8s-ci-robot · 2019-05-02T08:08:33Z

@Random-Liu: GitHub didn't allow me to request PR reviews from the following users: davidz627.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

It seems that this broke containerd test.

https://k8s-testgrid.appspot.com/sig-node-containerd#e2e-gci

/cc @davidz627 @msau42

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.

Operation generator migration metric fixes and test metrics retrieval…

213cc99

… code

k8s-ci-robot assigned ddebroy, jsafrane, leakingtapan, msau42 and saad-ali Apr 25, 2019

k8s-ci-robot requested review from copejon and jingxu97 April 25, 2019 23:32

k8s-ci-robot added area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Apr 25, 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 Apr 25, 2019

davidz627 mentioned this pull request Apr 25, 2019

Volume deletion not triggered when CSI Migration is enabled #77102

Closed

davidz627 changed the title ~~Add testing infra for checking whether an in-tree plugin is using backend that is shimmed to CSI~~ WIP Add testing infra for checking whether an in-tree plugin is using backend that is shimmed to CSI Apr 25, 2019

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 25, 2019

msau42 reviewed Apr 26, 2019

View reviewed changes

davidz627 force-pushed the feature/migrationTest branch from 83d89a0 to 385d573 Compare April 26, 2019 23:23

davidz627 changed the title ~~WIP Add testing infra for checking whether an in-tree plugin is using backend that is shimmed to CSI~~ Add testing infra for checking whether an in-tree plugin is using backend that is shimmed to CSI Apr 29, 2019

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 29, 2019

msau42 reviewed Apr 30, 2019

View reviewed changes

davidz627 force-pushed the feature/migrationTest branch from 385d573 to a1f5d27 Compare April 30, 2019 01:11

davidz627 mentioned this pull request Apr 30, 2019

Add test config for testing CSI Migration with PD Driver kubernetes-sigs/gcp-compute-persistent-disk-csi-driver#258

Merged

msau42 reviewed May 1, 2019

View reviewed changes

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

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

Add migration metrics checking to all test suites

1271237

davidz627 force-pushed the feature/migrationTest branch from a1f5d27 to 1271237 Compare May 1, 2019 18:24

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

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

k8s-ci-robot merged commit 2b56145 into kubernetes:master May 1, 2019

davidz627 deleted the feature/migrationTest branch May 1, 2019 22:52

k8s-ci-robot requested a review from msau42 May 2, 2019 08:08

Random-Liu mentioned this pull request May 4, 2019

Remove terminated pod from summary api. #77426

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add testing infra for checking whether an in-tree plugin is using backend that is shimmed to CSI #77101

Add testing infra for checking whether an in-tree plugin is using backend that is shimmed to CSI #77101

davidz627 commented Apr 25, 2019

davidz627 commented Apr 25, 2019

msau42 Apr 25, 2019

msau42 Apr 25, 2019

msau42 Apr 26, 2019

msau42 Apr 26, 2019

davidz627 Apr 26, 2019

msau42 Apr 26, 2019

davidz627 Apr 26, 2019 •

edited

msau42 Apr 26, 2019

msau42 Apr 26, 2019

davidz627 Apr 26, 2019

msau42 Apr 26, 2019

davidz627 Apr 26, 2019

davidz627 Apr 26, 2019

msau42 Apr 26, 2019

davidz627 Apr 26, 2019

msau42 Apr 26, 2019

davidz627 Apr 26, 2019

msau42 Apr 26, 2019

msau42 Apr 26, 2019

davidz627 Apr 26, 2019

davidz627 commented Apr 26, 2019

msau42 Apr 30, 2019

msau42 Apr 30, 2019

davidz627 Apr 30, 2019

msau42 May 1, 2019

msau42 commented May 1, 2019

k8s-ci-robot commented May 1, 2019

davidz627 commented May 1, 2019

msau42 commented May 1, 2019

Random-Liu commented May 2, 2019

k8s-ci-robot commented May 2, 2019


		newInTreeOps, newMigratedOps := getMigrationVolumeOpCounts(cs, pluginName)

		if sets.NewString(strings.Split(*migratedPlugins, ",")...).Has(pluginName) {

Add testing infra for checking whether an in-tree plugin is using backend that is shimmed to CSI #77101

Add testing infra for checking whether an in-tree plugin is using backend that is shimmed to CSI #77101

Conversation

davidz627 commented Apr 25, 2019

davidz627 commented Apr 25, 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

davidz627 Apr 26, 2019 • edited

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

Choose a reason for hiding this comment

davidz627 commented Apr 26, 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

msau42 commented May 1, 2019

k8s-ci-robot commented May 1, 2019

davidz627 commented May 1, 2019

msau42 commented May 1, 2019

Random-Liu commented May 2, 2019

k8s-ci-robot commented May 2, 2019

davidz627 Apr 26, 2019 •

edited