From 1f4f2a5d6014dc8f98b25a9484d4a6064a6ae18e Mon Sep 17 00:00:00 2001 From: Marek Siarkowicz Date: Wed, 30 Aug 2023 16:26:20 +0200 Subject: [PATCH 1/3] Refactor common WithRange case From API call WithRange and WithPrefix work the same, they just set the range end. The difference is when the range end is provided: * WithRange(end) requires providing the end while calling * WithPrefix() calculates the end based on key provided to the Get. For example, those are equal: * client.Get(ctx, "/pods/", WithPrefix()) * client.Get(ctx, "/pods/", WithRange(GetPrfixRangeEnd("/pods/"))) As keyPrefix is equal preparedKey there should not be a difference. --- .../k8s.io/apiserver/pkg/storage/etcd3/store.go | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/staging/src/k8s.io/apiserver/pkg/storage/etcd3/store.go b/staging/src/k8s.io/apiserver/pkg/storage/etcd3/store.go index e4014ccd967..3182be090bf 100644 --- a/staging/src/k8s.io/apiserver/pkg/storage/etcd3/store.go +++ b/staging/src/k8s.io/apiserver/pkg/storage/etcd3/store.go @@ -671,11 +671,7 @@ func (s *store) GetList(ctx context.Context, key string, opts storage.ListOption if len(resourceVersion) > 0 && resourceVersion != "0" { return apierrors.NewBadRequest("specifying resource version is not allowed when using continue") } - - rangeEnd := clientv3.GetPrefixRangeEnd(keyPrefix) - options = append(options, clientv3.WithRange(rangeEnd)) preparedKey = continueKey - // If continueRV > 0, the LIST request needs a specific resource version. // continueRV==0 is invalid. // If continueRV < 0, the request is for the latest resource version. @@ -698,9 +694,6 @@ func (s *store) GetList(ctx context.Context, key string, opts storage.ListOption return fmt.Errorf("unknown ResourceVersionMatch value: %v", match) } } - - rangeEnd := clientv3.GetPrefixRangeEnd(keyPrefix) - options = append(options, clientv3.WithRange(rangeEnd)) default: if fromRV != nil { switch match { @@ -714,10 +707,11 @@ func (s *store) GetList(ctx context.Context, key string, opts storage.ListOption return fmt.Errorf("unknown ResourceVersionMatch value: %v", match) } } + } - if recursive { - options = append(options, clientv3.WithPrefix()) - } + if recursive { + rangeEnd := clientv3.GetPrefixRangeEnd(keyPrefix) + options = append(options, clientv3.WithRange(rangeEnd)) } if withRev != 0 { options = append(options, clientv3.WithRev(withRev)) From 10553a1966892e305252c65ebeb9043416304f48 Mon Sep 17 00:00:00 2001 From: Marek Siarkowicz Date: Wed, 30 Aug 2023 16:42:34 +0200 Subject: [PATCH 2/3] Flatten switch case --- .../apiserver/pkg/storage/etcd3/store.go | 19 +++---------------- 1 file changed, 3 insertions(+), 16 deletions(-) diff --git a/staging/src/k8s.io/apiserver/pkg/storage/etcd3/store.go b/staging/src/k8s.io/apiserver/pkg/storage/etcd3/store.go index 3182be090bf..c1ddcabe0f4 100644 --- a/staging/src/k8s.io/apiserver/pkg/storage/etcd3/store.go +++ b/staging/src/k8s.io/apiserver/pkg/storage/etcd3/store.go @@ -678,22 +678,6 @@ func (s *store) GetList(ctx context.Context, key string, opts storage.ListOption if continueRV > 0 { withRev = continueRV } - case recursive && s.pagingEnabled && pred.Limit > 0: - if fromRV != nil { - switch match { - case metav1.ResourceVersionMatchNotOlderThan: - // The not older than constraint is checked after we get a response from etcd, - // and returnedRV is then set to the revision we get from the etcd response. - case metav1.ResourceVersionMatchExact: - withRev = int64(*fromRV) - case "": // legacy case - if *fromRV > 0 { - withRev = int64(*fromRV) - } - default: - return fmt.Errorf("unknown ResourceVersionMatch value: %v", match) - } - } default: if fromRV != nil { switch match { @@ -703,6 +687,9 @@ func (s *store) GetList(ctx context.Context, key string, opts storage.ListOption case metav1.ResourceVersionMatchExact: withRev = int64(*fromRV) case "": // legacy case + if recursive && s.pagingEnabled && pred.Limit > 0 && *fromRV > 0 { + withRev = int64(*fromRV) + } default: return fmt.Errorf("unknown ResourceVersionMatch value: %v", match) } From e01bd641447a315e28fab8148e99ac6afba9bcd7 Mon Sep 17 00:00:00 2001 From: Marek Siarkowicz Date: Wed, 30 Aug 2023 16:51:40 +0200 Subject: [PATCH 3/3] Avoid creating local variables that don't change Having local variables gives false impression that this is overwritten in the function block. --- .../apiserver/pkg/storage/etcd3/store.go | 56 +++++++++---------- 1 file changed, 26 insertions(+), 30 deletions(-) diff --git a/staging/src/k8s.io/apiserver/pkg/storage/etcd3/store.go b/staging/src/k8s.io/apiserver/pkg/storage/etcd3/store.go index c1ddcabe0f4..2cde92abab7 100644 --- a/staging/src/k8s.io/apiserver/pkg/storage/etcd3/store.go +++ b/staging/src/k8s.io/apiserver/pkg/storage/etcd3/store.go @@ -607,17 +607,13 @@ func (s *store) GetList(ctx context.Context, key string, opts storage.ListOption if err != nil { return err } - recursive := opts.Recursive - resourceVersion := opts.ResourceVersion - match := opts.ResourceVersionMatch - pred := opts.Predicate - ctx, span := tracing.Start(ctx, fmt.Sprintf("List(recursive=%v) etcd3", recursive), + ctx, span := tracing.Start(ctx, fmt.Sprintf("List(recursive=%v) etcd3", opts.Recursive), attribute.String("audit-id", audit.GetAuditIDTruncated(ctx)), attribute.String("key", key), - attribute.String("resourceVersion", resourceVersion), - attribute.String("resourceVersionMatch", string(match)), - attribute.Int("limit", int(pred.Limit)), - attribute.String("continue", pred.Continue)) + attribute.String("resourceVersion", opts.ResourceVersion), + attribute.String("resourceVersionMatch", string(opts.ResourceVersionMatch)), + attribute.Int("limit", int(opts.Predicate.Limit)), + attribute.String("continue", opts.Predicate.Continue)) defer span.End(500 * time.Millisecond) listPtr, err := meta.GetItemsPtr(listObj) if err != nil { @@ -632,17 +628,17 @@ func (s *store) GetList(ctx context.Context, key string, opts storage.ListOption // get children "directories". e.g. if we have key "/a", "/a/b", "/ab", getting keys // with prefix "/a" will return all three, while with prefix "/a/" will return only // "/a/b" which is the correct answer. - if recursive && !strings.HasSuffix(preparedKey, "/") { + if opts.Recursive && !strings.HasSuffix(preparedKey, "/") { preparedKey += "/" } keyPrefix := preparedKey // set the appropriate clientv3 options to filter the returned data set var limitOption *clientv3.OpOption - limit := pred.Limit + limit := opts.Predicate.Limit var paging bool options := make([]clientv3.OpOption, 0, 4) - if s.pagingEnabled && pred.Limit > 0 { + if s.pagingEnabled && opts.Predicate.Limit > 0 { paging = true options = append(options, clientv3.WithLimit(limit)) limitOption = &options[len(options)-1] @@ -651,8 +647,8 @@ func (s *store) GetList(ctx context.Context, key string, opts storage.ListOption newItemFunc := getNewItemFunc(listObj, v) var fromRV *uint64 - if len(resourceVersion) > 0 { - parsedRV, err := s.versioner.ParseResourceVersion(resourceVersion) + if len(opts.ResourceVersion) > 0 { + parsedRV, err := s.versioner.ParseResourceVersion(opts.ResourceVersion) if err != nil { return apierrors.NewBadRequest(fmt.Sprintf("invalid resource version: %v", err)) } @@ -662,13 +658,13 @@ func (s *store) GetList(ctx context.Context, key string, opts storage.ListOption var continueRV, withRev int64 var continueKey string switch { - case recursive && s.pagingEnabled && len(pred.Continue) > 0: - continueKey, continueRV, err = storage.DecodeContinue(pred.Continue, keyPrefix) + case opts.Recursive && s.pagingEnabled && len(opts.Predicate.Continue) > 0: + continueKey, continueRV, err = storage.DecodeContinue(opts.Predicate.Continue, keyPrefix) if err != nil { return apierrors.NewBadRequest(fmt.Sprintf("invalid continue token: %v", err)) } - if len(resourceVersion) > 0 && resourceVersion != "0" { + if len(opts.ResourceVersion) > 0 && opts.ResourceVersion != "0" { return apierrors.NewBadRequest("specifying resource version is not allowed when using continue") } preparedKey = continueKey @@ -680,23 +676,23 @@ func (s *store) GetList(ctx context.Context, key string, opts storage.ListOption } default: if fromRV != nil { - switch match { + switch opts.ResourceVersionMatch { case metav1.ResourceVersionMatchNotOlderThan: // The not older than constraint is checked after we get a response from etcd, // and returnedRV is then set to the revision we get from the etcd response. case metav1.ResourceVersionMatchExact: withRev = int64(*fromRV) case "": // legacy case - if recursive && s.pagingEnabled && pred.Limit > 0 && *fromRV > 0 { + if opts.Recursive && s.pagingEnabled && opts.Predicate.Limit > 0 && *fromRV > 0 { withRev = int64(*fromRV) } default: - return fmt.Errorf("unknown ResourceVersionMatch value: %v", match) + return fmt.Errorf("unknown ResourceVersionMatch value: %v", opts.ResourceVersionMatch) } } } - if recursive { + if opts.Recursive { rangeEnd := clientv3.GetPrefixRangeEnd(keyPrefix) options = append(options, clientv3.WithRange(rangeEnd)) } @@ -718,7 +714,7 @@ func (s *store) GetList(ctx context.Context, key string, opts storage.ListOption }() metricsOp := "get" - if recursive { + if opts.Recursive { metricsOp = "list" } @@ -727,10 +723,10 @@ func (s *store) GetList(ctx context.Context, key string, opts storage.ListOption getResp, err = s.client.KV.Get(ctx, preparedKey, options...) metrics.RecordEtcdRequest(metricsOp, s.groupResourceString, err, startTime) if err != nil { - return interpretListError(err, len(pred.Continue) > 0, continueKey, keyPrefix) + return interpretListError(err, len(opts.Predicate.Continue) > 0, continueKey, keyPrefix) } numFetched += len(getResp.Kvs) - if err = s.validateMinimumResourceVersion(resourceVersion, uint64(getResp.Header.Revision)); err != nil { + if err = s.validateMinimumResourceVersion(opts.ResourceVersion, uint64(getResp.Header.Revision)); err != nil { return err } hasMore = getResp.More @@ -741,7 +737,7 @@ func (s *store) GetList(ctx context.Context, key string, opts storage.ListOption // avoid small allocations for the result slice, since this can be called in many // different contexts and we don't know how significantly the result will be filtered - if pred.Empty() { + if opts.Predicate.Empty() { growSlice(v, len(getResp.Kvs)) } else { growSlice(v, 2048, len(getResp.Kvs)) @@ -749,7 +745,7 @@ func (s *store) GetList(ctx context.Context, key string, opts storage.ListOption // take items from the response until the bucket is full, filtering as we go for i, kv := range getResp.Kvs { - if paging && int64(v.Len()) >= pred.Limit { + if paging && int64(v.Len()) >= opts.Predicate.Limit { hasMore = true break } @@ -760,7 +756,7 @@ func (s *store) GetList(ctx context.Context, key string, opts storage.ListOption return storage.NewInternalErrorf("unable to transform key %q: %v", kv.Key, err) } - if err := appendListItem(v, data, uint64(kv.ModRevision), pred, s.codec, s.versioner, newItemFunc); err != nil { + if err := appendListItem(v, data, uint64(kv.ModRevision), opts.Predicate, s.codec, s.versioner, newItemFunc); err != nil { recordDecodeError(s.groupResourceString, string(kv.Key)) return err } @@ -775,7 +771,7 @@ func (s *store) GetList(ctx context.Context, key string, opts storage.ListOption break } // we're paging but we have filled our bucket - if int64(v.Len()) >= pred.Limit { + if int64(v.Len()) >= opts.Predicate.Limit { break } @@ -816,8 +812,8 @@ func (s *store) GetList(ctx context.Context, key string, opts storage.ListOption // getResp.Count counts in objects that do not match the pred. // Instead of returning inaccurate count for non-empty selectors, we return nil. // Only set remainingItemCount if the predicate is empty. - if pred.Empty() { - c := int64(getResp.Count - pred.Limit) + if opts.Predicate.Empty() { + c := int64(getResp.Count - opts.Predicate.Limit) remainingItemCount = &c } return s.versioner.UpdateList(listObj, uint64(withRev), next, remainingItemCount)