From 7accddcaf41f40ad03fdd4221e521178f4d28b8f Mon Sep 17 00:00:00 2001 From: Sargun Dhillon Date: Tue, 3 Sep 2019 10:10:35 -0700 Subject: [PATCH 1/5] Fix linting errors in node/podcontroller.go --- node/podcontroller.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/node/podcontroller.go b/node/podcontroller.go index ca788cb0f..97675b6e8 100644 --- a/node/podcontroller.go +++ b/node/podcontroller.go @@ -262,20 +262,20 @@ func (pc *PodController) Ready() <-chan struct{} { } // runWorker is a long-running function that will continually call the processNextWorkItem function in order to read and process an item on the work queue. -func (pc *PodController) runWorker(ctx context.Context, workerId string, q workqueue.RateLimitingInterface) { - for pc.processNextWorkItem(ctx, workerId, q) { +func (pc *PodController) runWorker(ctx context.Context, workerID string, q workqueue.RateLimitingInterface) { + for pc.processNextWorkItem(ctx, workerID, q) { } } // processNextWorkItem will read a single work item off the work queue and attempt to process it,by calling the syncHandler. -func (pc *PodController) processNextWorkItem(ctx context.Context, workerId string, q workqueue.RateLimitingInterface) bool { +func (pc *PodController) processNextWorkItem(ctx context.Context, workerID string, q workqueue.RateLimitingInterface) bool { // We create a span only after popping from the queue so that we can get an adequate picture of how long it took to process the item. ctx, span := trace.StartSpan(ctx, "processNextWorkItem") defer span.End() // Add the ID of the current worker as an attribute to the current span. - ctx = span.WithField(ctx, "workerId", workerId) + ctx = span.WithField(ctx, "workerId", workerID) return handleQueueItem(ctx, q, pc.syncHandler) } From 9cce8640a5c34353a02cae1725960574f871b9e1 Mon Sep 17 00:00:00 2001 From: Sargun Dhillon Date: Tue, 3 Sep 2019 10:13:19 -0700 Subject: [PATCH 2/5] Fix linting errors in node/pod_test.go This moves away from defining pods independently. It moves pod (spec) generation to an independent function. --- node/pod_test.go | 159 +++++++++-------------------------------------- 1 file changed, 30 insertions(+), 129 deletions(-) diff --git a/node/pod_test.go b/node/pod_test.go index 6ab084cae..7fecc0826 100644 --- a/node/pod_test.go +++ b/node/pod_test.go @@ -71,80 +71,25 @@ func TestPodsEqual(t *testing.T) { }, } - p2 := &corev1.Pod{ - Spec: corev1.PodSpec{ - Containers: []corev1.Container{ - corev1.Container{ - Name: "nginx", - Image: "nginx:1.15.12-perl", - Ports: []corev1.ContainerPort{ - corev1.ContainerPort{ - ContainerPort: 443, - Protocol: "tcp", - }, - }, - }, - }, - }, - } + p2 := p1.DeepCopy() assert.Assert(t, podsEqual(p1, p2)) } func TestPodsDifferent(t *testing.T) { p1 := &corev1.Pod{ - Spec: corev1.PodSpec{ - Containers: []corev1.Container{ - corev1.Container{ - Name: "nginx", - Image: "nginx:1.15.12", - Ports: []corev1.ContainerPort{ - corev1.ContainerPort{ - ContainerPort: 443, - Protocol: "tcp", - }, - }, - }, - }, - }, + Spec: newPodSpec(), } - p2 := &corev1.Pod{ - Spec: corev1.PodSpec{ - Containers: []corev1.Container{ - corev1.Container{ - Name: "nginx", - Image: "nginx:1.15.12-perl", - Ports: []corev1.ContainerPort{ - corev1.ContainerPort{ - ContainerPort: 443, - Protocol: "tcp", - }, - }, - }, - }, - }, - } + p2 := p1.DeepCopy() + p2.Spec.Containers[0].Image = "nginx:1.15.12-perl" assert.Assert(t, !podsEqual(p1, p2)) } func TestPodsDifferentIgnoreValue(t *testing.T) { p1 := &corev1.Pod{ - Spec: corev1.PodSpec{ - Containers: []corev1.Container{ - corev1.Container{ - Name: "nginx", - Image: "nginx:1.15.12", - Ports: []corev1.ContainerPort{ - corev1.ContainerPort{ - ContainerPort: 443, - Protocol: "tcp", - }, - }, - }, - }, - }, + Spec: newPodSpec(), } p2 := p1.DeepCopy() @@ -157,22 +102,9 @@ func TestPodCreateNewPod(t *testing.T) { svr := newTestController() pod := &corev1.Pod{} - pod.ObjectMeta.Namespace = "default" - pod.ObjectMeta.Name = "nginx" - pod.Spec = corev1.PodSpec{ - Containers: []corev1.Container{ - corev1.Container{ - Name: "nginx", - Image: "nginx:1.15.12", - Ports: []corev1.ContainerPort{ - corev1.ContainerPort{ - ContainerPort: 443, - Protocol: "tcp", - }, - }, - }, - }, - } + pod.ObjectMeta.Namespace = "default" //nolint:goconst + pod.ObjectMeta.Name = "nginx" //nolint:goconst + pod.Spec = newPodSpec() err := svr.createOrUpdatePod(context.Background(), pod.DeepCopy()) @@ -188,43 +120,15 @@ func TestPodUpdateExisting(t *testing.T) { pod := &corev1.Pod{} pod.ObjectMeta.Namespace = "default" pod.ObjectMeta.Name = "nginx" - pod.Spec = corev1.PodSpec{ - Containers: []corev1.Container{ - corev1.Container{ - Name: "nginx", - Image: "nginx:1.15.12", - Ports: []corev1.ContainerPort{ - corev1.ContainerPort{ - ContainerPort: 443, - Protocol: "tcp", - }, - }, - }, - }, - } + pod.Spec = newPodSpec() err := svr.createOrUpdatePod(context.Background(), pod.DeepCopy()) assert.Check(t, is.Nil(err)) assert.Check(t, is.Equal(svr.mock.creates.read(), 1)) assert.Check(t, is.Equal(svr.mock.updates.read(), 0)) - pod2 := &corev1.Pod{} - pod2.ObjectMeta.Namespace = "default" - pod2.ObjectMeta.Name = "nginx" - pod2.Spec = corev1.PodSpec{ - Containers: []corev1.Container{ - corev1.Container{ - Name: "nginx", - Image: "nginx:1.15.12-perl", - Ports: []corev1.ContainerPort{ - corev1.ContainerPort{ - ContainerPort: 443, - Protocol: "tcp", - }, - }, - }, - }, - } + pod2 := pod.DeepCopy() + pod2.Spec.Containers[0].Image = "nginx:1.15.12-perl" err = svr.createOrUpdatePod(context.Background(), pod2.DeepCopy()) assert.Check(t, is.Nil(err)) @@ -240,20 +144,7 @@ func TestPodNoSpecChange(t *testing.T) { pod := &corev1.Pod{} pod.ObjectMeta.Namespace = "default" pod.ObjectMeta.Name = "nginx" - pod.Spec = corev1.PodSpec{ - Containers: []corev1.Container{ - corev1.Container{ - Name: "nginx", - Image: "nginx:1.15.12", - Ports: []corev1.ContainerPort{ - corev1.ContainerPort{ - ContainerPort: 443, - Protocol: "tcp", - }, - }, - }, - }, - } + pod.Spec = newPodSpec() err := svr.createOrUpdatePod(context.Background(), pod.DeepCopy()) assert.Check(t, is.Nil(err)) @@ -288,14 +179,7 @@ func TestPodDelete(t *testing.T) { pod := &corev1.Pod{} pod.ObjectMeta.Namespace = "default" pod.ObjectMeta.Name = "nginx" - pod.Spec = corev1.PodSpec{ - Containers: []corev1.Container{ - corev1.Container{ - Name: "nginx", - Image: "nginx:1.15.12", - }, - }, - } + pod.Spec = newPodSpec() pc := c.client.CoreV1().Pods("default") @@ -327,3 +211,20 @@ func TestPodDelete(t *testing.T) { }) } } + +func newPodSpec() corev1.PodSpec { + return corev1.PodSpec{ + Containers: []corev1.Container{ + corev1.Container{ + Name: "nginx", + Image: "nginx:1.15.12", + Ports: []corev1.ContainerPort{ + corev1.ContainerPort{ + ContainerPort: 443, + Protocol: "tcp", + }, + }, + }, + }, + } +} From 5949e6279d52306b08e16eee64b5e1cf777c20cb Mon Sep 17 00:00:00 2001 From: Sargun Dhillon Date: Tue, 3 Sep 2019 10:23:19 -0700 Subject: [PATCH 3/5] Miscellaneous cleanup for linting --- internal/manager/resource_test.go | 14 +++++++------- node/lifecycle_test.go | 2 +- node/mock_test.go | 2 +- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/internal/manager/resource_test.go b/internal/manager/resource_test.go index 030a1d861..c28581674 100644 --- a/internal/manager/resource_test.go +++ b/internal/manager/resource_test.go @@ -17,13 +17,13 @@ package manager_test import ( "testing" + "github.com/virtual-kubelet/virtual-kubelet/internal/manager" + testutil "github.com/virtual-kubelet/virtual-kubelet/internal/test/util" + "gotest.tools/assert" v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" corev1listers "k8s.io/client-go/listers/core/v1" "k8s.io/client-go/tools/cache" - - "github.com/virtual-kubelet/virtual-kubelet/internal/manager" - testutil "github.com/virtual-kubelet/virtual-kubelet/internal/test/util" ) // TestGetPods verifies that the resource manager acts as a passthrough to a pod lister. @@ -38,7 +38,7 @@ func TestGetPods(t *testing.T) { // Create a pod lister that will list the pods defined above. indexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{cache.NamespaceIndex: cache.MetaNamespaceIndexFunc}) for _, pod := range lsPods { - indexer.Add(pod) + assert.NilError(t, indexer.Add(pod)) } podLister := corev1listers.NewPodLister(indexer) @@ -67,7 +67,7 @@ func TestGetSecret(t *testing.T) { // Create a secret lister that will list the secrets defined above. indexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{cache.NamespaceIndex: cache.MetaNamespaceIndexFunc}) for _, secret := range lsSecrets { - indexer.Add(secret) + assert.NilError(t, indexer.Add(secret)) } secretLister := corev1listers.NewSecretLister(indexer) @@ -106,7 +106,7 @@ func TestGetConfigMap(t *testing.T) { // Create a config map lister that will list the config maps defined above. indexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{cache.NamespaceIndex: cache.MetaNamespaceIndexFunc}) for _, secret := range lsConfigMaps { - indexer.Add(secret) + assert.NilError(t, indexer.Add(secret)) } configMapLister := corev1listers.NewConfigMapLister(indexer) @@ -145,7 +145,7 @@ func TestListServices(t *testing.T) { // Create a pod lister that will list the pods defined above. indexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{cache.NamespaceIndex: cache.MetaNamespaceIndexFunc}) for _, service := range lsServices { - indexer.Add(service) + assert.NilError(t, indexer.Add(service)) } serviceLister := corev1listers.NewServiceLister(indexer) diff --git a/node/lifecycle_test.go b/node/lifecycle_test.go index 1bc07800e..c10cc10ba 100644 --- a/node/lifecycle_test.go +++ b/node/lifecycle_test.go @@ -23,11 +23,11 @@ import ( kubeinformers "k8s.io/client-go/informers" "k8s.io/client-go/kubernetes/fake" corev1client "k8s.io/client-go/kubernetes/typed/core/v1" + ktesting "k8s.io/client-go/testing" "k8s.io/client-go/tools/cache" "k8s.io/client-go/tools/record" watchutils "k8s.io/client-go/tools/watch" "k8s.io/klog" - ktesting "k8s.io/client-go/testing" ) var ( diff --git a/node/mock_test.go b/node/mock_test.go index 62ec97329..3ff7cebb7 100644 --- a/node/mock_test.go +++ b/node/mock_test.go @@ -58,7 +58,7 @@ func (w *waitableInt) until(ctx context.Context, f func(int) bool) error { func (w *waitableInt) increment() { w.cond.L.Lock() defer w.cond.L.Unlock() - w.val += 1 + w.val++ w.cond.Broadcast() } From 7133a372d62dddb034c3353eb920ed44f9c0c6ac Mon Sep 17 00:00:00 2001 From: Sargun Dhillon Date: Tue, 3 Sep 2019 10:43:37 -0700 Subject: [PATCH 4/5] Mark current linting errors as non-errors This is basically claiming linting bankruptcy. It marks all of the issues we had up until this point as nolint. --- cmd/virtual-kubelet/internal/commands/root/flag.go | 2 +- cmd/virtual-kubelet/internal/provider/mock/mock.go | 11 ++++++----- cmd/virtual-kubelet/register.go | 4 ++-- node/api/helpers.go | 2 +- node/api/pods.go | 2 +- node/node.go | 2 +- 6 files changed, 12 insertions(+), 11 deletions(-) diff --git a/cmd/virtual-kubelet/internal/commands/root/flag.go b/cmd/virtual-kubelet/internal/commands/root/flag.go index 6c567c226..6736f3925 100644 --- a/cmd/virtual-kubelet/internal/commands/root/flag.go +++ b/cmd/virtual-kubelet/internal/commands/root/flag.go @@ -69,7 +69,7 @@ func installFlags(flags *pflag.FlagSet, c *Opts) { flags.StringVar(&c.TaintKey, "taint", c.TaintKey, "Set node taint key") flags.BoolVar(&c.DisableTaint, "disable-taint", c.DisableTaint, "disable the virtual-kubelet node taint") - flags.MarkDeprecated("taint", "Taint key should now be configured using the VK_TAINT_KEY environment variable") + flags.MarkDeprecated("taint", "Taint key should now be configured using the VK_TAINT_KEY environment variable") //nolint:errcheck flags.IntVar(&c.PodSyncWorkers, "pod-sync-workers", c.PodSyncWorkers, `set the number of pod synchronization workers`) flags.BoolVar(&c.EnableNodeLease, "enable-node-lease", c.EnableNodeLease, `use node leases (1.13) for node heartbeats`) diff --git a/cmd/virtual-kubelet/internal/provider/mock/mock.go b/cmd/virtual-kubelet/internal/provider/mock/mock.go index 65e7c28b6..48b9e8322 100644 --- a/cmd/virtual-kubelet/internal/provider/mock/mock.go +++ b/cmd/virtual-kubelet/internal/provider/mock/mock.go @@ -42,7 +42,7 @@ var ( */ // MockV0Provider implements the virtual-kubelet provider interface and stores pods in memory. -type MockV0Provider struct { +type MockV0Provider struct { //nolint:golint nodeName string operatingSystem string internalIP string @@ -54,12 +54,12 @@ type MockV0Provider struct { } // MockProvider is like MockV0Provider, but implements the PodNotifier interface -type MockProvider struct { +type MockProvider struct { //nolint:golint *MockV0Provider } // MockConfig contains a mock virtual-kubelet's configurable parameters. -type MockConfig struct { +type MockConfig struct { //nolint:golint CPU string `json:"cpu,omitempty"` Memory string `json:"memory,omitempty"` Pods string `json:"pods,omitempty"` @@ -355,7 +355,7 @@ func (p *MockV0Provider) GetPods(ctx context.Context) ([]*v1.Pod, error) { } func (p *MockV0Provider) ConfigureNode(ctx context.Context, n *v1.Node) { - ctx, span := trace.StartSpan(ctx, "mock.ConfigureNode") + ctx, span := trace.StartSpan(ctx, "mock.ConfigureNode") //nolint:ineffassign defer span.End() n.Status.Capacity = p.capacity() @@ -453,7 +453,8 @@ func (p *MockV0Provider) nodeDaemonEndpoints() v1.NodeDaemonEndpoints { // GetStatsSummary returns dummy stats for all pods known by this provider. func (p *MockV0Provider) GetStatsSummary(ctx context.Context) (*stats.Summary, error) { - ctx, span := trace.StartSpan(ctx, "GetStatsSummary") + var span trace.Span + ctx, span = trace.StartSpan(ctx, "GetStatsSummary") //nolint: ineffassign defer span.End() // Grab the current timestamp so we can report it as the time the stats were generated. diff --git a/cmd/virtual-kubelet/register.go b/cmd/virtual-kubelet/register.go index 2d1eb16ec..e845e5642 100644 --- a/cmd/virtual-kubelet/register.go +++ b/cmd/virtual-kubelet/register.go @@ -6,7 +6,7 @@ import ( ) func registerMock(s *provider.Store) { - s.Register("mock", func(cfg provider.InitConfig) (provider.Provider, error) { + s.Register("mock", func(cfg provider.InitConfig) (provider.Provider, error) { //nolint:errcheck return mock.NewMockProvider( cfg.ConfigPath, cfg.NodeName, @@ -16,7 +16,7 @@ func registerMock(s *provider.Store) { ) }) - s.Register("mockV0", func(cfg provider.InitConfig) (provider.Provider, error) { + s.Register("mockV0", func(cfg provider.InitConfig) (provider.Provider, error) { //nolint:errcheck return mock.NewMockProvider( cfg.ConfigPath, cfg.NodeName, diff --git a/node/api/helpers.go b/node/api/helpers.go index 9f32d01e2..1af574d23 100644 --- a/node/api/helpers.go +++ b/node/api/helpers.go @@ -33,7 +33,7 @@ func handleError(f handlerFunc) http.HandlerFunc { code := httpStatusCode(err) w.WriteHeader(code) - io.WriteString(w, err.Error()) + io.WriteString(w, err.Error()) //nolint:errcheck logger := log.G(req.Context()).WithError(err).WithField("httpStatusCode", code) if code >= 500 { diff --git a/node/api/pods.go b/node/api/pods.go index 7d2ca65d5..071220505 100644 --- a/node/api/pods.go +++ b/node/api/pods.go @@ -28,7 +28,7 @@ type PodListerFunc func(context.Context) ([]*v1.Pod, error) func HandleRunningPods(getPods PodListerFunc) http.HandlerFunc { scheme := runtime.NewScheme() - v1.SchemeBuilder.AddToScheme(scheme) + v1.SchemeBuilder.AddToScheme(scheme) //nolint:errcheck codecs := serializer.NewCodecFactory(scheme) return handleError(func(w http.ResponseWriter, req *http.Request) error { diff --git a/node/node.go b/node/node.go index 91ed23af0..59ce9d674 100644 --- a/node/node.go +++ b/node/node.go @@ -38,7 +38,7 @@ import ( // // Note: Implementers can choose to manage a node themselves, in which case // it is not needed to provide an implementation for this interface. -type NodeProvider interface { +type NodeProvider interface { //nolint:golint // Ping checks if the node is still active. // This is intended to be lightweight as it will be called periodically as a // heartbeat to keep the node marked as ready in Kubernetes. From 3f857054615aac63c5230bd3a345525fdce8872d Mon Sep 17 00:00:00 2001 From: Sargun Dhillon Date: Tue, 3 Sep 2019 10:46:31 -0700 Subject: [PATCH 5/5] Upgrade linter, and move away from incremental linting Incremental linting doesn't seem to catch issues correctly. This runs the linters in a more standard way. --- .circleci/config.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 05c38aa31..78b13853b 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -15,10 +15,10 @@ jobs: command: V=1 CI=1 make vet - run: name: Install linters - command: curl -sfL https://install.goreleaser.com/github.com/golangci/golangci-lint.sh | bash -s v1.15.0 && mv ./bin/* /go/bin/ + command: curl -sfL https://install.goreleaser.com/github.com/golangci/golangci-lint.sh | bash -s v1.17.1 && mv ./bin/* /go/bin/ - run: name: Lint - command: golangci-lint run --new-from-rev "HEAD~$(git rev-list master.. --count)" ./... + command: golangci-lint run ./... - run: name: Dependencies command: scripts/validate/gomod.sh