feat: Add errcheck linter with golangci-lint

Signed-off-by: Kush Trivedi <[email protected]>

.golangci.yml

@@ -1,11 +1,23 @@
 run:
   timeout: 5m
 
+linters-settings:
+  golint:
+    min-confidence: 0
+  misspell:
+    locale: US
+
 linters:
   disable-all: true
   enable:
     - gofmt
     - golint
+    - errcheck
     # TODO(oxddr): enable more checkers
     # - deadcode
-    # - errcheck
+    # - misspell
+
+service:
+  golangci-lint-version: 1.23.x # use the fixed version to not introduce new linters unexpectedly
+  prepare:
+    - echo "here I can run custom commands, but no preparation needed for this repo"

benchmark/pkg/comparer/comparer.go

@@ -41,6 +41,6 @@ func CompareJobsUsingScheme(jobComparisonData *util.JobComparisonData, scheme st
 		schemes.CompareJobsUsingKSTest(jobComparisonData, matchThreshold, minMetricAvgForCompare)
 		return nil
 	default:
-		return fmt.Errorf("Unknown comparison scheme '%v'", scheme)
+		return fmt.Errorf("unknown comparison scheme '%v'", scheme)
 	}
 }

benchmark/pkg/metricsfetcher/runselector/selector.go

@@ -37,6 +37,6 @@ func GetJobRunsUsingScheme(job string, scheme string, nValue int, utils util.Job
 	case LastNHours:
 		return schemes.GetJobRunsFromLastNHours(job, nValue, utils)
 	default:
-		return nil, fmt.Errorf("Unknown run selection scheme '%v'", scheme)
+		return nil, fmt.Errorf("unknown run selection scheme '%v'", scheme)
 	}
 }

benchmark/pkg/metricsfetcher/util/mock_util.go

