From 4a8eccc503d146bf42b63f4a17a26bad64224b00 Mon Sep 17 00:00:00 2001 From: ahg-g Date: Tue, 24 Dec 2024 05:54:20 +0000 Subject: [PATCH] Fixed the rest of the lint errors and updating the linters to match what is typically used in other sub-projects --- .golangci.yml | 40 +++++++++---------- Makefile | 2 +- api/v1alpha1/inferencemodel_types.go | 2 +- ...e.networking.x-k8s.io_inferencemodels.yaml | 2 +- pkg/ext-proc/backend/datastore.go | 4 +- .../backend/endpointslice_reconciler.go | 4 +- .../backend/inferencepool_reconciler.go | 3 +- pkg/ext-proc/backend/vllm/metrics_test.go | 7 ++-- pkg/ext-proc/handlers/request.go | 8 ++-- pkg/ext-proc/handlers/server.go | 5 +-- pkg/ext-proc/main.go | 9 ++--- pkg/ext-proc/scheduling/filter.go | 7 ++-- pkg/ext-proc/scheduling/filter_test.go | 5 +-- pkg/ext-proc/scheduling/scheduler.go | 3 +- pkg/ext-proc/test/benchmark/benchmark.go | 3 +- pkg/ext-proc/test/hermetic_test.go | 13 +++--- pkg/ext-proc/test/utils.go | 13 +++--- test/e2e/e2e_test.go | 7 ++-- test/utils/utils.go | 2 +- 19 files changed, 63 insertions(+), 76 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index 6b297462..1462bcc7 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -2,45 +2,41 @@ run: timeout: 5m allow-parallel-runners: true +# Settings related to issues issues: - # don't skip warning about doc comments - # don't exclude the default set of lint - exclude-use-default: false - # restore some of the defaults - # (fill in the rest as needed) - exclude-rules: - - path: "api/*" - linters: - - lll - - path: "internal/*" - linters: - - dupl - - lll + # Which dirs to exclude: issues from them won't be reported + exclude-dirs: + - bin linters: disable-all: true enable: - - dupl - - errcheck - copyloopvar + - dupword + - durationcheck + - fatcontext + - gci - ginkgolinter + - gocritic + - govet + - loggercheck + - misspell + - perfsprint + - revive + - unconvert + - makezero + - errcheck - goconst - gocyclo - gofmt - goimports - gosimple - - govet - ineffassign - - lll - - misspell - nakedret - prealloc - - revive - - staticcheck - typecheck - - unconvert - unparam - unused - + linters-settings: revive: rules: diff --git a/Makefile b/Makefile index 8879a6b1..06b8942a 100644 --- a/Makefile +++ b/Makefile @@ -122,7 +122,7 @@ ci-lint: golangci-lint $(GOLANGCI_LINT) run --timeout 15m0s .PHONY: verify -verify: vet fmt-verify manifests generate ## ci-lint add back when all lint errors are fixed +verify: vet fmt-verify manifests generate ci-lint git --no-pager diff --exit-code config api client-go ##@ Build diff --git a/api/v1alpha1/inferencemodel_types.go b/api/v1alpha1/inferencemodel_types.go index 766ecfef..84748303 100644 --- a/api/v1alpha1/inferencemodel_types.go +++ b/api/v1alpha1/inferencemodel_types.go @@ -41,7 +41,7 @@ type InferenceModelSpec struct { // The name of the model as the users set in the "model" parameter in the requests. // The name should be unique among the workloads that reference the same backend pool. // This is the parameter that will be used to match the request with. In the future, we may - // allow to match on other request parameters. The other approach to support matching on + // allow to match on other request parameters. The other approach to support matching // on other request parameters is to use a different ModelName per HTTPFilter. // Names can be reserved without implementing an actual model in the pool. // This can be done by specifying a target model and setting the weight to zero, diff --git a/config/crd/bases/inference.networking.x-k8s.io_inferencemodels.yaml b/config/crd/bases/inference.networking.x-k8s.io_inferencemodels.yaml index 671a8431..757cf0d0 100644 --- a/config/crd/bases/inference.networking.x-k8s.io_inferencemodels.yaml +++ b/config/crd/bases/inference.networking.x-k8s.io_inferencemodels.yaml @@ -68,7 +68,7 @@ spec: The name of the model as the users set in the "model" parameter in the requests. The name should be unique among the workloads that reference the same backend pool. This is the parameter that will be used to match the request with. In the future, we may - allow to match on other request parameters. The other approach to support matching on + allow to match on other request parameters. The other approach to support matching on other request parameters is to use a different ModelName per HTTPFilter. Names can be reserved without implementing an actual model in the pool. This can be done by specifying a target model and setting the weight to zero, diff --git a/pkg/ext-proc/backend/datastore.go b/pkg/ext-proc/backend/datastore.go index b6d46f43..5914ac04 100644 --- a/pkg/ext-proc/backend/datastore.go +++ b/pkg/ext-proc/backend/datastore.go @@ -1,7 +1,7 @@ package backend import ( - "fmt" + "errors" "math/rand" "sync" @@ -53,7 +53,7 @@ func (ds *K8sDatastore) getInferencePool() (*v1alpha1.InferencePool, error) { ds.poolMu.RLock() defer ds.poolMu.RUnlock() if ds.inferencePool == nil { - return nil, fmt.Errorf("InferencePool hasn't been initialized yet") + return nil, errors.New("InferencePool hasn't been initialized yet") } return ds.inferencePool, nil } diff --git a/pkg/ext-proc/backend/endpointslice_reconciler.go b/pkg/ext-proc/backend/endpointslice_reconciler.go index f99ff61f..e318959b 100644 --- a/pkg/ext-proc/backend/endpointslice_reconciler.go +++ b/pkg/ext-proc/backend/endpointslice_reconciler.go @@ -2,7 +2,7 @@ package backend import ( "context" - "fmt" + "strconv" "inference.networking.x-k8s.io/llm-instance-gateway/api/v1alpha1" discoveryv1 "k8s.io/api/discovery/v1" @@ -57,7 +57,7 @@ func (c *EndpointSliceReconciler) updateDatastore( if c.validPod(endpoint) { pod := Pod{ Name: endpoint.TargetRef.Name, - Address: endpoint.Addresses[0] + ":" + fmt.Sprint(inferencePool.Spec.TargetPortNumber), + Address: endpoint.Addresses[0] + ":" + strconv.Itoa(int(inferencePool.Spec.TargetPortNumber)), } podMap[pod] = true c.Datastore.pods.Store(pod, true) diff --git a/pkg/ext-proc/backend/inferencepool_reconciler.go b/pkg/ext-proc/backend/inferencepool_reconciler.go index 662b6f41..b8657722 100644 --- a/pkg/ext-proc/backend/inferencepool_reconciler.go +++ b/pkg/ext-proc/backend/inferencepool_reconciler.go @@ -3,13 +3,12 @@ package backend import ( "context" - "sigs.k8s.io/controller-runtime/pkg/client" - "inference.networking.x-k8s.io/llm-instance-gateway/api/v1alpha1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/client-go/tools/record" "k8s.io/klog/v2" ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" ) // InferencePoolReconciler utilizes the controller runtime to reconcile Instance Gateway resources diff --git a/pkg/ext-proc/backend/vllm/metrics_test.go b/pkg/ext-proc/backend/vllm/metrics_test.go index 6121fa11..1225c709 100644 --- a/pkg/ext-proc/backend/vllm/metrics_test.go +++ b/pkg/ext-proc/backend/vllm/metrics_test.go @@ -1,14 +1,13 @@ package vllm import ( - "fmt" + "errors" "testing" - "inference.networking.x-k8s.io/llm-instance-gateway/pkg/ext-proc/backend" - dto "github.com/prometheus/client_model/go" "github.com/stretchr/testify/assert" "google.golang.org/protobuf/proto" + "inference.networking.x-k8s.io/llm-instance-gateway/pkg/ext-proc/backend" ) func TestPromToPodMetrics(t *testing.T) { @@ -215,7 +214,7 @@ func TestPromToPodMetrics(t *testing.T) { MaxActiveModels: 0, }, initialPodMetrics: &backend.PodMetrics{}, - expectedErr: fmt.Errorf("strconv.Atoi: parsing '2a': invalid syntax"), + expectedErr: errors.New("strconv.Atoi: parsing '2a': invalid syntax"), }, } for _, tc := range testCases { diff --git a/pkg/ext-proc/handlers/request.go b/pkg/ext-proc/handlers/request.go index 9deac950..83ab46d0 100644 --- a/pkg/ext-proc/handlers/request.go +++ b/pkg/ext-proc/handlers/request.go @@ -2,15 +2,15 @@ package handlers import ( "encoding/json" + "errors" "fmt" "strconv" configPb "github.com/envoyproxy/go-control-plane/envoy/config/core/v3" extProcPb "github.com/envoyproxy/go-control-plane/envoy/service/ext_proc/v3" - klog "k8s.io/klog/v2" - "inference.networking.x-k8s.io/llm-instance-gateway/pkg/ext-proc/backend" "inference.networking.x-k8s.io/llm-instance-gateway/pkg/ext-proc/scheduling" + klog "k8s.io/klog/v2" ) // HandleRequestBody handles body of the request to the backend server, such as parsing the "model" @@ -31,7 +31,7 @@ func (s *Server) HandleRequestBody(reqCtx *RequestContext, req *extProcPb.Proces // Resolve target models. model, ok := rb["model"].(string) if !ok { - return nil, fmt.Errorf("model not found in request") + return nil, errors.New("model not found in request") } klog.V(3).Infof("Model requested: %v", model) modelName := model @@ -87,7 +87,7 @@ func (s *Server) HandleRequestBody(reqCtx *RequestContext, req *extProcPb.Proces }, }, // We need to update the content length header if the body is mutated, see Envoy doc: - // https://www.envoyproxy.io/docs/envoy/latest/api-v3/extensions/filters/http/ext_proc/v3/processing_mode.proto#enum-extensions-filters-http-ext-proc-v3-processingmode-bodysendmode + // https://www.envoyproxy.io/docs/envoy/latest/api-v3/extensions/filters/http/ext_proc/v3/processing_mode.proto { Header: &configPb.HeaderValue{ Key: "Content-Length", diff --git a/pkg/ext-proc/handlers/server.go b/pkg/ext-proc/handlers/server.go index b443ef46..8ddd54d1 100644 --- a/pkg/ext-proc/handlers/server.go +++ b/pkg/ext-proc/handlers/server.go @@ -7,11 +7,10 @@ import ( envoyTypePb "github.com/envoyproxy/go-control-plane/envoy/type/v3" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" - klog "k8s.io/klog/v2" - "inference.networking.x-k8s.io/llm-instance-gateway/api/v1alpha1" "inference.networking.x-k8s.io/llm-instance-gateway/pkg/ext-proc/backend" "inference.networking.x-k8s.io/llm-instance-gateway/pkg/ext-proc/scheduling" + klog "k8s.io/klog/v2" ) func NewServer(pp PodProvider, scheduler Scheduler, targetPodHeader string, datastore ModelDataStore) *Server { @@ -73,7 +72,7 @@ func (s *Server) Process(srv extProcPb.ExternalProcessor_ProcessServer) error { return status.Errorf(codes.Unknown, "cannot receive stream request: %v", err) } - resp := &extProcPb.ProcessingResponse{} + var resp *extProcPb.ProcessingResponse switch v := req.Request.(type) { case *extProcPb.ProcessingRequest_RequestHeaders: resp = HandleRequestHeaders(reqCtx, req) diff --git a/pkg/ext-proc/main.go b/pkg/ext-proc/main.go index c0efdc5a..f7c76e6b 100644 --- a/pkg/ext-proc/main.go +++ b/pkg/ext-proc/main.go @@ -16,16 +16,15 @@ import ( healthPb "google.golang.org/grpc/health/grpc_health_v1" "google.golang.org/grpc/status" "inference.networking.x-k8s.io/llm-instance-gateway/api/v1alpha1" + "inference.networking.x-k8s.io/llm-instance-gateway/pkg/ext-proc/backend" + "inference.networking.x-k8s.io/llm-instance-gateway/pkg/ext-proc/backend/vllm" + "inference.networking.x-k8s.io/llm-instance-gateway/pkg/ext-proc/handlers" + "inference.networking.x-k8s.io/llm-instance-gateway/pkg/ext-proc/scheduling" "k8s.io/apimachinery/pkg/runtime" utilruntime "k8s.io/apimachinery/pkg/util/runtime" clientgoscheme "k8s.io/client-go/kubernetes/scheme" klog "k8s.io/klog/v2" ctrl "sigs.k8s.io/controller-runtime" - - "inference.networking.x-k8s.io/llm-instance-gateway/pkg/ext-proc/backend" - "inference.networking.x-k8s.io/llm-instance-gateway/pkg/ext-proc/backend/vllm" - "inference.networking.x-k8s.io/llm-instance-gateway/pkg/ext-proc/handlers" - "inference.networking.x-k8s.io/llm-instance-gateway/pkg/ext-proc/scheduling" ) var ( diff --git a/pkg/ext-proc/scheduling/filter.go b/pkg/ext-proc/scheduling/filter.go index 7a483aa6..6ce31fec 100644 --- a/pkg/ext-proc/scheduling/filter.go +++ b/pkg/ext-proc/scheduling/filter.go @@ -1,12 +1,11 @@ package scheduling import ( - "fmt" + "errors" "math" - klog "k8s.io/klog/v2" - "inference.networking.x-k8s.io/llm-instance-gateway/pkg/ext-proc/backend" + klog "k8s.io/klog/v2" ) type Filter interface { @@ -86,7 +85,7 @@ func toFilterFunc(pp podPredicate) filterFunc { } } if len(filtered) == 0 { - return nil, fmt.Errorf("no pods left") + return nil, errors.New("no pods left") } return filtered, nil } diff --git a/pkg/ext-proc/scheduling/filter_test.go b/pkg/ext-proc/scheduling/filter_test.go index e4c40dc2..c7ffe45d 100644 --- a/pkg/ext-proc/scheduling/filter_test.go +++ b/pkg/ext-proc/scheduling/filter_test.go @@ -1,11 +1,10 @@ package scheduling import ( - "fmt" + "errors" "testing" "github.com/google/go-cmp/cmp" - "inference.networking.x-k8s.io/llm-instance-gateway/pkg/ext-proc/backend" ) @@ -21,7 +20,7 @@ func TestFilter(t *testing.T) { { name: "simple filter without successor, failure", filter: &filter{filter: func(req *LLMRequest, pods []*backend.PodMetrics) ([]*backend.PodMetrics, error) { - return nil, fmt.Errorf("filter error") + return nil, errors.New("filter error") }}, err: true, }, diff --git a/pkg/ext-proc/scheduling/scheduler.go b/pkg/ext-proc/scheduling/scheduler.go index f2f0a09f..1c41794c 100644 --- a/pkg/ext-proc/scheduling/scheduler.go +++ b/pkg/ext-proc/scheduling/scheduler.go @@ -7,9 +7,8 @@ import ( "google.golang.org/grpc/codes" "google.golang.org/grpc/status" - klog "k8s.io/klog/v2" - "inference.networking.x-k8s.io/llm-instance-gateway/pkg/ext-proc/backend" + klog "k8s.io/klog/v2" ) const ( diff --git a/pkg/ext-proc/test/benchmark/benchmark.go b/pkg/ext-proc/test/benchmark/benchmark.go index 7c7e1691..559f3b7c 100644 --- a/pkg/ext-proc/test/benchmark/benchmark.go +++ b/pkg/ext-proc/test/benchmark/benchmark.go @@ -10,11 +10,10 @@ import ( "github.com/bojand/ghz/runner" "github.com/jhump/protoreflect/desc" "google.golang.org/protobuf/proto" - klog "k8s.io/klog/v2" - "inference.networking.x-k8s.io/llm-instance-gateway/api/v1alpha1" "inference.networking.x-k8s.io/llm-instance-gateway/pkg/ext-proc/backend" "inference.networking.x-k8s.io/llm-instance-gateway/pkg/ext-proc/test" + klog "k8s.io/klog/v2" ) var ( diff --git a/pkg/ext-proc/test/hermetic_test.go b/pkg/ext-proc/test/hermetic_test.go index 365524a8..b77d4ae3 100644 --- a/pkg/ext-proc/test/hermetic_test.go +++ b/pkg/ext-proc/test/hermetic_test.go @@ -4,20 +4,17 @@ package test import ( "context" "fmt" - "log" "testing" "time" - "inference.networking.x-k8s.io/llm-instance-gateway/api/v1alpha1" - "inference.networking.x-k8s.io/llm-instance-gateway/pkg/ext-proc/backend" - configPb "github.com/envoyproxy/go-control-plane/envoy/config/core/v3" extProcPb "github.com/envoyproxy/go-control-plane/envoy/service/ext_proc/v3" "github.com/google/go-cmp/cmp" - "google.golang.org/protobuf/testing/protocmp" - "google.golang.org/grpc" "google.golang.org/grpc/credentials/insecure" + "google.golang.org/protobuf/testing/protocmp" + "inference.networking.x-k8s.io/llm-instance-gateway/api/v1alpha1" + "inference.networking.x-k8s.io/llm-instance-gateway/pkg/ext-proc/backend" ) const ( @@ -145,13 +142,13 @@ func setUpServer(t *testing.T, pods []*backend.PodMetrics, models map[string]*v1 // Create a grpc connection conn, err := grpc.NewClient(address, grpc.WithTransportCredentials(insecure.NewCredentials())) if err != nil { - log.Fatalf("Failed to connect to %v: %v", address, err) + t.Fatalf("Failed to connect to %v: %v", address, err) } ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) client, err = extProcPb.NewExternalProcessorClient(conn).Process(ctx) if err != nil { - log.Fatalf("Failed to create client: %v", err) + t.Fatalf("Failed to create client: %v", err) } return client, func() { cancel() diff --git a/pkg/ext-proc/test/utils.go b/pkg/ext-proc/test/utils.go index 9b7eeafe..aabb34a1 100644 --- a/pkg/ext-proc/test/utils.go +++ b/pkg/ext-proc/test/utils.go @@ -6,16 +6,14 @@ import ( "net" "time" + extProcPb "github.com/envoyproxy/go-control-plane/envoy/service/ext_proc/v3" "google.golang.org/grpc" "google.golang.org/grpc/reflection" - klog "k8s.io/klog/v2" - - extProcPb "github.com/envoyproxy/go-control-plane/envoy/service/ext_proc/v3" - "inference.networking.x-k8s.io/llm-instance-gateway/api/v1alpha1" "inference.networking.x-k8s.io/llm-instance-gateway/pkg/ext-proc/backend" "inference.networking.x-k8s.io/llm-instance-gateway/pkg/ext-proc/handlers" "inference.networking.x-k8s.io/llm-instance-gateway/pkg/ext-proc/scheduling" + klog "k8s.io/klog/v2" ) func StartExtProc(port int, refreshPodsInterval, refreshMetricsInterval time.Duration, pods []*backend.PodMetrics, models map[string]*v1alpha1.InferenceModel) *grpc.Server { @@ -46,7 +44,12 @@ func startExtProc(port int, pp *backend.Provider, models map[string]*v1alpha1.In klog.Infof("Starting gRPC server on port :%v", port) reflection.Register(s) - go s.Serve(lis) + go func() { + err := s.Serve(lis) + if err != nil { + klog.Fatalf("Ext-proc failed with the err: %v", err) + } + }() return s } diff --git a/test/e2e/e2e_test.go b/test/e2e/e2e_test.go index 5af72529..54547c04 100644 --- a/test/e2e/e2e_test.go +++ b/test/e2e/e2e_test.go @@ -23,7 +23,6 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" - "inference.networking.x-k8s.io/llm-instance-gateway/test/utils" ) @@ -63,11 +62,11 @@ var _ = Describe("controller", Ordered, func() { var projectimage = "example.com/api:v0.0.1" By("building the manager(Operator) image") - cmd := exec.Command("make", "docker-build", fmt.Sprintf("IMG=%s", projectimage)) + cmd := exec.Command("make", "docker-build", "IMG=%s"+projectimage) _, err = utils.Run(cmd) ExpectWithOffset(1, err).NotTo(HaveOccurred()) - By("loading the the manager(Operator) image on Kind") + By("loading the manager(Operator) image on Kind") err = utils.LoadImageToKindClusterWithName(projectimage) ExpectWithOffset(1, err).NotTo(HaveOccurred()) @@ -77,7 +76,7 @@ var _ = Describe("controller", Ordered, func() { ExpectWithOffset(1, err).NotTo(HaveOccurred()) By("deploying the controller-manager") - cmd = exec.Command("make", "deploy", fmt.Sprintf("IMG=%s", projectimage)) + cmd = exec.Command("make", "deploy", "IMG=%s"+projectimage) _, err = utils.Run(cmd) ExpectWithOffset(1, err).NotTo(HaveOccurred()) diff --git a/test/utils/utils.go b/test/utils/utils.go index 6b96ab5d..23fea11a 100644 --- a/test/utils/utils.go +++ b/test/utils/utils.go @@ -135,6 +135,6 @@ func GetProjectDir() (string, error) { if err != nil { return wd, err } - wd = strings.Replace(wd, "/test/e2e", "", -1) + wd = strings.ReplaceAll(wd, "/test/e2e", "") return wd, nil }