IP Reservation functionality

Addressing PR comments

Fixing build

Fixing nits

Fixing commenting issues

Fixing review comments

Test

pkg/cloud_provider/file/fake.go

@@ -73,9 +73,9 @@ func (manager *fakeServiceManager) GetInstance(ctx context.Context, obj *Service
 	}
 }
 
-func (manager *fakeServiceManager) ListInstances(ctx context.Context, parent string) ([]*ServiceInstance, error) {
+func (manager *fakeServiceManager) ListInstances(ctx context.Context, obj *ServiceInstance) ([]*ServiceInstance, error) {
 	instances := []*ServiceInstance{
-		&ServiceInstance{
+		{
 			Project:  "test-project",
 			Location: "test-location",
 			Name:     "test",
@@ -84,7 +84,7 @@ func (manager *fakeServiceManager) ListInstances(ctx context.Context, parent str
 				ReservedIpRange: "192.168.92.32/29",
 			},
 		},
-		&ServiceInstance{
+		{
 			Project:  "test-project",
 			Location: "test-location",
 			Name:     "test",

pkg/cloud_provider/file/file.go

@@ -56,7 +56,7 @@ type Service interface {
 	CreateInstance(ctx context.Context, obj *ServiceInstance) (*ServiceInstance, error)
 	DeleteInstance(ctx context.Context, obj *ServiceInstance) error
 	GetInstance(ctx context.Context, obj *ServiceInstance) (*ServiceInstance, error)
-	ListInstances(ctx context.Context, parent string) ([]*ServiceInstance, error)
+	ListInstances(ctx context.Context, obj *ServiceInstance) ([]*ServiceInstance, error)
 }
 
 type gcfsServiceManager struct {
@@ -232,11 +232,14 @@ func (manager *gcfsServiceManager) DeleteInstance(ctx context.Context, obj *Serv
 	return nil
 }
 
-func (manager *gcfsServiceManager) ListInstances(ctx context.Context, parent string) ([]*ServiceInstance, error) {
-	instances, err := manager.instancesService.List(parent).Context(ctx).Do()
+// ListInstances returns a list of active instances in a project at a specific location
+func (manager *gcfsServiceManager) ListInstances(ctx context.Context, obj *ServiceInstance) ([]*ServiceInstance, error) {
+	// Calling cloud provider service to get list of active instances. - indicates we are looking for instances in all the locations for a project
+	instances, err := manager.instancesService.List(locationURI(obj.Project, "-")).Context(ctx).Do()
 	if err != nil {
 		return nil, err
 	}
+
 	var activeInstances []*ServiceInstance
 	for _, activeInstance := range instances.Instances {
 		serviceInstance, err := cloudInstanceToServiceInstance(activeInstance)

pkg/csi_driver/controller.go

@@ -19,7 +19,6 @@ package driver
 import (
 	"fmt"
 	"strings"
-	"sync"
 
 	csi "github.com/container-storage-interface/spec/lib/go/csi/v0"
 	"github.com/golang/glog"
@@ -52,7 +51,7 @@ const (
 	paramTier             = "tier"
 	paramLocation         = "location"
 	paramNetwork          = "network"
-	paramReservedIPV4CIDR = "cidr"
+	paramReservedIPV4CIDR = "reserved-ipv4-cidr"
 )
 
 // controllerServer handles volume provisioning
@@ -61,14 +60,14 @@ type controllerServer struct {
 }
 
 type controllerServerConfig struct {
-	driver           *GCFSDriver
-	fileService      file.Service
-	metaService      metadata.Service
-	reservedIPRanges map[string]bool
-	mutex            sync.Mutex
+	driver      *GCFSDriver
+	fileService file.Service
+	metaService metadata.Service
+	ipAllocator *util.IPAllocator
 }
 
 func newControllerServer(config *controllerServerConfig) csi.ControllerServer {
+	config.ipAllocator = util.NewIPAllocator(make(map[string]bool))
 	return &controllerServer{config: config}
 }
 
@@ -85,16 +84,7 @@ func (s *controllerServer) CreateVolume(ctx context.Context, req *csi.CreateVolu
 	if err := s.config.driver.validateVolumeCapabilities(req.GetVolumeCapabilities()); err != nil {
 		return nil, status.Error(codes.InvalidArgument, err.Error())
 	}
-	s.config.mutex.Lock()
-	instances, err := s.config.fileService.ListInstances(ctx, "-")
-	if err != nil {
-		return nil, status.Error(codes.Aborted, err.Error())
-	}
 
-	s.config.reservedIPRanges = make(map[string]bool)
-	for _, instance := range instances {
-		s.config.reservedIPRanges[instance.Network.ReservedIpRange] = true
-	}
 	capBytes := getRequestCapacity(req.GetCapacityRange())
 	glog.V(5).Infof("Using capacity bytes %q for volume %q", capBytes, name)
 
@@ -114,16 +104,64 @@ func (s *controllerServer) CreateVolume(ctx context.Context, req *csi.CreateVolu
 			return nil, status.Error(codes.AlreadyExists, err.Error())
 		}
 	} else {
+		// If we are creating a new instance, we need pick an unused /29 range from reserved-ipv4-cidr
+		// If the param was not provided, we default reservedIPRange to "" and cloud provider takes care of the allocation
+		if reservedIPV4CIDR, ok := req.GetParameters()[paramReservedIPV4CIDR]; ok {
+			validCIDR := s.config.ipAllocator.ValidateCIDR(reservedIPV4CIDR)
+			if !validCIDR {
+				return nil, fmt.Errorf("invalid reserved-ipv4-cidr %s", reservedIPV4CIDR)
+			}
+
+			reservedIPRange, err := s.reserveIPRange(ctx, newFiler, reservedIPV4CIDR)
+
+			// Possible cases are 1) CreateInstanceAborted, 2)CreateInstance running in background
+			// The ListInstances response will contain the reservedIPRange if the operation was started
+			// In case of abort, the /29 IP is released and available for reservation
+			defer s.config.ipAllocator.ReleaseIPRange(reservedIPRange)
+			if err != nil {
+				return nil, err
+			}
+
+			// Adding the reserved IP range to the instance object
+			newFiler.Network.ReservedIpRange = reservedIPRange
+		}
+
 		// Create the instance
 		filer, err = s.config.fileService.CreateInstance(ctx, newFiler)
 		if err != nil {
 			return nil, status.Error(codes.Internal, err.Error())
 		}
 	}
-	s.config.mutex.Unlock()
 	return &csi.CreateVolumeResponse{Volume: fileInstanceToCSIVolume(filer, modeInstance)}, nil
 }
 
+// reserveIPRange returns the available IP in the cidr
+func (s *controllerServer) reserveIPRange(ctx context.Context, filer *file.ServiceInstance, cidr string) (string, error) {
+	cloudInstancesReservedIPRanges, err := s.getCloudInstancesReservedIPRanges(ctx, filer)
+	if err != nil {
+		return "", err
+	}
+	unreservedIPBlock, err := s.config.ipAllocator.GetUnreservedIPRange(cidr, cloudInstancesReservedIPRanges)
+	if err != nil {
+		return "", err
+	}
+	return unreservedIPBlock, nil
+}
+
+//getCloudInstancesReservedIPRanges gets the list of reservedIPRanges from cloud instances
+func (s *controllerServer) getCloudInstancesReservedIPRanges(ctx context.Context, filer *file.ServiceInstance) (map[string]bool, error) {
+	instances, err := s.config.fileService.ListInstances(ctx, filer)
+	if err != nil {
+		return nil, status.Error(codes.Aborted, err.Error())
+	}
+	// Initialize an empty reserved list. It will be populated with all the reservedIPRanges obtained from the cloud instances
+	cloudInstancesReservedIPRanges := make(map[string]bool)
+	for _, instance := range instances {
+		cloudInstancesReservedIPRanges[instance.Network.ReservedIpRange] = true
+	}
+	return cloudInstancesReservedIPRanges, nil
+}
+
 // DeleteVolume deletes a GCFS instance
 func (s *controllerServer) DeleteVolume(ctx context.Context, req *csi.DeleteVolumeRequest) (*csi.DeleteVolumeResponse, error) {
 	glog.V(4).Infof("DeleteVolume called with request %v", *req)
@@ -225,7 +263,6 @@ func (s *controllerServer) generateNewFileInstance(name string, capBytes int64,
 	tier := defaultTier
 	network := defaultNetwork
 	location := s.config.metaService.GetZone()
-	reservedIPV4CIDR := ""
 	// Validate parameters (case-insensitive).
 	for k, v := range params {
 		switch strings.ToLower(k) {
@@ -236,15 +273,11 @@ func (s *controllerServer) generateNewFileInstance(name string, capBytes int64,
 			location = v
 		case paramNetwork:
 			network = v
-		case paramReservedIPV4CIDR:
-			reservedIPV4CIDR, err := util.GetUnreservedIPBlock(s.config.reservedIPRanges, v)
-			if err != nil {
-				return nil, err
-			}
-			if reservedIPV4CIDR == "" {
-				return nil, fmt.Errorf("Invalid unreserved IP block received for cidr %s", v)
-			}
 
+		// Ignore the cidr flag as it is not passed to the cloud provider
+		// It will be used to get unreserved IP in the reserveIPV4Range function
+		case paramReservedIPV4CIDR:
+			continue
 		case "csiprovisionersecretname", "csiprovisionersecretnamespace":
 		default:
 			return nil, fmt.Errorf("invalid parameter %q", k)
@@ -256,8 +289,7 @@ func (s *controllerServer) generateNewFileInstance(name string, capBytes int64,
 		Location: location,
 		Tier:     tier,
 		Network: file.Network{
-			Name:            network,
-			ReservedIpRange: reservedIPV4CIDR,
+			Name: network,
 		},
 		Volume: file.Volume{
 			Name:      newInstanceVolume,

pkg/csi_driver/controller_test.go

@@ -28,12 +28,13 @@ import (
 )
 
 const (
-	testProject   = "test-project"
-	testLocation  = "test-location"
-	testIp        = "test-ip"
-	testCSIVolume = "test-csi"
-	testVolumeId  = "modeInstance/test-location/test-csi/vol1"
-	testBytes     = 1 * util.Tb
+	testProject          = "test-project"
+	testLocation         = "test-location"
+	testIp               = "test-ip"
+	testCSIVolume        = "test-csi"
+	testVolumeId         = "modeInstance/test-location/test-csi/vol1"
+	testReservedIPV4CIDR = "192.168.92.0/26"
+	testBytes            = 1 * util.Tb
 )
 
 func initTestController(t *testing.T) csi.ControllerServer {