Merge pull request kubernetes#121049 from siyuanfoundation/refactor

k8s.io/apiserver/storage/etcd: refactor etcd GetList.

Author: k8s-ci-robot

staging/src/k8s.io/apiserver/pkg/storage/etcd3/store.go

@@ -627,17 +627,13 @@ func (s *store) GetList(ctx context.Context, key string, opts storage.ListOption
 		limitOption = &options[len(options)-1]
 	}
 
-	newItemFunc := getNewItemFunc(listObj, v)
-
-	var fromRV *uint64
-	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))
-		}
-		fromRV = &parsedRV
+	if opts.Recursive {
+		rangeEnd := clientv3.GetPrefixRangeEnd(keyPrefix)
+		options = append(options, clientv3.WithRange(rangeEnd))
 	}
 
+	newItemFunc := getNewItemFunc(listObj, v)
+
 	var continueRV, withRev int64
 	var continueKey string
 	switch {
@@ -657,28 +653,26 @@ func (s *store) GetList(ctx context.Context, key string, opts storage.ListOption
 		if continueRV > 0 {
 			withRev = continueRV
 		}
-	default:
-		if fromRV != nil {
-			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 opts.Recursive && opts.Predicate.Limit > 0 && *fromRV > 0 {
-					withRev = int64(*fromRV)
-				}
-			default:
-				return fmt.Errorf("unknown ResourceVersionMatch value: %v", opts.ResourceVersionMatch)
+	case len(opts.ResourceVersion) > 0:
+		parsedRV, err := s.versioner.ParseResourceVersion(opts.ResourceVersion)
+		if err != nil {
+			return apierrors.NewBadRequest(fmt.Sprintf("invalid resource version: %v", err))
+		}
+		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(parsedRV)
+		case "": // legacy case
+			if opts.Recursive && opts.Predicate.Limit > 0 && parsedRV > 0 {
+				withRev = int64(parsedRV)
 			}
+		default:
+			return fmt.Errorf("unknown ResourceVersionMatch value: %v", opts.ResourceVersionMatch)
 		}
 	}
 
-	if opts.Recursive {
-		rangeEnd := clientv3.GetPrefixRangeEnd(keyPrefix)
-		options = append(options, clientv3.WithRange(rangeEnd))
-	}
 	if withRev != 0 {
 		options = append(options, clientv3.WithRev(withRev))
 	}
@@ -717,6 +711,11 @@ func (s *store) GetList(ctx context.Context, key string, opts storage.ListOption
 		if len(getResp.Kvs) == 0 && getResp.More {
 			return fmt.Errorf("no results were found, but etcd indicated there were more values remaining")
 		}
+		// indicate to the client which resource version was returned, and use the same resource version for subsequent requests.
+		if withRev == 0 {
+			withRev = getResp.Header.Revision
+			options = append(options, clientv3.WithRev(withRev))
+		}
 
 		// 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
@@ -776,14 +775,6 @@ func (s *store) GetList(ctx context.Context, key string, opts storage.ListOption
 			*limitOption = clientv3.WithLimit(limit)
 		}
 		preparedKey = string(lastKey) + "\x00"
-		if withRev == 0 {
-			withRev = getResp.Header.Revision
-			options = append(options, clientv3.WithRev(withRev))
-		}
-	}
-	// indicate to the client which resource version was returned
-	if withRev == 0 {
-		withRev = getResp.Header.Revision
 	}
 
 	if v.IsNil() {

test/integration/apiserver/apiserver_test.go

@@ -527,12 +527,6 @@ func testListOptionsCase(t *testing.T, rsClient appsv1.ReplicaSetInterface, watc
 		}
 		return
 	}
-	if opts.ResourceVersion == invalidResourceVersion {
-		if err == nil || !strings.Contains(err.Error(), "Invalid value") {
-			t.Fatalf("expecting invalid value error, but got: %v", err)
-		}
-		return
-	}
 	if opts.Continue == invalidContinueToken {
 		if err == nil || !strings.Contains(err.Error(), "continue key is not valid") {
 			t.Fatalf("expected continue key not valid error, but got: %v", err)
@@ -546,6 +540,12 @@ func testListOptionsCase(t *testing.T, rsClient appsv1.ReplicaSetInterface, watc
 		}
 		return
 	}
+	if opts.ResourceVersion == invalidResourceVersion {
+		if err == nil || !strings.Contains(err.Error(), "Invalid value") {
+			t.Fatalf("expecting invalid value error, but got: %v", err)
+		}
+		return
+	}
 
 	// Check for too old errors
 	isExact := opts.ResourceVersionMatch == metav1.ResourceVersionMatchExact