Support Pagination for ClusterGroupMembership #3183

qiyueyao · 2022-01-10T23:43:11Z

To resolve #2492 .

Pagination:
API query: kubectl proxy &
curl "<k8s-apiserver>:8001/apis/controlplane.antrea.io/v1beta2/clustergroupmembers/<name>?page=<page#>&limit=<limit#>"
!! Notice the double quote, it's very important as curl would not interpret the & otherwise.
Page: the specific page to return in the sorted member list
Limit: max number of members to return per page
Response:

{
  "kind": "GroupMemberList",
  "apiVersion": "controlplane.antrea.tanzu.vmware.com/v1beta2",
  "metadata": {
    "name": <name>,
    "creationTimestamp": null
  },
  "effectiveMembers": [],
  "effectiveIPBlocks": null,
  "totalMembers": <number of members in entire list>,
  "currentPage": <page of response>,
}

Change:

Changed the Getter of cluster group membership querier to GetterWithOptions so it can take in more options.
Changed ClusterGroupMembers struct to include total members and current page info in the response.
Added PaginationGetOptions to both controlplane and v1beta2 types, registered in controlplane, v1beta2, and legacy APIs.
Changed parameterCodec from metav1 version to runtime scheme codec in pkg/apiserver/apiserver.go.
Added UT specifically for pagination Get.

Signed-off-by: Qiyue Yao yaoq@vmware.com

codecov-commenter · 2022-01-10T23:48:59Z

Codecov Report

Merging #3183 (13e0f70) into main (c6926ed) will increase coverage by 2.31%.
The diff coverage is 84.21%.

@@            Coverage Diff             @@
##             main    #3183      +/-   ##
==========================================
+ Coverage   61.03%   63.34%   +2.31%     
==========================================
  Files         268      268              
  Lines       26872    26907      +35     
==========================================
+ Hits        16400    17043     +643     
+ Misses       8654     7999     -655     
- Partials     1818     1865      +47

