Add FromFile and FromExistingClassName support for SnapshotClass in external storage e2e test #88669

mkimuram · 2020-02-28T16:11:24Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
This PR is to add a feature to pass parameters to SnapshotClass in e2e test via FromFile option. This is needed for CSI drivers to pass parameters like secret to SnapshotClass.

Which issue(s) this PR fixes:
Fixes #88668

Special notes for your reviewer:
/sig storage
/sig testing
cc @pohly @wongma7 @xing-yang @oomichi

Does this PR introduce a user-facing change?:

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

mkimuram · 2020-02-28T16:31:48Z

This will allow users to pass user defined SnapshotClass like below way.

# cat << EOF > /tmp/hostpath-testdriver-from-file.yaml
StorageClass:
  FromName: true
SnapshotClass:
  FromFile: "/tmp/snapshot-class.yaml"
DriverInfo:
  Name: hostpath.csi.k8s.io
  Capabilities:
    persistence: true
    snapshotDataSource: true
EOF

# cat << EOF > /tmp/snapshot-class.yaml
apiVersion: snapshot.storage.k8s.io/v1beta1
kind: VolumeSnapshotClass
metadata:
  name: csi-hostpath-snapclass
driver: hostpath.csi.k8s.io
deletionPolicy: Delete
parameters:
  csi.storage.k8s.io/snapshotter-secret-name: snapshot-secret
  csi.storage.k8s.io/snapshotter-secret-namespace: default
EOF

# ginkgo -p -focus='External.Storage.*hostpath.*snap.*' e2e.test -- -storage.testdriver=/tmp/hostpath-testdriver-from-file.yaml

