From 8c6ed31528fe1c5ed81536a6b22d285e5bdbe266 Mon Sep 17 00:00:00 2001 From: Alay Patel Date: Mon, 1 Nov 2021 18:21:55 -0400 Subject: [PATCH 1/2] - fix buggy pager func fix paging items in to use list options passed by the paging function The client-go pager sets the Limit options for the list call to paginate the request[1]. This PR fixes the paging function to use the options passed by the pager instead of shadowed options This is required for the pagination to work correctly. - simplify the pager list implementation by using pager.List() The List() function already implements a lot of the logic that was needed for paging here, using it simplifies the code. 1. https://github.com/kubernetes/kubernetes/blob/3f40906dd8a54fb91650553a6457496181f591bc/staging/src/k8s.io/client-go/tools/pager/pager.go#L219 Signed-off-by: Alay Patel --- pkg/backup/item_collector.go | 51 +++++++++++++++--------------------- 1 file changed, 21 insertions(+), 30 deletions(-) diff --git a/pkg/backup/item_collector.go b/pkg/backup/item_collector.go index 6029d8f973..3a44d5ffb2 100644 --- a/pkg/backup/item_collector.go +++ b/pkg/backup/item_collector.go @@ -26,7 +26,7 @@ import ( "github.com/pkg/errors" "github.com/sirupsen/logrus" - apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/labels" @@ -293,7 +293,6 @@ func (r *itemCollector) getResourceItems(log logrus.FieldLogger, gv schema.Group if selector := r.backupRequest.Spec.LabelSelector; selector != nil { labelSelector = metav1.FormatLabelSelector(selector) } - listOptions := metav1.ListOptions{LabelSelector: labelSelector} log.Info("Listing items") unstructuredItems := make([]unstructured.Unstructured, 0) @@ -301,50 +300,42 @@ func (r *itemCollector) getResourceItems(log logrus.FieldLogger, gv schema.Group if r.pageSize > 0 { // If limit is positive, use a pager to split list over multiple requests // Use Velero's dynamic list function instead of the default - listFunc := pager.SimplePageFunc(func(opts metav1.ListOptions) (runtime.Object, error) { - list, err := resourceClient.List(listOptions) - if err != nil { - return nil, err - } - return list, nil - }) - listPager := pager.New(listFunc) + listPager := pager.New(pager.SimplePageFunc(func(opts metav1.ListOptions) (runtime.Object, error) { + return resourceClient.List(opts) + })) // Use the page size defined in the server config // TODO allow configuration of page buffer size listPager.PageSize = int64(r.pageSize) // Add each item to temporary slice - var items []unstructured.Unstructured - err := listPager.EachListItem(context.Background(), listOptions, func(object runtime.Object) error { - item, isUnstructured := object.(*unstructured.Unstructured) - if !isUnstructured { - // We should never hit this - log.Error("Got type other than Unstructured from pager func") - return nil + list, paginated, err := listPager.List(context.Background(), metav1.ListOptions{LabelSelector: labelSelector}) + if err != nil { + log.WithError(errors.WithStack(err)).Error("Error listing resources") + continue + } + if !paginated { + log.Infof("list for groupResource %s was not paginated", gr) + } + err = meta.EachListItem(list, func(object runtime.Object) error { + u, ok := object.(*unstructured.Unstructured) + if !ok { + log.WithError(errors.WithStack(fmt.Errorf("expected *unstructured.Unstructured but got %T", u))).Error("unable to understand entry in the list") + return fmt.Errorf("expected *unstructured.Unstructured but got %T", u) } - items = append(items, *item) + unstructuredItems = append(unstructuredItems, *u) return nil }) - if statusError, isStatusError := err.(*apierrors.StatusError); isStatusError && statusError.Status().Reason == metav1.StatusReasonExpired { - log.WithError(errors.WithStack(err)).Error("Error paging item list. Falling back on unpaginated list") - unstructuredList, err := resourceClient.List(listOptions) - if err != nil { - log.WithError(errors.WithStack(err)).Error("Error listing items") - continue - } - items = unstructuredList.Items - } else if err != nil { - log.WithError(errors.WithStack(err)).Error("Error paging item list") + if err != nil { + log.WithError(errors.WithStack(err)).Error("unable to understand paginated list") continue } - unstructuredItems = append(unstructuredItems, items...) } else { // If limit is not positive, do not use paging. Instead, request all items at once unstructuredList, err := resourceClient.List(metav1.ListOptions{LabelSelector: labelSelector}) - unstructuredItems = append(unstructuredItems, unstructuredList.Items...) if err != nil { log.WithError(errors.WithStack(err)).Error("Error listing items") continue } + unstructuredItems = append(unstructuredItems, unstructuredList.Items...) } log.Infof("Retrieved %d items", len(unstructuredItems)) From bf10709f98d43c1d964f59a7aa00fd3c0c1e051e Mon Sep 17 00:00:00 2001 From: Alay Patel Date: Mon, 1 Nov 2021 18:30:27 -0400 Subject: [PATCH 2/2] add 4358 changelog Signed-off-by: Alay Patel --- changelogs/unreleased/4358-alaypatel07 | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelogs/unreleased/4358-alaypatel07 diff --git a/changelogs/unreleased/4358-alaypatel07 b/changelogs/unreleased/4358-alaypatel07 new file mode 100644 index 0000000000..e11c33039e --- /dev/null +++ b/changelogs/unreleased/4358-alaypatel07 @@ -0,0 +1 @@ +fix buggy pager func \ No newline at end of file