Merge pull request #60208 from soltysh/remove_factory_metricsclient_method

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",
     ],
 )
 

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
 }
@@ -548,10 +542,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.

pkg/kubectl/cmd/top.go

@@ -54,6 +54,7 @@ func NewCmdTop(f cmdutil.Factory, out, errOut io.Writer) *cobra.Command {
 	// create subcommands
 	cmd.AddCommand(NewCmdTopNode(f, nil, out))
 	cmd.AddCommand(NewCmdTopPod(f, nil, out))
+
 	return cmd
 }
 

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
 	}

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()

pkg/kubectl/cmd/top_pod.go

@@ -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
 	}

pkg/kubectl/cmd/top_pod_test.go

@@ -226,7 +226,7 @@ func TestTopPodWithMetricsServer(t *testing.T) {
 	testCases := []struct {
 		name            string
 		namespace       string
-		flags           map[string]string
+		options         *TopPodOptions
 		args            []string
 		expectedPath    string
 		expectedQuery   string
@@ -236,7 +236,7 @@ func TestTopPodWithMetricsServer(t *testing.T) {
 	}{
 		{
 			name:            "all namespaces",
-			flags:           map[string]string{"all-namespaces": "true"},
+			options:         &TopPodOptions{AllNamespaces: true},
 			expectedPath:    topMetricsAPIPathPrefix + "/pods",
 			namespaces:      []string{testNS, "secondtestns", "thirdtestns"},
 			listsNamespaces: true,
@@ -254,14 +254,14 @@ func TestTopPodWithMetricsServer(t *testing.T) {
 		},
 		{
 			name:          "pod with label selector",
-			flags:         map[string]string{"selector": "key=value"},
+			options:       &TopPodOptions{Selector: "key=value"},
 			expectedPath:  topMetricsAPIPathPrefix + "/namespaces/" + testNS + "/pods",
 			expectedQuery: "labelSelector=" + url.QueryEscape("key=value"),
 			namespaces:    []string{testNS, testNS},
 		},
 		{
 			name:         "pod with container metrics",
-			flags:        map[string]string{"containers": "true"},
+			options:      &TopPodOptions{PrintContainers: true},
 			args:         []string{"pod1"},
 			expectedPath: topMetricsAPIPathPrefix + "/namespaces/" + testNS + "/pods/pod1",
 			namespaces:   []string{testNS},
@@ -270,91 +270,106 @@ 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)
 				}
-			} 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 {
-			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.MetricsClientSet = fakemetricsClientset
-		tf.Namespace = testNS
-		tf.ClientConfig = defaultClientConfig()
-		buf := bytes.NewBuffer([]byte{})
+			f, tf := cmdtesting.NewAPIFactory()
+			ns := legacyscheme.Codecs
+
+			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{})
+
+			cmd := NewCmdTopPod(f, nil, buf)
+			var cmdOptions *TopPodOptions
+			if testCase.options != nil {
+				cmdOptions = testCase.options
+			} else {
+				cmdOptions = &TopPodOptions{}
+			}
 
-		cmd := NewCmdTopPod(f, nil, buf)
-		for name, value := range testCase.flags {
-			cmd.Flags().Set(name, value)
-		}
-		cmd.Run(cmd, testCase.args)
+			// 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)
+			}
 
-		// 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)
-			}
-			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 _, 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)
+				}
 			}
-		}
-		for _, name := range nonExpectedMetricsNames {
-			if strings.Contains(result, name) {
-				t.Errorf("%s: unexpected metrics for %s: \n%s", testCase.name, name, result)
+			for _, name := range nonExpectedMetricsNames {
+				if strings.Contains(result, name) {
+					t.Errorf("unexpected metrics for %s: \n%s", name, result)
+				}
 			}
-		}
+		})
 	}
 }
 

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",
     ],
 )

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

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.

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)
 }