Note that due to the change in SnapshotClass in v1.17, we need to back port this PR to v1.16 or prior and run back ported version of e2e binary, if e2e test is required for v1.16 or prior. (E2E test binary for certain version creates SnapshotClass for the API of its binary version, not k8s cluster's version. So, running latest e2e binary in older k8s cluster won't work if there's API incompatibility.)

pohly

/lgtm

xing-yang · 2020-02-28T16:50:47Z

/lgtm

mkimuram · 2020-02-28T17:48:49Z

/retest

oomichi · 2020-02-28T18:47:23Z

Thanks for doing this.

/lgtm

msau42 · 2020-02-28T19:15:40Z

test/e2e/storage/external/external.go

+		//
+		// This can be used when the snapshot class is meant to have
+		// additional parameters.
+		FromFile string


Can we also add a "From Existing" option, similar to Storage Class above?

Good suggestion. I will work on it.

mkimuram · 2020-02-28T21:26:49Z

Added FromExistingClassName for SnapshotClass. It works like below way. PTAL

# kubectl get volumesnapshotclasses
NAME                     AGE
csi-hostpath-snapclass   8d

# cat << EOF > /tmp/hostpath-testdriver-FromExistingClassName.yaml
StorageClass:
  FromName: true
SnapshotClass:
  FromExistingClassName: csi-hostpath-snapclass
DriverInfo:
  Name: hostpath.csi.k8s.io
  Capabilities:
    persistence: true
    snapshotDataSource: true
EOF

# ginkgo -p -focus='External.Storage.*hostpath.*snap.*' e2e.test -- -storage.testdriver=/tmp/hostpath-testdriver-FromExistingClassName.yaml

Note that:

Without clearing resourceVersion, resource creation fails with "resourceVersion should not be set on objects to be created" error.
For StorageClass's FromExistingClassName, rename is done in testsuites.GetStorageClass, but for SnapshotClass's FromExistingClassName rename is done directly.

mkimuram · 2020-02-29T00:27:52Z

/retest

xing-yang · 2020-02-29T01:31:33Z

/lgtm

pohly · 2020-02-29T12:20:56Z

test/e2e/storage/testsuites/snapshottable.go

-	snapshotGVR        = schema.GroupVersionResource{Group: snapshotGroup, Version: "v1beta1", Resource: "volumesnapshots"}
-	snapshotClassGVR   = schema.GroupVersionResource{Group: snapshotGroup, Version: "v1beta1", Resource: "volumesnapshotclasses"}
-	snapshotContentGVR = schema.GroupVersionResource{Group: snapshotGroup, Version: "v1beta1", Resource: "volumesnapshotcontents"}
+	SnapshotGVR        = schema.GroupVersionResource{Group: snapshotGroup, Version: "v1beta1", Resource: "volumesnapshots"}


Exported symbols must be documented (https://prow.k8s.io/view/gcs/kubernetes-jenkins/pr-logs/pull/88669/pull-kubernetes-verify/1233549485305499653).

Thank you for pointing it out. Fixed.

mkimuram · 2020-03-02T21:25:32Z

Resolved merge conflict.

mkimuram · 2020-03-03T00:18:27Z

/retest

pohly

/lgtm

pohly · 2020-03-03T15:03:07Z

/approve

msau42 · 2020-03-03T17:12:08Z

test/e2e/storage/external/external.go

+	case d.SnapshotClass.FromName:
+		snapshotter := d.DriverInfo.Name
+		parameters := map[string]string{}
+		ns := config.Framework.Namespace.Name


Can we have all 3 cases use GetSnapshotClass so that they all use the same ns and suffix formats?

I also thought that it would be nice to do so, after I wrote my last comment.
However, to do so, we need to get parameters from *unstructured.Unstructured. Is there a good way to achieve it? I couldn't find a good way from here.

Does something like this work?

Fixed and rebased. PTAL

msau42 · 2020-03-03T17:12:34Z

test/e2e/storage/external/external.go

+		snapshotter := d.DriverInfo.Name
+		parameters := map[string]string{}
+		ns := config.Framework.Namespace.Name
+		suffix := snapshotter + "-vsc"


suffix should just be "-vsc"?

Actually, it came from the original code. And it could be both, because it adds ns to suffix inside GetSnapshotClass to be unique. (If there will be a test case that creates multiple SnapshotClass, it's safe to keep snapshotter, though.)

kubernetes/test/e2e/storage/testsuites/driveroperations.go

Line 100 in 0a2a69a

"name": ns + "-" + suffix,

mkimuram

@msau42

Thank you for your review.

I'll change both if you prefer and tell me how on getting parameters.

mkimuram · 2020-03-03T17:34:54Z

test/e2e/storage/external/external.go

+	case d.SnapshotClass.FromName:
+		snapshotter := d.DriverInfo.Name
+		parameters := map[string]string{}
+		ns := config.Framework.Namespace.Name


I also thought that it would be nice to do so, after I wrote my last comment.
However, to do so, we need to get parameters from *unstructured.Unstructured. Is there a good way to achieve it? I couldn't find a good way from here.

mkimuram · 2020-03-03T17:37:00Z

test/e2e/storage/external/external.go

+		snapshotter := d.DriverInfo.Name
+		parameters := map[string]string{}
+		ns := config.Framework.Namespace.Name
+		suffix := snapshotter + "-vsc"


Actually, it came from the original code. And it could be both, because it adds ns to suffix inside GetSnapshotClass to be unique. (If there will be a test case that creates multiple SnapshotClass, it's safe to keep snapshotter, though.)

kubernetes/test/e2e/storage/testsuites/driveroperations.go

Line 100 in 0a2a69a

"name": ns + "-" + suffix,

…xternal storage e2e test

mkimuram · 2020-03-03T22:28:15Z

/retest

msau42 · 2020-03-03T23:32:34Z

/lgtm
/approve
Thanks!

k8s-ci-robot · 2020-03-03T23:33:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mkimuram, msau42, pohly

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

fejta-bot · 2020-03-04T06:28:27Z

/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 requested review from gnufied and msau42 February 28, 2020 16:12

k8s-ci-robot added the area/test label Feb 28, 2020

pohly approved these changes Feb 28, 2020

View reviewed changes

k8s-ci-robot assigned pohly Feb 28, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 28, 2020

k8s-ci-robot assigned xing-yang Feb 28, 2020

k8s-ci-robot assigned oomichi Feb 28, 2020

msau42 reviewed Feb 28, 2020

View reviewed changes

mkimuram force-pushed the snapfromfile branch from cab843a to cd9f54a Compare February 28, 2020 21:22

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 28, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 29, 2020

pohly suggested changes Feb 29, 2020

View reviewed changes

mkimuram force-pushed the snapfromfile branch from cd9f54a to bcd3340 Compare March 2, 2020 18:02

k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 2, 2020

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 2, 2020

mkimuram force-pushed the snapfromfile branch from bcd3340 to bf7bef8 Compare March 2, 2020 21:24

pohly approved these changes Mar 3, 2020

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 3, 2020

msau42 reviewed Mar 3, 2020

View reviewed changes

mkimuram commented Mar 3, 2020

View reviewed changes

Add FromFile and FromExistingClassName support for SnapshotClass in e…

401b85e

…xternal storage e2e test

mkimuram force-pushed the snapfromfile branch from bf7bef8 to 401b85e Compare March 3, 2020 20:28

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 3, 2020

mkimuram changed the title ~~Add FromFile support for SnapshotClass in external storage e2e test~~ Add FromFile and FromExistingClassName support for SnapshotClass in external storage e2e test Mar 3, 2020

k8s-ci-robot assigned msau42 Mar 3, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 3, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 3, 2020

k8s-ci-robot merged commit c2593d3 into kubernetes:master Mar 4, 2020

k8s-ci-robot added this to the v1.18 milestone Mar 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add FromFile and FromExistingClassName support for SnapshotClass in external storage e2e test #88669

Add FromFile and FromExistingClassName support for SnapshotClass in external storage e2e test #88669

mkimuram commented Feb 28, 2020

mkimuram commented Feb 28, 2020

pohly left a comment

xing-yang commented Feb 28, 2020

mkimuram commented Feb 28, 2020

oomichi commented Feb 28, 2020

msau42 Feb 28, 2020

mkimuram Feb 28, 2020

mkimuram commented Feb 28, 2020

mkimuram commented Feb 29, 2020

xing-yang commented Feb 29, 2020

pohly Feb 29, 2020

mkimuram Mar 2, 2020

mkimuram commented Mar 2, 2020

mkimuram commented Mar 3, 2020

pohly left a comment

pohly commented Mar 3, 2020

msau42 Mar 3, 2020

mkimuram Mar 3, 2020

msau42 Mar 3, 2020 •

edited

mkimuram Mar 3, 2020

msau42 Mar 3, 2020

mkimuram Mar 3, 2020

mkimuram left a comment

mkimuram Mar 3, 2020

mkimuram Mar 3, 2020

mkimuram commented Mar 3, 2020

msau42 commented Mar 3, 2020

k8s-ci-robot commented Mar 3, 2020

fejta-bot commented Mar 4, 2020

Add FromFile and FromExistingClassName support for SnapshotClass in external storage e2e test #88669

Add FromFile and FromExistingClassName support for SnapshotClass in external storage e2e test #88669

Conversation

mkimuram commented Feb 28, 2020

mkimuram commented Feb 28, 2020

pohly left a comment

Choose a reason for hiding this comment

xing-yang commented Feb 28, 2020

mkimuram commented Feb 28, 2020

oomichi commented Feb 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkimuram commented Feb 28, 2020

mkimuram commented Feb 29, 2020

xing-yang commented Feb 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkimuram commented Mar 2, 2020

mkimuram commented Mar 3, 2020

pohly left a comment

Choose a reason for hiding this comment

pohly commented Mar 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msau42 Mar 3, 2020 • 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

mkimuram left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkimuram commented Mar 3, 2020

msau42 commented Mar 3, 2020

k8s-ci-robot commented Mar 3, 2020

fejta-bot commented Mar 4, 2020

msau42 Mar 3, 2020 •

edited