-
Notifications
You must be signed in to change notification settings - Fork 554
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
Add E2E tests for rbd snapshot #431
Conversation
78f6228
to
25d7651
Compare
CI passed locally
i will check why its failing in travis (may to due to rawblock mode of PVC) |
6cedde5
to
11b9919
Compare
@Madhu-1 yep, please cross check |
@humblec I will be skipping the block mode e2e now ( need some more debugging) |
Adding extenal-provisoner will help us to test snapshot functionality Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>
Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>
tls.VersionTLS13 requires 1.12.x Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>
e2e/cephfs.go
Outdated
validateNormalUserPVCAccess(pvcPath, f) | ||
}) | ||
|
||
By("create/delete multiple PVC and App", func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PVCs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
validateNormalUserPVCAccess(pvcPath, f) | ||
}) | ||
|
||
By("create/delete multiple PVC and App", func() { | ||
totalCount := 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
may be make it as a variable at global level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to limit the scope of value within this context, if required maybe we can have different variable bound to a particular context to create a different number of PVC's
e2e/rbd.go
Outdated
if err != nil { | ||
Fail(err.Error()) | ||
} | ||
snapList, err := listSnapshots(f, "replicapool", images[0]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pool name can be variable for later use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
e2e/rbd.go
Outdated
// validatePVCAndAppBinding(rawPvcPath, rawAppPath, f) | ||
// }) | ||
|
||
By("create/delete multiple PVC and App", func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Madhu-1 what about making this function of creation more generic in util ? so that cephfs and rbd can just call that instead of repeating it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the functionality includes 3 different validation
- PVC create and bind it to app
- validate backend rbd images (doesnot apply for cephfs)
- delete app and PVC
IMO combining both create and delete operation is not a good option
apierrs "k8s.io/apimachinery/pkg/api/errors" | ||
|
||
"github.com/kubernetes-csi/external-snapshotter/pkg/apis/volumesnapshot/v1alpha1" | ||
snapClient "github.com/kubernetes-csi/external-snapshotter/pkg/client/clientset/versioned/typed/volumesnapshot/v1alpha1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Madhu-1 somehow I feel that we are not using the correct package import above. Is this the correct reference for the client?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes humble works as expected and I think its correct reference
got a reference from here
https://github.com/kubernetes-csi/external-snapshotter/blob/54a21f108e31331ab64c3c6b9fb199ee3070bc75/pkg/controller/snapshot_controller_base.go#L24
https://github.com/kubernetes-csi/external-snapshotter/blob/567d14c09c0fab4ff959b5e8177454462f79c57d/pkg/controller/framework_test.go#L33
Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>
Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>
@humblec @ShyamsundarR @poornimag can you guys review this, I would like to take this patch in to catch some earlier bugs in the code. |
@Madhu-1 did you plan to squash the commits ? or would like to keep |
@humblec each commit is independent I would like to keep it as it is. |
I would like to revert back c8d120f commit once I figure out why block mode is failing in Travis (this will be in follow up PR) |
@ShyamsundarR @poornimag PTAL. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than the Source.Name
and Source.Kind
comment here this is good to go.
Marking this as approved, as the above, if possible, can be corrected subsequently.
} | ||
snap := getSnapshot(snapshotPath) | ||
snap.Namespace = f.UniqueName | ||
snap.Spec.Source.Name = pvc.Name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Source.Name
and Source.Kind
should already be what we need? IOW, the examples are interlinked and should not need to specialize these fields. Looks like extra code for the moment, and will not catch deviation of the Name
and Kind
if changed in a commit in the examples (between pvc and snapshot).
framework.Logf("backend image count %d expected image count %d", len(images), 1) | ||
Fail("validate backend image failed") | ||
} | ||
snap := getSnapshot(snapshotPath) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(nit) This should be loadSnapshot like loadPVC et. al.
@@ -68,15 +81,113 @@ var _ = Describe("RBD", func() { | |||
} | |||
|
|||
By("create a PVC and Bind it to an app", func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The narrative here creates the PVC twice once for normal PVC testing and once for the snapshot testing (and later a different one for 2 PVC/App create test). A shorter but similar end result narrative for the tests would be to use the PVC created in the step here for the snapshot test, and later create an additional one for the 2 PVC test.
Something to consider in the future if the CI time is longer, would be to create the above narrative to be more efficient in testing the flow.
NOTE: As per my understanding Ginkgo runs the BeforeEach and AfterEach for each It
block and not each By
block, if it is the latter then the above narrative is not feasible.
Syncing latest changes from upstream devel for ceph-csi
This PR adds the E2E test cases for