From f084311326098da387a1e70b5f697d417c40239f Mon Sep 17 00:00:00 2001 From: David Eads Date: Wed, 21 Feb 2018 11:01:09 -0500 Subject: [PATCH] remove metrics client factory method --- pkg/kubectl/cmd/testing/BUILD | 1 - pkg/kubectl/cmd/testing/fake.go | 10 -- pkg/kubectl/cmd/top.go | 4 +- pkg/kubectl/cmd/top_node.go | 7 +- pkg/kubectl/cmd/top_node_test.go | 48 +++++- pkg/kubectl/cmd/top_pod.go | 10 +- pkg/kubectl/cmd/top_pod_test.go | 162 ++++++++++-------- pkg/kubectl/cmd/util/BUILD | 1 - pkg/kubectl/cmd/util/clientcache.go | 29 ---- pkg/kubectl/cmd/util/factory.go | 4 - pkg/kubectl/cmd/util/factory_client_access.go | 5 - 11 files changed, 145 insertions(+), 136 deletions(-) diff --git a/pkg/kubectl/cmd/testing/BUILD b/pkg/kubectl/cmd/testing/BUILD index 1734ca2024a..68152e62d8b 100644 --- a/pkg/kubectl/cmd/testing/BUILD +++ b/pkg/kubectl/cmd/testing/BUILD @@ -40,7 +40,6 @@ go_library( "//vendor/k8s.io/client-go/kubernetes:go_default_library", "//vendor/k8s.io/client-go/rest:go_default_library", "//vendor/k8s.io/client-go/rest/fake:go_default_library", - "//vendor/k8s.io/metrics/pkg/client/clientset_generated/clientset:go_default_library", ], ) diff --git a/pkg/kubectl/cmd/testing/fake.go b/pkg/kubectl/cmd/testing/fake.go index e93e4de00ab..5775351ecaa 100644 --- a/pkg/kubectl/cmd/testing/fake.go +++ b/pkg/kubectl/cmd/testing/fake.go @@ -49,7 +49,6 @@ import ( "k8s.io/kubernetes/pkg/kubectl/scheme" "k8s.io/kubernetes/pkg/kubectl/validation" "k8s.io/kubernetes/pkg/printers" - metricsclientset "k8s.io/metrics/pkg/client/clientset_generated/clientset" ) // +k8s:deepcopy-gen=true @@ -245,7 +244,6 @@ type TestFactory struct { Command string TmpDir string CategoryExpander categories.CategoryExpander - MetricsClientSet metricsclientset.Interface ClientForMappingFunc func(mapping *meta.RESTMapping) (resource.RESTClient, error) UnstructuredClientForMappingFunc func(mapping *meta.RESTMapping) (resource.RESTClient, error) @@ -299,10 +297,6 @@ func (f *FakeFactory) KubernetesClientSet() (*kubernetes.Clientset, error) { return nil, nil } -func (f *FakeFactory) MetricsClientSet() (metricsclientset.Interface, error) { - return f.tf.MetricsClientSet, f.tf.Err -} - func (f *FakeFactory) ClientSet() (internalclientset.Interface, error) { return nil, nil } @@ -555,10 +549,6 @@ func (f *fakeAPIFactory) KubernetesClientSet() (*kubernetes.Clientset, error) { return clientset, f.tf.Err } -func (f *fakeAPIFactory) MetricsClientSet() (metricsclientset.Interface, error) { - return f.tf.MetricsClientSet, f.tf.Err -} - func (f *fakeAPIFactory) ClientSet() (internalclientset.Interface, error) { // Swap the HTTP client out of the REST client with the fake // version. diff --git a/pkg/kubectl/cmd/top.go b/pkg/kubectl/cmd/top.go index bc58e2b86ed..451e5bab7d4 100644 --- a/pkg/kubectl/cmd/top.go +++ b/pkg/kubectl/cmd/top.go @@ -52,8 +52,10 @@ func NewCmdTop(f cmdutil.Factory, out, errOut io.Writer) *cobra.Command { } // create subcommands + topPod, _ := NewCmdTopPod(f, nil, out) cmd.AddCommand(NewCmdTopNode(f, nil, out)) - cmd.AddCommand(NewCmdTopPod(f, nil, out)) + cmd.AddCommand(topPod) + return cmd } diff --git a/pkg/kubectl/cmd/top_node.go b/pkg/kubectl/cmd/top_node.go index f4eac86442c..293f81a6bdb 100644 --- a/pkg/kubectl/cmd/top_node.go +++ b/pkg/kubectl/cmd/top_node.go @@ -131,7 +131,12 @@ func (o *TopNodeOptions) Complete(f cmdutil.Factory, cmd *cobra.Command, args [] } o.DiscoveryClient = clientset.DiscoveryClient - o.MetricsClient, err = f.MetricsClientSet() + + config, err := f.ClientConfig() + if err != nil { + return err + } + o.MetricsClient, err = metricsclientset.NewForConfig(config) if err != nil { return err } diff --git a/pkg/kubectl/cmd/top_node_test.go b/pkg/kubectl/cmd/top_node_test.go index a0d4f55b596..fdb0d40c340 100644 --- a/pkg/kubectl/cmd/top_node_test.go +++ b/pkg/kubectl/cmd/top_node_test.go @@ -303,13 +303,25 @@ func TestTopNodeAllMetricsFromMetricsServer(t *testing.T) { fakemetricsClientset.AddReactor("list", "nodes", func(action core.Action) (handled bool, ret runtime.Object, err error) { return true, expectedMetrics, nil }) - tf.MetricsClientSet = fakemetricsClientset tf.Namespace = "test" tf.ClientConfig = defaultClientConfig() buf := bytes.NewBuffer([]byte{}) cmd := NewCmdTopNode(f, nil, buf) - cmd.Run(cmd, []string{}) + + // TODO in the long run, we want to test most of our commands like this. Wire the options struct with specific mocks + // TODO then check the particular Run functionality and harvest results from fake clients + cmdOptions := &TopNodeOptions{} + if err := cmdOptions.Complete(f, cmd, []string{}, buf); err != nil { + t.Fatal(err) + } + cmdOptions.MetricsClient = fakemetricsClientset + if err := cmdOptions.Validate(); err != nil { + t.Fatal(err) + } + if err := cmdOptions.RunTopNode(); err != nil { + t.Fatal(err) + } // Check the presence of node names in the output. result := buf.String() @@ -355,13 +367,25 @@ func TestTopNodeWithNameMetricsFromMetricsServer(t *testing.T) { fakemetricsClientset.AddReactor("get", "nodes", func(action core.Action) (handled bool, ret runtime.Object, err error) { return true, &expectedMetrics, nil }) - tf.MetricsClientSet = fakemetricsClientset tf.Namespace = "test" tf.ClientConfig = defaultClientConfig() buf := bytes.NewBuffer([]byte{}) cmd := NewCmdTopNode(f, nil, buf) - cmd.Run(cmd, []string{expectedMetrics.Name}) + + // TODO in the long run, we want to test most of our commands like this. Wire the options struct with specific mocks + // TODO then check the particular Run functionality and harvest results from fake clients + cmdOptions := &TopNodeOptions{} + if err := cmdOptions.Complete(f, cmd, []string{expectedMetrics.Name}, buf); err != nil { + t.Fatal(err) + } + cmdOptions.MetricsClient = fakemetricsClientset + if err := cmdOptions.Validate(); err != nil { + t.Fatal(err) + } + if err := cmdOptions.RunTopNode(); err != nil { + t.Fatal(err) + } // Check the presence of node names in the output. result := buf.String() @@ -418,14 +442,26 @@ func TestTopNodeWithLabelSelectorMetricsFromMetricsServer(t *testing.T) { fakemetricsClientset.AddReactor("list", "nodes", func(action core.Action) (handled bool, ret runtime.Object, err error) { return true, expectedMetrics, nil }) - tf.MetricsClientSet = fakemetricsClientset tf.Namespace = "test" tf.ClientConfig = defaultClientConfig() buf := bytes.NewBuffer([]byte{}) cmd := NewCmdTopNode(f, nil, buf) cmd.Flags().Set("selector", label) - cmd.Run(cmd, []string{}) + + // TODO in the long run, we want to test most of our commands like this. Wire the options struct with specific mocks + // TODO then check the particular Run functionality and harvest results from fake clients + cmdOptions := &TopNodeOptions{} + if err := cmdOptions.Complete(f, cmd, []string{}, buf); err != nil { + t.Fatal(err) + } + cmdOptions.MetricsClient = fakemetricsClientset + if err := cmdOptions.Validate(); err != nil { + t.Fatal(err) + } + if err := cmdOptions.RunTopNode(); err != nil { + t.Fatal(err) + } // Check the presence of node names in the output. result := buf.String() diff --git a/pkg/kubectl/cmd/top_pod.go b/pkg/kubectl/cmd/top_pod.go index f358dcdb648..007dc1484b0 100644 --- a/pkg/kubectl/cmd/top_pod.go +++ b/pkg/kubectl/cmd/top_pod.go @@ -78,7 +78,7 @@ var ( kubectl top pod -l name=myLabel`)) ) -func NewCmdTopPod(f cmdutil.Factory, options *TopPodOptions, out io.Writer) *cobra.Command { +func NewCmdTopPod(f cmdutil.Factory, options *TopPodOptions, out io.Writer) (*cobra.Command, *TopPodOptions) { if options == nil { options = &TopPodOptions{} } @@ -106,7 +106,7 @@ func NewCmdTopPod(f cmdutil.Factory, options *TopPodOptions, out io.Writer) *cob cmd.Flags().BoolVar(&options.PrintContainers, "containers", false, "If present, print usage of containers within a pod.") cmd.Flags().BoolVar(&options.AllNamespaces, "all-namespaces", false, "If present, list the requested object(s) across all namespaces. Namespace in current context is ignored even if specified with --namespace.") options.HeapsterOptions.Bind(cmd.Flags()) - return cmd + return cmd, options } func (o *TopPodOptions) Complete(f cmdutil.Factory, cmd *cobra.Command, args []string, out io.Writer) error { @@ -127,7 +127,11 @@ func (o *TopPodOptions) Complete(f cmdutil.Factory, cmd *cobra.Command, args []s } o.DiscoveryClient = clientset.DiscoveryClient - o.MetricsClient, err = f.MetricsClientSet() + config, err := f.ClientConfig() + if err != nil { + return err + } + o.MetricsClient, err = metricsclientset.NewForConfig(config) if err != nil { return err } diff --git a/pkg/kubectl/cmd/top_pod_test.go b/pkg/kubectl/cmd/top_pod_test.go index 0c15354338e..e23371ae595 100644 --- a/pkg/kubectl/cmd/top_pod_test.go +++ b/pkg/kubectl/cmd/top_pod_test.go @@ -190,7 +190,7 @@ func TestTopPod(t *testing.T) { tf.ClientConfig = defaultClientConfig() buf := bytes.NewBuffer([]byte{}) - cmd := NewCmdTopPod(f, nil, buf) + cmd, _ := NewCmdTopPod(f, nil, buf) for name, value := range testCase.flags { cmd.Flags().Set(name, value) } @@ -270,91 +270,103 @@ func TestTopPodWithMetricsServer(t *testing.T) { } initTestErrorHandler(t) for _, testCase := range testCases { - t.Logf("Running test case: %s", testCase.name) - metricsList := testV1beta1PodMetricsData() - var expectedMetrics []metricsv1beta1api.PodMetrics - var expectedContainerNames, nonExpectedMetricsNames []string - for n, m := range metricsList { - if n < len(testCase.namespaces) { - m.Namespace = testCase.namespaces[n] - expectedMetrics = append(expectedMetrics, m) - for _, c := range m.Containers { - expectedContainerNames = append(expectedContainerNames, c.Name) + t.Run(testCase.name, func(t *testing.T) { + metricsList := testV1beta1PodMetricsData() + var expectedMetrics []metricsv1beta1api.PodMetrics + var expectedContainerNames, nonExpectedMetricsNames []string + for n, m := range metricsList { + if n < len(testCase.namespaces) { + m.Namespace = testCase.namespaces[n] + expectedMetrics = append(expectedMetrics, m) + for _, c := range m.Containers { + expectedContainerNames = append(expectedContainerNames, c.Name) + } + } else { + nonExpectedMetricsNames = append(nonExpectedMetricsNames, m.Name) } + } + + fakemetricsClientset := &metricsfake.Clientset{} + + if len(expectedMetrics) == 1 { + fakemetricsClientset.AddReactor("get", "pods", func(action core.Action) (handled bool, ret runtime.Object, err error) { + return true, &expectedMetrics[0], nil + }) } else { - nonExpectedMetricsNames = append(nonExpectedMetricsNames, m.Name) + fakemetricsClientset.AddReactor("list", "pods", func(action core.Action) (handled bool, ret runtime.Object, err error) { + res := &metricsv1beta1api.PodMetricsList{ + ListMeta: metav1.ListMeta{ + ResourceVersion: "2", + }, + Items: expectedMetrics, + } + return true, res, nil + }) } - } - fakemetricsClientset := &metricsfake.Clientset{} + f, tf := cmdtesting.NewAPIFactory() + ns := legacyscheme.Codecs - if len(expectedMetrics) == 1 { - fakemetricsClientset.AddReactor("get", "pods", func(action core.Action) (handled bool, ret runtime.Object, err error) { - return true, &expectedMetrics[0], nil - }) - } else { - fakemetricsClientset.AddReactor("list", "pods", func(action core.Action) (handled bool, ret runtime.Object, err error) { - res := &metricsv1beta1api.PodMetricsList{ - ListMeta: metav1.ListMeta{ - ResourceVersion: "2", - }, - Items: expectedMetrics, - } - return true, res, nil - }) - } + tf.Client = &fake.RESTClient{ + NegotiatedSerializer: ns, + Client: fake.CreateHTTPClient(func(req *http.Request) (*http.Response, error) { + switch p := req.URL.Path; { + case p == "/api": + return &http.Response{StatusCode: 200, Header: defaultHeader(), Body: ioutil.NopCloser(bytes.NewReader([]byte(apibody)))}, nil + case p == "/apis": + return &http.Response{StatusCode: 200, Header: defaultHeader(), Body: ioutil.NopCloser(bytes.NewReader([]byte(apisbodyWithMetrics)))}, nil + default: + t.Fatalf("%s: unexpected request: %#v\nGot URL: %#v", + testCase.name, req, req.URL) + return nil, nil + } + }), + } + tf.Namespace = testNS + tf.ClientConfig = defaultClientConfig() + buf := bytes.NewBuffer([]byte{}) - f, tf := cmdtesting.NewAPIFactory() - ns := legacyscheme.Codecs + cmd, cmdOptions := NewCmdTopPod(f, nil, buf) + for name, value := range testCase.flags { + cmd.Flags().Set(name, value) + } - tf.Client = &fake.RESTClient{ - NegotiatedSerializer: ns, - Client: fake.CreateHTTPClient(func(req *http.Request) (*http.Response, error) { - switch p := req.URL.Path; { - case p == "/api": - return &http.Response{StatusCode: 200, Header: defaultHeader(), Body: ioutil.NopCloser(bytes.NewReader([]byte(apibody)))}, nil - case p == "/apis": - return &http.Response{StatusCode: 200, Header: defaultHeader(), Body: ioutil.NopCloser(bytes.NewReader([]byte(apisbodyWithMetrics)))}, nil - default: - t.Fatalf("%s: unexpected request: %#v\nGot URL: %#v", - testCase.name, req, req.URL) - return nil, nil - } - }), - } - tf.MetricsClientSet = fakemetricsClientset - tf.Namespace = testNS - tf.ClientConfig = defaultClientConfig() - buf := bytes.NewBuffer([]byte{}) + // TODO in the long run, we want to test most of our commands like this. Wire the options struct with specific mocks + // TODO then check the particular Run functionality and harvest results from fake clients. We probably end up skipping the factory altogether. + if err := cmdOptions.Complete(f, cmd, testCase.args, buf); err != nil { + t.Fatal(err) + } + cmdOptions.MetricsClient = fakemetricsClientset + if err := cmdOptions.Validate(); err != nil { + t.Fatal(err) + } + if err := cmdOptions.RunTopPod(); err != nil { + t.Fatal(err) + } - cmd := NewCmdTopPod(f, nil, buf) - for name, value := range testCase.flags { - cmd.Flags().Set(name, value) - } - cmd.Run(cmd, testCase.args) - - // Check the presence of pod names&namespaces/container names in the output. - result := buf.String() - if testCase.containers { - for _, containerName := range expectedContainerNames { - if !strings.Contains(result, containerName) { - t.Errorf("%s: missing metrics for container %s: \n%s", testCase.name, containerName, result) + // Check the presence of pod names&namespaces/container names in the output. + result := buf.String() + if testCase.containers { + for _, containerName := range expectedContainerNames { + if !strings.Contains(result, containerName) { + t.Errorf("missing metrics for container %s: \n%s", containerName, result) + } } } - } - for _, m := range expectedMetrics { - if !strings.Contains(result, m.Name) { - t.Errorf("%s: missing metrics for %s: \n%s", testCase.name, m.Name, result) + for _, m := range expectedMetrics { + if !strings.Contains(result, m.Name) { + t.Errorf("missing metrics for %s: \n%s", m.Name, result) + } + if testCase.listsNamespaces && !strings.Contains(result, m.Namespace) { + t.Errorf("missing metrics for %s/%s: \n%s", m.Namespace, m.Name, result) + } } - if testCase.listsNamespaces && !strings.Contains(result, m.Namespace) { - t.Errorf("%s: missing metrics for %s/%s: \n%s", testCase.name, m.Namespace, m.Name, result) + for _, name := range nonExpectedMetricsNames { + if strings.Contains(result, name) { + t.Errorf("unexpected metrics for %s: \n%s", name, result) + } } - } - for _, name := range nonExpectedMetricsNames { - if strings.Contains(result, name) { - t.Errorf("%s: unexpected metrics for %s: \n%s", testCase.name, name, result) - } - } + }) } } @@ -520,7 +532,7 @@ func TestTopPodCustomDefaults(t *testing.T) { }, DiscoveryClient: &fakeDiscovery{}, } - cmd := NewCmdTopPod(f, opts, buf) + cmd, _ := NewCmdTopPod(f, opts, buf) for name, value := range testCase.flags { cmd.Flags().Set(name, value) } diff --git a/pkg/kubectl/cmd/util/BUILD b/pkg/kubectl/cmd/util/BUILD index c62a1199796..d031d2d2ebe 100644 --- a/pkg/kubectl/cmd/util/BUILD +++ b/pkg/kubectl/cmd/util/BUILD @@ -81,7 +81,6 @@ go_library( "//vendor/k8s.io/client-go/scale:go_default_library", "//vendor/k8s.io/client-go/tools/clientcmd:go_default_library", "//vendor/k8s.io/client-go/util/homedir:go_default_library", - "//vendor/k8s.io/metrics/pkg/client/clientset_generated/clientset:go_default_library", "//vendor/k8s.io/utils/exec:go_default_library", ], ) diff --git a/pkg/kubectl/cmd/util/clientcache.go b/pkg/kubectl/cmd/util/clientcache.go index 3aca7400772..124216d2e8f 100644 --- a/pkg/kubectl/cmd/util/clientcache.go +++ b/pkg/kubectl/cmd/util/clientcache.go @@ -27,7 +27,6 @@ import ( "k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset" oldclient "k8s.io/kubernetes/pkg/client/unversioned" "k8s.io/kubernetes/pkg/version" - metricsclientset "k8s.io/metrics/pkg/client/clientset_generated/clientset" ) func NewClientCache(loader clientcmd.ClientConfig, discoveryClientFactory DiscoveryClientFactory) *ClientCache { @@ -59,7 +58,6 @@ type ClientCache struct { discoveryClient discovery.DiscoveryInterface kubernetesClientCache kubernetesClientCache - metricsClientCache metricsClientCache } // kubernetesClientCache creates a new kubernetes.Clientset one time @@ -73,17 +71,6 @@ type kubernetesClientCache struct { err error } -// metricsClientCache creates a new metricsclientset.Clientset one time -// and then returns the result for all future requests -type metricsClientCache struct { - // once makes sure the client is only initialized once - once sync.Once - // client is the cached client value - client *metricsclientset.Clientset - // err is the cached error value - err error -} - // KubernetesClientSetForVersion returns a new kubernetes.Clientset. It will cache the value // the first time it is called and return the cached value on subsequent calls. // If an error is encountered the first time KubernetesClientSetForVersion is called, @@ -100,22 +87,6 @@ func (c *ClientCache) KubernetesClientSetForVersion(requiredVersion *schema.Grou return c.kubernetesClientCache.client, c.kubernetesClientCache.err } -// MetricsClientSetForVersion returns a new kubernetes.Clientset. It will cache the value -// the first time it is called and return the cached value on subsequent calls. -// If an error is encountered the first time MetircsClientSetForVersion is called, -// the error will be cached. -func (c *ClientCache) MetricsClientSetForVersion(requiredVersion *schema.GroupVersion) (*metricsclientset.Clientset, error) { - c.metricsClientCache.once.Do(func() { - config, err := c.ClientConfigForVersion(requiredVersion) - if err != nil { - c.kubernetesClientCache.err = err - return - } - c.metricsClientCache.client, c.metricsClientCache.err = metricsclientset.NewForConfig(config) - }) - return c.metricsClientCache.client, c.metricsClientCache.err -} - // also looks up the discovery client. We can't do this during init because the flags won't have been set // because this is constructed pre-command execution before the command tree is // even set up. Requires the lock to already be acquired diff --git a/pkg/kubectl/cmd/util/factory.go b/pkg/kubectl/cmd/util/factory.go index 0471b63ad33..824ea52cce7 100644 --- a/pkg/kubectl/cmd/util/factory.go +++ b/pkg/kubectl/cmd/util/factory.go @@ -47,7 +47,6 @@ import ( "k8s.io/kubernetes/pkg/kubectl/resource" "k8s.io/kubernetes/pkg/kubectl/validation" "k8s.io/kubernetes/pkg/printers" - metricsclientset "k8s.io/metrics/pkg/client/clientset_generated/clientset" ) const ( @@ -95,9 +94,6 @@ type ClientAccessFactory interface { // KubernetesClientSet gives you back an external clientset KubernetesClientSet() (*kubernetes.Clientset, error) - // MetricsClientSet gives you back an external clientset for the metrics API - MetricsClientSet() (metricsclientset.Interface, error) - // Returns a RESTClient for accessing Kubernetes resources or an error. RESTClient() (*restclient.RESTClient, error) // Returns a client.Config for accessing the Kubernetes server. diff --git a/pkg/kubectl/cmd/util/factory_client_access.go b/pkg/kubectl/cmd/util/factory_client_access.go index 2c2558ec930..4c0aee65664 100644 --- a/pkg/kubectl/cmd/util/factory_client_access.go +++ b/pkg/kubectl/cmd/util/factory_client_access.go @@ -31,7 +31,6 @@ import ( "time" "k8s.io/api/core/v1" - metricsclientset "k8s.io/metrics/pkg/client/clientset_generated/clientset" "github.com/spf13/cobra" "github.com/spf13/pflag" @@ -200,10 +199,6 @@ func (f *ring0Factory) KubernetesClientSet() (*kubernetes.Clientset, error) { return f.clientCache.KubernetesClientSetForVersion(nil) } -func (f *ring0Factory) MetricsClientSet() (metricsclientset.Interface, error) { - return f.clientCache.MetricsClientSetForVersion(nil) -} - func (f *ring0Factory) ClientSet() (internalclientset.Interface, error) { return f.clientCache.ClientSetForVersion(nil) }