From 77952fb38322067b73ddc5482c3b738429fc35c4 Mon Sep 17 00:00:00 2001 From: David Zhu Date: Fri, 2 Mar 2018 12:12:35 -0800 Subject: [PATCH] Bugfix: PV should use capacity bytes returned by plugin, not PVC bytes --- pkg/controller/controller.go | 28 ++++++++- pkg/controller/controller_test.go | 96 +++++++++++++++++++++++++++++++ 2 files changed, 122 insertions(+), 2 deletions(-) diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go index 44a97831ea..4a732e248a 100644 --- a/pkg/controller/controller.go +++ b/pkg/controller/controller.go @@ -23,6 +23,9 @@ import ( "strings" "time" + "k8s.io/apimachinery/pkg/api/resource" + _ "k8s.io/apimachinery/pkg/util/json" + "github.com/golang/glog" "github.com/kubernetes-incubator/external-storage/lib/controller" @@ -289,7 +292,6 @@ func (p *csiProvisioner) Provision(options controller.VolumeOptions) (*v1.Persis }, CapacityRange: &csi.CapacityRange{ RequiredBytes: int64(volSizeBytes), - LimitBytes: int64(volSizeBytes), }, } rep := &csi.CreateVolumeResponse{} @@ -336,6 +338,28 @@ func (p *csiProvisioner) Provision(options controller.VolumeOptions) (*v1.Persis for k, v := range rep.Volume.Attributes { volumeAttributes[k] = v } + respCap := rep.GetVolume().GetCapacityBytes() + if respCap < volSizeBytes { + capErr := fmt.Errorf("created volume capacity %v less than requested capacity %v", respCap, volSizeBytes) + delReq := &csi.DeleteVolumeRequest{ + VolumeId: rep.GetVolume().GetId(), + } + if options.Parameters != nil { + credentials, err := getCredentialsFromParameters(p.client, options.Parameters) + if err != nil { + return nil, err + } + delReq.ControllerDeleteSecrets = credentials + } + ctx, cancel := context.WithTimeout(context.Background(), p.timeout) + defer cancel() + _, err := p.csiClient.DeleteVolume(ctx, delReq) + if err != nil { + capErr = fmt.Errorf("%v. Cleanup of volume %s failed, volume is orphaned: %v", capErr, share, err) + } + return nil, capErr + } + repBytesString := fmt.Sprintf("%v", respCap) pv := &v1.PersistentVolume{ ObjectMeta: metav1.ObjectMeta{ Name: share, @@ -344,7 +368,7 @@ func (p *csiProvisioner) Provision(options controller.VolumeOptions) (*v1.Persis PersistentVolumeReclaimPolicy: options.PersistentVolumeReclaimPolicy, AccessModes: options.PVC.Spec.AccessModes, Capacity: v1.ResourceList{ - v1.ResourceName(v1.ResourceStorage): options.PVC.Spec.Resources.Requests[v1.ResourceName(v1.ResourceStorage)], + v1.ResourceName(v1.ResourceStorage): resource.MustParse(repBytesString), }, // TODO wait for CSI VolumeSource API PersistentVolumeSource: v1.PersistentVolumeSource{ diff --git a/pkg/controller/controller_test.go b/pkg/controller/controller_test.go index cbd6a0daf6..ceebf23382 100644 --- a/pkg/controller/controller_test.go +++ b/pkg/controller/controller_test.go @@ -18,13 +18,18 @@ package controller import ( "fmt" + "strconv" "testing" "time" csi "github.com/container-storage-interface/spec/lib/go/csi/v0" "github.com/golang/mock/gomock" "github.com/kubernetes-csi/csi-test/driver" + "github.com/kubernetes-incubator/external-storage/lib/controller" "google.golang.org/grpc" + "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) const ( @@ -384,3 +389,94 @@ func TestGetDriverName(t *testing.T) { } } } + +func TestCreateDriverReturnsInvalidCapacityDuringProvision(t *testing.T) { + // Set up mocks + var requestedBytes int64 = 100 + mockController, driver, identityServer, controllerServer, csiConn, err := createMockServer(t) + if err != nil { + t.Fatal(err) + } + defer mockController.Finish() + defer driver.Stop() + + csiProvisioner := NewCSIProvisioner(nil, driver.Address(), 5*time.Second, "test-provisioner", "test", 5, csiConn.conn) + + // Requested PVC with requestedBytes storage + opts := controller.VolumeOptions{ + PersistentVolumeReclaimPolicy: v1.PersistentVolumeReclaimDelete, + PVName: "test-name", + PVC: createFakePVC(requestedBytes), + Parameters: map[string]string{}, + } + + // Drivers CreateVolume response with lower capacity bytes than request + out := &csi.CreateVolumeResponse{ + Volume: &csi.Volume{ + CapacityBytes: requestedBytes - 1, + Id: "test-volume-id", + }, + } + + // Set up Mocks + provisionMockServerSetupExpectations(identityServer, controllerServer) + controllerServer.EXPECT().CreateVolume(gomock.Any(), gomock.Any()).Return(out, nil).Times(1) + // Since capacity returned by driver is invalid, we expect the provision call to clean up the volume + controllerServer.EXPECT().DeleteVolume(gomock.Any(), &csi.DeleteVolumeRequest{ + VolumeId: "test-volume-id", + }).Return(&csi.DeleteVolumeResponse{}, nil).Times(1) + + // Call provision + _, err = csiProvisioner.Provision(opts) + if err == nil { + t.Errorf("Provision did not cause an error when one was expected") + return + } + t.Logf("Provision encountered an error: %v, expected: create volume capacity less than requested capacity", err) +} + +func provisionMockServerSetupExpectations(identityServer *driver.MockIdentityServer, controllerServer *driver.MockControllerServer) { + identityServer.EXPECT().GetPluginCapabilities(gomock.Any(), gomock.Any()).Return(&csi.GetPluginCapabilitiesResponse{ + Capabilities: []*csi.PluginCapability{ + &csi.PluginCapability{ + Type: &csi.PluginCapability_Service_{ + Service: &csi.PluginCapability_Service{ + Type: csi.PluginCapability_Service_CONTROLLER_SERVICE, + }, + }, + }, + }, + }, nil).Times(1) + controllerServer.EXPECT().ControllerGetCapabilities(gomock.Any(), gomock.Any()).Return(&csi.ControllerGetCapabilitiesResponse{ + Capabilities: []*csi.ControllerServiceCapability{ + &csi.ControllerServiceCapability{ + Type: &csi.ControllerServiceCapability_Rpc{ + Rpc: &csi.ControllerServiceCapability_RPC{ + Type: csi.ControllerServiceCapability_RPC_CREATE_DELETE_VOLUME, + }, + }, + }, + }, + }, nil).Times(1) + identityServer.EXPECT().GetPluginInfo(gomock.Any(), gomock.Any()).Return(&csi.GetPluginInfoResponse{ + Name: "test-driver", + VendorVersion: "test-vendor", + }, nil).Times(1) +} + +// Minimal PVC required for tests to function +func createFakePVC(requestBytes int64) *v1.PersistentVolumeClaim { + return &v1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + UID: "testid", + }, + Spec: v1.PersistentVolumeClaimSpec{ + Selector: nil, // Provisioner doesn't support selector + Resources: v1.ResourceRequirements{ + Requests: v1.ResourceList{ + v1.ResourceName(v1.ResourceStorage): resource.MustParse(strconv.FormatInt(requestBytes, 10)), + }, + }, + }, + } +}