From 96b97f7f58be75b65cc74cfd242fb79b626f5688 Mon Sep 17 00:00:00 2001 From: shaoyue Date: Fri, 24 Jun 2022 15:34:14 +0800 Subject: [PATCH] RESTful api use same port as metrics & healthcheck (#17732) Signed-off-by: shaoyue.chen --- configs/milvus.yaml | 3 -- internal/distributed/proxy/service.go | 45 ++++++--------------- internal/distributed/proxy/service_test.go | 17 ++++++-- internal/util/paramtable/http_param.go | 27 ++----------- internal/util/paramtable/http_param_test.go | 4 -- tests/scripts/e2e-restful.sh | 27 +++++-------- 6 files changed, 40 insertions(+), 83 deletions(-) diff --git a/configs/milvus.yaml b/configs/milvus.yaml index f30fd7a61d..1f0fd7e09e 100644 --- a/configs/milvus.yaml +++ b/configs/milvus.yaml @@ -133,9 +133,6 @@ proxy: http: enabled: true # Whether to enable the http server debug_mode: false # Whether to enable http server debug mode - port: 8080 # Whether to enable the http server - readTimeout: 30000 # 30000 ms http read timeout - writeTimeout: 30000 # 30000 ms http write timeout timeTickInterval: 200 # ms, the interval that proxy synchronize the time tick msgStream: diff --git a/internal/distributed/proxy/service.go b/internal/distributed/proxy/service.go index ec4809807f..a7bfade893 100644 --- a/internal/distributed/proxy/service.go +++ b/internal/distributed/proxy/service.go @@ -73,8 +73,12 @@ var HTTPParams paramtable.HTTPConfig var ( errMissingMetadata = status.Errorf(codes.InvalidArgument, "missing metadata") errInvalidToken = status.Errorf(codes.Unauthenticated, "invalid token") + // registerHTTPHandlerOnce avoid register http handler multiple times + registerHTTPHandlerOnce sync.Once ) +const apiPathPrefix = "/api/v1" + // Server is the Proxy Server type Server struct { ctx context.Context @@ -82,9 +86,6 @@ type Server struct { proxy types.ProxyComponent grpcInternalServer *grpc.Server grpcExternalServer *grpc.Server - httpServer *http.Server - // avoid race - httpServerMtx sync.Mutex etcdCli *clientv3.Client rootCoordClient types.RootCoord @@ -111,9 +112,8 @@ func NewServer(ctx context.Context, factory dependency.Factory) (*Server, error) return server, err } -// startHTTPServer starts the http server, panic when failed -func (s *Server) startHTTPServer(port int) { - defer s.wg.Done() +// registerHTTPServer register the http server, panic when failed +func (s *Server) registerHTTPServer() { // (Embedded Milvus Only) Discard gin logs if logging is disabled. // We might need to put these logs in some files in the further. // But we don't care about these logs now, at least not in embedded Milvus. @@ -125,21 +125,9 @@ func (s *Server) startHTTPServer(port int) { gin.SetMode(gin.ReleaseMode) } ginHandler := gin.Default() - apiv1 := ginHandler.Group("/api/v1") + apiv1 := ginHandler.Group(apiPathPrefix) httpserver.NewHandlers(s.proxy).RegisterRoutesTo(apiv1) - s.httpServerMtx.Lock() - s.httpServer = &http.Server{ - Addr: fmt.Sprintf(":%d", port), - Handler: ginHandler, - ReadTimeout: HTTPParams.ReadTimeout, - WriteTimeout: HTTPParams.WriteTimeout, - } - s.httpServerMtx.Unlock() - if err := s.httpServer.ListenAndServe(); err != nil { - if err != http.ErrServerClosed { - panic("failed to start http server: " + err.Error()) - } - } + http.Handle("/", ginHandler) } func (s *Server) startInternalRPCServer(grpcInternalPort int, errChan chan error) { @@ -358,9 +346,10 @@ func (s *Server) init() error { } if HTTPParams.Enabled { - log.Info("start http server of proxy", zap.Int("port", HTTPParams.Port)) - s.wg.Add(1) - go s.startHTTPServer(HTTPParams.Port) + registerHTTPHandlerOnce.Do(func() { + log.Info("register http server of proxy") + s.registerHTTPServer() + }) } if s.rootCoordClient == nil { @@ -523,16 +512,6 @@ func (s *Server) Stop() error { } gracefulWg := sync.WaitGroup{} - gracefulWg.Add(1) - go func() { - defer gracefulWg.Done() - s.httpServerMtx.Lock() - defer s.httpServerMtx.Unlock() - if s.httpServer != nil { - log.Debug("Graceful stop http server...") - s.httpServer.Shutdown(context.TODO()) - } - }() gracefulWg.Add(1) go func() { diff --git a/internal/distributed/proxy/service_test.go b/internal/distributed/proxy/service_test.go index f5120ab595..8e93e2e4d7 100644 --- a/internal/distributed/proxy/service_test.go +++ b/internal/distributed/proxy/service_test.go @@ -1327,7 +1327,7 @@ func TestServer_Watch(t *testing.T) { assert.Equal(t, grpc_health_v1.HealthCheckResponse_SERVING, ret.Status) } -func Test_NewServer_HTTPServerDisabled(t *testing.T) { +func Test_NewServer_HTTPServer_Enabled(t *testing.T) { ctx := context.Background() server, err := NewServer(ctx, nil) assert.NotNil(t, server) @@ -1340,13 +1340,21 @@ func Test_NewServer_HTTPServerDisabled(t *testing.T) { server.dataCoordClient = &MockDataCoord{} HTTPParams.InitOnce() - HTTPParams.Enabled = false + HTTPParams.Enabled = true err = runAndWaitForServerReady(server) assert.Nil(t, err) - assert.Nil(t, server.httpServer) err = server.Stop() assert.Nil(t, err) + + defer func() { + e := recover() + if e == nil { + t.Fatalf("test should have panicked but did not") + } + }() + // if disable workds path not registered, so it shall not panic + server.registerHTTPServer() } func getServer(t *testing.T) *Server { @@ -1371,6 +1379,7 @@ func Test_NewServer_TLS_TwoWay(t *testing.T) { Params.ServerPemPath = "../../../configs/cert/server.pem" Params.ServerKeyPath = "../../../configs/cert/server.key" Params.CaPemPath = "../../../configs/cert/ca.pem" + HTTPParams.Enabled = false err := runAndWaitForServerReady(server) assert.Nil(t, err) @@ -1386,6 +1395,7 @@ func Test_NewServer_TLS_OneWay(t *testing.T) { Params.TLSMode = 1 Params.ServerPemPath = "../../../configs/cert/server.pem" Params.ServerKeyPath = "../../../configs/cert/server.key" + HTTPParams.Enabled = false err := runAndWaitForServerReady(server) assert.Nil(t, err) @@ -1401,6 +1411,7 @@ func Test_NewServer_TLS_FileNotExisted(t *testing.T) { Params.TLSMode = 1 Params.ServerPemPath = "../not/existed/server.pem" Params.ServerKeyPath = "../../../configs/cert/server.key" + HTTPParams.Enabled = false err := runAndWaitForServerReady(server) assert.NotNil(t, err) server.Stop() diff --git a/internal/util/paramtable/http_param.go b/internal/util/paramtable/http_param.go index 532ccc6d38..b49e798b1b 100644 --- a/internal/util/paramtable/http_param.go +++ b/internal/util/paramtable/http_param.go @@ -2,18 +2,14 @@ package paramtable import ( "sync" - "time" ) type HTTPConfig struct { BaseTable - once sync.Once - Enabled bool - DebugMode bool - Port int - ReadTimeout time.Duration - WriteTimeout time.Duration + once sync.Once + Enabled bool + DebugMode bool } // InitOnce initialize HTTPConfig @@ -28,9 +24,6 @@ func (p *HTTPConfig) init() { p.initHTTPEnabled() p.initHTTPDebugMode() - p.initHTTPPort() - p.initHTTPReadTimeout() - p.initHTTPWriteTimeout() } func (p *HTTPConfig) initHTTPEnabled() { @@ -40,17 +33,3 @@ func (p *HTTPConfig) initHTTPEnabled() { func (p *HTTPConfig) initHTTPDebugMode() { p.DebugMode = p.ParseBool("proxy.http.debug_mode", false) } - -func (p *HTTPConfig) initHTTPPort() { - p.Port = p.ParseIntWithDefault("proxy.http.port", 8080) -} - -func (p *HTTPConfig) initHTTPReadTimeout() { - interval := p.ParseIntWithDefault("proxy.http.readTimeout", 30000) - p.ReadTimeout = time.Duration(interval) * time.Millisecond -} - -func (p *HTTPConfig) initHTTPWriteTimeout() { - interval := p.ParseIntWithDefault("proxy.http.writeTimeout", 30000) - p.WriteTimeout = time.Duration(interval) * time.Millisecond -} diff --git a/internal/util/paramtable/http_param_test.go b/internal/util/paramtable/http_param_test.go index 76b1e0f18a..00559c2d5f 100644 --- a/internal/util/paramtable/http_param_test.go +++ b/internal/util/paramtable/http_param_test.go @@ -2,7 +2,6 @@ package paramtable import ( "testing" - "time" "github.com/stretchr/testify/assert" ) @@ -12,7 +11,4 @@ func TestHTTPConfig_Init(t *testing.T) { cf.InitOnce() assert.Equal(t, cf.Enabled, true) assert.Equal(t, cf.DebugMode, false) - assert.Equal(t, cf.Port, 8080) - assert.Equal(t, cf.ReadTimeout, time.Second*30) - assert.Equal(t, cf.WriteTimeout, time.Second*30) } diff --git a/tests/scripts/e2e-restful.sh b/tests/scripts/e2e-restful.sh index f7e617fb51..96956bd660 100755 --- a/tests/scripts/e2e-restful.sh +++ b/tests/scripts/e2e-restful.sh @@ -18,23 +18,18 @@ DATA_PATH="${ROOT}/tests/scripts/restful-data/" MILVUS_CLUSTER_ENABLED="${MILVUS_CLUSTER_ENABLED:-false}" -# TODO: use service instead of podIP when milvus-helm supports -if [[ "${MILVUS_CLUSTER_ENABLED}" == "false" ]]; then - MILVUS_SERVICE_NAME=$(kubectl -n ${MILVUS_HELM_NAMESPACE} get pods -l app.kubernetes.io/name=milvus -l component=standalone -l app.kubernetes.io/instance=${MILVUS_HELM_RELEASE_NAME} -o=jsonpath='{.items[0].status.podIP}') -else - MILVUS_SERVICE_NAME=$(kubectl -n ${MILVUS_HELM_NAMESPACE} get pods -l app.kubernetes.io/name=milvus -l component=proxy -l app.kubernetes.io/instance=${MILVUS_HELM_RELEASE_NAME} -o=jsonpath='{.items[0].status.podIP}') -fi +MILVUS_SERVICE_ADDRESS="${MILVUS_HELM_RELEASE_NAME}-milvus.${MILVUS_HELM_NAMESPACE}:9091" # Create a collection curl -X 'POST' \ - "http://${MILVUS_SERVICE_NAME}:8080/api/v1/collection" \ + "http://${MILVUS_SERVICE_ADDRESS}/api/v1/collection" \ -H 'accept: application/json' \ -H 'Content-Type: application/json' \ -d @${DATA_PATH}/create-collection.json # Has collection curl -X 'GET' \ - "http://${MILVUS_SERVICE_NAME}:8080/api/v1/collection/existence" \ + "http://${MILVUS_SERVICE_ADDRESS}/api/v1/collection/existence" \ -H 'accept: application/json' \ -H 'Content-Type: application/json' \ -d '{ @@ -43,7 +38,7 @@ curl -X 'GET' \ # Check collection details curl -X 'GET' \ - "http://${MILVUS_SERVICE_NAME}:8080/api/v1/collection" \ + "http://${MILVUS_SERVICE_ADDRESS}/api/v1/collection" \ -H 'accept: application/json' \ -H 'Content-Type: application/json' \ -d '{ @@ -52,7 +47,7 @@ curl -X 'GET' \ # Load collection curl -X 'POST' \ - "http://${MILVUS_SERVICE_NAME}:8080/api/v1/collection/load" \ + "http://${MILVUS_SERVICE_ADDRESS}/api/v1/collection/load" \ -H 'accept: application/json' \ -H 'Content-Type: application/json' \ -d '{ @@ -62,14 +57,14 @@ curl -X 'POST' \ ### Data # Insert Data curl -X 'POST' \ - "http://${MILVUS_SERVICE_NAME}:8080/api/v1/entities" \ + "http://${MILVUS_SERVICE_ADDRESS}/api/v1/entities" \ -H 'accept: application/json' \ -H 'Content-Type: application/json' \ -d @${DATA_PATH}/insert-data.json # Build Index curl -X 'POST' \ - "http://${MILVUS_SERVICE_NAME}:8080/api/v1/index" \ + "http://${MILVUS_SERVICE_ADDRESS}/api/v1/index" \ -H 'accept: application/json' \ -H 'Content-Type: application/json' \ -d '{ @@ -84,14 +79,14 @@ curl -X 'POST' \ # KNN Search curl -X 'POST' \ - "http://${MILVUS_SERVICE_NAME}:8080/api/v1/search" \ + "http://${MILVUS_SERVICE_ADDRESS}/api/v1/search" \ -H 'accept: application/json' \ -H 'Content-Type: application/json' \ -d @${DATA_PATH}/search.json # Drop Index curl -X 'DELETE' \ - "http://${MILVUS_SERVICE_NAME}:8080/api/v1/index" \ + "http://${MILVUS_SERVICE_ADDRESS}/api/v1/index" \ -H 'accept: application/json' \ -H 'Content-Type: application/json' \ -d '{ @@ -101,7 +96,7 @@ curl -X 'DELETE' \ # Release collection curl -X 'DELETE' \ - "http://${MILVUS_SERVICE_NAME}:8080/api/v1/collection/load" \ + "http://${MILVUS_SERVICE_ADDRESS}/api/v1/collection/load" \ -H 'accept: application/json' \ -H 'Content-Type: application/json' \ -d '{ @@ -110,7 +105,7 @@ curl -X 'DELETE' \ # Drop collection curl -X 'DELETE' \ - "http://${MILVUS_SERVICE_NAME}:8080/api/v1/collection" \ + "http://${MILVUS_SERVICE_ADDRESS}/api/v1/collection" \ -H 'accept: application/json' \ -H 'Content-Type: application/json' \ -d '{