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

Wait for volume status in e2e test #34

Merged
merged 1 commit into from
Oct 10, 2018
Merged

Wait for volume status in e2e test #34

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
105 changes: 54 additions & 51 deletions tests/e2e/e2e_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,14 @@ import (
"context"
"fmt"
"io/ioutil"
"math/rand"
"os"
"path/filepath"
"time"

"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/aws/awserr"
"github.com/aws/aws-sdk-go/service/ec2"
"github.com/bertinatto/ebs-csi-driver/pkg/cloud"
csi "github.com/container-storage-interface/spec/lib/go/csi/v0"
. "github.com/onsi/ginkgo"
. "github.com/onsi/gomega"
Expand All @@ -48,10 +49,11 @@ var (

var _ = Describe("EBS CSI Driver", func() {

It("Should create->attach->stage->mount volume and check if it is writable, then unmount->unstage->detach->delete and check disk is deleted", func() {
It("Should create, attach, stage and mount volume, check if it's writable, unmount, unstage, detach, delete, and check if it's deleted", func() {

r1 := rand.New(rand.NewSource(time.Now().UnixNano()))
req := &csi.CreateVolumeRequest{
Name: "volume-name-e2e-test",
Name: fmt.Sprintf("volume-name-e2e-test-%d", r1.Uint64()),
CapacityRange: stdCapRange,
VolumeCapabilities: stdVolCap,
Parameters: nil,
Expand All @@ -62,37 +64,20 @@ var _ = Describe("EBS CSI Driver", func() {

volume := resp.GetVolume()
Expect(volume).NotTo(BeNil(), "Expected valid volume, got nil")

// Verifying that volume was created ans is valid
descParams := &ec2.DescribeVolumesInput{
Filters: []*ec2.Filter{
{
Name: aws.String("tag:" + cloud.VolumeNameTagKey),
Values: []*string{aws.String(req.GetName())},
},
},
}
waitForVolumes(descParams, 1 /* number of expected volumes */)
waitForVolumeState(volume.Id, "available")
Copy link
Contributor

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.

Copy link
Member Author

@bertinatto bertinatto Oct 8, 2018 •
edited
Loading

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.

[1] https://github.com/bertinatto/kubernetes/blob/5fdb0c7177010711ca7b463c7e1d4f32a163a344/pkg/cloudprovider/providers/aws/aws.go#L1862

Copy link
Contributor

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.


// Delete volume
defer func() {
_, err = csiClient.ctrl.DeleteVolume(context.Background(), &csi.DeleteVolumeRequest{VolumeId: volume.Id})
Expect(err).To(BeNil(), "Could not delete volume")
waitForVolumes(descParams, 0 /* number of expected volumes */)
waitForVolumes(volume.Id, 0 /* number of expected volumes */)

// Deleting volume twice
_, err = csiClient.ctrl.DeleteVolume(context.Background(), &csi.DeleteVolumeRequest{VolumeId: volume.Id})
Expect(err).To(BeNil(), "Error when trying to delete volume twice")

// Trying to delete non-existent volume
nonexistentVolume := "vol-0f13f3ff21126cabf"
if nonexistentVolume != volume.Id {
_, err = csiClient.ctrl.DeleteVolume(context.Background(), &csi.DeleteVolumeRequest{VolumeId: nonexistentVolume})
Expect(err).To(BeNil(), "Error deleting non-existing volume: %v", err)
}
}()

// TODO: attach, stage, publish, unpublish, unstage, detach
// Attach, stage, publish, unpublish, unstage, detach
nodeID := ebs.GetMetadata().GetInstanceID()
testAttachWriteReadDetach(volume.Id, req.GetName(), nodeID, false)

Expand Down Expand Up @@ -155,16 +140,6 @@ func testAttachWriteReadDetach(volumeID, volName, nodeID string, readOnly bool)
VolumeCapability: stdVolCap[0],
})
Expect(err).To(BeNil(), "NodePublishVolume failed with error")
//err = testutils.ForceChmod(instance, filepath.Join("/tmp/", volName), "777")
//Expect(err).To(BeNil(), "Chmod failed with error")
testFileContents := "test"
if !readOnly {
// Write a file
testFile := filepath.Join(publishDir, "testfile")
//err = testutils.WriteFile(instance, testFile, testFileContents)
err := ioutil.WriteFile(testFile, []byte(testFileContents), 0644)
Expect(err).To(BeNil(), "Failed to write file")
}

// Unmount Disk
defer func() {
Expand All @@ -175,30 +150,57 @@ func testAttachWriteReadDetach(volumeID, volName, nodeID string, readOnly bool)
Expect(err).To(BeNil(), "NodeUnpublishVolume failed with error")
}()

if !readOnly {
// Write a file
testFileContents := []byte("sample content")
testFile := filepath.Join(publishDir, "testfile")
err := ioutil.WriteFile(testFile, testFileContents, 0644)
Expect(err).To(BeNil(), "Failed to write file")
// 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")
}
}

func waitForVolumes(params *ec2.DescribeVolumesInput, nVolumes int) {
func waitForVolumeState(volumeID, state string) {
// Most attach/detach operations on AWS finish within 1-4 seconds.
// By using 1 second starting interval with a backoff of 1.8,
// we get [1, 1.8, 3.24, 5.832000000000001, 10.4976].
// In total we wait for 2601 seconds.
backoff := wait.Backoff{
Duration: 60 * time.Second,
Factor: 1.2,
Steps: 21,
Duration: 1 * time.Second,
Factor: 1.8,
Copy link
Contributor

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?

Copy link
Member Author

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.

Steps: 13,
}
verifyVolumeFunc := func() (bool, error) {
params := &ec2.DescribeVolumesInput{
VolumeIds: []*string{aws.String(volumeID)},
}
volumes, err := describeVolumes(params)
if err != nil {
return false, err
}
if len(volumes) != nVolumes {
if len(volumes) != 1 {
return false, fmt.Errorf("expected 1 volume, got %d", len(volumes))
}
if aws.StringValue(volumes[0].State) != state {
return false, nil
}
// We need to check the atachment state when the volume is "in-use",
// as it might still be "attaching" rather than "attached".
if state == "in-use" {
if aws.StringValue(volumes[0].Attachments[0].State) != "attached" {
return false, nil
}
}
return true, nil

}
waitErr := wait.ExponentialBackoff(backoff, verifyVolumeFunc)
Expect(waitErr).To(BeNil(), "Timeout error when looking for volume: %v", waitErr)
Expect(waitErr).To(BeNil(), "Timeout error waiting for volume state %q: %v", waitErr, state)
}

func waitForVolumeState(volumeID, state string) {
func waitForVolumes(volumeID string, nVolumes int) {
backoff := wait.Backoff{
Duration: 1 * time.Second,
Factor: 1.8,
Expand All @@ -208,23 +210,24 @@ func waitForVolumeState(volumeID, state string) {
params := &ec2.DescribeVolumesInput{
VolumeIds: []*string{aws.String(volumeID)},
}
// FIXME: for some reason this always returns the same state
volumes, err := describeVolumes(params)
if err != nil {
if nVolumes == 0 {
if awsErr, ok := err.(awserr.Error); ok {
if awsErr.Code() == "InvalidVolume.NotFound" {
return true, nil
}
}
}
return false, err
}
if len(volumes) < 1 {
return false, fmt.Errorf("expected 1 valid volume, got nothing")
}
for _, attachment := range volumes[0].Attachments {
if aws.StringValue(attachment.State) == state {
return true, nil
}
if len(volumes) != nVolumes {
return false, nil
}
return false, nil
return true, nil
}
waitErr := wait.ExponentialBackoff(backoff, verifyVolumeFunc)
Expect(waitErr).To(BeNil(), "Timeout error waiting for volume state %q: %v", waitErr, state)
Expect(waitErr).To(BeNil(), "Timeout error when looking for volume %q: %v", volumeID, waitErr)
}

func describeVolumes(params *ec2.DescribeVolumesInput) ([]*ec2.Volume, error) {
Expand Down