End2End tests for DynamicVolumeProvisioning of EBS #67102
Conversation
k8s-ci-robot
added
release-note-none
|
/ok-to-test |
k8s-ci-robot
removed
the
needs-ok-to-test
| defer func() { | ||
| framework.DeletePodOrFail(client, pod.Namespace, pod.Name) | ||
| }() | ||
|
|
There was a problem hiding this comment.
should we validate that the pod came up running?
There was a problem hiding this comment.
CreateClientPod -> CreatePod -> WaitForPodNameRunningInNamespace waits and polls for pod to get to a PodRunning state. So I think that should be good?
| func addAllowedTopologiesToStorageClass(c clientset.Interface, sc *storage.StorageClass) sets.String { | ||
| zones, err := framework.GetClusterZones(c) | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| zoneCount := rand.Intn(zones.Len()) |
There was a problem hiding this comment.
what happens if zones.Len() == 0?
instead of randomly picking a number of zones, I would make the test deterministic and have a test for 1 zone for now. Maybe in the future, we can have a test case for multiple zones, and we can deterministically control which zones to use (ie through pod node selector or something)
There was a problem hiding this comment.
Assuming the nodes in the cluster where the tests are launched can span multiple zones (i.e. cannot assume a single zone): For the WaitForFirstConsumer + AllowedTopology case, the test will (1) pick a random schedule-able node from the cluster (2) use selected node's zone to set the allowed topology and (3) use a node selector in the test pod to schedule on the selected node and perform checks. For the AllowedTopology (without WaitForFirstConsumer) case, steps (2) and (3) will be skipped.
There was a problem hiding this comment.
Why do you need to do 3)? The purpose of volume scheduling is that the pod will go where the storage is
There was a problem hiding this comment.
True .. AllowedTopology is ignored during provisioning so we can set it to any random zone and the test should still succeed. So (3) is unnecessary above.
| }) | ||
| }) | ||
| Describe("EBS allowedTopology [Feature:DynamicProvisioningScheduling]", func() { | ||
| It("should create persistent volume in the zones specified in allowedTopology of storageclass", func() { |
There was a problem hiding this comment.
one more test case for allowedTopology + delayed binding?
|
Addressed review comments and squashed commits |
| @@ -874,6 +972,85 @@ var _ = utils.SIGDescribe("Dynamic Provisioning", func() { | |||
| Expect(err).NotTo(HaveOccurred()) | |||
| }) | |||
| }) | |||
| Describe("EBS delayed binding [Feature:DynamicProvisioningScheduling]", func() { | |||
There was a problem hiding this comment.
It might be better to structure it like the "should provision storage with different parameters" test cases, where all the providers are under a test case.
So, something like Describe("DynamicProvisioner delayed [Feature...]")
| @@ -874,6 +972,85 @@ var _ = utils.SIGDescribe("Dynamic Provisioning", func() { | |||
| Expect(err).NotTo(HaveOccurred()) | |||
| }) | |||
| }) | |||
| Describe("EBS delayed binding [Feature:DynamicProvisioningScheduling]", func() { | |||
| It("should create persistent volume in the same zone as node after a pod mounting the claim is started", func() { | |||
There was a problem hiding this comment.
How long does this test take? It may need [Slow] tag if it's more than 2 minutes
There was a problem hiding this comment.
When running by itself it took about 100 seconds typically but I did see the test infra marking them slow: [SLOW TEST:96.519 seconds] . So I have put the [Slow] tags for the delayed binding tests.
There was a problem hiding this comment.
I think under 2 minutes is fineeeeee :-)
Signed-off-by: Deep Debroy <ddebroy@docker.com>
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ddebroy, 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 |
k8s-ci-robot
added
the
approved
|
/test all [submit-queue is verifying that this PR is safe to merge] |
|
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here. |
What this PR does / why we need it:
Add end2end tests to exercise
DynamicProvisioningSchedulingfeatures for EBS. The tests make sureWaitForFirstConsumerandAllowedTopologiesspecified in a EBS storage class has the desired effect.Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #
Special notes for your reviewer:
Tests features added to 217a3d8
Release note:
/sig storage
/assign @msau42 @jsafrane