move cached_discovery to client-go/discovery #63550

juanvallejo · 2018-05-08T17:52:14Z

Release note:

NONE

Moves the cmd/util CachedDiscoveryClient to client-go

cc @soltysh @deads2k

deads2k · 2018-05-08T20:00:48Z

pkg/kubectl/cmd/util/shortcut_restmapper_test.go

@@ -194,3 +200,76 @@ func TestKindFor(t *testing.T) {
 		}
 	}
 }
+
+type fakeDiscoveryClient struct {
+	groupCalls    int


slim down to what is actually needed.

@juanvallejo someone was working on a real fake discovery client. We need to find that pull again.

@deads2k

someone was working on a real fake discovery client. We need to find that pull again.

was it #62069 ?

deads2k · 2018-05-08T20:01:43Z

staging/src/k8s.io/client-go/discovery/cached_discovery.go

@@ -30,15 +30,14 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/version"
-	"k8s.io/client-go/discovery"
 	restclient "k8s.io/client-go/rest"
 	"k8s.io/kubernetes/pkg/kubectl/scheme"


You may not have this dependency in client-go. nothing from kube/kube

Switched to k8s.io/client-go/kubernetes/scheme

witched to k8s.io/client-go/kubernetes/scheme

What is it used for? I expected you to create your own scheme locally like our other clients do.

deads2k · 2018-05-08T20:05:33Z

I'm 95% sure you need the associated transport and that your external constructor needs to wrap the entire thing.

deads2k · 2018-05-08T20:40:36Z

I still don't see the transport

soltysh · 2018-05-14T13:18:19Z

pkg/kubectl/cmd/util/config_flags.go

+	}
+
+	discoveryCacheDir := computeDiscoverCacheDir(filepath.Join(homedir.HomeDir(), ".kube", "cache", "discovery"), config.Host)
+	return discovery.NewCachedDiscoveryClientForConfig(config, discoveryCacheDir, httpCacheDir, time.Duration(10*time.Minute))


You eventually went with double cache options? Any particular reason?

@soltysh

You eventually went with double cache options? Any particular reason?

Per our conversation, I thought about this a bit further and decided that it might be best to add any changes in behavior in a separate PR explicitly for doing so.

Per @deads2k 's comments below, will add a godoc explaining the current behavior

deads2k · 2018-05-14T13:50:58Z

pkg/kubectl/genericclioptions/resource/builder_test.go

@@ -1380,3 +1418,76 @@ func TestMultipleTypesRequested(t *testing.T) {
 		}
 	}
 }
