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.

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

Include e2e tests #29

Merged
merged 1 commit into from Feb 27, 2020

Conversation

huffmanca
Copy link
Contributor

@huffmanca huffmanca commented Feb 6, 2020

This includes the e2e test for the CSI Snapshot Controller Operator. It creates a dummy PV, PVC, VolumeSnapshotClass, VolumeSnapshot, and then confirms that a matching VolumeSnapshotContent can be found.

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 6, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: huffmanca

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 6, 2020
@huffmanca
Copy link
Contributor Author

@jsafrane ,

I'm looking to see what I'm doing incorrectly, because the PVC never binds to the PV. They get the same SC name, but there are never any events and it remains Pending indefinitely. Any thoughts?

@huffmanca
Copy link
Contributor Author

@jsafrane,

To update you on where this currently is - the tests pass, and all of the resources EXCEPT for the VolumeSnapshotContent are cleaned up after it runs. By specifying the DeletionPolicy as Retain no Finalizers are added to the VolumeSnapshot.

The VolumeSnapshotContent is still having one automatically added. Scheduling the VolumeSnapshotContent for deletion and then removing the Finalizer programmatically results in being re-added. The only way I've been able to fully delete the object from the cluster is to manually remove it using oc edit volumesnapshotcontent $NAME.

The rest of the test should be straightforward.

@huffmanca
Copy link
Contributor Author

/retest

@huffmanca
Copy link
Contributor Author

This is now running and cleaning up all objects successfully, including the VolumeSnapshotContent.

This PR should be ready for review.

@huffmanca huffmanca changed the title WIP: Include e2e tests Include e2e tests Feb 19, 2020
@openshift-ci-robot openshift-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 Feb 19, 2020
@huffmanca huffmanca force-pushed the include_e2e_tests branch 2 times, most recently from edf9ebc to d8d8cc8 Compare February 20, 2020 16:34
@huffmanca
Copy link
Contributor Author

/retest

test/helpers/helpers.go Outdated Show resolved Hide resolved
test/e2e/operator_test.go Outdated Show resolved Hide resolved
go.mod Outdated
k8s.io/legacy-cloud-providers => k8s.io/legacy-cloud-providers v0.17.0
k8s.io/metrics => k8s.io/metrics v0.17.0
k8s.io/sample-apiserver => k8s.io/sample-apiserver v0.17.0
)
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this change be part of the "Update deps" commit instead?

Also, will we "backport" this patch to 4.4? (= cherry-pick torelease-4.4 branch)?

IMO we should have e2e tests there, but this is a rather big change (1.200+ files updated).backport all

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should now be in the second commit.

As far as backporting goes... we probably should, but I'm not certain if it's realistic.

test/e2e/operator_test.go Outdated Show resolved Hide resolved
test/e2e/operator_test.go Outdated Show resolved Hide resolved
t.Logf("Found snapshot content %v", snapshotContent.ObjectMeta.Name)

// Ensure the Snapshot reports ReadyToUse = true
err = waitForSnapshotReady(ctx, snapshot, t, namespace)
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if we should store some data in the PV, take the snaphost, restore and check if the data is equal?

@bertinatto
Copy link
Member

Should we add a make target to run this e2e test?

@huffmanca
Copy link
Contributor Author

/retest

test/e2e/operator_test.go Outdated Show resolved Hide resolved
test/e2e/operator_test.go Show resolved Hide resolved
test/e2e/operator_test.go Show resolved Hide resolved
test/e2e/operator_test.go Show resolved Hide resolved
@huffmanca
Copy link
Contributor Author

/retest

@jsafrane
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 27, 2020
@jsafrane
Copy link
Contributor

/retest

@openshift-merge-robot openshift-merge-robot merged commit 601d399 into openshift:master Feb 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants