From f0406c80cf7d93996d6bf9f07cc37f4945922156 Mon Sep 17 00:00:00 2001 From: Sam DeHaan Date: Tue, 26 Nov 2024 09:49:19 -0500 Subject: [PATCH 1/3] Refactor ui remtoecfg components to avoid race condition --- internal/service/ui/ui.go | 5 +- internal/web/api/api.go | 110 ++++++++++++++++++++++++-------------- 2 files changed, 72 insertions(+), 43 deletions(-) diff --git a/internal/service/ui/ui.go b/internal/service/ui/ui.go index ae462ddfe7..ea69448a82 100644 --- a/internal/service/ui/ui.go +++ b/internal/service/ui/ui.go @@ -78,10 +78,7 @@ func (s *Service) Data() any { func (s *Service) ServiceHandler(host service.Host) (base string, handler http.Handler) { r := mux.NewRouter() - remotecfgSvc, _ := host.GetService(remotecfg_service.ServiceName) - remotecfgHost := remotecfgSvc.Data().(remotecfg_service.Data).Host - - fa := api.NewAlloyAPI(host, remotecfgHost, s.opts.CallbackManager) + fa := api.NewAlloyAPI(host, s.opts.CallbackManager) fa.RegisterRoutes(path.Join(s.opts.UIPrefix, "/api/v0/web"), r) ui.RegisterRoutes(s.opts.UIPrefix, r) diff --git a/internal/web/api/api.go b/internal/web/api/api.go index 3c6688d04e..746252cca6 100644 --- a/internal/web/api/api.go +++ b/internal/web/api/api.go @@ -18,19 +18,19 @@ import ( "github.com/grafana/alloy/internal/service" "github.com/grafana/alloy/internal/service/cluster" "github.com/grafana/alloy/internal/service/livedebugging" + "github.com/grafana/alloy/internal/service/remotecfg" "github.com/prometheus/prometheus/util/httputil" ) // AlloyAPI is a wrapper around the component API. type AlloyAPI struct { alloy service.Host - remotecfg service.Host CallbackManager livedebugging.CallbackManager } // NewAlloyAPI instantiates a new Alloy API. -func NewAlloyAPI(alloy, remotecfg service.Host, CallbackManager livedebugging.CallbackManager) *AlloyAPI { - return &AlloyAPI{alloy: alloy, remotecfg: remotecfg, CallbackManager: CallbackManager} +func NewAlloyAPI(alloy service.Host, CallbackManager livedebugging.CallbackManager) *AlloyAPI { + return &AlloyAPI{alloy: alloy, CallbackManager: CallbackManager} } // RegisterRoutes registers all the API's routes. @@ -40,13 +40,13 @@ func (a *AlloyAPI) RegisterRoutes(urlPrefix string, r *mux.Router) { // component IDs. r.Handle(path.Join(urlPrefix, "/modules/{moduleID:.+}/components"), httputil.CompressionHandler{Handler: listComponentsHandler(a.alloy)}) - r.Handle(path.Join(urlPrefix, "/remotecfg/modules/{moduleID:.+}/components"), httputil.CompressionHandler{Handler: listComponentsHandler(a.remotecfg)}) + r.Handle(path.Join(urlPrefix, "/remotecfg/modules/{moduleID:.+}/components"), httputil.CompressionHandler{Handler: listComponentsHandlerRemoteCfg(a.alloy)}) r.Handle(path.Join(urlPrefix, "/components"), httputil.CompressionHandler{Handler: listComponentsHandler(a.alloy)}) - r.Handle(path.Join(urlPrefix, "/remotecfg/components"), httputil.CompressionHandler{Handler: listComponentsHandler(a.remotecfg)}) + r.Handle(path.Join(urlPrefix, "/remotecfg/components"), httputil.CompressionHandler{Handler: listComponentsHandlerRemoteCfg(a.alloy)}) r.Handle(path.Join(urlPrefix, "/components/{id:.+}"), httputil.CompressionHandler{Handler: getComponentHandler(a.alloy)}) - r.Handle(path.Join(urlPrefix, "/remotecfg/components/{id:.+}"), httputil.CompressionHandler{Handler: getComponentHandler(a.remotecfg)}) + r.Handle(path.Join(urlPrefix, "/remotecfg/components/{id:.+}"), httputil.CompressionHandler{Handler: getComponentHandlerRemoteCfg(a.alloy)}) r.Handle(path.Join(urlPrefix, "/peers"), httputil.CompressionHandler{Handler: getClusteringPeersHandler(a.alloy)}) r.Handle(path.Join(urlPrefix, "/debug/{id:.+}"), liveDebugging(a.alloy, a.CallbackManager)) @@ -54,53 +54,85 @@ func (a *AlloyAPI) RegisterRoutes(urlPrefix string, r *mux.Router) { func listComponentsHandler(host service.Host) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { - // moduleID is set from the /modules/{moduleID:.+}/components route above - // but not from the /components route. - var moduleID string - if vars := mux.Vars(r); vars != nil { - moduleID = vars["moduleID"] - } + listComponentsHandlerInternal(host, w, r) + } +} - components, err := host.ListComponents(moduleID, component.InfoOptions{ - GetHealth: true, - }) - if err != nil { - http.Error(w, err.Error(), http.StatusInternalServerError) +func listComponentsHandlerRemoteCfg(host service.Host) http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + svc, found := host.GetService(remotecfg.ServiceName) + if !found { + http.Error(w, "remote config service not available", http.StatusInternalServerError) return } - bb, err := json.Marshal(components) - if err != nil { - http.Error(w, err.Error(), http.StatusInternalServerError) - return - } - _, _ = w.Write(bb) + listComponentsHandlerInternal(svc.Data().(*remotecfg.Data).Host, w, r) } } +func listComponentsHandlerInternal(host service.Host, w http.ResponseWriter, r *http.Request) { + // moduleID is set from the /modules/{moduleID:.+}/components route above + // but not from the /components route. + var moduleID string + if vars := mux.Vars(r); vars != nil { + moduleID = vars["moduleID"] + } + + components, err := host.ListComponents(moduleID, component.InfoOptions{ + GetHealth: true, + }) + if err != nil { + http.Error(w, err.Error(), http.StatusInternalServerError) + return + } + + bb, err := json.Marshal(components) + if err != nil { + http.Error(w, err.Error(), http.StatusInternalServerError) + return + } + _, _ = w.Write(bb) +} + func getComponentHandler(host service.Host) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { - vars := mux.Vars(r) - requestedComponent := component.ParseID(vars["id"]) + getComponentHandlerInternal(host, w, r) + } +} - component, err := host.GetComponent(requestedComponent, component.InfoOptions{ - GetHealth: true, - GetArguments: true, - GetExports: true, - GetDebugInfo: true, - }) - if err != nil { - http.NotFound(w, r) +func getComponentHandlerRemoteCfg(host service.Host) http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + svc, found := host.GetService(remotecfg.ServiceName) + if !found { + http.Error(w, "remote config service not available", http.StatusInternalServerError) return } - bb, err := json.Marshal(component) - if err != nil { - http.Error(w, err.Error(), http.StatusInternalServerError) - return - } - _, _ = w.Write(bb) + getComponentHandlerInternal(svc.Data().(*remotecfg.Data).Host, w, r) + } +} + +func getComponentHandlerInternal(host service.Host, w http.ResponseWriter, r *http.Request) { + vars := mux.Vars(r) + requestedComponent := component.ParseID(vars["id"]) + + component, err := host.GetComponent(requestedComponent, component.InfoOptions{ + GetHealth: true, + GetArguments: true, + GetExports: true, + GetDebugInfo: true, + }) + if err != nil { + http.NotFound(w, r) + return + } + + bb, err := json.Marshal(component) + if err != nil { + http.Error(w, err.Error(), http.StatusInternalServerError) + return } + _, _ = w.Write(bb) } func getClusteringPeersHandler(host service.Host) http.HandlerFunc { From b73749445771cebb6b654cb0169fe776e5a36162 Mon Sep 17 00:00:00 2001 From: Sam DeHaan Date: Tue, 26 Nov 2024 09:52:19 -0500 Subject: [PATCH 2/3] Fix accidental cast to pointer that should have been struct --- internal/web/api/api.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/web/api/api.go b/internal/web/api/api.go index 746252cca6..4e364e6333 100644 --- a/internal/web/api/api.go +++ b/internal/web/api/api.go @@ -66,7 +66,7 @@ func listComponentsHandlerRemoteCfg(host service.Host) http.HandlerFunc { return } - listComponentsHandlerInternal(svc.Data().(*remotecfg.Data).Host, w, r) + listComponentsHandlerInternal(svc.Data().(remotecfg.Data).Host, w, r) } } @@ -108,7 +108,7 @@ func getComponentHandlerRemoteCfg(host service.Host) http.HandlerFunc { return } - getComponentHandlerInternal(svc.Data().(*remotecfg.Data).Host, w, r) + getComponentHandlerInternal(svc.Data().(remotecfg.Data).Host, w, r) } } From 8d02873f115c532156ad90bf230e85622f0e8561 Mon Sep 17 00:00:00 2001 From: Sam DeHaan Date: Tue, 26 Nov 2024 09:56:13 -0500 Subject: [PATCH 3/3] Update changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1c325e6043..41b0e8c7b9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,6 +39,8 @@ Main (unreleased) - Fixed a race condition that could lead to a deadlock when using `import` statements, which could lead to a memory leak on `/metrics` endpoint of an Alloy instance. (@thampiotr) +- Fix a race condition where the ui service was dependent on starting after the remotecfg service, which is not guaranteed. (@dehaansa & @erikbaranowski) + ### Other changes - Change the stability of the `livedebugging` feature from "experimental" to "generally available". (@wildum)