-
Notifications
You must be signed in to change notification settings - Fork 807
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
Wait for volume status in e2e test #34
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bertinatto 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 |
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.
One general comment, could you add some simple description on how do you run this e2e test on EC2? We are looking at automate running this. If you are doing it manually, it could be our good first step. If its already automated, it will be great.
tests/e2e/e2e_test.go
Outdated
// Read the file and check if content is correct | ||
data, err := ioutil.ReadFile(testFile) | ||
Expect(err).To(BeNil(), "Failed to read file") | ||
Expect(data).To(Equal(testFileContents), "File content is incorrect") | ||
} | ||
|
||
// Unmount Disk |
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.
Should move the umount to be right after mount disk succeeds. So that any failures in side file read/write won't cause file is not umounted.
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.
Good catch, thanks.
Factor: 1.2, | ||
Steps: 21, | ||
Duration: 1 * time.Second, | ||
Factor: 1.8, |
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.
This value looks a bit tricky. Why is 1.8 better than 1.2? Could you add some docs around this?
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.
Indeed. These values come from Kubernetes and are mostly related to attachment/detachment from an "exponential backoff" strategy.
In order to verify if the volume was created, Kubernetes fetches the volume state every second for 30 seconds (no exponential backoff). This sounds like too many requests for the purposes of this test, so I decided to use the same approach for both attach/detach and provision/deletion.
In any case, I added a comment (taken from Kubernetes) to take the magic away of these numbers.
}, | ||
} | ||
waitForVolumes(descParams, 1 /* number of expected volumes */) | ||
waitForVolumeState(volume.Id, "available") |
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.
Feels to me its better to move the watiForVolume
creation and deletion inside driver CreateVolume
and DeleteVolume
so that the volume is guaranteed to be available/removed after the call completes. This also void the case where ControllerPublishVolume
is called during volume is still being created.
We could do similar thing for attach (saw your TODO comment on AttachDisk
) and detach.
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 agree that we need to have similar functionality in the cloud
package (e.g., when creating, deleting, attaching and detaching volumes). However, it seems like the implementation of these functions in the driver should be a lot more sophisticated than this. For instance, the function that waits for the volume attachment status in Kubernetes has some magic [1] that I don't quite understand yet and can be tricky to write (in the right way).
Also, assuming that CreateDisk
only returns when the volume is ready, e.g., with available
status, I this test should make sure that the volume is indeed created here.
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.
Got ya. We can punt this to #38 when we have time to properly implement this.
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.
Thanks for looking into this, @leakingtapan. I added a few comments and fixed some issues that you pointed out. Please take a look.
One small complement about the wait-for-volume-status functions: I'll start working on the implementation to be used in the driver, I just need to understand better how Kubernetes is handling this at the moment. After that I'll probably rename waitForVolumeStatus
to something like validateVolumeStatus
.
Factor: 1.2, | ||
Steps: 21, | ||
Duration: 1 * time.Second, | ||
Factor: 1.8, |
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.
Indeed. These values come from Kubernetes and are mostly related to attachment/detachment from an "exponential backoff" strategy.
In order to verify if the volume was created, Kubernetes fetches the volume state every second for 30 seconds (no exponential backoff). This sounds like too many requests for the purposes of this test, so I decided to use the same approach for both attach/detach and provision/deletion.
In any case, I added a comment (taken from Kubernetes) to take the magic away of these numbers.
}, | ||
} | ||
waitForVolumes(descParams, 1 /* number of expected volumes */) | ||
waitForVolumeState(volume.Id, "available") |
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 agree that we need to have similar functionality in the cloud
package (e.g., when creating, deleting, attaching and detaching volumes). However, it seems like the implementation of these functions in the driver should be a lot more sophisticated than this. For instance, the function that waits for the volume attachment status in Kubernetes has some magic [1] that I don't quite understand yet and can be tricky to write (in the right way).
Also, assuming that CreateDisk
only returns when the volume is ready, e.g., with available
status, I this test should make sure that the volume is indeed created here.
tests/e2e/e2e_test.go
Outdated
// Read the file and check if content is correct | ||
data, err := ioutil.ReadFile(testFile) | ||
Expect(err).To(BeNil(), "Failed to read file") | ||
Expect(data).To(Equal(testFileContents), "File content is incorrect") | ||
} | ||
|
||
// Unmount Disk |
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.
Good catch, thanks.
/lgtm |
These are the bits missing in the unfinished e2e test.
I ran this test in a loop for about 20 minutes and it didn't flake.