comments improvements

Author: kon-angelo

pkg/csi/cinder/controllerserver.go

@@ -32,20 +32,26 @@ import (
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
 	"google.golang.org/protobuf/types/known/timestamppb"
-
-	"k8s.io/klog/v2"
-
 	sharedcsi "k8s.io/cloud-provider-openstack/pkg/csi"
 	"k8s.io/cloud-provider-openstack/pkg/csi/cinder/openstack"
 	"k8s.io/cloud-provider-openstack/pkg/util"
 	cpoerrors "k8s.io/cloud-provider-openstack/pkg/util/errors"
+	"k8s.io/klog/v2"
 )
 
 type controllerServer struct {
 	Driver *Driver
 	Clouds map[string]openstack.IOpenStack
 }
 
+type cloudsStartingToken struct {
+	// CloudName is the cloud that we have to list next.
+	CloudName string `json:"cloud"`
+	// Token is the pagination token returned by the last Cinder list volume operation. If empty, we will list all volumes
+	// from the beginning.
+	Token string `json:"token"`
+}
+
 const (
 	cinderCSIClusterIDKey = "cinder.csi.openstack.org/cluster"
 	affinityKey           = "cinder.csi.openstack.org/affinity"
@@ -403,11 +409,6 @@ func (cs *controllerServer) ControllerUnpublishVolume(ctx context.Context, req *
 	return &csi.ControllerUnpublishVolumeResponse{}, nil
 }
 
-type CloudsStartingToken struct {
-	CloudName string `json:"cloud"`
-	Token     string `json:"token"`
-}
-
 func (cs *controllerServer) extractNodeIDs(attachments []volumes.Attachment) []string {
 	nodeIDs := make([]string, len(attachments))
 	for i, attachment := range attachments {
@@ -441,10 +442,7 @@ func (cs *controllerServer) ListVolumes(ctx context.Context, req *csi.ListVolume
 	maxEntries := int(req.MaxEntries)
 
 	var err error
-	var cloudsToken = CloudsStartingToken{
-		CloudName: "",
-		Token:     "",
-	}
+	var cloudsToken = cloudsStartingToken{}
 
 	cloudsNames := maps.Keys(cs.Clouds)
 	sort.Strings(cloudsNames)
@@ -466,7 +464,9 @@ func (cs *controllerServer) ListVolumes(ctx context.Context, req *csi.ListVolume
 		}
 		idx++
 	}
-	volumeList, nextPageToken, err := cs.Clouds[cloudsNames[idx]].ListVolumes(ctx, maxEntries, startingToken)
+
+	var volumeList []volumes.Volume
+	volumeList, cloudsToken.Token, err = cs.Clouds[cloudsNames[idx]].ListVolumes(ctx, maxEntries, startingToken)
 	if err != nil {
 		klog.Errorf("Failed to ListVolumes: %v", err)
 		if cpoerrors.IsInvalidError(err) {
@@ -475,12 +475,11 @@ func (cs *controllerServer) ListVolumes(ctx context.Context, req *csi.ListVolume
 		return nil, status.Errorf(codes.Internal, "ListVolumes failed with error %v", err)
 	}
 	volumeEntries := cs.createVolumeEntries(volumeList)
-	klog.V(4).Infof("ListVolumes: retrieved %d entries and %q next token from cloud %q", len(volumeEntries), nextPageToken, cloudsNames[idx])
+	klog.V(4).Infof("ListVolumes: retrieved %d entries and %q next token from cloud %q", len(volumeEntries), cloudsToken.Token, cloudsNames[idx])
 
-	cloudsToken.Token = nextPageToken
 	switch {
 	// if we have not finished listing all volumes from this cloud, we will continue on next call.
-	case nextPageToken != "":
+	case cloudsToken.Token != "":
 		// if we listed all volumes from this cloud but more clouds exist, return a token of the next cloud.
 	case idx+1 < len(cloudsNames):
 		cloudsToken.CloudName = cloudsNames[idx+1]

pkg/csi/cinder/controllerserver_test.go

@@ -438,7 +438,7 @@ func genFakeVolumeEntry(fakeVol volumes.Volume) *csi.ListVolumesResponse_Entry {
 	}
 }
 func genFakeVolumeEntries(fakeVolumes []volumes.Volume) []*csi.ListVolumesResponse_Entry {
-	entries := make([]*csi.ListVolumesResponse_Entry, 0)
+	entries := make([]*csi.ListVolumesResponse_Entry, 0, len(fakeVolumes))
 	for _, fakeVol := range fakeVolumes {
 		entries = append(entries, genFakeVolumeEntry(fakeVol))
 	}
@@ -450,7 +450,7 @@ func TestListVolumes(t *testing.T) {
 
 	// Init assert
 	assert := assert.New(t)
-	token := CloudsStartingToken{
+	token := cloudsStartingToken{
 		CloudName: "",
 		Token:     FakeVolID,
 	}
@@ -476,7 +476,7 @@ func TestListVolumes(t *testing.T) {
 type ListVolumesTest struct {
 	name          string
 	maxEntries    int
-	startingToken *CloudsStartingToken
+	startingToken *cloudsStartingToken
 	volumeSet     map[string]ListVolumeTestOSMock
 	result        ListVolumesTestResult
 }
@@ -489,11 +489,11 @@ type ListVolumeTestOSMock struct {
 }
 
 type ListVolumesTestRequest struct {
-	StartingToken CloudsStartingToken
+	StartingToken cloudsStartingToken
 }
 
 type ListVolumesTestResult struct {
-	ExpectedToken *CloudsStartingToken
+	ExpectedToken *cloudsStartingToken
 	Entries       []*csi.ListVolumesResponse_Entry
 }
 
@@ -531,7 +531,7 @@ func TestGlobalListVolumesMultipleClouds(t *testing.T) {
 			},
 
 			result: ListVolumesTestResult{
-				ExpectedToken: &CloudsStartingToken{
+				ExpectedToken: &cloudsStartingToken{
 					Token: "vol2",
 				},
 				Entries: genFakeVolumeEntries([]volumes.Volume{
@@ -586,15 +586,15 @@ func TestGlobalListVolumesMultipleClouds(t *testing.T) {
 				},
 			},
 			result: ListVolumesTestResult{
-				ExpectedToken: &CloudsStartingToken{
+				ExpectedToken: &cloudsStartingToken{
 					CloudName: "region-x",
 				},
 				Entries: genFakeVolumeEntries([]volumes.Volume{}),
 			},
 		},
 		{
 			name: "cloud2_no_pagination_no_volumes",
-			startingToken: &CloudsStartingToken{
+			startingToken: &cloudsStartingToken{
 				CloudName: "region-x",
 			},
 			maxEntries: 0,
@@ -645,7 +645,7 @@ func TestGlobalListVolumesMultipleClouds(t *testing.T) {
 				},
 			},
 			result: ListVolumesTestResult{
-				ExpectedToken: &CloudsStartingToken{
+				ExpectedToken: &cloudsStartingToken{
 					CloudName: "region-x",
 				},
 				Entries: genFakeVolumeEntries([]volumes.Volume{
@@ -659,7 +659,7 @@ func TestGlobalListVolumesMultipleClouds(t *testing.T) {
 		{
 			name:       "cloud2_no_pagination_has_volumes",
 			maxEntries: 0,
-			startingToken: &CloudsStartingToken{
+			startingToken: &cloudsStartingToken{
 				CloudName: "region-x",
 			},
 			volumeSet: map[string]ListVolumeTestOSMock{
@@ -713,7 +713,7 @@ func TestGlobalListVolumesMultipleClouds(t *testing.T) {
 				},
 			},
 			result: ListVolumesTestResult{
-				ExpectedToken: &CloudsStartingToken{
+				ExpectedToken: &cloudsStartingToken{
 					CloudName: "region-x",
 				},
 				Entries: genFakeVolumeEntries([]volumes.Volume{}),
@@ -739,7 +739,7 @@ func TestGlobalListVolumesMultipleClouds(t *testing.T) {
 				},
 			},
 			result: ListVolumesTestResult{
-				ExpectedToken: &CloudsStartingToken{
+				ExpectedToken: &cloudsStartingToken{
 					CloudName: "region-x",
 				},
 				Entries: genFakeVolumeEntries([]volumes.Volume{
@@ -767,7 +767,7 @@ func TestGlobalListVolumesMultipleClouds(t *testing.T) {
 				},
 			},
 			result: ListVolumesTestResult{
-				ExpectedToken: &CloudsStartingToken{
+				ExpectedToken: &cloudsStartingToken{
 					CloudName: "region-x",
 				},
 				Entries: genFakeVolumeEntries([]volumes.Volume{}),
@@ -794,7 +794,7 @@ func TestGlobalListVolumesMultipleClouds(t *testing.T) {
 				},
 			},
 			result: ListVolumesTestResult{
-				ExpectedToken: &CloudsStartingToken{
+				ExpectedToken: &cloudsStartingToken{
 					CloudName: "region-x",
 				},
 				Entries: genFakeVolumeEntries([]volumes.Volume{
@@ -826,7 +826,7 @@ func TestGlobalListVolumesMultipleClouds(t *testing.T) {
 				},
 			},
 			result: ListVolumesTestResult{
-				ExpectedToken: &CloudsStartingToken{
+				ExpectedToken: &cloudsStartingToken{
 					CloudName: "",
 					Token:     "vol2",
 				},
@@ -839,7 +839,7 @@ func TestGlobalListVolumesMultipleClouds(t *testing.T) {
 		{
 			name:       "cloud1_3volume_cloud2_1volume_2st_call",
 			maxEntries: 2,
-			startingToken: &CloudsStartingToken{
+			startingToken: &cloudsStartingToken{
 				CloudName: "region-x",
 				Token:     "",
 			},

pkg/csi/cinder/openstack/openstack_volumes.go

@@ -30,6 +30,7 @@ import (
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
 	"k8s.io/apimachinery/pkg/util/wait"
+
 	"k8s.io/cloud-provider-openstack/pkg/metrics"
 	"k8s.io/cloud-provider-openstack/pkg/util"
 	cpoerrors "k8s.io/cloud-provider-openstack/pkg/util/errors"
@@ -82,8 +83,18 @@ func (os *OpenStack) ListVolumes(ctx context.Context, limit int, startingToken s
 	var nextPageToken string
 	var vols []volumes.Volume
 
-	opts := volumes.ListOpts{Limit: limit, Marker: startingToken}
 	mc := metrics.NewMetricContext("volume", "list")
+	opts := volumes.ListOpts{Limit: limit, Marker: startingToken}
+	if limit == 0 {
+		page, err := volumes.List(os.blockstorage, opts).AllPages(ctx)
+		if err != nil {
+			return nil, "", err
+		}
+		vols, err = volumes.ExtractVolumes(page)
+		if mc.ObserveRequest(err) != nil {
+			return vols, "", err
+		}
+	}
 	err := volumes.List(os.blockstorage, opts).EachPage(ctx, func(_ context.Context, page pagination.Page) (bool, error) {
 		var err error