Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use slightly modified local copy of latest upstream memcache client #414

Merged
merged 4 commits into from
Feb 12, 2019

Conversation

lblackstone
Copy link
Member

The latest upstream memcache client includes retry logic. Use this code,
but remove the klog logging statements for failed cache operations, and
switch to a --v9 glog info message.

Fixes #398

…ient

The latest upstream memcache client includes retry logic. Use this code,
but remove the klog logging statements for failed cache operations, and
switch to a --v9 glog info message.
Copy link
Contributor

@hausdorff hausdorff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks ok, but can you also post the diff between the two for posterity?

Copy link
Contributor

@hausdorff hausdorff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh actually, a couple more questions.

  • We copied code in ksonnet and in the original k8s provider, too -- just out of curiosity, do we plan to keep this up to date? I think it's ok if the answer is no, just curious.

pkg/clients/memcache.go Outdated Show resolved Hide resolved
@lblackstone
Copy link
Member Author

lblackstone commented Feb 12, 2019

Here's the diff:

diff --git a/pkg/clients/memcache.go b/pkg/clients/memcache.go
index bd6b478..fb4104e 100644
--- a/pkg/clients/memcache.go
+++ b/pkg/clients/memcache.go
@@ -14,23 +14,31 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-package cached
+//
+// Note: this code is taken nearly verbatim from the upstream memcache client [1].
+// The `refreshLocked` method does not return errors, but instead logs them using
+// the `HandleError` method, which dispatches to klog. This bypasses Pulumi's logging
+// mechanism, and causes unactionable error messages to appear in the CLI.
+// This error logging was disabled.
+//
+// [1] https://github.com/kubernetes/client-go/blob/2dda7ceeec102b5a60e2abf37758642118501910/discovery/cached/memcache.go
+
+package clients
 
 import (
-	"errors"
 	"fmt"
 	"net"
 	"net/url"
 	"sync"
 	"syscall"
 
+	"github.com/golang/glog"
 	"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"
 	"k8s.io/client-go/discovery"
+	"k8s.io/client-go/discovery/cached"
 	restclient "k8s.io/client-go/rest"
 )
 
@@ -53,11 +61,6 @@ type memCacheClient struct {
 	cacheValid             bool
 }
 
-// Error Constants
-var (
-	ErrCacheNotFound = errors.New("not found")
-)
-
 var _ discovery.CachedDiscoveryInterface = &memCacheClient{}
 
 // isTransientConnectionError checks whether given error is "Connection refused" or
@@ -102,13 +105,13 @@ func (d *memCacheClient) ServerResourcesForGroupVersion(groupVersion string) (*m
 	}
 	cachedVal, ok := d.groupToServerResources[groupVersion]
 	if !ok {
-		return nil, ErrCacheNotFound
+		return nil, cached.ErrCacheNotFound
 	}
 
 	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))
+			glog.V(9).Infof("couldn't get resource list for %v: %v", groupVersion, err)
 		}
 		cachedVal = &cacheEntry{r, err}
 		d.groupToServerResources[groupVersion] = cachedVal
@@ -180,7 +183,7 @@ func (d *memCacheClient) refreshLocked() error {
 	// 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: %v", err))
+		glog.V(9).Infof("couldn't get current server API group list: %v", err)
 		return err
 	}
 
@@ -190,7 +193,7 @@ func (d *memCacheClient) refreshLocked() error {
 			r, err := d.serverResourcesForGroupVersion(v.GroupVersion)
 			rl[v.GroupVersion] = &cacheEntry{r, err}
 			if err != nil {
-				utilruntime.HandleError(fmt.Errorf("couldn't get resource list for %v: %v", v.GroupVersion, err))
+				glog.V(9).Infof("couldn't get resource list for %v: %v", v.GroupVersion, err)
 			}
 		}
 	}

I'm not currently intending on keeping this up to date with upstream, but we could pull in changes if they've made obvious improvements. The main problem is that the upstream client logs messages outside of our control, and it doesn't seem likely that they will make that configurable upstream.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants