Merge pull request #70994 from mborsz/cache

Refactor the memcached discovery client

Kubernetes-commit: 3f9673bf5d6d9adeeca5eb349c83ece6095a41ba

Godeps/Godeps.json

@@ -1,7 +1,7 @@
 {
 	"ImportPath": "k8s.io/client-go",
 	"GoVersion": "go1.11",
-	"GodepVersion": "v80",
+	"GodepVersion": "v80-k8s-r1",
 	"Packages": [
 		"./..."
 	],
@@ -404,207 +404,207 @@
 		},
 		{
 			"ImportPath": "k8s.io/apimachinery/pkg/api/apitesting",
-			"Rev": "4d029f0333996cf231080e108e0bd1ece2a94d9f"
+			"Rev": "d04500c8c3dda9c980b668c57abc2ca61efcf5c4"
 		},
 		{
 			"ImportPath": "k8s.io/apimachinery/pkg/api/apitesting/fuzzer",
-			"Rev": "4d029f0333996cf231080e108e0bd1ece2a94d9f"
+			"Rev": "d04500c8c3dda9c980b668c57abc2ca61efcf5c4"
 		},
 		{
 			"ImportPath": "k8s.io/apimachinery/pkg/api/apitesting/roundtrip",
-			"Rev": "4d029f0333996cf231080e108e0bd1ece2a94d9f"
+			"Rev": "d04500c8c3dda9c980b668c57abc2ca61efcf5c4"
 		},
 		{
 			"ImportPath": "k8s.io/apimachinery/pkg/api/equality",
-			"Rev": "4d029f0333996cf231080e108e0bd1ece2a94d9f"
+			"Rev": "d04500c8c3dda9c980b668c57abc2ca61efcf5c4"
 		},
 		{
 			"ImportPath": "k8s.io/apimachinery/pkg/api/errors",
-			"Rev": "4d029f0333996cf231080e108e0bd1ece2a94d9f"
+			"Rev": "d04500c8c3dda9c980b668c57abc2ca61efcf5c4"
 		},
 		{
 			"ImportPath": "k8s.io/apimachinery/pkg/api/meta",
-			"Rev": "4d029f0333996cf231080e108e0bd1ece2a94d9f"
+			"Rev": "d04500c8c3dda9c980b668c57abc2ca61efcf5c4"
 		},
 		{
 			"ImportPath": "k8s.io/apimachinery/pkg/api/resource",
-			"Rev": "4d029f0333996cf231080e108e0bd1ece2a94d9f"
+			"Rev": "d04500c8c3dda9c980b668c57abc2ca61efcf5c4"
 		},
 		{
 			"ImportPath": "k8s.io/apimachinery/pkg/apis/meta/fuzzer",
-			"Rev": "4d029f0333996cf231080e108e0bd1ece2a94d9f"
+			"Rev": "d04500c8c3dda9c980b668c57abc2ca61efcf5c4"
 		},
 		{
 			"ImportPath": "k8s.io/apimachinery/pkg/apis/meta/internalversion",
-			"Rev": "4d029f0333996cf231080e108e0bd1ece2a94d9f"
+			"Rev": "d04500c8c3dda9c980b668c57abc2ca61efcf5c4"
 		},
 		{
 			"ImportPath": "k8s.io/apimachinery/pkg/apis/meta/v1",
-			"Rev": "4d029f0333996cf231080e108e0bd1ece2a94d9f"
+			"Rev": "d04500c8c3dda9c980b668c57abc2ca61efcf5c4"
 		},
 		{
 			"ImportPath": "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured",
-			"Rev": "4d029f0333996cf231080e108e0bd1ece2a94d9f"
+			"Rev": "d04500c8c3dda9c980b668c57abc2ca61efcf5c4"
 		},
 		{
 			"ImportPath": "k8s.io/apimachinery/pkg/apis/meta/v1beta1",
-			"Rev": "4d029f0333996cf231080e108e0bd1ece2a94d9f"
+			"Rev": "d04500c8c3dda9c980b668c57abc2ca61efcf5c4"
 		},
 		{
 			"ImportPath": "k8s.io/apimachinery/pkg/conversion",
-			"Rev": "4d029f0333996cf231080e108e0bd1ece2a94d9f"
+			"Rev": "d04500c8c3dda9c980b668c57abc2ca61efcf5c4"
 		},
 		{
 			"ImportPath": "k8s.io/apimachinery/pkg/conversion/queryparams",
-			"Rev": "4d029f0333996cf231080e108e0bd1ece2a94d9f"
+			"Rev": "d04500c8c3dda9c980b668c57abc2ca61efcf5c4"
 		},
 		{
 			"ImportPath": "k8s.io/apimachinery/pkg/fields",
-			"Rev": "4d029f0333996cf231080e108e0bd1ece2a94d9f"
+			"Rev": "d04500c8c3dda9c980b668c57abc2ca61efcf5c4"
 		},
 		{
 			"ImportPath": "k8s.io/apimachinery/pkg/labels",
-			"Rev": "4d029f0333996cf231080e108e0bd1ece2a94d9f"
+			"Rev": "d04500c8c3dda9c980b668c57abc2ca61efcf5c4"
 		},
 		{
 			"ImportPath": "k8s.io/apimachinery/pkg/runtime",
-			"Rev": "4d029f0333996cf231080e108e0bd1ece2a94d9f"
+			"Rev": "d04500c8c3dda9c980b668c57abc2ca61efcf5c4"
 		},
 		{
 			"ImportPath": "k8s.io/apimachinery/pkg/runtime/schema",
-			"Rev": "4d029f0333996cf231080e108e0bd1ece2a94d9f"
+			"Rev": "d04500c8c3dda9c980b668c57abc2ca61efcf5c4"
 		},
 		{
 			"ImportPath": "k8s.io/apimachinery/pkg/runtime/serializer",
-			"Rev": "4d029f0333996cf231080e108e0bd1ece2a94d9f"
+			"Rev": "d04500c8c3dda9c980b668c57abc2ca61efcf5c4"
 		},
 		{
 			"ImportPath": "k8s.io/apimachinery/pkg/runtime/serializer/json",
-			"Rev": "4d029f0333996cf231080e108e0bd1ece2a94d9f"
+			"Rev": "d04500c8c3dda9c980b668c57abc2ca61efcf5c4"
 		},
 		{
 			"ImportPath": "k8s.io/apimachinery/pkg/runtime/serializer/protobuf",
-			"Rev": "4d029f0333996cf231080e108e0bd1ece2a94d9f"
+			"Rev": "d04500c8c3dda9c980b668c57abc2ca61efcf5c4"
 		},
 		{
 			"ImportPath": "k8s.io/apimachinery/pkg/runtime/serializer/recognizer",
-			"Rev": "4d029f0333996cf231080e108e0bd1ece2a94d9f"
+			"Rev": "d04500c8c3dda9c980b668c57abc2ca61efcf5c4"
 		},
 		{
 			"ImportPath": "k8s.io/apimachinery/pkg/runtime/serializer/streaming",
-			"Rev": "4d029f0333996cf231080e108e0bd1ece2a94d9f"
+			"Rev": "d04500c8c3dda9c980b668c57abc2ca61efcf5c4"
 		},
 		{
 			"ImportPath": "k8s.io/apimachinery/pkg/runtime/serializer/versioning",
-			"Rev": "4d029f0333996cf231080e108e0bd1ece2a94d9f"
+			"Rev": "d04500c8c3dda9c980b668c57abc2ca61efcf5c4"
 		},
 		{
 			"ImportPath": "k8s.io/apimachinery/pkg/selection",
-			"Rev": "4d029f0333996cf231080e108e0bd1ece2a94d9f"
+			"Rev": "d04500c8c3dda9c980b668c57abc2ca61efcf5c4"
 		},
 		{
 			"ImportPath": "k8s.io/apimachinery/pkg/types",
-			"Rev": "4d029f0333996cf231080e108e0bd1ece2a94d9f"
+			"Rev": "d04500c8c3dda9c980b668c57abc2ca61efcf5c4"
 		},
 		{
 			"ImportPath": "k8s.io/apimachinery/pkg/util/cache",
-			"Rev": "4d029f0333996cf231080e108e0bd1ece2a94d9f"
+			"Rev": "d04500c8c3dda9c980b668c57abc2ca61efcf5c4"
 		},
 		{
 			"ImportPath": "k8s.io/apimachinery/pkg/util/clock",
-			"Rev": "4d029f0333996cf231080e108e0bd1ece2a94d9f"
+			"Rev": "d04500c8c3dda9c980b668c57abc2ca61efcf5c4"
 		},
 		{
 			"ImportPath": "k8s.io/apimachinery/pkg/util/diff",
-			"Rev": "4d029f0333996cf231080e108e0bd1ece2a94d9f"
+			"Rev": "d04500c8c3dda9c980b668c57abc2ca61efcf5c4"
 		},
 		{
 			"ImportPath": "k8s.io/apimachinery/pkg/util/errors",
-			"Rev": "4d029f0333996cf231080e108e0bd1ece2a94d9f"
+			"Rev": "d04500c8c3dda9c980b668c57abc2ca61efcf5c4"
 		},
 		{
 			"ImportPath": "k8s.io/apimachinery/pkg/util/framer",
-			"Rev": "4d029f0333996cf231080e108e0bd1ece2a94d9f"
+			"Rev": "d04500c8c3dda9c980b668c57abc2ca61efcf5c4"
 		},
 		{
 			"ImportPath": "k8s.io/apimachinery/pkg/util/httpstream",
-			"Rev": "4d029f0333996cf231080e108e0bd1ece2a94d9f"
+			"Rev": "d04500c8c3dda9c980b668c57abc2ca61efcf5c4"
 		},
 		{
 			"ImportPath": "k8s.io/apimachinery/pkg/util/httpstream/spdy",
-			"Rev": "4d029f0333996cf231080e108e0bd1ece2a94d9f"
+			"Rev": "d04500c8c3dda9c980b668c57abc2ca61efcf5c4"
 		},
 		{
 			"ImportPath": "k8s.io/apimachinery/pkg/util/intstr",
-			"Rev": "4d029f0333996cf231080e108e0bd1ece2a94d9f"
+			"Rev": "d04500c8c3dda9c980b668c57abc2ca61efcf5c4"
 		},
 		{
 			"ImportPath": "k8s.io/apimachinery/pkg/util/json",
-			"Rev": "4d029f0333996cf231080e108e0bd1ece2a94d9f"
+			"Rev": "d04500c8c3dda9c980b668c57abc2ca61efcf5c4"
 		},
 		{
 			"ImportPath": "k8s.io/apimachinery/pkg/util/mergepatch",
-			"Rev": "4d029f0333996cf231080e108e0bd1ece2a94d9f"
+			"Rev": "d04500c8c3dda9c980b668c57abc2ca61efcf5c4"
 		},
 		{
 			"ImportPath": "k8s.io/apimachinery/pkg/util/naming",
-			"Rev": "4d029f0333996cf231080e108e0bd1ece2a94d9f"
+			"Rev": "d04500c8c3dda9c980b668c57abc2ca61efcf5c4"
 		},
 		{
 			"ImportPath": "k8s.io/apimachinery/pkg/util/net",
-			"Rev": "4d029f0333996cf231080e108e0bd1ece2a94d9f"
+			"Rev": "d04500c8c3dda9c980b668c57abc2ca61efcf5c4"
 		},
 		{
 			"ImportPath": "k8s.io/apimachinery/pkg/util/remotecommand",
-			"Rev": "4d029f0333996cf231080e108e0bd1ece2a94d9f"
+			"Rev": "d04500c8c3dda9c980b668c57abc2ca61efcf5c4"
 		},
 		{
 			"ImportPath": "k8s.io/apimachinery/pkg/util/runtime",
-			"Rev": "4d029f0333996cf231080e108e0bd1ece2a94d9f"
+			"Rev": "d04500c8c3dda9c980b668c57abc2ca61efcf5c4"
 		},
 		{
 			"ImportPath": "k8s.io/apimachinery/pkg/util/sets",
-			"Rev": "4d029f0333996cf231080e108e0bd1ece2a94d9f"
+			"Rev": "d04500c8c3dda9c980b668c57abc2ca61efcf5c4"
 		},
 		{
 			"ImportPath": "k8s.io/apimachinery/pkg/util/strategicpatch",
-			"Rev": "4d029f0333996cf231080e108e0bd1ece2a94d9f"
+			"Rev": "d04500c8c3dda9c980b668c57abc2ca61efcf5c4"
 		},
 		{
 			"ImportPath": "k8s.io/apimachinery/pkg/util/validation",
-			"Rev": "4d029f0333996cf231080e108e0bd1ece2a94d9f"
+			"Rev": "d04500c8c3dda9c980b668c57abc2ca61efcf5c4"
 		},
 		{
 			"ImportPath": "k8s.io/apimachinery/pkg/util/validation/field",
-			"Rev": "4d029f0333996cf231080e108e0bd1ece2a94d9f"
+			"Rev": "d04500c8c3dda9c980b668c57abc2ca61efcf5c4"
 		},
 		{
 			"ImportPath": "k8s.io/apimachinery/pkg/util/wait",
-			"Rev": "4d029f0333996cf231080e108e0bd1ece2a94d9f"
+			"Rev": "d04500c8c3dda9c980b668c57abc2ca61efcf5c4"
 		},
 		{
 			"ImportPath": "k8s.io/apimachinery/pkg/util/yaml",
-			"Rev": "4d029f0333996cf231080e108e0bd1ece2a94d9f"
+			"Rev": "d04500c8c3dda9c980b668c57abc2ca61efcf5c4"
 		},
 		{
 			"ImportPath": "k8s.io/apimachinery/pkg/version",
-			"Rev": "4d029f0333996cf231080e108e0bd1ece2a94d9f"
+			"Rev": "d04500c8c3dda9c980b668c57abc2ca61efcf5c4"
 		},
 		{
 			"ImportPath": "k8s.io/apimachinery/pkg/watch",
-			"Rev": "4d029f0333996cf231080e108e0bd1ece2a94d9f"
+			"Rev": "d04500c8c3dda9c980b668c57abc2ca61efcf5c4"
 		},
 		{
 			"ImportPath": "k8s.io/apimachinery/third_party/forked/golang/json",
-			"Rev": "4d029f0333996cf231080e108e0bd1ece2a94d9f"
+			"Rev": "d04500c8c3dda9c980b668c57abc2ca61efcf5c4"
 		},
 		{
 			"ImportPath": "k8s.io/apimachinery/third_party/forked/golang/netutil",
-			"Rev": "4d029f0333996cf231080e108e0bd1ece2a94d9f"
+			"Rev": "d04500c8c3dda9c980b668c57abc2ca61efcf5c4"
 		},
 		{
 			"ImportPath": "k8s.io/apimachinery/third_party/forked/golang/reflect",
-			"Rev": "4d029f0333996cf231080e108e0bd1ece2a94d9f"
+			"Rev": "d04500c8c3dda9c980b668c57abc2ca61efcf5c4"
 		},
 		{
 			"ImportPath": "k8s.io/klog",

discovery/cached/memcache.go

@@ -19,10 +19,14 @@ package cached
 import (
 	"errors"
 	"fmt"
+	"net"
+	"net/url"
 	"sync"
+	"syscall"
 
 	"github.com/googleapis/gnostic/OpenAPIv2"
 
+	errorsutil "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
 	"k8s.io/apimachinery/pkg/version"
@@ -30,40 +34,87 @@ import (
 	restclient "k8s.io/client-go/rest"
 )
 
+type cacheEntry struct {
+	resourceList *metav1.APIResourceList
+	err          error
+}
+
 // memCacheClient can Invalidate() to stay up-to-date with discovery
 // information.
 //
-// TODO: Switch to a watch interface. Right now it will poll anytime
+// TODO: Switch to a watch interface. Right now it will poll after each
-// Invalidate() is called.
+// Invalidate() call.
 type memCacheClient struct {
 	delegate discovery.DiscoveryInterface
 
 	lock                   sync.RWMutex
-	groupToServerResources map[string]*metav1.APIResourceList
+	groupToServerResources map[string]*cacheEntry
 	groupList              *metav1.APIGroupList
 	cacheValid             bool
 }
 
 // Error Constants
 var (
-	ErrCacheEmpty    = errors.New("the cache has not been filled yet")
 	ErrCacheNotFound = errors.New("not found")
 )
 
 var _ discovery.CachedDiscoveryInterface = &memCacheClient{}
 
+// isTransientConnectionError checks whether given error is "Connection refused" or
+// "Connection reset" error which usually means that apiserver is temporarily
+// unavailable.
+func isTransientConnectionError(err error) bool {
+	urlError, ok := err.(*url.Error)
+	if !ok {
+		return false
+	}
+	opError, ok := urlError.Err.(*net.OpError)
+	if !ok {
+		return false
+	}
+	errno, ok := opError.Err.(syscall.Errno)
+	if !ok {
+		return false
+	}
+	return errno == syscall.ECONNREFUSED || errno == syscall.ECONNRESET
+}
+
+func isTransientError(err error) bool {
+	if isTransientConnectionError(err) {
+		return true
+	}
+
+	if t, ok := err.(errorsutil.APIStatus); ok && t.Status().Code >= 500 {
+		return true
+	}
+
+	return errorsutil.IsTooManyRequests(err)
+}
+
 // ServerResourcesForGroupVersion returns the supported resources for a group and version.
 func (d *memCacheClient) ServerResourcesForGroupVersion(groupVersion string) (*metav1.APIResourceList, error) {
-	d.lock.RLock()
+	d.lock.Lock()
-	defer d.lock.RUnlock()
+	defer d.lock.Unlock()
 	if !d.cacheValid {
-		return nil, ErrCacheEmpty
+		if err := d.refreshLocked(); err != nil {
+			return nil, err
+		}
 	}
 	cachedVal, ok := d.groupToServerResources[groupVersion]
 	if !ok {
 		return nil, ErrCacheNotFound
 	}
-	return cachedVal, nil
+
+	if cachedVal.err != nil && isTransientError(cachedVal.err) {
+		r, err := d.serverResourcesForGroupVersion(groupVersion)
+		if err != nil {
+			utilruntime.HandleError(fmt.Errorf("couldn't get resource list for %v: %v", groupVersion, err))
+		}
+		cachedVal = &cacheEntry{r, err}
+		d.groupToServerResources[groupVersion] = cachedVal
+	}
+
+	return cachedVal.resourceList, cachedVal.err
 }
 
 // ServerResources returns the supported resources for all groups and versions.
@@ -72,10 +123,12 @@ func (d *memCacheClient) ServerResources() ([]*metav1.APIResourceList, error) {
 }
 
 func (d *memCacheClient) ServerGroups() (*metav1.APIGroupList, error) {
-	d.lock.RLock()
+	d.lock.Lock()
-	defer d.lock.RUnlock()
+	defer d.lock.Unlock()
-	if d.groupList == nil {
+	if !d.cacheValid {
-		return nil, ErrCacheEmpty
+		if err := d.refreshLocked(); err != nil {
+			return nil, err
+		}
 	}
 	return d.groupList, nil
 }
@@ -103,49 +156,59 @@ func (d *memCacheClient) OpenAPISchema() (*openapi_v2.Document, error) {
 func (d *memCacheClient) Fresh() bool {
 	d.lock.RLock()
 	defer d.lock.RUnlock()
-	// Fresh is supposed to tell the caller whether or not to retry if the cache
+	// Return whether the cache is populated at all. It is still possible that
-	// fails to find something. The idea here is that Invalidate will be called
+	// a single entry is missing due to transient errors and the attempt to read
-	// periodically and therefore we'll always be returning the latest data. (And
+	// that entry will trigger retry.
-	// in the future we can watch and stay even more up-to-date.) So we only
-	// return false if the cache has never been filled.
 	return d.cacheValid
 }
 
-// Invalidate refreshes the cache, blocking calls until the cache has been
+// Invalidate enforces that no cached data that is older than the current time
-// refreshed. It would be trivial to make a version that does this in the
+// is used.
-// background while continuing to respond to requests if needed.
 func (d *memCacheClient) Invalidate() {
 	d.lock.Lock()
 	defer d.lock.Unlock()
+	d.cacheValid = false
+	d.groupToServerResources = nil
+	d.groupList = nil
+}
 
+// refreshLocked refreshes the state of cache. The caller must hold d.lock for
+// writing.
+func (d *memCacheClient) refreshLocked() error {
 	// TODO: Could this multiplicative set of calls be replaced by a single call
 	// to ServerResources? If it's possible for more than one resulting
 	// APIResourceList to have the same GroupVersion, the lists would need merged.
 	gl, err := d.delegate.ServerGroups()
 	if err != nil || len(gl.Groups) == 0 {
-		utilruntime.HandleError(fmt.Errorf("couldn't get current server API group list; will keep using cached value. (%v)", err))
+		utilruntime.HandleError(fmt.Errorf("couldn't get current server API group list: %v", err))
-		return
+		return err
 	}
 
-	rl := map[string]*metav1.APIResourceList{}
+	rl := map[string]*cacheEntry{}
 	for _, g := range gl.Groups {
 		for _, v := range g.Versions {
-			r, err := d.delegate.ServerResourcesForGroupVersion(v.GroupVersion)
+			r, err := d.serverResourcesForGroupVersion(v.GroupVersion)
-			if err != nil || len(r.APIResources) == 0 {
+			rl[v.GroupVersion] = &cacheEntry{r, err}
+			if err != nil {
 				utilruntime.HandleError(fmt.Errorf("couldn't get resource list for %v: %v", v.GroupVersion, err))
-				if cur, ok := d.groupToServerResources[v.GroupVersion]; ok {
-					// retain the existing list, if we had it.
-					r = cur
-				} else {
-					continue
-				}
 			}
-			rl[v.GroupVersion] = r
 		}
 	}
 
 	d.groupToServerResources, d.groupList = rl, gl
 	d.cacheValid = true
+	return nil
+}
+
+func (d *memCacheClient) serverResourcesForGroupVersion(groupVersion string) (*metav1.APIResourceList, error) {
+	r, err := d.delegate.ServerResourcesForGroupVersion(groupVersion)
+	if err != nil {
+		return r, err
+	}
+	if len(r.APIResources) == 0 {
+		return r, fmt.Errorf("Got empty response for: %v", groupVersion)
+	}
+	return r, nil
 }
 
 // NewMemCacheClient creates a new CachedDiscoveryInterface which caches
@@ -156,6 +219,6 @@ func (d *memCacheClient) Invalidate() {
 func NewMemCacheClient(delegate discovery.DiscoveryInterface) discovery.CachedDiscoveryInterface {
 	return &memCacheClient{
 		delegate:               delegate,
-		groupToServerResources: map[string]*metav1.APIResourceList{},
+		groupToServerResources: map[string]*cacheEntry{},
 	}
 }

discovery/cached/memcache_test.go

@@ -18,27 +18,35 @@ package cached
 
 import (
 	"errors"
+	"net/http"
 	"reflect"
 	"sync"
 	"testing"
 
+	errorsutil "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/client-go/discovery/fake"
 )
 
+type resourceMapEntry struct {
+	list *metav1.APIResourceList
+	err  error
+}
+
 type fakeDiscovery struct {
 	*fake.FakeDiscovery
 
-	lock        sync.Mutex
+	lock         sync.Mutex
-	groupList   *metav1.APIGroupList
+	groupList    *metav1.APIGroupList
-	resourceMap map[string]*metav1.APIResourceList
+	groupListErr error
+	resourceMap  map[string]*resourceMapEntry
 }
 
 func (c *fakeDiscovery) ServerResourcesForGroupVersion(groupVersion string) (*metav1.APIResourceList, error) {
 	c.lock.Lock()
 	defer c.lock.Unlock()
 	if rl, ok := c.resourceMap[groupVersion]; ok {
-		return rl, nil
+		return rl.list, rl.err
 	}
 	return nil, errors.New("doesn't exist")
 }
@@ -49,7 +57,7 @@ func (c *fakeDiscovery) ServerGroups() (*metav1.APIGroupList, error) {
 	if c.groupList == nil {
 		return nil, errors.New("doesn't exist")
 	}
-	return c.groupList, nil
+	return c.groupList, c.groupListErr
 }
 
 func TestClient(t *testing.T) {
@@ -63,33 +71,37 @@ func TestClient(t *testing.T) {
 				}},
 			}},
 		},
-		resourceMap: map[string]*metav1.APIResourceList{
+		resourceMap: map[string]*resourceMapEntry{
 			"astronomy/v8beta1": {
-				GroupVersion: "astronomy/v8beta1",
+				list: &metav1.APIResourceList{
-				APIResources: []metav1.APIResource{{
+					GroupVersion: "astronomy/v8beta1",
-					Name:         "dwarfplanets",
+					APIResources: []metav1.APIResource{{
-					SingularName: "dwarfplanet",
+						Name:         "dwarfplanets",
-					Namespaced:   true,
+						SingularName: "dwarfplanet",
-					Kind:         "DwarfPlanet",
+						Namespaced:   true,
-					ShortNames:   []string{"dp"},
+						Kind:         "DwarfPlanet",
-				}},
+						ShortNames:   []string{"dp"},
+					}},
+				},
 			},
 		},
 	}
 
 	c := NewMemCacheClient(fake)
-	g, err := c.ServerGroups()
-	if err == nil {
-		t.Errorf("Unexpected non-error.")
-	}
 	if c.Fresh() {
 		t.Errorf("Expected not fresh.")
 	}
-
+	g, err := c.ServerGroups()
-	c.Invalidate()
+	if err != nil {
+		t.Errorf("Unexpected error: %v", err)
+	}
 	if !c.Fresh() {
 		t.Errorf("Expected fresh.")
 	}
+	c.Invalidate()
+	if c.Fresh() {
+		t.Errorf("Expected not fresh.")
+	}
 
 	g, err = c.ServerGroups()
 	if err != nil {
@@ -98,25 +110,30 @@ func TestClient(t *testing.T) {
 	if e, a := fake.groupList, g; !reflect.DeepEqual(e, a) {
 		t.Errorf("Expected %#v, got %#v", e, a)
 	}
+	if !c.Fresh() {
+		t.Errorf("Expected fresh.")
+	}
 	r, err := c.ServerResourcesForGroupVersion("astronomy/v8beta1")
 	if err != nil {
 		t.Errorf("Unexpected error: %v", err)
 	}
-	if e, a := fake.resourceMap["astronomy/v8beta1"], r; !reflect.DeepEqual(e, a) {
+	if e, a := fake.resourceMap["astronomy/v8beta1"].list, r; !reflect.DeepEqual(e, a) {
 		t.Errorf("Expected %#v, got %#v", e, a)
 	}
 
 	fake.lock.Lock()
-	fake.resourceMap = map[string]*metav1.APIResourceList{
+	fake.resourceMap = map[string]*resourceMapEntry{
 		"astronomy/v8beta1": {
-			GroupVersion: "astronomy/v8beta1",
+			list: &metav1.APIResourceList{
-			APIResources: []metav1.APIResource{{
+				GroupVersion: "astronomy/v8beta1",
-				Name:         "stars",
+				APIResources: []metav1.APIResource{{
-				SingularName: "star",
+					Name:         "stars",
-				Namespaced:   true,
+					SingularName: "star",
-				Kind:         "Star",
+					Namespaced:   true,
-				ShortNames:   []string{"s"},
+					Kind:         "Star",
-			}},
+					ShortNames:   []string{"s"},
+				}},
+			},
 		},
 	}
 	fake.lock.Unlock()
@@ -126,7 +143,235 @@ func TestClient(t *testing.T) {
 	if err != nil {
 		t.Errorf("Unexpected error: %v", err)
 	}
-	if e, a := fake.resourceMap["astronomy/v8beta1"], r; !reflect.DeepEqual(e, a) {
+	if e, a := fake.resourceMap["astronomy/v8beta1"].list, r; !reflect.DeepEqual(e, a) {
+		t.Errorf("Expected %#v, got %#v", e, a)
+	}
+}
+
+func TestServerGroupsFails(t *testing.T) {
+	fake := &fakeDiscovery{
+		groupList: &metav1.APIGroupList{
+			Groups: []metav1.APIGroup{{
+				Name: "astronomy",
+				Versions: []metav1.GroupVersionForDiscovery{{
+					GroupVersion: "astronomy/v8beta1",
+					Version:      "v8beta1",
+				}},
+			}},
+		},
+		groupListErr: errors.New("some error"),
+		resourceMap: map[string]*resourceMapEntry{
+			"astronomy/v8beta1": {
+				list: &metav1.APIResourceList{
+					GroupVersion: "astronomy/v8beta1",
+					APIResources: []metav1.APIResource{{
+						Name:         "dwarfplanets",
+						SingularName: "dwarfplanet",
+						Namespaced:   true,
+						Kind:         "DwarfPlanet",
+						ShortNames:   []string{"dp"},
+					}},
+				},
+			},
+		},
+	}
+
+	c := NewMemCacheClient(fake)
+	if c.Fresh() {
+		t.Errorf("Expected not fresh.")
+	}
+	_, err := c.ServerGroups()
+	if err == nil {
+		t.Errorf("Expected error")
+	}
+	if c.Fresh() {
+		t.Errorf("Expected not fresh.")
+	}
+	fake.lock.Lock()
+	fake.groupListErr = nil
+	fake.lock.Unlock()
+	r, err := c.ServerResourcesForGroupVersion("astronomy/v8beta1")
+	if err != nil {
+		t.Errorf("Unexpected error: %v", err)
+	}
+	if e, a := fake.resourceMap["astronomy/v8beta1"].list, r; !reflect.DeepEqual(e, a) {
+		t.Errorf("Expected %#v, got %#v", e, a)
+	}
+	if !c.Fresh() {
+		t.Errorf("Expected not fresh.")
+	}
+}
+
+func TestPartialPermanentFailure(t *testing.T) {
+	fake := &fakeDiscovery{
+		groupList: &metav1.APIGroupList{
+			Groups: []metav1.APIGroup{{
+				Name: "astronomy",
+				Versions: []metav1.GroupVersionForDiscovery{{
+					GroupVersion: "astronomy/v8beta1",
+					Version:      "v8beta1",
+				}, {
+					GroupVersion: "astronomy2/v8beta1",
+					Version:      "v8beta1",
+				}},
+			}},
+		},
+		resourceMap: map[string]*resourceMapEntry{
+			"astronomy/v8beta1": {
+				err: errors.New("some permanent error"),
+			},
+			"astronomy2/v8beta1": {
+				list: &metav1.APIResourceList{
+					GroupVersion: "astronomy2/v8beta1",
+					APIResources: []metav1.APIResource{{
+						Name:         "dwarfplanets",
+						SingularName: "dwarfplanet",
+						Namespaced:   true,
+						Kind:         "DwarfPlanet",
+						ShortNames:   []string{"dp"},
+					}},
+				},
+			},
+		},
+	}
+
+	c := NewMemCacheClient(fake)
+	if c.Fresh() {
+		t.Errorf("Expected not fresh.")
+	}
+	r, err := c.ServerResourcesForGroupVersion("astronomy2/v8beta1")
+	if err != nil {
+		t.Errorf("Unexpected error: %v", err)
+	}
+	if e, a := fake.resourceMap["astronomy2/v8beta1"].list, r; !reflect.DeepEqual(e, a) {
+		t.Errorf("Expected %#v, got %#v", e, a)
+	}
+	_, err = c.ServerResourcesForGroupVersion("astronomy/v8beta1")
+	if err == nil {
+		t.Errorf("Expected error, got nil")
+	}
+
+	fake.lock.Lock()
+	fake.resourceMap["astronomy/v8beta1"] = &resourceMapEntry{
+		list: &metav1.APIResourceList{
+			GroupVersion: "astronomy/v8beta1",
+			APIResources: []metav1.APIResource{{
+				Name:         "dwarfplanets",
+				SingularName: "dwarfplanet",
+				Namespaced:   true,
+				Kind:         "DwarfPlanet",
+				ShortNames:   []string{"dp"},
+			}},
+		},
+		err: nil,
+	}
+	fake.lock.Unlock()
+	// We don't retry permanent errors, so it should fail.
+	_, err = c.ServerResourcesForGroupVersion("astronomy/v8beta1")
+	if err == nil {
+		t.Errorf("Expected error, got nil")
+	}
+	c.Invalidate()
+
+	// After Invalidate, we should retry.
+	r, err = c.ServerResourcesForGroupVersion("astronomy/v8beta1")
+	if err != nil {
+		t.Errorf("Unexpected error: %v", err)
+	}
+	if e, a := fake.resourceMap["astronomy/v8beta1"].list, r; !reflect.DeepEqual(e, a) {
+		t.Errorf("Expected %#v, got %#v", e, a)
+	}
+}
+
+func TestPartialRetryableFailure(t *testing.T) {
+	fake := &fakeDiscovery{
+		groupList: &metav1.APIGroupList{
+			Groups: []metav1.APIGroup{{
+				Name: "astronomy",
+				Versions: []metav1.GroupVersionForDiscovery{{
+					GroupVersion: "astronomy/v8beta1",
+					Version:      "v8beta1",
+				}, {
+					GroupVersion: "astronomy2/v8beta1",
+					Version:      "v8beta1",
+				}},
+			}},
+		},
+		resourceMap: map[string]*resourceMapEntry{
+			"astronomy/v8beta1": {
+				err: &errorsutil.StatusError{
+					ErrStatus: metav1.Status{
+						Message: "Some retryable error",
+						Code:    int32(http.StatusServiceUnavailable),
+						Reason:  metav1.StatusReasonServiceUnavailable,
+					},
+				},
+			},
+			"astronomy2/v8beta1": {
+				list: &metav1.APIResourceList{
+					GroupVersion: "astronomy2/v8beta1",
+					APIResources: []metav1.APIResource{{
+						Name:         "dwarfplanets",
+						SingularName: "dwarfplanet",
+						Namespaced:   true,
+						Kind:         "DwarfPlanet",
+						ShortNames:   []string{"dp"},
+					}},
+				},
+			},
+		},
+	}
+
+	c := NewMemCacheClient(fake)
+	if c.Fresh() {
+		t.Errorf("Expected not fresh.")
+	}
+	r, err := c.ServerResourcesForGroupVersion("astronomy2/v8beta1")
+	if err != nil {
+		t.Errorf("Unexpected error: %v", err)
+	}
+	if e, a := fake.resourceMap["astronomy2/v8beta1"].list, r; !reflect.DeepEqual(e, a) {
+		t.Errorf("Expected %#v, got %#v", e, a)
+	}
+	_, err = c.ServerResourcesForGroupVersion("astronomy/v8beta1")
+	if err == nil {
+		t.Errorf("Expected error, got nil")
+	}
+
+	fake.lock.Lock()
+	fake.resourceMap["astronomy/v8beta1"] = &resourceMapEntry{
+		list: &metav1.APIResourceList{
+			GroupVersion: "astronomy/v8beta1",
+			APIResources: []metav1.APIResource{{
+				Name:         "dwarfplanets",
+				SingularName: "dwarfplanet",
+				Namespaced:   true,
+				Kind:         "DwarfPlanet",
+				ShortNames:   []string{"dp"},
+			}},
+		},
+		err: nil,
+	}
+	fake.lock.Unlock()
+	// We should retry retryable error even without Invalidate() being called,
+	// so no error is expected.
+	r, err = c.ServerResourcesForGroupVersion("astronomy/v8beta1")
+	if err != nil {
+		t.Errorf("Expected no error, got %v", err)
+	}
+	if e, a := fake.resourceMap["astronomy/v8beta1"].list, r; !reflect.DeepEqual(e, a) {
+		t.Errorf("Expected %#v, got %#v", e, a)
+	}
+
+	// Check that the last result was cached and we don't retry further.
+	fake.lock.Lock()
+	fake.resourceMap["astronomy/v8beta1"].err = errors.New("some permanent error")
+	fake.lock.Unlock()
+	r, err = c.ServerResourcesForGroupVersion("astronomy/v8beta1")
+	if err != nil {
+		t.Errorf("Expected no error, got %v", err)
+	}
+	if e, a := fake.resourceMap["astronomy/v8beta1"].list, r; !reflect.DeepEqual(e, a) {
 		t.Errorf("Expected %#v, got %#v", e, a)
 	}
 }

discovery/discovery_client.go

@@ -26,7 +26,7 @@ import (
 	"time"
 
 	"github.com/golang/protobuf/proto"
-	"github.com/googleapis/gnostic/OpenAPIv2"
+	openapi_v2 "github.com/googleapis/gnostic/OpenAPIv2"
 
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -60,6 +60,9 @@ type DiscoveryInterface interface {
 }
 
 // CachedDiscoveryInterface is a DiscoveryInterface with cache invalidation and freshness.
+// Note that If the ServerResourcesForGroupVersion method returns a cache miss
+// error, the user needs to explicitly call Invalidate to clear the cache,
+// otherwise the same cache miss error will be returned next time.
 type CachedDiscoveryInterface interface {
 	DiscoveryInterface
 	// Fresh is supposed to tell the caller whether or not to retry if the cache
@@ -68,7 +71,8 @@ type CachedDiscoveryInterface interface {
 	// TODO: this needs to be revisited, this interface can't be locked properly
 	// and doesn't make a lot of sense.
 	Fresh() bool
-	// Invalidate enforces that no cached data is used in the future that is older than the current time.
+	// Invalidate enforces that no cached data that is older than the current time
+	// is used.
 	Invalidate()
 }