From 2639b75d84cbfb513f90f65de1efb46c50a2fbb6 Mon Sep 17 00:00:00 2001 From: Dan Mace Date: Tue, 27 Jun 2017 11:31:32 -0400 Subject: [PATCH] Refactor cached discovery client --- .../k8s.io/client-go/discovery/cached}/BUILD | 21 +---- .../client-go/discovery/cached/memcache.go | 86 +++++++++++-------- .../discovery/cached/memcache_test.go | 4 +- .../client-go/discovery/discovery_client.go | 6 +- 4 files changed, 63 insertions(+), 54 deletions(-) rename {pkg/controller/garbagecollector/memcachediscovery => staging/src/k8s.io/client-go/discovery/cached}/BUILD (69%) rename pkg/controller/garbagecollector/memcachediscovery/client.go => staging/src/k8s.io/client-go/discovery/cached/memcache.go (53%) rename pkg/controller/garbagecollector/memcachediscovery/client_test.go => staging/src/k8s.io/client-go/discovery/cached/memcache_test.go (98%) diff --git a/pkg/controller/garbagecollector/memcachediscovery/BUILD b/staging/src/k8s.io/client-go/discovery/cached/BUILD similarity index 69% rename from pkg/controller/garbagecollector/memcachediscovery/BUILD rename to staging/src/k8s.io/client-go/discovery/cached/BUILD index c9c80eb6175..6db3441fb54 100644 --- a/pkg/controller/garbagecollector/memcachediscovery/BUILD +++ b/staging/src/k8s.io/client-go/discovery/cached/BUILD @@ -10,7 +10,7 @@ load( go_test( name = "go_default_test", - srcs = ["client_test.go"], + srcs = ["memcache_test.go"], library = ":go_default_library", tags = ["automanaged"], deps = [ @@ -21,29 +21,16 @@ go_test( go_library( name = "go_default_library", - srcs = ["client.go"], + srcs = ["memcache.go"], tags = ["automanaged"], deps = [ "//vendor/github.com/emicklei/go-restful-swagger12:go_default_library", - "//vendor/github.com/go-openapi/spec:go_default_library", - "//vendor/github.com/golang/glog:go_default_library", + "//vendor/github.com/googleapis/gnostic/OpenAPIv2:go_default_library", "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", "//vendor/k8s.io/apimachinery/pkg/runtime/schema:go_default_library", + "//vendor/k8s.io/apimachinery/pkg/util/runtime:go_default_library", "//vendor/k8s.io/apimachinery/pkg/version:go_default_library", "//vendor/k8s.io/client-go/discovery:go_default_library", "//vendor/k8s.io/client-go/rest:go_default_library", ], ) - -filegroup( - name = "package-srcs", - srcs = glob(["**"]), - tags = ["automanaged"], - visibility = ["//visibility:private"], -) - -filegroup( - name = "all-srcs", - srcs = [":package-srcs"], - tags = ["automanaged"], -) diff --git a/pkg/controller/garbagecollector/memcachediscovery/client.go b/staging/src/k8s.io/client-go/discovery/cached/memcache.go similarity index 53% rename from pkg/controller/garbagecollector/memcachediscovery/client.go rename to staging/src/k8s.io/client-go/discovery/cached/memcache.go index 15d53456ef2..7489a5bbf17 100644 --- a/pkg/controller/garbagecollector/memcachediscovery/client.go +++ b/staging/src/k8s.io/client-go/discovery/cached/memcache.go @@ -14,28 +14,30 @@ See the License for the specific language governing permissions and limitations under the License. */ -// Package memcachediscovery includes a Client which is a CachedDiscoveryInterface. -package memcachediscovery +package cached import ( "errors" + "fmt" "sync" "github.com/emicklei/go-restful-swagger12" - "github.com/go-openapi/spec" - "github.com/golang/glog" + "github.com/googleapis/gnostic/OpenAPIv2" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" + utilruntime "k8s.io/apimachinery/pkg/util/runtime" "k8s.io/apimachinery/pkg/version" "k8s.io/client-go/discovery" restclient "k8s.io/client-go/rest" ) -// Client can Refresh() to stay up-to-date with discovery information. Before -// this is moved some place where it's easier to call, it needs to switch to a -// watch interface. Right now it will poll anytime Refresh() is called. -type Client struct { +// memCacheClient can Invalidate() to stay up-to-date with discovery +// information. +// +// TODO: Switch to a watch interface. Right now it will poll anytime +// Invalidate() is called. +type memCacheClient struct { delegate discovery.DiscoveryInterface lock sync.RWMutex @@ -45,24 +47,28 @@ type Client struct { } var ( - ErrCacheEmpty = errors.New("the cache has not been filled yet") + ErrCacheEmpty = errors.New("the cache has not been filled yet") + ErrCacheNotFound = errors.New("not found") ) -var _ discovery.CachedDiscoveryInterface = &Client{} +var _ discovery.CachedDiscoveryInterface = &memCacheClient{} // ServerResourcesForGroupVersion returns the supported resources for a group and version. -func (d *Client) ServerResourcesForGroupVersion(groupVersion string) (*metav1.APIResourceList, error) { +func (d *memCacheClient) ServerResourcesForGroupVersion(groupVersion string) (*metav1.APIResourceList, error) { d.lock.RLock() defer d.lock.RUnlock() + if !d.cacheValid { + return nil, ErrCacheEmpty + } cachedVal, ok := d.groupToServerResources[groupVersion] if !ok { - return nil, ErrCacheEmpty + return nil, ErrCacheNotFound } return cachedVal, nil } // ServerResources returns the supported resources for all groups and versions. -func (d *Client) ServerResources() ([]*metav1.APIResourceList, error) { +func (d *memCacheClient) ServerResources() ([]*metav1.APIResourceList, error) { apiGroups, err := d.ServerGroups() if err != nil { return nil, err @@ -79,7 +85,7 @@ func (d *Client) ServerResources() ([]*metav1.APIResourceList, error) { return result, nil } -func (d *Client) ServerGroups() (*metav1.APIGroupList, error) { +func (d *memCacheClient) ServerGroups() (*metav1.APIGroupList, error) { d.lock.RLock() defer d.lock.RUnlock() if d.groupList == nil { @@ -88,51 +94,60 @@ func (d *Client) ServerGroups() (*metav1.APIGroupList, error) { return d.groupList, nil } -func (d *Client) RESTClient() restclient.Interface { +func (d *memCacheClient) RESTClient() restclient.Interface { return d.delegate.RESTClient() } -func (d *Client) ServerPreferredResources() ([]*metav1.APIResourceList, error) { +// TODO: Should this also be cached? The results seem more likely to be +// inconsistent with ServerGroups and ServerResources given the requirement to +// actively Invalidate. +func (d *memCacheClient) ServerPreferredResources() ([]*metav1.APIResourceList, error) { return d.delegate.ServerPreferredResources() } -func (d *Client) ServerPreferredNamespacedResources() ([]*metav1.APIResourceList, error) { +// TODO: Should this also be cached? The results seem more likely to be +// inconsistent with ServerGroups and ServerResources given the requirement to +// actively Invalidate. +func (d *memCacheClient) ServerPreferredNamespacedResources() ([]*metav1.APIResourceList, error) { return d.delegate.ServerPreferredNamespacedResources() } -func (d *Client) ServerVersion() (*version.Info, error) { +func (d *memCacheClient) ServerVersion() (*version.Info, error) { return d.delegate.ServerVersion() } -func (d *Client) SwaggerSchema(version schema.GroupVersion) (*swagger.ApiDeclaration, error) { +func (d *memCacheClient) SwaggerSchema(version schema.GroupVersion) (*swagger.ApiDeclaration, error) { return d.delegate.SwaggerSchema(version) } -func (d *Client) OpenAPISchema() (*spec.Swagger, error) { +func (d *memCacheClient) OpenAPISchema() (*openapi_v2.Document, error) { return d.delegate.OpenAPISchema() } -func (d *Client) Fresh() bool { +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 fails to find something. The idea here is that Refresh and/or - // Invalidate will be called periodically and therefore we'll always be - // returning the latest data. (And 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. + // Fresh is supposed to tell the caller whether or not to retry if the cache + // fails to find something. The idea here is that Invalidate will be called + // periodically and therefore we'll always be returning the latest data. (And + // 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 // refreshed. It would be trivial to make a version that does this in the // background while continuing to respond to requests if needed. -func (d *Client) Invalidate() { +func (d *memCacheClient) Invalidate() { d.lock.Lock() defer d.lock.Unlock() + + // 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 { - glog.V(2).Infof("Error getting current server API group list, will keep using cached value. (%v)", err) + utilruntime.HandleError(fmt.Errorf("couldn't get current server API group list; will keep using cached value. (%v)", err)) return } @@ -141,7 +156,7 @@ func (d *Client) Invalidate() { for _, v := range g.Versions { r, err := d.delegate.ServerResourcesForGroupVersion(v.GroupVersion) if err != nil || len(r.APIResources) == 0 { - glog.V(2).Infof("Error getting resource list for %v: %v", v.GroupVersion, err) + 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 @@ -157,10 +172,13 @@ func (d *Client) Invalidate() { d.cacheValid = true } -// NewClient creates a new Client which caches discovery information in memory -// and will stay up-to-date if Invalidate is called with regularity. -func NewClient(delegate discovery.DiscoveryInterface) *Client { - return &Client{ +// NewMemCacheClient creates a new CachedDiscoveryInterface which caches +// discovery information in memory and will stay up-to-date if Invalidate is +// called with regularity. +// +// NOTE: The client will NOT resort to live lookups on cache misses. +func NewMemCacheClient(delegate discovery.DiscoveryInterface) discovery.CachedDiscoveryInterface { + return &memCacheClient{ delegate: delegate, groupToServerResources: map[string]*metav1.APIResourceList{}, } diff --git a/pkg/controller/garbagecollector/memcachediscovery/client_test.go b/staging/src/k8s.io/client-go/discovery/cached/memcache_test.go similarity index 98% rename from pkg/controller/garbagecollector/memcachediscovery/client_test.go rename to staging/src/k8s.io/client-go/discovery/cached/memcache_test.go index f7ac08e5556..a9caa6e83d3 100644 --- a/pkg/controller/garbagecollector/memcachediscovery/client_test.go +++ b/staging/src/k8s.io/client-go/discovery/cached/memcache_test.go @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package memcachediscovery +package cached import ( "errors" @@ -77,7 +77,7 @@ func TestClient(t *testing.T) { }, } - c := NewClient(fake) + c := NewMemCacheClient(fake) g, err := c.ServerGroups() if err == nil { t.Errorf("Unexpected non-error.") diff --git a/staging/src/k8s.io/client-go/discovery/discovery_client.go b/staging/src/k8s.io/client-go/discovery/discovery_client.go index 45ae17c35e2..0a6855c25f8 100644 --- a/staging/src/k8s.io/client-go/discovery/discovery_client.go +++ b/staging/src/k8s.io/client-go/discovery/discovery_client.go @@ -55,7 +55,11 @@ type DiscoveryInterface interface { // CachedDiscoveryInterface is a DiscoveryInterface with cache invalidation and freshness. type CachedDiscoveryInterface interface { DiscoveryInterface - // Fresh returns true if no cached data was used that had been retrieved before the instantiation. + // Fresh is supposed to tell the caller whether or not to retry if the cache + // fails to find something (false = retry, true = no need to retry). + // + // 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()