@@ -35,7 +35,7 @@ type MockJobLogUtils struct {
 func (utils MockJobLogUtils) GetLatestBuildNumberForJob(job string) (int, error) {
 	length := len(utils.MockBuildNumbers)
 	if length == 0 {
-		return 0, fmt.Errorf("Array of mock build numbers is empty")
+		return 0, fmt.Errorf("array of mock build numbers is empty")
 	}
 	return utils.MockBuildNumbers[length-1], nil
 }
@@ -49,7 +49,7 @@ func (utils MockJobLogUtils) GetBuildNumbersForJob(job string) ([]int, error) {
 func (utils MockJobLogUtils) GetJobRunStartTimestamp(job string, run int) (uint64, error) {
 	value, ok := utils.MockStartTimestamps[run]
 	if !ok {
-		return 0, fmt.Errorf("Run number %v not a key in the mock start timestamps map", run)
+		return 0, fmt.Errorf("run number %v not a key in the mock start timestamps map", run)
 	}
 	return value, nil
 }
@@ -58,7 +58,7 @@ func (utils MockJobLogUtils) GetJobRunStartTimestamp(job string, run int) (uint6
 func (utils MockJobLogUtils) GetJobRunFinishedStatus(job string, run int) (bool, error) {
 	value, ok := utils.MockFinishedStatuses[run]
 	if !ok {
-		return false, fmt.Errorf("Run number %v not a key in the mock finished statuses map", run)
+		return false, fmt.Errorf("run number %v not a key in the mock finished statuses map", run)
 	}
 	return value, nil
 }
@@ -67,11 +67,11 @@ func (utils MockJobLogUtils) GetJobRunFinishedStatus(job string, run int) (bool,
 func (utils MockJobLogUtils) GetJobRunFileContents(job string, run int, filepath string) ([]byte, error) {
 	files, ok := utils.MockFileContents[run]
 	if !ok {
-		return nil, fmt.Errorf("Run number %v not a 1st key in the mock file contents map", run)
+		return nil, fmt.Errorf("run number %v not a 1st key in the mock file contents map", run)
 	}
 	file, ok := files[filepath]
 	if !ok {
-		return nil, fmt.Errorf("Filepath %v not a 2nd key in the mock file contents map for 1st key %v", filepath, run)
+		return nil, fmt.Errorf("filepath %v not a 2nd key in the mock file contents map for 1st key %v", filepath, run)
 	}
 	return file, nil
 }
@@ -80,7 +80,7 @@ func (utils MockJobLogUtils) GetJobRunFileContents(job string, run int, filepath
 func (utils MockJobLogUtils) ListJobRunFilesWithPrefix(job string, run int, prefix string) ([]string, error) {
 	filesWithPrefixes, ok := utils.MockFilesWithPrefix[run]
 	if !ok {
-		return nil, fmt.Errorf("Run number %v not a 1st key in the mock files with prefix map", run)
+		return nil, fmt.Errorf("run number %v not a 1st key in the mock files with prefix map", run)
 	}
 	filesWithPrefix, ok := filesWithPrefixes[prefix]
 	return filesWithPrefix, nil

benchmark/pkg/metricsfetcher/util/util.go

@@ -81,7 +81,7 @@ func (utils GCSLogUtils) GetJobRunFinishedStatus(job string, run int) (bool, err
 func (utils GCSLogUtils) GetJobRunFileContents(job string, run int, filepath string) ([]byte, error) {
 	response, err := utils.googleGCSBucketUtils.GetFileFromJenkinsGoogleBucket(job, run, filepath)
 	if err != nil {
-		return nil, fmt.Errorf("Couldn't read file from GCS: %v", err)
+		return nil, fmt.Errorf("couldn't read file from GCS: %v", err)
 	}
 	defer response.Body.Close()
 	return ioutil.ReadAll(response.Body)
@@ -98,6 +98,6 @@ func GetJobLogUtilsForMode(mode string) (JobLogUtils, error) {
 	case GCS:
 		return NewGCSLogUtils(), nil
 	default:
-		return nil, fmt.Errorf("Unknown source mode '%v'", mode)
+		return nil, fmt.Errorf("unknown source mode '%v'", mode)
 	}
 }

clusterloader2/cmd/clusterloader.go

@@ -70,9 +70,17 @@ func initClusterFlags() {
 	flags.StringEnvVar(&clusterLoaderConfig.ClusterConfig.EtcdKeyPath, "etcd-key", "ETCD_KEY", "/etc/srv/kubernetes/pki/etcd-apiserver-server.key", "Path to the etcd key on the master machine")
 	flags.IntEnvVar(&clusterLoaderConfig.ClusterConfig.EtcdInsecurePort, "etcd-insecure-port", "ETCD_INSECURE_PORT", 2382, "Inscure http port")
 	flags.BoolEnvVar(&clusterLoaderConfig.ClusterConfig.DeleteStaleNamespaces, "delete-stale-namespaces", "DELETE_STALE_NAMESPACES", false, "DEPRECATED: Whether to delete all stale namespaces before the test execution.")
-	flags.MarkDeprecated("delete-stale-namespaces", "specify deleteStaleNamespaces in testconfig file instead.")
+	err := flags.MarkDeprecated("delete-stale-namespaces", "specify deleteStaleNamespaces in testconfig file instead.")
+	if err != nil {
+		klog.Fatal("unable to mark flag deprecated %v", err)
+		return
+	}
 	flags.BoolEnvVar(&clusterLoaderConfig.ClusterConfig.DeleteAutomanagedNamespaces, "delete-automanaged-namespaces", "DELETE_AUTOMANAGED_NAMESPACES", true, "DEPRECATED: Whether to delete all automanaged namespaces after the test execution.")
-	flags.MarkDeprecated("delete-automanaged-namespaces", "specify deleteAutomanagedNamespaces in testconfig file instead.")
+	err = flags.MarkDeprecated("delete-automanaged-namespaces", "specify deleteAutomanagedNamespaces in testconfig file instead.")
+	if err != nil {
+		klog.Fatal("unable to mark flag deprecated %v", err)
+		return
+	}
 	flags.StringEnvVar(&clusterLoaderConfig.ClusterConfig.MasterName, "mastername", "MASTER_NAME", "", "Name of the masternode")
 	// TODO(#595): Change the name of the MASTER_IP and MASTER_INTERNAL_IP flags and vars to plural
 	flags.StringSliceEnvVar(&clusterLoaderConfig.ClusterConfig.MasterIPs, "masterip", "MASTER_IP", nil /*defaultValue*/, "Hostname/IP of the master node, supports multiple values when separated by commas")

clusterloader2/pkg/config/template_provider.go

@@ -225,7 +225,10 @@ func GetMapping(clusterLoaderConfig *ClusterLoaderConfig) (map[string]interface{
 	if err != nil {
 		return nil, errors.NewErrorList(goerrors.Errorf("mapping creation error: %v", err))
 	}
-	MergeMappings(mapping, envMapping)
+	err = MergeMappings(mapping, envMapping)
+	if err != nil {
+		return nil, errors.NewErrorList(goerrors.Errorf("mapping merging error: %v", err))
+	}
 	return mapping, nil
 }
 

clusterloader2/pkg/measurement/common/service_creation_latency.go

@@ -50,7 +50,10 @@ const (
 )
 
 func init() {
-	measurement.Register(serviceCreationLatencyName, createServiceCreationLatencyMeasurement)
+	if err := measurement.Register(serviceCreationLatencyName, createServiceCreationLatencyMeasurement); err != nil {
+		klog.Errorf("cant register service %v", err)
+		return
+	}
 }
 
 func createServiceCreationLatencyMeasurement() measurement.Measurement {

clusterloader2/pkg/measurement/common/slos/api_responsiveness_prometheus_test.go

@@ -46,8 +46,16 @@ var (
 func turnOffLoggingToStderrInKlog() {
 	if klogLogToStderr {
 		klog.InitFlags(nil)
-		flag.Set("logtostderr", "false")
-		flag.Set("v", "2")
+		err := flag.Set("logtostderr", "false")
+		if err != nil {
+			klog.Errorf("Unable to set flag %v", err)
+			return
+		}
+		err = flag.Set("v", "2")
+		if err != nil {
+			klog.Errorf("Unable to set flag %v", err)
+			return
+		}
 		flag.Parse()
 		klogLogToStderr = false
 	}
@@ -565,7 +573,11 @@ func TestLogging(t *testing.T) {
 			gatherer := &apiResponsivenessGatherer{}
 			config := &measurement.Config{}
 
-			gatherer.Gather(executor, time.Now(), config)
+			_, err := gatherer.Gather(executor, time.Now(), config)
+			if err != nil {
+				klog.Errorf("cant gather config %v", err)
+				return
+			}
 			klog.Flush()
 
 			for _, msg := range tc.expectedMessages {

clusterloader2/pkg/measurement/common/slos/pod_startup_latency.go

@@ -45,7 +45,10 @@ const (
 )
 
 func init() {
-	measurement.Register(podStartupLatencyMeasurementName, createPodStartupLatencyMeasurement)
+	if err := measurement.Register(podStartupLatencyMeasurementName, createPodStartupLatencyMeasurement); err != nil {
+		klog.Errorf("cant register service %v", err)
+		return
+	}
 }
 
 func createPodStartupLatencyMeasurement() measurement.Measurement {

clusterloader2/pkg/prometheus/prometheus.go

@@ -299,7 +299,7 @@ func (pc *Controller) runNodeExporter() error {
 			g.Go(func() error {
 				f, err := os.Open(os.ExpandEnv(pc.clusterLoaderConfig.PrometheusConfig.NodeExporterPod))
 				if err != nil {
-					return fmt.Errorf("Unable to open manifest file: %v", err)
+					return fmt.Errorf("unable to open manifest file: %v", err)
 				}
 				defer f.Close()
 				return pc.ssh.Exec("sudo tee /etc/kubernetes/manifests/node-exporter.yaml > /dev/null", &node, f)

clusterloader2/pkg/provider/util.go

@@ -45,7 +45,7 @@ func runSSHCommand(cmd, host string) (string, string, int, error) {
 		return "", "", 0, err
 	}
 	if signer == nil {
-		return "", "", 0, fmt.Errorf("Cannot find sshkey")
+		return "", "", 0, fmt.Errorf("cannot find sshkey")
 	}
 
 	user := defaultSSHUser()
@@ -89,7 +89,7 @@ func sshSignerFromKeyFile(KeyPathEnv string, defaultKeyPath string) (ssh.Signer,
 	}
 
 	if keyfile == "" {
-		return nil, fmt.Errorf("Cannot find ssh key file")
+		return nil, fmt.Errorf("cannot find ssh key file")
 	}
 
 	return sshutil.MakePrivateKeySignerFromFile(keyfile)

clusterloader2/pkg/tuningset/global_qps_load.go

@@ -22,6 +22,7 @@ import (
 
 	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/perf-tests/clusterloader2/api"
+	"k8s.io/klog"
 
 	"golang.org/x/time/rate"
 )
@@ -59,7 +60,11 @@ func newGlobalQPSLoad(params *api.GlobalQPSLoad) *globalQPSLoad {
 func (ql *globalQPSLoad) Execute(actions []func()) {
 	var wg wait.Group
 	for i := range actions {
-		ql.limiter.Wait(context.TODO())
+		err := ql.limiter.Wait(context.TODO())
+		if err != nil {
+			klog.Errorf("Error while waiting for rate limitter: %v - skipping the action", err)
+            continue
+		}
 		wg.Start(actions[i])
 	}
 	wg.Wait()

network/benchmarks/netperf/launch.go

@@ -136,16 +136,22 @@ func cleanup(c *kubernetes.Clientset) {
 	}
 	for _, svc := range svcs.Items {
 		fmt.Println("Deleting svc", svc.GetName())
-		c.Core().Services(testNamespace).Delete(
+		err := c.Core().Services(testNamespace).Delete(
 			svc.GetName(), &metav1.DeleteOptions{})
+		if err != nil {
+			fmt.Println("Failed to get services", err)
+		}
 	}
 }
 
 // createServices: Long-winded function to programmatically create our two services
 func createServices(c *kubernetes.Clientset) bool {
 	// Create our namespace if not present
 	if _, err := c.Core().Namespaces().Get(testNamespace, metav1.GetOptions{}); err != nil {
-		c.Core().Namespaces().Create(&api.Namespace{ObjectMeta: metav1.ObjectMeta{Name: testNamespace}})
+		_, err := c.Core().Namespaces().Create(&api.Namespace{ObjectMeta: metav1.ObjectMeta{Name: testNamespace}})
+		if err != nil {
+			fmt.Println("Failed to create services", err)
+		}
 	}
 
 	// Create the orchestrator service that points to the coordinator pod
@@ -344,14 +350,23 @@ func processCsvData(csvData *string) bool {
 	outputFilePrefix := fmt.Sprintf("%s-%s_%s.", testNamespace, tag, t.Format("20060102150405"))
 	fmt.Printf("Test concluded - CSV raw data written to %s/%scsv\n", outputFileDirectory, outputFilePrefix)
 	if _, err := os.Stat(outputFileDirectory); os.IsNotExist(err) {
-		os.Mkdir(outputFileDirectory, 0766)
+		err := os.Mkdir(outputFileDirectory, 0766)
+		if err != nil {
+			fmt.Println("Error creating directory %v", err)
+			return false
+		}
+
 	}
 	fd, err := os.OpenFile(fmt.Sprintf("%s/%scsv", outputFileDirectory, outputFilePrefix), os.O_RDWR|os.O_CREATE, 0666)
 	if err != nil {
 		fmt.Println("ERROR writing output CSV datafile", err)
 		return false
 	}
-	fd.WriteString(*csvData)
+	_, err = fd.WriteString(*csvData)
+	if err != nil {
+		fmt.Println("Error writing string %v", err)
+		return false
+	}
 	fd.Close()
 	return true
 }

network/benchmarks/netperf/nptest/nptest.go

@@ -509,13 +509,21 @@ func (t *NetPerfRPC) ReceiveOutput(data *WorkerOutput, reply *int) error {
 
 func serveRPCRequests(port string) {
 	baseObject := new(NetPerfRPC)
-	rpc.Register(baseObject)
+	err := rpc.Register(baseObject)
+	if err != nil {
+		log.Fatal("failed to register rpc", err)
+		return
+	}
 	rpc.HandleHTTP()
 	listener, e := net.Listen("tcp", ":"+port)
 	if e != nil {
 		log.Fatal("listen error:", e)
 	}
-	http.Serve(listener, nil)
+	err = http.Serve(listener, nil)
+	if err != nil {
+		log.Fatal("failed to start server", err)
+		return
+	}
 }
 
 // Blocking RPC server start - only runs on the orchestrator
@@ -555,11 +563,19 @@ func handleClientWorkItem(client *rpc.Client, workItem *WorkItem) {
 	case workItem.ClientItem.Type == iperfTCPTest || workItem.ClientItem.Type == iperfUDPTest || workItem.ClientItem.Type == iperfSctpTest:
 		outputString := iperfClient(workItem.ClientItem.Host, workItem.ClientItem.Port, workItem.ClientItem.MSS, workItem.ClientItem.Type)
 		var reply int
-		client.Call("NetPerfRPC.ReceiveOutput", WorkerOutput{Output: outputString, Worker: worker, Type: workItem.ClientItem.Type}, &reply)
+		err := client.Call("NetPerfRPC.ReceiveOutput", WorkerOutput{Output: outputString, Worker: worker, Type: workItem.ClientItem.Type}, &reply)
+		if err != nil {
+			log.Fatal("failed to call client", err)
+			return
+		}
 	case workItem.ClientItem.Type == netperfTest:
 		outputString := netperfClient(workItem.ClientItem.Host, workItem.ClientItem.Port, workItem.ClientItem.Type)
 		var reply int
-		client.Call("NetPerfRPC.ReceiveOutput", WorkerOutput{Output: outputString, Worker: worker, Type: workItem.ClientItem.Type}, &reply)
+		err := client.Call("NetPerfRPC.ReceiveOutput", WorkerOutput{Output: outputString, Worker: worker, Type: workItem.ClientItem.Type}, &reply)
+		if err != nil {
+			log.Fatal("failed to call client", err)
+			return
+		}
 	}
 	// Client COOLDOWN period before asking for next work item to replenish burst allowance policers etc
 	time.Sleep(10 * time.Second)

perfdash/metrics-downloader-helper.go

@@ -48,12 +48,18 @@ func serveHTTPObject(res http.ResponseWriter, req *http.Request, obj interface{}
 	if err != nil {
 		res.Header().Set("Content-type", "text/html")
 		res.WriteHeader(http.StatusInternalServerError)
-		res.Write([]byte(fmt.Sprintf("<h3>Internal Error</h3><p>%v", err)))
+		_, err = res.Write([]byte(fmt.Sprintf("<h3>Internal Error</h3><p>%v", err)))
+		if err != nil {
+			klog.Errorf("unable to write error %v", err)
+		}
 		return
 	}
 	res.Header().Set("Content-type", "application/json")
 	res.WriteHeader(http.StatusOK)
-	res.Write(data)
+	_, err = res.Write(data)
+	if err != nil {
+		klog.Errorf("unable to write response data %v", err)
+	}
 }
 
 func getURLParam(req *http.Request, name string) (string, bool) {
@@ -80,7 +86,7 @@ func (j *JobToCategoryData) ServeJobNames(res http.ResponseWriter, req *http.Req
 func (j *JobToCategoryData) ServeCategoryNames(res http.ResponseWriter, req *http.Request) {
 	jobname, ok := getURLParam(req, "jobname")
 	if !ok {
-		klog.Warningf("Url Param 'jobname' is missing")
+		klog.Warningf("url Param 'jobname' is missing")
 		return
 	}
 

perfdash/metrics-downloader.go

@@ -82,7 +82,7 @@ func (g *Downloader) getData() (JobToCategoryData, error) {
 	wg.Add(len(newJobs))
 	for job, tests := range newJobs {
 		if tests.Prefix == "" {
-			return nil, fmt.Errorf("Invalid empty Prefix for job %s", job)
+			return nil, fmt.Errorf("invalid empty Prefix for job %s", job)
 		}
 		go g.getJobData(&wg, result, &resultLock, job, tests)
 	}
@@ -124,7 +124,7 @@ func (g *Downloader) getJobData(wg *sync.WaitGroup, result JobToCategoryData, re
 					searchPrefix := fmt.Sprintf("artifacts/%v", filePrefix)
 					artifacts, err := g.MetricsBkt.ListFilesInBuild(job, buildNumber, searchPrefix)
 					if err != nil || len(artifacts) == 0 {
-						klog.Errorf("Error while looking for %s* in %s build %v: %v", searchPrefix, job, buildNumber, err)
+						klog.Infof("Error while looking for %s* in %s build %v: %v", searchPrefix, job, buildNumber, err)
 						continue
 					}
 					for _, artifact := range artifacts {
@@ -133,7 +133,7 @@ func (g *Downloader) getJobData(wg *sync.WaitGroup, result JobToCategoryData, re
 						testDataResponse, err := g.MetricsBkt.ReadFile(job, buildNumber,
 							fmt.Sprintf("artifacts/%v", metricsFileName))
 						if err != nil {
-							klog.Errorf("Error when reading response Body: %v", err)
+							klog.Infof("Error when reading response Body: %v", err)
 							continue
 						}
 						buildData := getBuildData(result, tests.Prefix, resultCategory, testLabel, job, resultLock)