Flag	Coverage Δ
kind-e2e-tests	`51.09% <10.52%> (+3.64%)`	⬆️
unit-tests	`42.45% <77.77%> (+0.03%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/apis/controlplane/types.go	`0.00% <ø> (ø)`
.../registry/networkpolicy/clustergroupmember/rest.go	`84.31% <82.85%> (-3.93%)`	⬇️
pkg/apis/controlplane/register.go	`90.47% <100.00%> (+0.47%)`	⬆️
pkg/apis/controlplane/v1beta2/register.go	`95.23% <100.00%> (+0.23%)`	⬆️
pkg/apiserver/apiserver.go	`89.47% <100.00%> (+1.50%)`	⬆️
pkg/agent/controller/networkpolicy/reject.go	`78.22% <0.00%> (-9.68%)`	⬇️
...agent/flowexporter/connections/deny_connections.go	`83.07% <0.00%> (-3.08%)`	⬇️
pkg/agent/cniserver/ipam/ipam_service.go	`52.17% <0.00%> (-2.18%)`	⬇️
pkg/ipam/poolallocator/allocator.go	`50.00% <0.00%> (-1.16%)`	⬇️
...gent/controller/noderoute/node_route_controller.go	`54.91% <0.00%> (-1.10%)`	⬇️
... and 26 more

Dyanngg

Implementation looks good overall

pkg/apis/controlplane/v1beta2/generated.proto

pkg/apis/controlplane/v1beta2/types.go

pkg/apiserver/registry/networkpolicy/clustergroupmember/rest.go

pkg/apis/controlplane/types.go

pkg/apiserver/registry/networkpolicy/clustergroupmember/rest.go

Dyanngg · 2022-02-09T00:18:31Z

LGTM overall, will let other reviewers take a look as well

pkg/apis/controlplane/v1beta2/conversion.go

tnqn · 2022-02-09T15:43:18Z

pkg/apis/controlplane/types.go

+
+// PaginationGetOptions is used to retrieve page number and page limit info from the request.
+type PaginationGetOptions struct {
+	metav1.TypeMeta `json:",inline"`


internal type doesn't need json tag as it will not be serialized

Keeping the jason tag as needed by +k8s:conversion-gen:explicit-from=net/url.Values to autogenerate.🤔

tnqn · 2022-02-09T15:47:50Z

pkg/legacyapis/controlplane/register.go

@@ -58,6 +58,7 @@ func addKnownTypes(scheme *runtime.Scheme) error {
 		&newcontrolplane.NetworkPolicyStatus{},
 		&newcontrolplane.NodeStatsSummary{},
 		&newcontrolplane.ClusterGroupMembers{},
+		&newcontrolplane.PaginationGetOptions{},


We don't need to add support for legacy APIs. They will be removed soon.

+1. Actually I think we can remove them for v1.6. Let me see if I can open a PR.

👌 Sure, though keeping this before the removal PR gets merged to build successfully. Because during API initialization it looks for this type in group version. Tested that w/o this registration, controller would throw an error.

legacyAPI has been removed, you could rebase on main now.

pkg/apis/controlplane/types.go

pkg/apiserver/registry/networkpolicy/clustergroupmember/rest.go

pkg/apis/controlplane/types.go

pkg/apiserver/registry/networkpolicy/clustergroupmember/rest.go

antoninbas · 2022-02-09T19:49:08Z

pkg/legacyapis/controlplane/register.go

@@ -58,6 +58,7 @@ func addKnownTypes(scheme *runtime.Scheme) error {
 		&newcontrolplane.NetworkPolicyStatus{},
 		&newcontrolplane.NodeStatsSummary{},
 		&newcontrolplane.ClusterGroupMembers{},
+		&newcontrolplane.PaginationGetOptions{},


+1. Actually I think we can remove them for v1.6. Let me see if I can open a PR.

pkg/apiserver/registry/networkpolicy/clustergroupmember/rest.go

tnqn · 2022-02-15T18:12:06Z

pkg/legacyapis/controlplane/register.go

@@ -58,6 +58,7 @@ func addKnownTypes(scheme *runtime.Scheme) error {
 		&newcontrolplane.NetworkPolicyStatus{},
 		&newcontrolplane.NodeStatsSummary{},
 		&newcontrolplane.ClusterGroupMembers{},
+		&newcontrolplane.PaginationGetOptions{},


legacyAPI has been removed, you could rebase on main now.

qiyueyao · 2022-02-22T21:10:49Z

/test-all

antoninbas

That doesn't have to be addressed in this PR, but it seems to me that we are doing a lot of repeated work every time a new page is requested: we build the entire slice of members, then sort it and then we index into it using the page number. I wonder if there is a way to implement it differently, while remaining stateless. Adding @tnqn to see if he has any insight on whether this is possible.

pkg/apiserver/registry/networkpolicy/clustergroupmember/rest.go

antoninbas · 2022-02-23T18:58:22Z

pkg/apiserver/registry/networkpolicy/clustergroupmember/rest_test.go

@@ -162,3 +214,133 @@ func TestRESTGet(t *testing.T) {
 		assert.Equal(t, tt.expectedObj, actualGroupList)
 	}
 }
+
+func TestRESTGetPagination(t *testing.T) {
+	tests := []struct {


It seems you have no negative test cases (invalid query parameters for which the server must return an error). Would it make sense to add one or two of them?

I've modified the test cases, (total 3 members) one of page 5, limit 2 where page index exceeds actual, one of page 1, limit -2 where limit is invalid.
If I understand your comment correctly, in both cases, the current design is to return full list instead of throwing an error. Let me know if returning an error works better here, but I checked K8s List pagination it only pages for limit>0, otherwise it's treated as normal.
For invalid parameters like "providing string for int"/"key not found", these are handled by the conversion function generated by +k8s:conversion-gen:explicit-from=net/url.Values, not tested in UT.

So there is no error either if page < 0?

salv-orlando · 2022-02-25T22:48:47Z

pkg/apiserver/registry/networkpolicy/clustergroupmember/rest.go

+			} else {
+				return true
+			}
+		})


Question about behaviour: If the user does not require pagination, the response is not sorted. On the other hand, if the user requires pagination, the response is sorted by Pod name or External entity. Is my understanding correct?

Yes that is the case, to reduce extra sorting when user does not require pagination.

salv-orlando · 2022-02-25T22:50:08Z

pkg/apiserver/registry/networkpolicy/clustergroupmember/rest.go

+			memberList.EffectiveMembers = memberList.EffectiveMembers[:pageInfo.pageLimit]
+		}
+	} else {
+		pageInfo.pageNumber = 0 // set to 0 in case given page number exceeds total pages


If I understand correctly if a user requires a page whose number is higher than the total number of pages they will get back the first page of results? Why not an empty page?

They will get back the entire result of members, as if pagination is not requested. In the response, totalPages is the number of pages based on user's correct limit, but pageNumber is set to 0 to indicate that no specific page is returned.
Currently to keep error handling consistent, all errors are handled by returning full list (added comment for func), let me know if full list or an empty page works best here. Thanks!

I feel this may cause client a bit complex to handle this situation. For example, if some members are removed between two page queries, leading to the second query get empty result, and we return the entire result of members, it would waste the bandwidth spent on transfering the whole data and client has to add a page number check to distinguish whether this is a legal response before displaying it.

That use case makes total sense, I'll change it to empty result, thanks!

tnqn · 2022-03-01T16:34:14Z

That doesn't have to be addressed in this PR, but it seems to me that we are doing a lot of repeated work every time a new page is requested: we build the entire slice of members, then sort it and then we index into it using the page number. I wonder if there is a way to implement it differently, while remaining stateless. Adding @tnqn to see if he has any insight on whether this is possible.

@antoninbas I thought about caching the result but it seems too complicated to associate queries and clean up cache. But If we assume the pagination will be used in a situation that only the first few pages will be queried quite often, maybe an optimization worth to do is sorting only the first pageNumber*pageLimit (say k) instead of the entire slice (say N) to reduce the runtime from O(NlogN) to O(klogN)?

antoninbas · 2022-03-02T00:25:02Z

@tnqn and is the sorting actually required? Given that for this API, the user wouldn't expect any specific ordering of the results, there should be no need to sort as long as r.querier.GetGroupMembers(name) always returns members in the same order. Is that not the case?

qiyueyao · 2022-03-02T01:32:17Z

That doesn't have to be addressed in this PR, but it seems to me that we are doing a lot of repeated work every time a new page is requested: we build the entire slice of members, then sort it and then we index into it using the page number. I wonder if there is a way to implement it differently, while remaining stateless. Adding @tnqn to see if he has any insight on whether this is possible.

@antoninbas I thought about caching the result but it seems too complicated to associate queries and clean up cache. But If we assume the pagination will be used in a situation that only the first few pages will be queried quite often, maybe an optimization worth to do is sorting only the first pageNumber*pageLimit (say n) instead of the entire slice (say N) to reduce the runtime from O(NlogN) to O(nlogn)?

Trying to understand the first pageNumber*pageLimit (say n), is it based on the original unsorted members? Then should there be another option for users to decide if they want continuous result (no member change) across queries?

tnqn · 2022-03-02T11:37:57Z

@antoninbas I suppose the API will be used for listing members of a group in web application, it's possible that there are some updates to the members when user requests next page. If we don't sort it at all, the result of next page may be totally discontinuous from previous page even there is just one member removed/added to the group because the map may be resized. If we sort it, the pages could be continuous overall with a few duplicate or missing members, which is very common in list page (For example, if someone opens a new PR when I'm viewing page 1, I may still see the last PR of page 1 when I view page 2).

@qiyueyao I mean sorting the requested members only via something like heap sort. For example, when user requests page 1, pageLimit is 10, and the total members is 1000. In current implementation, the cost is sorting 1000 items => O(1000 * log1000). If we use heap sort, we only need to build a heap and get the least or largest 10 items => O(1000+10*log1000).
The runtime should be O(N+klogN), not O(klogk) I commented.

qiyueyao · 2022-03-04T22:07:20Z

@antoninbas I suppose the API will be used for listing members of a group in web application, it's possible that there are some updates to the members when user requests next page. If we don't sort it at all, the result of next page may be totally discontinuous from previous page even there is just one member removed/added to the group because the map may be resized. If we sort it, the pages could be continuous overall with a few duplicate or missing members, which is very common in list page (For example, if someone opens a new PR when I'm viewing page 1, I may still see the last PR of page 1 when I view page 2).

@qiyueyao I mean sorting the requested members only via something like heap sort. For example, when user requests page 1, pageLimit is 10, and the total members is 1000. In current implementation, the cost is sorting 1000 items => O(1000 * log1000). If we use heap sort, we only need to build a heap and get the least or largest 10 items => O(1000+10*log1000). The runtime should be O(N+klogN), not O(klogk) I commented.

Understood. If we have the use case for heap sort, I could open an issue to track, and follow up with a new PR. Let me know if that works, thanks.

antoninbas

LGTM

antoninbas · 2022-03-07T23:04:22Z

pkg/apiserver/registry/networkpolicy/clustergroupmember/rest.go

+	if !ok {
+		getOptions = &controlplane.PaginationGetOptions{Page: 0, Limit: 0}
+	}


IMO, if ok is false, it could be treated as an internal server error. It should not be possible based on the K8s code.

Updated the error handling cases including this ok and limit | page < 0.

antoninbas · 2022-03-07T23:12:13Z

pkg/apiserver/registry/networkpolicy/clustergroupmember/rest_test.go

@@ -162,3 +214,133 @@ func TestRESTGet(t *testing.T) {
 		assert.Equal(t, tt.expectedObj, actualGroupList)
 	}
 }
+
+func TestRESTGetPagination(t *testing.T) {
+	tests := []struct {


So there is no error either if page < 0?

antoninbas · 2022-03-08T02:26:44Z

pkg/apiserver/registry/networkpolicy/clustergroupmember/rest.go

+		return nil
+	}
+	if pageInfo.Page < 0 || pageInfo.Limit < 0 {
+		return errors.NewInternalError(fmt.Errorf("received invalid page number %d or limit %d for pagination", pageInfo.Page, pageInfo.Limit))


That's not the right HTTP error code
You need NewBadRequest here. You can also return a different error message based on which parameter is wrong.

tnqn · 2022-03-08T14:55:22Z

pkg/apiserver/registry/networkpolicy/clustergroupmember/rest_test.go

+		} else {
+			require.NoError(t, err)
+		}
+		if tt.name == "default-zero-group-member-pagination" {


why does it need to be checked specially? doesn't assert.Equal(t, tt.expectedObj, actualGroupList) work?

Because in default zero case, no pagination is performed thus the three returned members are sorted. assert.ElementsMatch is needed separately to resolve flakiness.

Maybe we should sort in the non-pagination case as well for consistency. I don't think the performance argument in that case is a very valid one.

tnqn · 2022-03-08T14:56:52Z

pkg/apiserver/registry/networkpolicy/clustergroupmember/rest_test.go

+			assert.Equal(t, tt.expectedObj.(*controlplane.ClusterGroupMembers).TotalPages, actualGroupList.(*controlplane.ClusterGroupMembers).TotalPages)
+			assert.Equal(t, tt.expectedObj.(*controlplane.ClusterGroupMembers).CurrentPage, actualGroupList.(*controlplane.ClusterGroupMembers).CurrentPage)
+			assert.ElementsMatch(t, tt.expectedObj.(*controlplane.ClusterGroupMembers).EffectiveMembers, actualGroupList.(*controlplane.ClusterGroupMembers).EffectiveMembers)
+		} else if tt.name == "err-page-group-member-pagination" || tt.name == "err-limit-group-member-pagination" {


I think you could return early when expectedErr is true, as actualGroupList and err is mutually exclusive.

tnqn · 2022-03-08T15:00:21Z

pkg/apiserver/registry/networkpolicy/clustergroupmember/rest.go

+		if pageInfo.Limit < int64(len(memberList.EffectiveMembers)) {
+			memberList.EffectiveMembers = memberList.EffectiveMembers[:pageInfo.Limit]
+		}
+	} else if totalPages < pageInfo.Page {


if Page is 0, it seems it will get all members?

Yes, it will be treated as no pagination option is provided, same as limit = 0.

Signed-off-by: Qiyue Yao <yaoq@vmware.com> Investigation Signed-off-by: Qiyue Yao <yaoq@vmware.com> add UT Signed-off-by: Qiyue Yao <yaoq@vmware.com> update case page exceeds Signed-off-by: Qiyue Yao <yaoq@vmware.com> update error handling Signed-off-by: Qiyue Yao <yaoq@vmware.com> update HTTP error Signed-off-by: Qiyue Yao <yaoq@vmware.com> change sort Signed-off-by: Qiyue Yao <yaoq@vmware.com>

tnqn

LGTM

antoninbas · 2022-03-09T19:43:57Z

/test-all

qiyueyao marked this pull request as draft January 10, 2022 23:56

qiyueyao force-pushed the pagination branch 2 times, most recently from e05f4de to 3b80708 Compare January 11, 2022 20:01

Dyanngg reviewed Jan 13, 2022

View reviewed changes

pkg/apis/controlplane/v1beta2/generated.proto Outdated Show resolved Hide resolved

pkg/apis/controlplane/v1beta2/types.go Outdated Show resolved Hide resolved

qiyueyao force-pushed the pagination branch 2 times, most recently from 4b603e9 to b7464e1 Compare January 27, 2022 07:59

qiyueyao marked this pull request as ready for review January 27, 2022 18:46

qiyueyao added this to the Antrea v1.6 release milestone Jan 27, 2022

qiyueyao added action/backport Indicates a PR that requires backports. area/network-policy Issues or PRs related to network policies. and removed action/backport Indicates a PR that requires backports. labels Jan 27, 2022

Dyanngg reviewed Feb 3, 2022

View reviewed changes

pkg/apiserver/registry/networkpolicy/clustergroupmember/rest.go Outdated Show resolved Hide resolved

pkg/apis/controlplane/types.go Show resolved Hide resolved

qiyueyao force-pushed the pagination branch from fe2fe70 to 16ab667 Compare February 4, 2022 23:55

Dyanngg reviewed Feb 9, 2022

View reviewed changes

pkg/apiserver/registry/networkpolicy/clustergroupmember/rest.go Outdated Show resolved Hide resolved

qiyueyao force-pushed the pagination branch from 16ab667 to 69578e4 Compare February 9, 2022 01:23

qiyueyao requested a review from GraysonWu February 9, 2022 01:24

tnqn reviewed Feb 9, 2022

View reviewed changes

antoninbas added the api-review Categorizes an issue or PR as actively needing an API review. label Feb 9, 2022

antoninbas reviewed Feb 9, 2022

View reviewed changes

qiyueyao force-pushed the pagination branch 2 times, most recently from ec7d69f to 37b123c Compare February 15, 2022 01:52

tnqn reviewed Feb 15, 2022

View reviewed changes

qiyueyao force-pushed the pagination branch from 37b123c to 4db1cbd Compare February 22, 2022 19:02

qiyueyao requested review from antoninbas, Dyanngg and tnqn February 23, 2022 00:55

antoninbas reviewed Feb 23, 2022

View reviewed changes

salv-orlando reviewed Feb 25, 2022

View reviewed changes

qiyueyao force-pushed the pagination branch 2 times, most recently from 245da18 to 26adb90 Compare February 26, 2022 06:34

qiyueyao force-pushed the pagination branch from 26adb90 to 632f412 Compare March 2, 2022 01:20

qiyueyao force-pushed the pagination branch 2 times, most recently from 5bf8d77 to c0e0f39 Compare March 4, 2022 21:52

qiyueyao requested a review from antoninbas March 7, 2022 22:46

antoninbas previously approved these changes Mar 7, 2022

View reviewed changes

qiyueyao dismissed antoninbas’s stale review via 95ccbdd March 8, 2022 02:04

qiyueyao force-pushed the pagination branch from c0e0f39 to 95ccbdd Compare March 8, 2022 02:04

antoninbas reviewed Mar 8, 2022

View reviewed changes

qiyueyao force-pushed the pagination branch 2 times, most recently from b623283 to 9451e33 Compare March 8, 2022 02:55

tnqn reviewed Mar 8, 2022

View reviewed changes

qiyueyao force-pushed the pagination branch from 9451e33 to 4483c0c Compare March 8, 2022 18:39

qiyueyao force-pushed the pagination branch from 4483c0c to 13e0f70 Compare March 8, 2022 23:17

tnqn approved these changes Mar 9, 2022

View reviewed changes

tnqn added the action/release-note Indicates a PR that should be included in release notes. label Mar 9, 2022

antoninbas approved these changes Mar 9, 2022

View reviewed changes

tnqn merged commit cd37a9b into antrea-io:main Mar 10, 2022

qiyueyao deleted the pagination branch March 10, 2022 19:14

Support Pagination for ClusterGroupMembership #3183

Support Pagination for ClusterGroupMembership #3183

Conversation

qiyueyao commented Jan 10, 2022 • edited

codecov-commenter commented Jan 10, 2022 • edited

Codecov Report

Dyanngg left a comment

Choose a reason for hiding this comment

Dyanngg commented Feb 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiyueyao commented Feb 22, 2022

antoninbas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiyueyao Feb 25, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnqn commented Mar 1, 2022 • edited

antoninbas commented Mar 2, 2022

qiyueyao commented Mar 2, 2022

tnqn commented Mar 2, 2022

qiyueyao commented Mar 4, 2022

antoninbas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

antoninbas commented Mar 9, 2022

qiyueyao commented Jan 10, 2022 •

edited

codecov-commenter commented Jan 10, 2022 •

edited

qiyueyao Feb 25, 2022 •

edited

tnqn commented Mar 1, 2022 •

edited