Skip to content

Commit

Permalink
Improved volume expansion code
Browse files Browse the repository at this point in the history
bertinatto committed Jun 11, 2019
1 parent c646891 commit 84d17fd
Showing 6 changed files with 52 additions and 66 deletions.
77 changes: 40 additions & 37 deletions pkg/cloud/cloud.go
Original file line number Diff line number Diff line change
@@ -801,61 +801,61 @@ func isAWSErrorSnapshotNotFound(err error) bool {
return isAWSError(err, "InvalidSnapshot.NotFound")
}

// ResizeDisk resizes an EBS volume in GiB increments,
// rouding up to the next GiB if reqSize isn't an even GiB.
// It returns the new size or a negative number if the size couldn't be determined.
func (c *cloud) ResizeDisk(ctx context.Context, volumeID string, reqSizeBytes int64) (int64, error) {
// If current size isgreater or equal, than there's nothing to be done here
// ResizeDisk resizes an EBS volume in GiB increments, rouding up to the next possible allocatable unit.
// It returns the volume size after this call or an error if the size couldn't be determined.
func (c *cloud) ResizeDisk(ctx context.Context, volumeID string, newSizeBytes int64) (int64, error) {
request := &ec2.DescribeVolumesInput{
VolumeIds: []*string{
aws.String(volumeID),
},
}
volume, err := c.getVolume(ctx, request)
if err != nil {
return -1, err
return 0, err
}

// AWS resizes in chunks of GiB (not GB)
reqSizeGiB := util.RoundUpGiB(reqSizeBytes)
newSizeGiB := util.RoundUpGiB(newSizeBytes)
oldSizeGiB := aws.Int64Value(volume.Size)

if oldSizeGiB >= reqSizeGiB {
err := fmt.Errorf("could not expand volume %q: current size (%d GiB) is greater or equal to requested size (%d GiB)", volumeID, oldSizeGiB, reqSizeGiB)
return oldSizeGiB, err
if oldSizeGiB >= newSizeGiB {
klog.V(5).Infof("Volume %q's current size (%d GiB) is greater or equal to the new size (%d GiB)", volumeID, oldSizeGiB, newSizeGiB)
return oldSizeGiB, nil
}

// Modify volume
req := &ec2.ModifyVolumeInput{
VolumeId: aws.String(volumeID),
Size: aws.Int64(reqSizeGiB),
Size: aws.Int64(newSizeGiB),
}

output, err := c.ec2.ModifyVolumeWithContext(ctx, req)
var mod *ec2.VolumeModification
response, err := c.ec2.ModifyVolumeWithContext(ctx, req)
if err != nil {
if isAWSErrorIncorrectModification(err) {
m, err2 := c.getVolumeModification(ctx, volumeID)
if err2 != nil {
return -1, fmt.Errorf("could not get modification state of volume %q: %v", volumeID, err)
}
if !isAWSErrorIncorrectModification(err) {
return 0, fmt.Errorf("could not modify AWS volume %q: %v", volumeID, err)
}

// Check for idempotency
state := aws.StringValue(m.ModificationState)
if state == ec2.VolumeModificationStateCompleted || state == ec2.VolumeModificationStateOptimizing {
return aws.Int64Value(m.TargetSize), nil
}
m, err := c.getLatestVolumeModification(ctx, volumeID)
if err != nil {
return 0, err
}
return oldSizeGiB, fmt.Errorf("could not modify AWS volume %q: %v", volumeID, err)
mod = m
}

if mod == nil {
mod = response.VolumeModification
}

m := output.VolumeModification
if aws.StringValue(m.ModificationState) == ec2.VolumeModificationStateCompleted || aws.StringValue(m.ModificationState) == ec2.VolumeModificationStateOptimizing {
return aws.Int64Value(m.TargetSize), nil
state := aws.StringValue(mod.ModificationState)
if state == ec2.VolumeModificationStateCompleted || state == ec2.VolumeModificationStateOptimizing {
return aws.Int64Value(mod.TargetSize), nil
}

// The volume should go through the states "modifying", "optimizing", and finally "completed".
// If we get at this point, the volume isn't ready yet, so we wait for the correct state before we return.
// TODO: according to AWS docs, you can resize the file system as soon as the volume enters the optimizing state.
return c.waitForVolumeSize(ctx, volumeID)
}

// waitForVolumeSize waits for a volume modification to finish and return its size.
func (c *cloud) waitForVolumeSize(ctx context.Context, volumeID string) (int64, error) {
backoff := wait.Backoff{
Duration: 1 * time.Second,
Factor: 1.8,
@@ -864,26 +864,29 @@ func (c *cloud) ResizeDisk(ctx context.Context, volumeID string, reqSizeBytes in

var modVolSizeGiB int64
waitErr := wait.ExponentialBackoff(backoff, func() (bool, error) {
m, err := c.getVolumeModification(ctx, volumeID)
m, err := c.getLatestVolumeModification(ctx, volumeID)
if err != nil {
return false, err
}
if aws.StringValue(m.ModificationState) == ec2.VolumeModificationStateCompleted || aws.StringValue(m.ModificationState) == ec2.VolumeModificationStateOptimizing {

state := aws.StringValue(m.ModificationState)
if state == ec2.VolumeModificationStateCompleted || state == ec2.VolumeModificationStateOptimizing {
modVolSizeGiB = aws.Int64Value(m.TargetSize)
return true, nil
}
return true, nil

return false, nil
})

if waitErr != nil {
// Return negative number to avoid describing the volume
// to get the actual volume size after the modification failed.
return -1, waitErr
return 0, waitErr
}

return modVolSizeGiB, nil
}

func (c *cloud) getVolumeModification(ctx context.Context, volumeID string) (*ec2.VolumeModification, error) {
// getLatestVolumeModification returns the last modification of the volume.
func (c *cloud) getLatestVolumeModification(ctx context.Context, volumeID string) (*ec2.VolumeModification, error) {
request := &ec2.DescribeVolumesModificationsInput{
VolumeIds: []*string{
aws.String(volumeID),
11 changes: 0 additions & 11 deletions pkg/cloud/cloud_test.go
Original file line number Diff line number Diff line change
@@ -658,17 +658,6 @@ func TestResizeDisk(t *testing.T) {
reqSizeGiB: 2,
expErr: fmt.Errorf("ResizeDisk generic error"),
},
{
name: "fail: current volume has bigger size that requested",
volumeID: "vol-test",
existingVolume: &ec2.Volume{
VolumeId: aws.String("vol-test"),
Size: aws.Int64(10),
AvailabilityZone: aws.String(defaultZone),
},
reqSizeGiB: 5,
expErr: fmt.Errorf("ResizeDisk generic error"),
},
{
name: "sucess: there is a resizing in progress",
volumeID: "vol-test",
2 changes: 1 addition & 1 deletion pkg/driver/controller.go
Original file line number Diff line number Diff line change
@@ -333,7 +333,7 @@ func (d *controllerService) ControllerExpandVolume(ctx context.Context, req *csi
}

actualSizeGiB, err := d.cloud.ResizeDisk(ctx, volumeID, newSize)
if err != nil || actualSizeGiB < 0 {
if err != nil {
return nil, status.Errorf(codes.Internal, "Could not resize volume %q: %v", volumeID, err)
}

11 changes: 0 additions & 11 deletions pkg/driver/controller_test.go
Original file line number Diff line number Diff line change
@@ -1756,17 +1756,6 @@ func TestControllerExpandVolume(t *testing.T) {
},
expError: true,
},
{
name: "fail negative size from ResizeDisk",
req: &csi.ControllerExpandVolumeRequest{
VolumeId: "vol-test",
CapacityRange: &csi.CapacityRange{
RequiredBytes: 5 * util.GiB,
},
},
newSize: -1,
expError: true,
},
}

for _, tc := range testCases {
15 changes: 10 additions & 5 deletions pkg/driver/node.go
Original file line number Diff line number Diff line change
@@ -30,6 +30,7 @@ import (
"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"
"k8s.io/klog"
"k8s.io/kubernetes/pkg/util/mount"
"k8s.io/kubernetes/pkg/util/resizefs"
)

@@ -236,10 +237,8 @@ func (d *nodeService) NodeExpandVolume(ctx context.Context, req *csi.NodeExpandV
return nil, status.Error(codes.InvalidArgument, "Volume ID not provided")
}

// TODO: range specified

args := []string{"-o", "source", "--noheadings", "--target", req.GetVolumePath()}
output, err := d.mounter.Exec.Run("findmnt", args...)
output, err := d.mounter.Run("findmnt", args...)
if err != nil {
return nil, status.Errorf(codes.Internal, "Could not determine device path: %v", err)

@@ -250,9 +249,15 @@ func (d *nodeService) NodeExpandVolume(ctx context.Context, req *csi.NodeExpandV
return nil, status.Errorf(codes.Internal, "Could not get valid device for mount path: %q", req.GetVolumePath())
}

r := resizefs.NewResizeFs(d.mounter)
// TODO: refactor Mounter to expose a mount.SafeFormatAndMount object
r := resizefs.NewResizeFs(&mount.SafeFormatAndMount{
Interface: mount.New(""),
Exec: mount.NewOsExec(),
})

// TODO: lock per volume ID to have some idempotency
if _, err := r.Resize(devicePath, req.GetVolumePath()); err != nil {
return nil, status.Errorf(codes.Internal, "Could not resize volume %q: %v", volumeID, err)
return nil, status.Errorf(codes.Internal, "Could not resize volume %q (%q): %v", volumeID, devicePath, err)
}

return &csi.NodeExpandVolumeResponse{}, nil
2 changes: 1 addition & 1 deletion tests/sanity/fake_cloud_provider.go
Original file line number Diff line number Diff line change
@@ -219,5 +219,5 @@ func (c *fakeCloudProvider) ResizeDisk(ctx context.Context, volumeID string, new
return newSize, nil
}
}
return -1, cloud.ErrNotFound
return 0, cloud.ErrNotFound
}

0 comments on commit 84d17fd

Please sign in to comment.