diff --git a/.golangci.yml b/.golangci.yml new file mode 100644 index 0000000..97e595f --- /dev/null +++ b/.golangci.yml @@ -0,0 +1,40 @@ +version: "2" +run: + timeout: 5m + allow-parallel-runners: true +linters: + settings: + staticcheck: + dot-import-whitelist: # allow dot imports for the following packages + - github.com/onsi/gomega + - github.com/onsi/ginkgo/v2 + default: none + enable: + - copyloopvar + - dupl + - errcheck + - goconst + - gocyclo + - govet + - ineffassign + - lll + - misspell + - nakedret + - prealloc + - staticcheck + - unconvert + - unparam + - unused + exclusions: + rules: + - path: "api/*" + linters: + - lll + - path: "internal/controller/pie/*" + linters: + - dupl + - lll +formatters: + enable: + - gofmt + - goimports diff --git a/Makefile b/Makefile index 58f41a3..ff98521 100644 --- a/Makefile +++ b/Makefile @@ -68,6 +68,14 @@ vet: ## Run go vet against code. test: manifests generate fmt vet ## Run tests. PIE_ENVTEST_VERSION=$(ENVTEST_K8S_VERSION) PIE_ENVTEST_ASSETS_DIR=$(LOCALBIN) go test ./... -race -coverprofile cover.out +.PHONY: lint +lint: golangci-lint ## Run golangci-lint. + $(GOLANGCI_LINT) run + +.PHONY: lint-fix +lint-fix: golangci-lint ## Run golangci-lint and perform fixes. + $(GOLANGCI_LINT) run --fix + ##@ Build .PHONY: build @@ -152,6 +160,7 @@ $(LOCALBIN): KUBECTL ?= kubectl KUSTOMIZE ?= $(LOCALBIN)/kustomize CONTROLLER_GEN ?= $(LOCALBIN)/controller-gen +GOLANGCI_LINT ?= $(LOCALBIN)/golangci-lint .PHONY: kustomize kustomize: $(KUSTOMIZE) ## Download kustomize locally if necessary. If wrong version is installed, it will be removed before downloading. @@ -167,3 +176,9 @@ controller-gen: $(CONTROLLER_GEN) ## Download controller-gen locally if necessar $(CONTROLLER_GEN): $(LOCALBIN) test -s $(LOCALBIN)/controller-gen && $(LOCALBIN)/controller-gen --version | grep -q $(CONTROLLER_TOOLS_VERSION) || \ GOBIN=$(LOCALBIN) go install sigs.k8s.io/controller-tools/cmd/controller-gen@$(CONTROLLER_TOOLS_VERSION) + +.PHONY: golangci-lint +golangci-lint: $(GOLANGCI_LINT) ## Download golangci-lint locally if necessary. +$(GOLANGCI_LINT): $(LOCALBIN) + test -s $(GOLANGCI_LINT) && $(GOLANGCI_LINT) version | grep -q $(GOLANGCI_LINT_VERSION) || \ + GOBIN=$(LOCALBIN) go install github.com/golangci/golangci-lint/v2/cmd/golangci-lint@$(GOLANGCI_LINT_VERSION) diff --git a/cmd/controller.go b/cmd/controller.go index 2774a51..ada38a4 100644 --- a/cmd/controller.go +++ b/cmd/controller.go @@ -51,7 +51,12 @@ var ( func init() { flags := controllerCmd.Flags() flags.StringVar(&metricsAddr, "metrics-bind-address", ":8080", "The address the metric endpoint binds to.") - flags.StringVar(&healthProbeAddr, "health-probe-bind-address", ":8081", "The address the health probe endpoint binds to.") + flags.StringVar( + &healthProbeAddr, + "health-probe-bind-address", + ":8081", + "The address the health probe endpoint binds to.", + ) flags.BoolVar(&enableLeaderElection, "leader-elect", true, "Enable leader election for controller manager. "+ "Enabling this will ensure there is only one active controller manager.") @@ -177,7 +182,9 @@ func makeReceiveRunner(exporter metrics.MetricsExporter) manager.Runnable { go func() { <-ctx.Done() - s.Close() + if err := s.Close(); err != nil && !errors.Is(err, http.ErrServerClosed) { + setupLog.Error(err, "failed to close receiver server") + } }() return s.ListenAndServe() diff --git a/cmd/probe.go b/cmd/probe.go index 25282b4..e7ee952 100644 --- a/cmd/probe.go +++ b/cmd/probe.go @@ -45,7 +45,12 @@ var provisionProbeCmd = &cobra.Command{ func init() { fs := probeCmd.Flags() - fs.StringVar(&probeConfig.controllerAddr, "destination-address", "http://localhost:8080", "metrics aggregator's address") + fs.StringVar( + &probeConfig.controllerAddr, + "destination-address", + "http://localhost:8080", + "metrics aggregator's address", + ) fs.StringVar(&probeConfig.storageClass, "storage-class", "", "target StorageClass name") fs.StringVar(&probeConfig.fioFilename, "path", "/test", "target I/O test directory path") fs.StringVar(&probeConfig.nodeName, "node-name", "", "node name") diff --git a/internal/controller/pie/pieprobe_controller.go b/internal/controller/pie/pieprobe_controller.go index d378aae..9ab1ede 100644 --- a/internal/controller/pie/pieprobe_controller.go +++ b/internal/controller/pie/pieprobe_controller.go @@ -114,7 +114,10 @@ func (r *PieProbeReconciler) reconcileMountProbes(ctx context.Context, pieProbe return err } allNodeList := corev1.NodeList{} - r.client.List(ctx, &allNodeList) + err = r.client.List(ctx, &allNodeList) + if err != nil { + return err + } availableNodeList := []corev1.Node{} for _, node := range allNodeList.Items { if !nodeSelector.Match(&node) { @@ -258,13 +261,16 @@ func (r *PieProbeReconciler) SetupWithManager(mgr ctrl.Manager) error { Complete(r) } -func getPVCName(nodeName string, pieProbe *piev1alpha1.PieProbe) string { +func getPVCName(nodeName string, pieProbe *piev1alpha1.PieProbe) (string, error) { pieProbeName := pieProbe.Name storageClass := pieProbe.Spec.MonitoringStorageClass - sha1 := sha1.New() - io.WriteString(sha1, pieProbeName+"\000"+nodeName+"\000"+storageClass) - hashedName := hex.EncodeToString(sha1.Sum(nil)) + sha1Hash := sha1.New() + _, err := io.WriteString(sha1Hash, pieProbeName+"\000"+nodeName+"\000"+storageClass) + if err != nil { + return "", fmt.Errorf("failed to hash pvc name: %w", err) + } + hashedName := hex.EncodeToString(sha1Hash.Sum(nil)) if len(pieProbeName) > 10 { pieProbeName = pieProbeName[:10] @@ -275,7 +281,7 @@ func getPVCName(nodeName string, pieProbe *piev1alpha1.PieProbe) string { if len(storageClass) > 14 { storageClass = storageClass[:14] } - return fmt.Sprintf("%s-%s-%s-%s-%s", constants.PVCNamePrefix, pieProbeName, nodeName, storageClass, hashedName[:6]) + return fmt.Sprintf("%s-%s-%s-%s-%s", constants.PVCNamePrefix, pieProbeName, nodeName, storageClass, hashedName[:6]), nil } func (r *PieProbeReconciler) createOrUpdatePVC( @@ -285,7 +291,10 @@ func (r *PieProbeReconciler) createOrUpdatePVC( ) error { logger := log.FromContext(ctx) - pvcName := getPVCName(nodeName, pieProbe) + pvcName, err := getPVCName(nodeName, pieProbe) + if err != nil { + return err + } storageClass := pieProbe.Spec.MonitoringStorageClass pvc := &corev1.PersistentVolumeClaim{} @@ -308,7 +317,9 @@ func (r *PieProbeReconciler) createOrUpdatePVC( } pvc.Spec.Resources.Requests[corev1.ResourceStorage] = *pieProbe.Spec.PVCCapacity - ctrl.SetControllerReference(pieProbe, pvc, r.client.Scheme()) + if err := ctrl.SetControllerReference(pieProbe, pvc, r.client.Scheme()); err != nil { + return err + } return nil }) @@ -354,14 +365,18 @@ func (r *PieProbeReconciler) createOrUpdateJob( nodeName *string, ) error { _ = log.FromContext(ctx) + cronJobName, err := getCronJobName(kind, nodeName, pieProbe) + if err != nil { + return err + } cronjob := &batchv1.CronJob{} cronjob.SetNamespace(pieProbe.GetNamespace()) - cronjob.SetName(getCronJobName(kind, nodeName, pieProbe)) + cronjob.SetName(cronJobName) storageClass := pieProbe.Spec.MonitoringStorageClass - _, err := ctrl.CreateOrUpdate(ctx, r.client, cronjob, func() error { + _, err = ctrl.CreateOrUpdate(ctx, r.client, cronjob, func() error { label := map[string]string{ constants.ProbeStorageClassLabelKey: storageClass, constants.ProbePieProbeLabelKey: pieProbe.GetName(), @@ -470,25 +485,31 @@ func (r *PieProbeReconciler) createOrUpdateJob( }, }, } + pvcName, err := getPVCName(*nodeName, pieProbe) + if err != nil { + return err + } cronjob.Spec.JobTemplate.Spec.Template.Spec.Volumes = []corev1.Volume{ { Name: volumeName, VolumeSource: corev1.VolumeSource{ PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{ - ClaimName: getPVCName(*nodeName, pieProbe), + ClaimName: pvcName, }, }, }, } } - ctrl.SetControllerReference(pieProbe, cronjob, r.client.Scheme()) + if err := ctrl.SetControllerReference(pieProbe, cronjob, r.client.Scheme()); err != nil { + return err + } return nil }) if err != nil { - return fmt.Errorf("failed to create CronJob: %s", getCronJobName(kind, nodeName, pieProbe)) + return fmt.Errorf("failed to create CronJob: %s", cronJobName) } return nil @@ -501,7 +522,7 @@ func (r *PieProbeReconciler) createOrUpdateJob( // However, if the node and StorageClass names are too long, the CronJob name will not fit in 52 characters. // So we cut off the node and StorageClass names to an appropriate length and added a hash value at the end // to balance readability and uniqueness. -func getCronJobName(kind int, nodeNamePtr *string, pieProbe *piev1alpha1.PieProbe) string { +func getCronJobName(kind int, nodeNamePtr *string, pieProbe *piev1alpha1.PieProbe) (string, error) { nodeName := "" if nodeNamePtr != nil { nodeName = *nodeNamePtr @@ -510,9 +531,12 @@ func getCronJobName(kind int, nodeNamePtr *string, pieProbe *piev1alpha1.PieProb pieProbeName := pieProbe.Name storageClass := pieProbe.Spec.MonitoringStorageClass - sha1 := sha1.New() - io.WriteString(sha1, pieProbeName+"\000"+nodeName+"\000"+storageClass) - hashedName := hex.EncodeToString(sha1.Sum(nil)) + sha1Hash := sha1.New() + _, err := io.WriteString(sha1Hash, pieProbeName+"\000"+nodeName+"\000"+storageClass) + if err != nil { + return "", fmt.Errorf("failed to hash cronjob name: %w", err) + } + hashedName := hex.EncodeToString(sha1Hash.Sum(nil)) if len(pieProbeName) > 10 { pieProbeName = pieProbeName[:10] @@ -525,9 +549,9 @@ func getCronJobName(kind int, nodeNamePtr *string, pieProbe *piev1alpha1.PieProb } if kind == ProvisionProbe { - return fmt.Sprintf("%s-%s-%s-%s", constants.ProvisionProbeNamePrefix, pieProbeName, storageClass, hashedName[:6]) + return fmt.Sprintf("%s-%s-%s-%s", constants.ProvisionProbeNamePrefix, pieProbeName, storageClass, hashedName[:6]), nil } else { // kind == MountProbe - return fmt.Sprintf("%s-%s-%s-%s-%s", constants.MountProbeNamePrefix, pieProbeName, nodeName, storageClass, hashedName[:6]) + return fmt.Sprintf("%s-%s-%s-%s-%s", constants.MountProbeNamePrefix, pieProbeName, nodeName, storageClass, hashedName[:6]), nil } } diff --git a/internal/controller/pie/pieprobe_controller_test.go b/internal/controller/pie/pieprobe_controller_test.go index 317f8b0..5829e90 100644 --- a/internal/controller/pie/pieprobe_controller_test.go +++ b/internal/controller/pie/pieprobe_controller_test.go @@ -2,6 +2,7 @@ package pie import ( "context" + "fmt" "os" "strings" "time" @@ -98,23 +99,23 @@ func prepareObjects(ctx context.Context) error { func deletePieProbeAndReferencingResources(ctx context.Context, pieProbe *piev1alpha1.PieProbe) error { if err := k8sClient.Delete(ctx, pieProbe); err != nil { - return err + return fmt.Errorf("failed to delete PieProbe %s: %w", pieProbe.Name, err) } var pvcList corev1.PersistentVolumeClaimList if err := k8sClient.List(ctx, &pvcList, client.MatchingLabels(map[string]string{ "storage-class": pieProbe.Spec.MonitoringStorageClass, })); err != nil { - return err + return fmt.Errorf("failed to list PVCs: %w", err) } for _, pvc := range pvcList.Items { - pvc.ObjectMeta.Finalizers = []string{} + pvc.Finalizers = []string{} if err := k8sClient.Update(ctx, &pvc); err != nil { - return err + return fmt.Errorf("failed to update PVC %s: %w", pvc.Name, err) } - if err := k8sClient.Delete(ctx, &pvc); err != nil { - return err + if err := k8sClient.Delete(ctx, &pvc); err != nil && !apierrors.IsNotFound(err) { + return fmt.Errorf("failed to delete PVC %s: %w", pvc.Name, err) } } @@ -122,12 +123,12 @@ func deletePieProbeAndReferencingResources(ctx context.Context, pieProbe *piev1a if err := k8sClient.List(ctx, &cronjobList, client.MatchingLabels(map[string]string{ "storage-class": pieProbe.Spec.MonitoringStorageClass, })); err != nil { - return err + return fmt.Errorf("failed to list CronJobs: %w", err) } for _, cronjob := range cronjobList.Items { - if err := k8sClient.Delete(ctx, &cronjob); err != nil { - return err + if err := k8sClient.Delete(ctx, &cronjob); err != nil && !apierrors.IsNotFound(err) { + return fmt.Errorf("failed to delete CronJob %s: %w", cronjob.Name, err) } } @@ -210,7 +211,10 @@ var _ = Describe("PieProbe controller for specifying resources", func() { } _, err := ctrl.CreateOrUpdate(ctx, k8sClient, pieProbe, func() error { return nil }) Expect(err).NotTo(HaveOccurred()) - defer deletePieProbeAndReferencingResources(ctx, pieProbe) + defer func() { + err := deletePieProbeAndReferencingResources(ctx, pieProbe) + Expect(err).NotTo(HaveOccurred()) + }() By("checking the CronJob's resource specified") Eventually(func(g Gomega) { diff --git a/internal/controller/probe_pod_controller.go b/internal/controller/probe_pod_controller.go index ac2610e..9567003 100644 --- a/internal/controller/probe_pod_controller.go +++ b/internal/controller/probe_pod_controller.go @@ -105,7 +105,9 @@ func (r *ProbePodReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c // SetupWithManager sets up the controller with the Manager. func (r *ProbePodReconciler) SetupWithManager(mgr ctrl.Manager) error { - mgr.Add(r.po) + if err := mgr.Add(r.po); err != nil { + return err + } return ctrl.NewControllerManagedBy(mgr). For(&corev1.Pod{}). Complete(r) diff --git a/internal/controller/provision_observer.go b/internal/controller/provision_observer.go index 0ff1842..5526bc0 100644 --- a/internal/controller/provision_observer.go +++ b/internal/controller/provision_observer.go @@ -98,7 +98,10 @@ func (p *provisionObserver) deleteEventTime(namespace, podName string) { delete(p.podPieProbeName, namespacePod{namespace, podName}) } -func (p *provisionObserver) getNodeNameAndStorageClass(ctx context.Context, namespace, podName string) (string, string, error) { +func (p *provisionObserver) getNodeNameAndStorageClass( + ctx context.Context, + namespace, podName string, +) (string, string, error) { var pod corev1.Pod err := p.client.Get(ctx, client.ObjectKey{Namespace: namespace, Name: podName}, &pod) if err != nil { @@ -109,7 +112,9 @@ func (p *provisionObserver) getNodeNameAndStorageClass(ctx context.Context, name } func isProbeJob2(o metav1.OwnerReference) bool { - return o.Kind == "Job" && (strings.HasPrefix(o.Name, constants.MountProbeNamePrefix) || strings.HasPrefix(o.Name, constants.ProvisionProbeNamePrefix)) + return o.Kind == "Job" && + (strings.HasPrefix(o.Name, constants.MountProbeNamePrefix) || + strings.HasPrefix(o.Name, constants.ProvisionProbeNamePrefix)) } func (p *provisionObserver) deleteOwnerJobOfPod(ctx context.Context, namespace, podName string) error { diff --git a/metrics/metrics.go b/metrics/metrics.go index bf19d72..9699e80 100644 --- a/metrics/metrics.go +++ b/metrics/metrics.go @@ -1,6 +1,8 @@ package metrics import ( + "strconv" + "github.com/prometheus/client_golang/prometheus" "sigs.k8s.io/controller-runtime/pkg/metrics" ) @@ -12,9 +14,9 @@ const ( type MetricsExporter interface { SetLatencyOnMountProbe(pieProbeName, node, storageClass string, readLatency, writeLatency float64) - IncrementPerformanceOnMountProbeCount(pieProbeName string, node string, storageClass string, succeed bool) + IncrementPerformanceOnMountProbeCount(pieProbeName, node, storageClass string, succeed bool) IncrementProvisionProbeCount(pieProbeName string, storageClass string, onTime bool) - IncrementMountProbeCount(pieProbeName string, node string, storageClass string, onTime bool) + IncrementMountProbeCount(pieProbeName, node, storageClass string, onTime bool) } type metricExporterImpl struct { @@ -83,16 +85,19 @@ func (m *metricExporterImpl) registerMetrics() { metrics.Registry.MustRegister(m.mountProbeCount) } -func (m *metricExporterImpl) SetLatencyOnMountProbe(pieProbeName string, node string, storageClass string, readLatency, writeLatency float64) { +func (m *metricExporterImpl) SetLatencyOnMountProbe( + pieProbeName, node, storageClass string, + readLatency, writeLatency float64, +) { m.writeLatencyOnMountProbeGauge.WithLabelValues(pieProbeName, node, storageClass).Set(writeLatency) m.readLatencyOnMountProbeGauge.WithLabelValues(pieProbeName, node, storageClass).Set(readLatency) } -func (m *metricExporterImpl) IncrementPerformanceOnMountProbeCount(pieProbeName string, node string, storageClass string, succeed bool) { - succeedStr := "false" - if succeed { - succeedStr = "true" - } +func (m *metricExporterImpl) IncrementPerformanceOnMountProbeCount( + pieProbeName, node, storageClass string, + succeed bool, +) { + succeedStr := strconv.FormatBool(succeed) m.performanceOnMountProbeCount.WithLabelValues(pieProbeName, node, storageClass, succeedStr).Inc() } @@ -104,10 +109,10 @@ func (m *metricExporterImpl) IncrementProvisionProbeCount(pieProbeName string, s m.provisionProbeCount.WithLabelValues(pieProbeName, storageClass, onTimeStr).Inc() } -func (m *metricExporterImpl) IncrementMountProbeCount(pieProbeName string, node string, storageClass string, onTime bool) { - onTimeStr := "false" - if onTime { - onTimeStr = "true" - } +func (m *metricExporterImpl) IncrementMountProbeCount( + pieProbeName, node, storageClass string, + onTime bool, +) { + onTimeStr := strconv.FormatBool(onTime) m.mountProbeCount.WithLabelValues(pieProbeName, node, storageClass, onTimeStr).Inc() } diff --git a/metrics/receiver.go b/metrics/receiver.go index 29e4890..684ff67 100644 --- a/metrics/receiver.go +++ b/metrics/receiver.go @@ -2,8 +2,8 @@ package metrics import ( "encoding/json" - "fmt" "io" + "log/slog" "net/http" "github.com/topolvm/pie/types" @@ -28,14 +28,27 @@ func (rh *receiver) ServeHTTP(w http.ResponseWriter, r *http.Request) { } if receivedData.PieProbeName == "" { - http.Error(w, err.Error(), http.StatusInternalServerError) + http.Error(w, "PieProbeName is empty", http.StatusBadRequest) return } - rh.metrics.SetLatencyOnMountProbe(receivedData.PieProbeName, receivedData.Node, receivedData.StorageClass, receivedData.ReadLatency, receivedData.WriteLatency) - rh.metrics.IncrementPerformanceOnMountProbeCount(receivedData.PieProbeName, receivedData.Node, receivedData.StorageClass, receivedData.PerformanceProbeSucceed) - - fmt.Fprintf(w, "OK") + rh.metrics.SetLatencyOnMountProbe( + receivedData.PieProbeName, + receivedData.Node, + receivedData.StorageClass, + receivedData.ReadLatency, + receivedData.WriteLatency, + ) + rh.metrics.IncrementPerformanceOnMountProbeCount( + receivedData.PieProbeName, + receivedData.Node, + receivedData.StorageClass, + receivedData.PerformanceProbeSucceed, + ) + + if _, err := w.Write([]byte("OK")); err != nil { + slog.Error("failed to write data", "error", err) + } } func NewReceiver(m MetricsExporter) http.Handler { diff --git a/test/e2e/suite_test.go b/test/e2e/suite_test.go index 1324f61..3028970 100644 --- a/test/e2e/suite_test.go +++ b/test/e2e/suite_test.go @@ -6,7 +6,6 @@ import ( _ "embed" "errors" "fmt" - "math/rand" "net" "net/http" "os" @@ -69,8 +68,6 @@ func TestMtest(t *testing.T) { t.Fatal("No TEST_NAMESPACE specified.") } - rand.Seed(time.Now().UnixNano()) - RegisterFailHandler(Fail) SetDefaultEventuallyPollingInterval(time.Second) @@ -153,7 +150,9 @@ var _ = Describe("PieProbe resource", func() { Eventually(func(g Gomega) { resp, err := http.Get("http://localhost:8080/metrics") g.Expect(err).NotTo(HaveOccurred()) - defer resp.Body.Close() + defer func() { + g.Expect(resp.Body.Close()).To(Succeed()) + }() parser := expfmt.NewTextParser(model.UTF8Validation) metricFamilies, err := parser.TextToMetricFamilies(resp.Body) @@ -253,7 +252,7 @@ func portForward(ctx context.Context, wg *sync.WaitGroup, ns, target, port strin var conn net.Conn conn, err = net.Dial("tcp", "localhost:"+localPort) if err == nil { - conn.Close() + _ = conn.Close() return nil } time.Sleep(5 * time.Second) diff --git a/versions.mk b/versions.mk index e50dd65..80ddda4 100644 --- a/versions.mk +++ b/versions.mk @@ -2,6 +2,8 @@ CHART_TESTING_VERSION := 3.14.0 # https://github.com/kubernetes-sigs/controller-tools/releases CONTROLLER_TOOLS_VERSION := v0.20.1 +# https://github.com/golangci/golangci-lint/releases +GOLANGCI_LINT_VERSION := v2.11.4 # https://github.com/helm/helm/releases HELM_VERSION := 4.1.4 # https://github.com/kubernetes-sigs/kind/releases