From 6cc00f6ddc9fe4970dc2e8d0fd193ccf6a409ced Mon Sep 17 00:00:00 2001 From: Alastair Flynn Date: Tue, 15 Oct 2024 10:36:09 +0200 Subject: [PATCH 1/4] ci: remove limits on commit body length in CI Testing this with a really long body. I am doing this because sometimes the dependabot commits have a really long body and it causes issues with the CI. The length of the body is now a warning not an error. Also remove the limit on the foote since this is also arbitrary. Testing with a really really long footer. --- .github/commitlint.config.mjs | 8 ++++++++ .github/workflows/static-analysis.yml | 2 ++ 2 files changed, 10 insertions(+) create mode 100644 .github/commitlint.config.mjs diff --git a/.github/commitlint.config.mjs b/.github/commitlint.config.mjs new file mode 100644 index 00000000000..1c70206537b --- /dev/null +++ b/.github/commitlint.config.mjs @@ -0,0 +1,8 @@ +export default { + extends: ['@commitlint/config-conventional'], + rules: { + /* The 1 means this is a warning, 2 would mean an error. */ + 'body-max-line-length': [1, 'always', 100], + 'footer-max-line-length': [1, 'always', 100], + }, +}; diff --git a/.github/workflows/static-analysis.yml b/.github/workflows/static-analysis.yml index 2124aebb540..926b48f2df9 100644 --- a/.github/workflows/static-analysis.yml +++ b/.github/workflows/static-analysis.yml @@ -146,6 +146,8 @@ jobs: steps: - uses: actions/checkout@v4 - uses: wagoid/commitlint-github-action@v6 + with: + configFile: .github/commitlint.config.mjs go-version: name: Go Version From a6364145828f8e4cc78e10c6f928ca81cbb9cf5b Mon Sep 17 00:00:00 2001 From: Guillaume Fouillet Date: Tue, 22 Oct 2024 14:53:40 +0200 Subject: [PATCH 2/4] chore(logsender): comments some unusual pattern for closing writer In worker.go, we have allocate a resource and close it in several code branch, which is unusual and error-prone. Since I don't find a better way to do it, I added few comments to explicit the intent. Signed-off-by: Guillaume Fouillet --- worker/logsender/worker.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/worker/logsender/worker.go b/worker/logsender/worker.go index 0ed87a53903..fa1118ed49b 100644 --- a/worker/logsender/worker.go +++ b/worker/logsender/worker.go @@ -41,6 +41,8 @@ func New(logs LogRecordCh, logSenderAPI *logsender.API) worker.Worker { select { case sender <- logWriter: case <-stop: + // Loop has been stopped before returning the writer. Need to close it in order to avoid + // possible ressources leak logWriter.Close() } }() @@ -53,6 +55,8 @@ func New(logs LogRecordCh, logSenderAPI *logsender.API) worker.Worker { case <-stop: return nil } + // the logwriter has been successfully retrieved from the inside goroutine and its lifecycle is now handled + // in the loop function. defer logWriter.Close() for { select { From 0d2b0df811fc81bbda0fdd89ab83bf66a838a84d Mon Sep 17 00:00:00 2001 From: Guillaume Fouillet Date: Tue, 22 Oct 2024 14:55:54 +0200 Subject: [PATCH 3/4] fix(logs): ensure connection closure in streamDebugLog - Before this commit, there is a possible connection leak when requesting the debug log, either from command line or during a migration. - After this commit, the connection to the API is correctly closed once the log stream is dried. Signed-off-by: Guillaume Fouillet --- api/common/logs.go | 1 + 1 file changed, 1 insertion(+) diff --git a/api/common/logs.go b/api/common/logs.go index b707f33047c..84f3d67e52a 100644 --- a/api/common/logs.go +++ b/api/common/logs.go @@ -116,6 +116,7 @@ func StreamDebugLog(ctx context.Context, source base.StreamConnector, args Debug messages := make(chan LogMessage) go func() { + defer func() { _ = connection.Close() }() defer close(messages) for { From 118791bbe16239359de92094dd16ab5c3004a7cc Mon Sep 17 00:00:00 2001 From: Guillaume Fouillet Date: Tue, 22 Oct 2024 16:36:46 +0200 Subject: [PATCH 4/4] refactor(apiserver): decorate all apiserver handlers to count all connections We have a running production issues where we seems to leak connections for some reason. This change allows to track all connections targeting the API server. - Before this commit, handlers did not have a standardized way to track and monitor API connections, making it harder to measure and manage resource usage. - After this commit, handlers are wrapped with `monitoredHandler`, providing consistent and automated tracking of metrics, improving resource management and observability, except for logsink who requires specific metrics. Signed-off-by: Guillaume Fouillet --- apiserver/apiserver.go | 78 +++++++++++++++++++++++------------------- 1 file changed, 42 insertions(+), 36 deletions(-) diff --git a/apiserver/apiserver.go b/apiserver/apiserver.go index 6d758aec53f..7aa5d8077af 100644 --- a/apiserver/apiserver.go +++ b/apiserver/apiserver.go @@ -708,11 +708,11 @@ func (srv *Server) endpoints() ([]apihttp.Endpoint, error) { } httpCtxt := httpContext{srv: srv} - mainAPIHandler := http.HandlerFunc(srv.apiHandler) - healthHandler := http.HandlerFunc(srv.healthHandler) - logStreamHandler := newLogStreamEndpointHandler(httpCtxt) - embeddedCLIHandler := newEmbeddedCLIHandler(httpCtxt) - debugLogHandler := newDebugLogDBHandler( + mainAPIHandler := srv.monitoredHandler(http.HandlerFunc(srv.apiHandler), "api") + healthHandler := srv.monitoredHandler(http.HandlerFunc(srv.healthHandler), "health") + logStreamHandler := srv.monitoredHandler(newLogStreamEndpointHandler(httpCtxt), "logstream") + embeddedCLIHandler := srv.monitoredHandler(newEmbeddedCLIHandler(httpCtxt), "commands") + debugLogHandler := srv.monitoredHandler(newDebugLogDBHandler( httpCtxt, httpAuthenticator, tagKindAuthorizer{ @@ -721,8 +721,8 @@ func (srv *Server) endpoints() ([]apihttp.Endpoint, error) { names.UserTagKind, names.ApplicationTagKind, }, - ) - pubsubHandler := newPubSubHandler(httpCtxt, srv.shared.centralHub) + ), "log") + pubsubHandler := srv.monitoredHandler(newPubSubHandler(httpCtxt, srv.shared.centralHub), "pubsub") logSinkHandler := logsink.NewHTTPHandler( newAgentLogWriteCloserFunc(httpCtxt, srv.logSinkWriter, &srv.apiServerLoggers), httpCtxt.stop(), @@ -744,36 +744,36 @@ func (srv *Server) endpoints() ([]apihttp.Endpoint, error) { ctxt: httpCtxt, dataDir: srv.dataDir, } - modelRestServer := &RestHTTPHandler{ + modelRestServer := srv.monitoredHandler(&RestHTTPHandler{ GetHandler: modelRestHandler.ServeGet, - } + }, "rest") modelCharmsHandler := &charmsHandler{ ctxt: httpCtxt, dataDir: srv.dataDir, stateAuthFunc: httpCtxt.stateForRequestAuthenticatedUser, } - modelCharmsHTTPHandler := &CharmsHTTPHandler{ + modelCharmsHTTPHandler := srv.monitoredHandler(&CharmsHTTPHandler{ PostHandler: modelCharmsHandler.ServePost, GetHandler: modelCharmsHandler.ServeGet, - } + }, "charms") modelCharmsUploadAuthorizer := tagKindAuthorizer{names.UserTagKind} modelObjectsCharmsHandler := &objectsCharmHandler{ ctxt: httpCtxt, } - modelObjectsCharmsHTTPHandler := &objectsCharmHTTPHandler{ + modelObjectsCharmsHTTPHandler := srv.monitoredHandler(&objectsCharmHTTPHandler{ GetHandler: modelObjectsCharmsHandler.ServeGet, PutHandler: modelObjectsCharmsHandler.ServePut, LegacyCharmsHandler: modelCharmsHTTPHandler, - } + }, "charms") - modelToolsUploadHandler := &toolsUploadHandler{ + modelToolsUploadHandler := srv.monitoredHandler(&toolsUploadHandler{ ctxt: httpCtxt, stateAuthFunc: httpCtxt.stateForRequestAuthenticatedUser, - } + }, "tools") modelToolsUploadAuthorizer := tagKindAuthorizer{names.UserTagKind} - modelToolsDownloadHandler := newToolsDownloadHandler(httpCtxt) - resourcesHandler := &ResourcesHandler{ + modelToolsDownloadHandler := srv.monitoredHandler(newToolsDownloadHandler(httpCtxt), "tools") + resourcesHandler := srv.monitoredHandler(&ResourcesHandler{ StateAuthFunc: func(req *http.Request, tagKinds ...string) (ResourcesBackend, state.PoolHelper, names.Tag, error) { st, entity, err := httpCtxt.stateForRequestAuthenticatedTag(req, tagKinds...) @@ -797,8 +797,8 @@ func (srv *Server) endpoints() ([]apihttp.Endpoint, error) { } return nil }, - } - unitResourcesHandler := &UnitResourcesHandler{ + }, "applications") + unitResourcesHandler := srv.monitoredHandler(&UnitResourcesHandler{ NewOpener: func(req *http.Request, tagKinds ...string) (resources.Opener, state.PoolHelper, error) { st, _, err := httpCtxt.stateForRequestAuthenticatedTag(req, tagKinds...) if err != nil { @@ -816,7 +816,7 @@ func (srv *Server) endpoints() ([]apihttp.Endpoint, error) { } return opener, st, nil }, - } + }, "units") controllerAdminAuthorizer := controllerAdminAuthorizer{ controllerTag: systemState.ControllerTag(), @@ -830,21 +830,21 @@ func (srv *Server) endpoints() ([]apihttp.Endpoint, error) { PostHandler: migrateCharmsHandler.ServePost, GetHandler: migrateCharmsHandler.ServeUnsupported, } - migrateObjectsCharmsHTTPHandler := &objectsCharmHTTPHandler{ + migrateObjectsCharmsHTTPHandler := srv.monitoredHandler(&objectsCharmHTTPHandler{ PutHandler: modelObjectsCharmsHandler.ServePut, GetHandler: modelObjectsCharmsHandler.ServeUnsupported, LegacyCharmsHandler: migrateCharmsHTTPHandler, - } - migrateToolsUploadHandler := &toolsUploadHandler{ + }, "charms") + migrateToolsUploadHandler := srv.monitoredHandler(&toolsUploadHandler{ ctxt: httpCtxt, stateAuthFunc: httpCtxt.stateForMigrationImporting, - } - resourcesMigrationUploadHandler := &resourcesMigrationUploadHandler{ + }, "tools") + resourcesMigrationUploadHandler := srv.monitoredHandler(&resourcesMigrationUploadHandler{ ctxt: httpCtxt, stateAuthFunc: httpCtxt.stateForMigrationImporting, - } - backupHandler := &backupHandler{ctxt: httpCtxt} - registerHandler := ®isterUserHandler{ctxt: httpCtxt} + }, "resources") + backupHandler := srv.monitoredHandler(&backupHandler{ctxt: httpCtxt}, "backups") + registerHandler := srv.monitoredHandler(®isterUserHandler{ctxt: httpCtxt}, "register") // HTTP handler for application offer macaroon authentication. addOfferAuthHandlers(srv.offerAuthCtxt, srv.mux) @@ -919,7 +919,7 @@ func (srv *Server) endpoints() ([]apihttp.Endpoint, error) { }, { // Legacy migration endpoint. Used by Juju 3.3 and prior pattern: "/migrate/charms", - handler: migrateCharmsHTTPHandler, + handler: srv.monitoredHandler(migrateCharmsHTTPHandler, "charms"), authorizer: controllerAdminAuthorizer, }, { pattern: "/migrate/charms/:object", @@ -1008,7 +1008,7 @@ func (srv *Server) endpoints() ([]apihttp.Endpoint, error) { add := func(subpath string, h http.Handler) { handlers = append(handlers, handler{ pattern: path.Join("/introspection/", subpath), - handler: introspectionHandler{httpCtxt, h}, + handler: srv.monitoredHandler(introspectionHandler{httpCtxt, h}, "introspection"), }) } srv.registerIntrospectionHandlers(add) @@ -1069,12 +1069,6 @@ func (srv *Server) healthHandler(w http.ResponseWriter, req *http.Request) { } func (srv *Server) apiHandler(w http.ResponseWriter, req *http.Request) { - srv.metricsCollector.TotalConnections.Inc() - - gauge := srv.metricsCollector.APIConnections.WithLabelValues("api") - gauge.Inc() - defer gauge.Dec() - connectionID := atomic.AddUint64(&srv.lastConnectionID, 1) apiObserver := srv.newObserver() @@ -1174,3 +1168,15 @@ func (srv *Server) GetAuditConfig() auditlog.Config { // Delegates to the getter passed in. return srv.getAuditConfig() } + +// monitoredHandler wraps an HTTP handler for tracking metrics with given label. +// It increments and decrements connection counters for monitoring purposes. +func (srv *Server) monitoredHandler(handler http.Handler, label string) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + srv.metricsCollector.TotalConnections.Inc() + gauge := srv.metricsCollector.APIConnections.WithLabelValues(label) + gauge.Inc() + defer gauge.Dec() + handler.ServeHTTP(w, r) + }) +}