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

Add VolumeSnapshot retain policy test and test for snapshot delete #89705

Merged
merged 1 commit into from Jun 19, 2020
Merged

Add VolumeSnapshot retain policy test and test for snapshot delete #89705

merged 1 commit into from Jun 19, 2020

Conversation

ggriffiths
Copy link
Member

Signed-off-by: Grant Griffiths grant@portworx.com
What type of PR is this?
/kind cleanup

What this PR does / why we need it:
Adds tests for RetainPolicy and adds an additional delete check in the DeletePolicy test.

Which issue(s) this PR fixes:
Fixes kubernetes-csi/external-snapshotter#269

Special notes for your reviewer:
n/a

Does this PR introduce a user-facing change?:

NONE

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

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Mar 31, 2020
@ggriffiths
Copy link
Member Author

ggriffiths commented Mar 31, 2020 •
edited

/sig storage

@k8s-ci-robot k8s-ci-robot added sig/storage Categorizes an issue or PR as relevant to SIG Storage. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Mar 31, 2020
@ggriffiths
Copy link
Member Author

/assign @xing-yang

@k8s-ci-robot k8s-ci-robot added area/e2e-test-framework Issues or PRs related to refactoring the kubernetes e2e test framework area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Mar 31, 2020
@ggriffiths ggriffiths force-pushed the add_snapshot_retainpolicy_e2e_test branch from 0abffa2 to 69672dd Compare March 31, 2020 23:14
@ggriffiths
Copy link
Member Author

/test pull-kubernetes-e2e-kind-ipv6

xing-yang
xing-yang reviewed Apr 1, 2020
View reviewed changes
test/e2e/storage/testsuites/snapshottable.go Outdated Show resolved Hide resolved
test/e2e/storage/testsuites/snapshottable.go Outdated Show resolved Hide resolved
test/e2e/storage/testsuites/snapshottable.go Show resolved Hide resolved
test/e2e/storage/testsuites/snapshottable.go Outdated Show resolved Hide resolved
@xing-yang
Copy link
Contributor

xing-yang commented Apr 1, 2020 •
edited

We don't have a test for pre-provisioned snapshots. Just opened an issue for that: kubernetes-csi/external-snapshotter#285. For pre-provisioned snapshots, we also should have tests for both Delete and Retain policy. For pre-provisioned snapshots with Retain policy, we need this fix: kubernetes-csi/external-snapshotter#249.

@ggriffiths ggriffiths force-pushed the add_snapshot_retainpolicy_e2e_test branch 10 times, most recently from 307b898 to aa4fb28 Compare April 2, 2020 00:36
@msau42
Copy link
Member

msau42 commented Jun 11, 2020

Opened #92045 to track job timeouts

@ggriffiths
Copy link
Member Author

@saikat-royc, ready for review. Seems like the remaining failing test is unrelated.

@msau42
Copy link
Member

msau42 commented Jun 15, 2020

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

saikat-royc
saikat-royc reviewed Jun 17, 2020
View reviewed changes
test/e2e/storage/testsuites/snapshottable.go Outdated
// The purpose of this block is to prevent physical snapshotContent leaks.
// We must update the SnapshotContent to have Delete Deletion policy,
// or else the physical snapshot content will be leaked.
defer func() {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: This need not be a defer. A simple function call is sufficient here.

saikat-royc
saikat-royc reviewed Jun 17, 2020
View reviewed changes
test/e2e/storage/testsuites/snapshottable.go Outdated
Comment on lines 254 to 257
l.cleanupSteps = append(l.cleanupSteps, func() {
framework.Logf("deleting SnapshotClass %s", l.vsc.GetName())
l.dc.Resource(SnapshotClassGVR).Delete(context.TODO(), l.vsc.GetName(), metav1.DeleteOptions{})
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets keep it uniform as to what is added in cleanupsteps and what is deferred within the function. This can be a simple defer similar to line 266 where the snapshot is getting deleted.

Copy link
Contributor

Choose a reason for hiding this comment

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

To me it looks like cleanup steps mainly cleanup things setup in the common init() call. resources setup inside the TestSnapshottable and TestSnapshotDeleted can be deleted by a defer inside the functions.

saikat-royc
saikat-royc reviewed Jun 17, 2020
View reviewed changes
Copy link
Contributor

@saikat-royc saikat-royc left a comment

Choose a reason for hiding this comment

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

LGTM from my side after minor comments of defer are addressed.

@ggriffiths ggriffiths force-pushed the add_snapshot_retainpolicy_e2e_test branch 3 times, most recently from 31c6548 to f22627e Compare June 18, 2020 01:51
Add retain policy test and refactor snapshottable tests
e1f0e4c 
Signed-off-by: Grant Griffiths <grant@portworx.com>
@ggriffiths ggriffiths force-pushed the add_snapshot_retainpolicy_e2e_test branch from f22627e to e1f0e4c Compare June 18, 2020 02:54
@ggriffiths
Copy link
Member Author

Thanks! I just addressed the remaining PR feedback @saikat-royc

@ggriffiths
Copy link
Member Author

/retest

saikat-royc
saikat-royc reviewed Jun 18, 2020
View reviewed changes
Copy link
Contributor

@saikat-royc saikat-royc left a comment

Choose a reason for hiding this comment

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

Thanks for making the changes @ggriffiths. LGTM from my side.

@ggriffiths
Copy link
Member Author

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

@xing-yang
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 18, 2020
@xing-yang
Copy link
Contributor

/assign @msau42

@msau42
Copy link
Member

msau42 commented Jun 18, 2020

/approve
Thanks!

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ggriffiths, 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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 18, 2020
@ggriffiths
Copy link
Member Author

seems stuck on a test hook:
/test pull-kubernetes-integration

@ggriffiths
Copy link
Member Author

ggriffiths commented Jun 18, 2020 •
edited

Test failure unrelated, trying again.

/test pull-kubernetes-node-e2e-containerd
/test pull-kubernetes-integration

@ggriffiths
Copy link
Member Author

/retest

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Jun 19, 2020 •
edited

@ggriffiths: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-kubernetes-node-e2e-containerd 3a598016c513f0ca5d8f5f7a561d5420d2975c70 link /test pull-kubernetes-node-e2e-containerd

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.

@fejta-bot
Copy link

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

@yuxiangqian yuxiangqian mentioned this pull request Jun 19, 2020
Closed
@k8s-ci-robot k8s-ci-robot merged commit f8705f2 into kubernetes:master Jun 19, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.19 milestone Jun 19, 2020
@xing-yang xing-yang mentioned this pull request Jun 22, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xing-yang xing-yang xing-yang left review comments

@msau42 msau42 msau42 left review comments

@saikat-royc saikat-royc saikat-royc left review comments

@jingxu97 jingxu97 Awaiting requested review from jingxu97

@krzyzacy krzyzacy Awaiting requested review from krzyzacy

Assignees

@xing-yang xing-yang

@msau42 msau42

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/e2e-test-framework Issues or PRs related to refactoring the kubernetes e2e test framework area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. release-note-none Denotes a PR that doesn't merit a release note. sig/storage Categorizes an issue or PR as relevant to SIG Storage. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.19
Development

Successfully merging this pull request may close these issues.

Add E2E test for retain policy (dynamic provisioning)
6 participants
@ggriffiths @xing-yang @msau42 @k8s-ci-robot @fejta-bot @saikat-royc