From 1e0855a7acfa0c2b83dff26f152bd3cdb4fa7cc9 Mon Sep 17 00:00:00 2001
From: Lukasz Szaszkiewicz <lukasz.szaszkiewicz@gmail.com>
Date: Wed, 4 Oct 2023 08:17:10 +0200
Subject: [PATCH] reflector: fallback to the previous mode on any error

originally we honored only apierrors.IsInvalid
but decided to fallback on every error
because it is better to make progress than deadlocking

Kubernetes-commit: 4b3915017950a114124a88c5d308bd8bfb9ec48e
---
 tools/cache/reflector.go                |  7 ++---
 tools/cache/reflector_watchlist_test.go | 41 +++++++++++++++++++------
 2 files changed, 33 insertions(+), 15 deletions(-)

diff --git a/tools/cache/reflector.go b/tools/cache/reflector.go
index 7893f5f6..2cf4723d 100644
--- a/tools/cache/reflector.go
+++ b/tools/cache/reflector.go
@@ -334,12 +334,9 @@ func (r *Reflector) ListAndWatch(stopCh <-chan struct{}) error {
 			return nil
 		}
 		if err != nil {
-			if !apierrors.IsInvalid(err) {
-				return err
-			}
-			klog.Warning("The watch-list feature is not supported by the server, falling back to the previous LIST/WATCH semantics")
+			klog.Warningf("The watchlist request ended with an error, falling back to the standard LIST/WATCH semantics because making progress is better than deadlocking, err = %v", err)
 			fallbackToList = true
-			// Ensure that we won't accidentally pass some garbage down the watch.
+			// ensure that we won't accidentally pass some garbage down the watch.
 			w = nil
 		}
 	}
diff --git a/tools/cache/reflector_watchlist_test.go b/tools/cache/reflector_watchlist_test.go
index 4b2d0f87..93741178 100644
--- a/tools/cache/reflector_watchlist_test.go
+++ b/tools/cache/reflector_watchlist_test.go
@@ -94,18 +94,39 @@ func TestWatchList(t *testing.T) {
 			expectedStoreContent: []v1.Pod{*makePod("p1", "1")},
 		},
 		{
-			name: "returning any other error than apierrors.NewInvalid stops the reflector and reports the error",
+			name: "returning any other error than apierrors.NewInvalid forces fallback",
 			watchOptionsPredicate: func(options metav1.ListOptions) error {
-				return fmt.Errorf("dummy error")
+				if options.SendInitialEvents != nil && *options.SendInitialEvents {
+					return fmt.Errorf("dummy error")
+				}
+				return nil
+			},
+			podList: &v1.PodList{
+				ListMeta: metav1.ListMeta{ResourceVersion: "1"},
+				Items:    []v1.Pod{*makePod("p1", "1")},
+			},
+			closeAfterWatchEvents: 1,
+			watchEvents:           []watch.Event{{Type: watch.Added, Object: makePod("p2", "2")}},
+			expectedWatchRequests: 2,
+			expectedListRequests:  1,
+			expectedStoreContent:  []v1.Pod{*makePod("p1", "1"), *makePod("p2", "2")},
+			expectedRequestOptions: []metav1.ListOptions{
+				{
+					SendInitialEvents:    pointer.Bool(true),
+					AllowWatchBookmarks:  true,
+					ResourceVersionMatch: metav1.ResourceVersionMatchNotOlderThan,
+					TimeoutSeconds:       pointer.Int64(1),
+				},
+				{
+					ResourceVersion: "0",
+					Limit:           500,
+				},
+				{
+					AllowWatchBookmarks: true,
+					ResourceVersion:     "1",
+					TimeoutSeconds:      pointer.Int64(1),
+				},
 			},
-			expectedError:         fmt.Errorf("dummy error"),
-			expectedWatchRequests: 1,
-			expectedRequestOptions: []metav1.ListOptions{{
-				SendInitialEvents:    pointer.Bool(true),
-				AllowWatchBookmarks:  true,
-				ResourceVersionMatch: metav1.ResourceVersionMatchNotOlderThan,
-				TimeoutSeconds:       pointer.Int64(1),
-			}},
 		},
 		{
 			name: "the reflector can fall back to old LIST/WATCH semantics when a server doesn't support streaming",