+
+type fakeDiscoveryClient struct {


this looks like it should be deleted.

deads2k · 2018-05-14T13:53:44Z

staging/src/k8s.io/client-go/discovery/cached_discovery.go

+// NewCachedDiscoveryClientForConfig creates a new DiscoveryClient for the given config, and wraps
+// the created client in a CachedDiscoveryClient. The provided configuration is upddated with a
+// custom transport that understands cache responses.
+func NewCachedDiscoveryClientForConfig(config *restclient.Config, discoveryCacheDir, httpCacheDir string, ttl time.Duration) (*CachedDiscoveryClient, error) {


Document why we have two cache dirs, even if it is just historical with a TODO to figure out how to stop having two.

and doc behavior on empty

juanvallejo · 2018-05-14T15:03:16Z

@deads2k thanks, comments addressed

deads2k · 2018-05-14T15:37:31Z

/retest

deads2k · 2018-05-14T15:39:08Z

/lgtm

deads2k · 2018-05-14T18:35:01Z

metrics change is a generated file based on client needs.

soltysh

/lgtm

k8s-ci-robot · 2018-05-15T14:38:57Z

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: deads2k, juanvallejo, soltysh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/kubectl/OWNERS~~ [deads2k,soltysh]
~~staging/src/k8s.io/apiextensions-apiserver/Godeps/OWNERS~~ [deads2k]
~~staging/src/k8s.io/apiserver/Godeps/OWNERS~~ [deads2k]
~~staging/src/k8s.io/client-go/OWNERS~~ [deads2k]
~~staging/src/k8s.io/kube-aggregator/Godeps/OWNERS~~ [deads2k]
staging/src/k8s.io/metrics/Godeps/OWNERS
~~staging/src/k8s.io/sample-apiserver/Godeps/OWNERS~~ [deads2k]
staging/src/k8s.io/sample-controller/Godeps/OWNERS
~~test/OWNERS~~ [deads2k,soltysh]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

juanvallejo · 2018-05-15T15:47:00Z

/test pull-kubernetes-e2e-gce

k8s-github-robot · 2018-05-15T16:54:29Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2018-05-15T17:47:00Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@soltysh

…-genericclioptions Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. move config flags to pkg/kubectl/genericclioptions **Release note**: ```release-note NONE ``` Moves ConfigFlags to `pkg/kubectl/genericclioptions` ~~Depends on #63550 cc @soltysh @deads2k

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 8, 2018

k8s-ci-robot requested review from sttts and vishh May 8, 2018 17:52

juanvallejo force-pushed the jvallejo/move-cached-discovery branch 2 times, most recently from a314732 to ef7b7bf Compare May 8, 2018 18:12

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 8, 2018

juanvallejo force-pushed the jvallejo/move-cached-discovery branch from ef7b7bf to 28a8021 Compare May 8, 2018 18:15

juanvallejo mentioned this pull request May 8, 2018

move config flags to pkg/kubectl/genericclioptions #63514

Merged

juanvallejo force-pushed the jvallejo/move-cached-discovery branch from 28a8021 to e036f67 Compare May 8, 2018 18:32

deads2k reviewed May 8, 2018

View reviewed changes

juanvallejo force-pushed the jvallejo/move-cached-discovery branch from e036f67 to bba674c Compare May 8, 2018 20:15

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 8, 2018

juanvallejo force-pushed the jvallejo/move-cached-discovery branch from bba674c to da5ca88 Compare May 8, 2018 20:21

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 8, 2018

juanvallejo force-pushed the jvallejo/move-cached-discovery branch from da5ca88 to ec57ab0 Compare May 8, 2018 20:24

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 8, 2018

juanvallejo force-pushed the jvallejo/move-cached-discovery branch from ec57ab0 to e4a6d04 Compare May 8, 2018 20:26

juanvallejo force-pushed the jvallejo/move-cached-discovery branch from e4a6d04 to 237d7f1 Compare May 8, 2018 20:49

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 8, 2018

juanvallejo force-pushed the jvallejo/move-cached-discovery branch from 90131dd to caa31e0 Compare May 8, 2018 21:19

juanvallejo force-pushed the jvallejo/move-cached-discovery branch 3 times, most recently from 319f5d8 to e3a82b4 Compare May 11, 2018 22:00

soltysh reviewed May 14, 2018

View reviewed changes

deads2k reviewed May 14, 2018

View reviewed changes

juanvallejo force-pushed the jvallejo/move-cached-discovery branch 4 times, most recently from 3f86e26 to d6c4009 Compare May 14, 2018 15:01

k8s-ci-robot assigned deads2k May 14, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 14, 2018

deads2k added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 14, 2018

juanvallejo added 2 commits May 15, 2018 10:20

move cached_discovery to client-go/discovery

4059355

restore old cached client behavior

57f308a

juanvallejo force-pushed the jvallejo/move-cached-discovery branch from d6c4009 to 57f308a Compare May 15, 2018 14:38

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 15, 2018

soltysh approved these changes May 15, 2018

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 15, 2018

k8s-github-robot merged commit f2ea83b into kubernetes:master May 15, 2018

juanvallejo deleted the jvallejo/move-cached-discovery branch May 15, 2018 18:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

move cached_discovery to client-go/discovery #63550

move cached_discovery to client-go/discovery #63550

juanvallejo commented May 8, 2018

deads2k May 8, 2018

juanvallejo May 8, 2018

deads2k May 8, 2018

juanvallejo May 8, 2018

deads2k May 8, 2018

deads2k commented May 8, 2018

deads2k commented May 8, 2018

soltysh May 14, 2018

juanvallejo May 14, 2018

deads2k May 14, 2018

deads2k May 14, 2018

deads2k May 14, 2018

juanvallejo commented May 14, 2018

deads2k commented May 14, 2018

deads2k commented May 14, 2018

deads2k commented May 14, 2018

soltysh left a comment

k8s-ci-robot commented May 15, 2018

juanvallejo commented May 15, 2018

k8s-github-robot commented May 15, 2018

k8s-github-robot commented May 15, 2018

move cached_discovery to client-go/discovery #63550

move cached_discovery to client-go/discovery #63550

Conversation

juanvallejo commented May 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented May 8, 2018

deads2k commented May 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juanvallejo commented May 14, 2018

deads2k commented May 14, 2018

deads2k commented May 14, 2018

deads2k commented May 14, 2018

soltysh left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented May 15, 2018

juanvallejo commented May 15, 2018

k8s-github-robot commented May 15, 2018

k8s-github-robot commented May 15, 2018