diff --git a/pkg/cloud/cloud.go b/pkg/cloud/cloud.go index 1408320fd7..ebfe39d1a0 100644 --- a/pkg/cloud/cloud.go +++ b/pkg/cloud/cloud.go @@ -801,11 +801,9 @@ 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), @@ -813,49 +811,51 @@ func (c *cloud) ResizeDisk(ctx context.Context, volumeID string, reqSizeBytes in } 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), diff --git a/pkg/cloud/cloud_test.go b/pkg/cloud/cloud_test.go index c5c117b044..7a040445f0 100644 --- a/pkg/cloud/cloud_test.go +++ b/pkg/cloud/cloud_test.go @@ -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", diff --git a/pkg/driver/controller.go b/pkg/driver/controller.go index 66398230f0..db23a3e58e 100644 --- a/pkg/driver/controller.go +++ b/pkg/driver/controller.go @@ -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) } diff --git a/pkg/driver/controller_test.go b/pkg/driver/controller_test.go index f5a582625b..fff2edb1de 100644 --- a/pkg/driver/controller_test.go +++ b/pkg/driver/controller_test.go @@ -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 { diff --git a/pkg/driver/node.go b/pkg/driver/node.go index 51c7522f56..2d67b09c9c 100644 --- a/pkg/driver/node.go +++ b/pkg/driver/node.go @@ -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 diff --git a/tests/sanity/fake_cloud_provider.go b/tests/sanity/fake_cloud_provider.go index 208c741ef6..2b532959a4 100644 --- a/tests/sanity/fake_cloud_provider.go +++ b/tests/sanity/fake_cloud_provider.go @@ -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 }