[cinder-csi-plugin]: fix pagination, avoid unnecessary memory allocat…

…ion, add more logs (#2296) * [cinder]: avoid unnecessary memory allocation with more logs * Fix pagination parsing
kubernetes · Jul 18, 2023 · 95dc81c · 95dc81c
1 parent a2c86f9
commit 95dc81c
Show file tree

Hide file tree

Showing 4 changed files with 14 additions and 7 deletions.
diff --git a/pkg/csi/cinder/controllerserver.go b/pkg/csi/cinder/controllerserver.go
@@ -280,6 +280,7 @@ func (cs *controllerServer) ControllerUnpublishVolume(ctx context.Context, req *
 }
 
 func (cs *controllerServer) ListVolumes(ctx context.Context, req *csi.ListVolumesRequest) (*csi.ListVolumesResponse, error) {
+	klog.V(4).Infof("ListVolumes: called with %+#v request", req)
 
 	if req.MaxEntries < 0 {
 		return nil, status.Error(codes.InvalidArgument, fmt.Sprintf(
@@ -296,7 +297,7 @@ func (cs *controllerServer) ListVolumes(ctx context.Context, req *csi.ListVolume
 		return nil, status.Error(codes.Internal, fmt.Sprintf("ListVolumes failed with error %v", err))
 	}
 
-	var ventries []*csi.ListVolumesResponse_Entry
+	ventries := make([]*csi.ListVolumesResponse_Entry, 0, len(vlist))
 	for _, v := range vlist {
 		ventry := csi.ListVolumesResponse_Entry{
 			Volume: &csi.Volume{
@@ -306,13 +307,16 @@ func (cs *controllerServer) ListVolumes(ctx context.Context, req *csi.ListVolume
 		}
 
 		status := &csi.ListVolumesResponse_VolumeStatus{}
+		status.PublishedNodeIds = make([]string, 0, len(v.Attachments))
 		for _, attachment := range v.Attachments {
 			status.PublishedNodeIds = append(status.PublishedNodeIds, attachment.ServerID)
 		}
 		ventry.Status = status
 
 		ventries = append(ventries, &ventry)
 	}
+
+	klog.V(4).Infof("ListVolumes: completed with %d entries and %q next token", len(ventries), nextPageToken)
 	return &csi.ListVolumesResponse{
 		Entries:   ventries,
 		NextToken: nextPageToken,
@@ -478,7 +482,7 @@ func (cs *controllerServer) ListSnapshots(ctx context.Context, req *csi.ListSnap
 		return nil, status.Errorf(codes.Internal, "ListSnapshots failed with error %v", err)
 	}
 
-	var sentries []*csi.ListSnapshotsResponse_Entry
+	sentries := make([]*csi.ListSnapshotsResponse_Entry, 0, len(slist))
 	for _, v := range slist {
 		ctime := timestamppb.New(v.CreatedAt)
 		if err := ctime.CheckValid(); err != nil {
@@ -582,6 +586,7 @@ func (cs *controllerServer) ControllerGetVolume(ctx context.Context, req *csi.Co
 	}
 
 	status := &csi.ControllerGetVolumeResponse_VolumeStatus{}
+	status.PublishedNodeIds = make([]string, 0, len(volume.Attachments))
 	for _, attachment := range volume.Attachments {
 		status.PublishedNodeIds = append(status.PublishedNodeIds, attachment.ServerID)
 	}

diff --git a/pkg/csi/cinder/controllerserver_test.go b/pkg/csi/cinder/controllerserver_test.go
@@ -441,7 +441,9 @@ func TestListVolumes(t *testing.T) {
 					VolumeId:      FakeVol3.ID,
 					CapacityBytes: int64(FakeVol3.Size * 1024 * 1024 * 1024),
 				},
-				Status: &csi.ListVolumesResponse_VolumeStatus{},
+				Status: &csi.ListVolumesResponse_VolumeStatus{
+					PublishedNodeIds: []string{},
+				},
 			},
 		},
 		NextToken: "",

diff --git a/pkg/csi/cinder/openstack/openstack_snapshots.go b/pkg/csi/cinder/openstack/openstack_snapshots.go
@@ -119,11 +119,11 @@ func (os *OpenStack) ListSnapshots(filters map[string]string) ([]snapshots.Snaps
 		}
 
 		if nextPageURL != "" {
-			queryParams, err := url.ParseQuery(nextPageURL)
+			pageURL, err := url.Parse(nextPageURL)
 			if err != nil {
 				return false, err
 			}
-			nextPageToken = queryParams.Get("marker")
+			nextPageToken = pageURL.Query().Get("marker")
 		}
 
 		return false, nil

diff --git a/pkg/csi/cinder/openstack/openstack_volumes.go b/pkg/csi/cinder/openstack/openstack_volumes.go
@@ -96,11 +96,11 @@ func (os *OpenStack) ListVolumes(limit int, startingToken string) ([]volumes.Vol
 		}
 
 		if nextPageURL != "" {
-			queryParams, err := url.ParseQuery(nextPageURL)
+			pageURL, err := url.Parse(nextPageURL)
 			if err != nil {
 				return false, err
 			}
-			nextPageToken = queryParams.Get("marker")
+			nextPageToken = pageURL.Query().Get("marker")
 		}
 
 		return false, nil