Skip to content

Fix Resize call to return size in Gi as expected when disk is already the request size or larger #462

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

Merged
merged 1 commit into from
Feb 4, 2020
Merged

Fix Resize call to return size in Gi as expected when disk is already the request size or larger #462

Show file tree
Hide file tree
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
9 changes: 7 additions & 2 deletions pkg/gce-cloud-provider/compute/gce-compute.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -620,6 +620,10 @@ func (cloud *CloudProvider) CreateSnapshot(ctx context.Context, volKey *meta.Key
}
}

// ResizeDisk takes in the requested disk size in bytes and returns the resized
// size in Gi
// TODO(#461) The whole driver could benefit from standardized usage of the
// k8s.io/apimachinery/quantity package for better size handling
func (cloud *CloudProvider) ResizeDisk(ctx context.Context, volKey *meta.Key, requestBytes int64) (int64, error) {
klog.V(5).Infof("Resizing disk %v to size %v", volKey, requestBytes)
cloudDisk, err := cloud.GetDisk(ctx, volKey)
Expand All @@ -630,9 +634,10 @@ func (cloud *CloudProvider) ResizeDisk(ctx context.Context, volKey *meta.Key, re
sizeGb := cloudDisk.GetSizeGb()
requestGb := common.BytesToGb(requestBytes)

// If disk is already of size equal or greater than requested size, we simply return
// If disk is already of size equal or greater than requested size, we
// simply return the found size
if sizeGb >= requestGb {
return requestBytes, nil
return sizeGb, nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gnufied also identified this as one of the problematic driver responses to resize. We should be returning the actual size, not the requested size.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nm it's too early in the morning to read properly :-)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a unit test for this case that verifies we return actual size instead of request?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's really hard to unit test this package right now as there was no easy way to fake out the actual cloud provider layer. So this "is" the cloud provider layer that I hacked together that we use as an interface to fake out. Suggestions welcome. It's in the backlog to switch over to the autogenerated cloud provider client layer that we switched over to in Kubernetes a while back - that provided the ability to fake out calls and unit test these types of things effectively.

}

switch volKey.Type() {
Expand Down
16 changes: 8 additions & 8 deletions pkg/gce-pd-csi-driver/node.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -436,45 +436,45 @@ func (ns *GCENodeServer) NodeGetVolumeStats(ctx context.Context, req *csi.NodeGe
func (ns *GCENodeServer) NodeExpandVolume(ctx context.Context, req *csi.NodeExpandVolumeRequest) (*csi.NodeExpandVolumeResponse, error) {
volumeID := req.GetVolumeId()
if len(volumeID) == 0 {
return nil, status.Error(codes.InvalidArgument, "ControllerExpandVolume volume ID must be provided")
return nil, status.Error(codes.InvalidArgument, "volume ID must be provided")
}
capacityRange := req.GetCapacityRange()
reqBytes, err := getRequestCapacity(capacityRange)
if err != nil {
return nil, status.Error(codes.InvalidArgument, fmt.Sprintf("ControllerExpandVolume capacity range is invalid: %v", err))
return nil, status.Error(codes.InvalidArgument, fmt.Sprintf("capacity range is invalid: %v", err))
}

volumePath := req.GetVolumePath()
if len(volumePath) == 0 {
return nil, status.Error(codes.InvalidArgument, "ControllerExpandVolume volume path must be provided")
return nil, status.Error(codes.InvalidArgument, "volume path must be provided")
}

volKey, err := common.VolumeIDToKey(volumeID)
if err != nil {
return nil, status.Error(codes.InvalidArgument, fmt.Sprintf("ControllerExpandVolume Volume ID is invalid: %v", err))
return nil, status.Error(codes.InvalidArgument, fmt.Sprintf("volume ID is invalid: %v", err))
}

devicePath, err := ns.getDevicePath(volumeID, "")
if err != nil {
return nil, status.Error(codes.Internal, fmt.Sprintf("ControllerExpandVolume error when getting device path for %s: %v", volumeID, err))
return nil, status.Error(codes.Internal, fmt.Sprintf("error when getting device path for %s: %v", volumeID, err))
}

// TODO(#328): Use requested size in resize if provided
resizer := resizefs.NewResizeFs(ns.Mounter)
_, err = resizer.Resize(devicePath, volumePath)
if err != nil {
return nil, status.Error(codes.Internal, fmt.Sprintf("ControllerExpandVolume error when resizing volume %s: %v", volKey.String(), err))
return nil, status.Error(codes.Internal, fmt.Sprintf("error when resizing volume %s: %v", volKey.String(), err))

}

// Check the block size
gotBlockSizeBytes, err := ns.getBlockSizeBytes(devicePath)
if err != nil {
return nil, status.Error(codes.Internal, fmt.Sprintf("ControllerExpandVolume error when getting size of block volume at path %s: %v", devicePath, err))
return nil, status.Error(codes.Internal, fmt.Sprintf("error when getting size of block volume at path %s: %v", devicePath, err))
}
if gotBlockSizeBytes < reqBytes {
// It's possible that the somewhere the volume size was rounded up, getting more size than requested is a success :)
return nil, status.Errorf(codes.Internal, "ControllerExpandVolume resize requested for %v but after resize volume was size %v", reqBytes, gotBlockSizeBytes)
return nil, status.Errorf(codes.Internal, "resize requested for %v but after resize volume was size %v", reqBytes, gotBlockSizeBytes)
}

// TODO(dyzz) Some sort of formatted volume could also check the fs size.
Expand Down