From 053c2039bb407a33d2af36609279213a6bc4d5f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Torkel=20=C3=96degaard?= Date: Tue, 1 May 2018 15:51:15 +0200 Subject: [PATCH 1/6] refactor: provisioning service refactoring --- pkg/api/http_server.go | 11 ++++- pkg/cmd/grafana-server/server.go | 8 +--- .../provisioning/dashboards/config_reader.go | 2 +- .../provisioning/dashboards/dashboard.go | 7 +--- pkg/services/provisioning/provisioning.go | 40 ++++++++++++------- pkg/setting/setting.go | 18 +++++---- 6 files changed, 49 insertions(+), 37 deletions(-) diff --git a/pkg/api/http_server.go b/pkg/api/http_server.go index 38858606579..f978a8c220c 100644 --- a/pkg/api/http_server.go +++ b/pkg/api/http_server.go @@ -26,9 +26,14 @@ import ( "github.com/grafana/grafana/pkg/middleware" "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/plugins" + "github.com/grafana/grafana/pkg/registry" "github.com/grafana/grafana/pkg/setting" ) +func init() { + registry.RegisterService(&HTTPServer{}) +} + type HTTPServer struct { log log.Logger macaron *macaron.Macaron @@ -41,12 +46,14 @@ type HTTPServer struct { Bus bus.Bus `inject:""` } -func (hs *HTTPServer) Init() { +func (hs *HTTPServer) Init() error { hs.log = log.New("http.server") hs.cache = gocache.New(5*time.Minute, 10*time.Minute) + + return nil } -func (hs *HTTPServer) Start(ctx context.Context) error { +func (hs *HTTPServer) Run(ctx context.Context) error { var err error hs.context = ctx diff --git a/pkg/cmd/grafana-server/server.go b/pkg/cmd/grafana-server/server.go index f20195b563a..05d10ea1afd 100644 --- a/pkg/cmd/grafana-server/server.go +++ b/pkg/cmd/grafana-server/server.go @@ -17,7 +17,6 @@ import ( "github.com/grafana/grafana/pkg/middleware" "github.com/grafana/grafana/pkg/registry" "github.com/grafana/grafana/pkg/services/dashboards" - "github.com/grafana/grafana/pkg/services/provisioning" "golang.org/x/sync/errgroup" @@ -37,6 +36,7 @@ import ( _ "github.com/grafana/grafana/pkg/services/alerting" _ "github.com/grafana/grafana/pkg/services/cleanup" _ "github.com/grafana/grafana/pkg/services/notifications" + _ "github.com/grafana/grafana/pkg/services/provisioning" _ "github.com/grafana/grafana/pkg/services/search" ) @@ -75,10 +75,6 @@ func (g *GrafanaServerImpl) Start() error { login.Init() social.NewOAuthService() - if err := provisioning.Init(g.context, setting.HomePath, g.cfg.Raw); err != nil { - return fmt.Errorf("Failed to provision Grafana from config. error: %v", err) - } - tracingCloser, err := tracing.Init(g.cfg.Raw) if err != nil { return fmt.Errorf("Tracing settings is not valid. error: %v", err) @@ -116,7 +112,7 @@ func (g *GrafanaServerImpl) Start() error { g.log.Info("Initializing " + reflect.TypeOf(service).Elem().Name()) if err := service.Init(); err != nil { - return fmt.Errorf("Service init failed %v", err) + return fmt.Errorf("Service init failed: %v", err) } } diff --git a/pkg/services/provisioning/dashboards/config_reader.go b/pkg/services/provisioning/dashboards/config_reader.go index 8ac79df0fac..4f9577f82db 100644 --- a/pkg/services/provisioning/dashboards/config_reader.go +++ b/pkg/services/provisioning/dashboards/config_reader.go @@ -69,7 +69,7 @@ func (cr *configReader) readConfig() ([]*DashboardsAsConfig, error) { parsedDashboards, err := cr.parseConfigs(file) if err != nil { - + return nil, err } if len(parsedDashboards) > 0 { diff --git a/pkg/services/provisioning/dashboards/dashboard.go b/pkg/services/provisioning/dashboards/dashboard.go index a5349517bbe..a856565bf01 100644 --- a/pkg/services/provisioning/dashboards/dashboard.go +++ b/pkg/services/provisioning/dashboards/dashboard.go @@ -10,19 +10,16 @@ import ( type DashboardProvisioner struct { cfgReader *configReader log log.Logger - ctx context.Context } -func Provision(ctx context.Context, configDirectory string) (*DashboardProvisioner, error) { +func NewDashboardProvisioner(configDirectory string) *DashboardProvisioner { log := log.New("provisioning.dashboard") d := &DashboardProvisioner{ cfgReader: &configReader{path: configDirectory, log: log}, log: log, - ctx: ctx, } - err := d.Provision(ctx) - return d, err + return d } func (provider *DashboardProvisioner) Provision(ctx context.Context) error { diff --git a/pkg/services/provisioning/provisioning.go b/pkg/services/provisioning/provisioning.go index 71385994bb5..9044ae97389 100644 --- a/pkg/services/provisioning/provisioning.go +++ b/pkg/services/provisioning/provisioning.go @@ -2,30 +2,40 @@ package provisioning import ( "context" + "fmt" "path" - "path/filepath" + "github.com/grafana/grafana/pkg/registry" "github.com/grafana/grafana/pkg/services/provisioning/dashboards" "github.com/grafana/grafana/pkg/services/provisioning/datasources" - ini "gopkg.in/ini.v1" + "github.com/grafana/grafana/pkg/setting" ) -func Init(ctx context.Context, homePath string, cfg *ini.File) error { - provisioningPath := makeAbsolute(cfg.Section("paths").Key("provisioning").String(), homePath) +func init() { + registry.RegisterService(&ProvisioningService{}) +} - datasourcePath := path.Join(provisioningPath, "datasources") +type ProvisioningService struct { + Cfg *setting.Cfg `inject:""` +} + +func (ps *ProvisioningService) Init() error { + datasourcePath := path.Join(ps.Cfg.ProvisioningPath, "datasources") if err := datasources.Provision(datasourcePath); err != nil { + return fmt.Errorf("Datasource provisioning error: %v", err) + } + + return nil +} + +func (ps *ProvisioningService) Run(ctx context.Context) error { + dashboardPath := path.Join(ps.Cfg.ProvisioningPath, "dashboards") + dashProvisioner := dashboards.NewDashboardProvisioner(dashboardPath) + + if err := dashProvisioner.Provision(ctx); err != nil { return err } - dashboardPath := path.Join(provisioningPath, "dashboards") - _, err := dashboards.Provision(ctx, dashboardPath) - return err -} - -func makeAbsolute(path string, root string) string { - if filepath.IsAbs(path) { - return path - } - return filepath.Join(root, path) + <-ctx.Done() + return ctx.Err() } diff --git a/pkg/setting/setting.go b/pkg/setting/setting.go index 40d7522f775..44d14e166d0 100644 --- a/pkg/setting/setting.go +++ b/pkg/setting/setting.go @@ -52,12 +52,11 @@ var ( ApplicationName string // Paths - LogsPath string - HomePath string - DataPath string - PluginsPath string - ProvisioningPath string - CustomInitPath = "conf/custom.ini" + LogsPath string + HomePath string + DataPath string + PluginsPath string + CustomInitPath = "conf/custom.ini" // Log settings. LogModes []string @@ -187,6 +186,9 @@ var ( type Cfg struct { Raw *ini.File + // Paths + ProvisioningPath string + // SMTP email settings Smtp SmtpSettings @@ -516,7 +518,7 @@ func (cfg *Cfg) Load(args *CommandLineArgs) error { Env = iniFile.Section("").Key("app_mode").MustString("development") InstanceName = iniFile.Section("").Key("instance_name").MustString("unknown_instance_name") PluginsPath = makeAbsolute(iniFile.Section("paths").Key("plugins").String(), HomePath) - ProvisioningPath = makeAbsolute(iniFile.Section("paths").Key("provisioning").String(), HomePath) + cfg.ProvisioningPath = makeAbsolute(iniFile.Section("paths").Key("provisioning").String(), HomePath) server := iniFile.Section("server") AppUrl, AppSubUrl = parseAppUrlAndSubUrl(server) @@ -719,6 +721,6 @@ func (cfg *Cfg) LogConfigSources() { logger.Info("Path Data", "path", DataPath) logger.Info("Path Logs", "path", LogsPath) logger.Info("Path Plugins", "path", PluginsPath) - logger.Info("Path Provisioning", "path", ProvisioningPath) + logger.Info("Path Provisioning", "path", cfg.ProvisioningPath) logger.Info("App mode " + Env) } From d04ad835e2d902a1d8fae33a871aa43befbbae04 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Torkel=20=C3=96degaard?= Date: Wed, 2 May 2018 14:14:42 +0200 Subject: [PATCH 2/6] refactoring: lots of refactoring around server shutdown flows, making sure process is terminated when background service has crashed --- pkg/api/http_server.go | 9 +------ pkg/cmd/grafana-server/main.go | 31 ++++++--------------- pkg/cmd/grafana-server/server.go | 46 +++++++++++++++++--------------- 3 files changed, 33 insertions(+), 53 deletions(-) diff --git a/pkg/api/http_server.go b/pkg/api/http_server.go index f978a8c220c..2cf8f3796ae 100644 --- a/pkg/api/http_server.go +++ b/pkg/api/http_server.go @@ -64,7 +64,7 @@ func (hs *HTTPServer) Run(ctx context.Context) error { hs.streamManager.Run(ctx) listenAddr := fmt.Sprintf("%s:%s", setting.HttpAddr, setting.HttpPort) - hs.log.Info("Initializing HTTP Server", "address", listenAddr, "protocol", setting.Protocol, "subUrl", setting.AppSubUrl, "socket", setting.SocketPath) + hs.log.Info("HTTP Server Listen", "address", listenAddr, "protocol", setting.Protocol, "subUrl", setting.AppSubUrl, "socket", setting.SocketPath) hs.httpSrv = &http.Server{Addr: listenAddr, Handler: hs.macaron} @@ -74,7 +74,6 @@ func (hs *HTTPServer) Run(ctx context.Context) error { if err := hs.httpSrv.Shutdown(context.Background()); err != nil { hs.log.Error("Failed to shutdown server", "error", err) } - hs.log.Info("Stopped HTTP Server") }() switch setting.Protocol { @@ -113,12 +112,6 @@ func (hs *HTTPServer) Run(ctx context.Context) error { return err } -func (hs *HTTPServer) Shutdown(ctx context.Context) error { - err := hs.httpSrv.Shutdown(ctx) - hs.log.Info("Stopped HTTP server") - return err -} - func (hs *HTTPServer) listenAndServeTLS(certfile, keyfile string) error { if certfile == "" { return fmt.Errorf("cert_file cannot be empty when using HTTPS") diff --git a/pkg/cmd/grafana-server/main.go b/pkg/cmd/grafana-server/main.go index 466e97ff2d6..23cac74ccdb 100644 --- a/pkg/cmd/grafana-server/main.go +++ b/pkg/cmd/grafana-server/main.go @@ -81,29 +81,20 @@ func main() { setting.Enterprise, _ = strconv.ParseBool(enterprise) metrics.M_Grafana_Version.WithLabelValues(version).Set(1) - shutdownCompleted := make(chan int) + server := NewGrafanaServer() - go listenToSystemSignals(server, shutdownCompleted) + go listenToSystemSignals(server) - go func() { - code := 0 - if err := server.Start(); err != nil { - log.Error2("Startup failed", "error", err) - code = 1 - } + err := server.Start() - exitChan <- code - }() - - code := <-shutdownCompleted - log.Info2("Grafana shutdown completed.", "code", code) + trace.Stop() log.Close() - os.Exit(code) + + server.Exit(err) } -func listenToSystemSignals(server *GrafanaServerImpl, shutdownCompleted chan int) { - var code int +func listenToSystemSignals(server *GrafanaServerImpl) { signalChan := make(chan os.Signal, 1) ignoreChan := make(chan os.Signal, 1) @@ -112,12 +103,6 @@ func listenToSystemSignals(server *GrafanaServerImpl, shutdownCompleted chan int select { case sig := <-signalChan: - trace.Stop() // Stops trace if profiling has been enabled - server.Shutdown(0, fmt.Sprintf("system signal: %s", sig)) - shutdownCompleted <- 0 - case code = <-exitChan: - trace.Stop() // Stops trace if profiling has been enabled - server.Shutdown(code, "startup error") - shutdownCompleted <- code + server.Shutdown(fmt.Sprintf("System signal: %s", sig)) } } diff --git a/pkg/cmd/grafana-server/server.go b/pkg/cmd/grafana-server/server.go index 05d10ea1afd..aff40111ae9 100644 --- a/pkg/cmd/grafana-server/server.go +++ b/pkg/cmd/grafana-server/server.go @@ -54,11 +54,12 @@ func NewGrafanaServer() *GrafanaServerImpl { } type GrafanaServerImpl struct { - context context.Context - shutdownFn context.CancelFunc - childRoutines *errgroup.Group - log log.Logger - cfg *setting.Cfg + context context.Context + shutdownFn context.CancelFunc + childRoutines *errgroup.Group + log log.Logger + cfg *setting.Cfg + shutdownReason string RouteRegister api.RouteRegister `inject:""` HttpServer *api.HTTPServer `inject:""` @@ -135,7 +136,8 @@ func (g *GrafanaServerImpl) Start() error { } sendSystemdNotification("READY=1") - return g.startHttpServer() + + return g.childRoutines.Wait() } func (g *GrafanaServerImpl) loadConfiguration() { @@ -154,28 +156,28 @@ func (g *GrafanaServerImpl) loadConfiguration() { g.cfg.LogConfigSources() } -func (g *GrafanaServerImpl) startHttpServer() error { - g.HttpServer.Init() - - err := g.HttpServer.Start(g.context) - - if err != nil { - return fmt.Errorf("Fail to start server. error: %v", err) - } - - return nil -} - -func (g *GrafanaServerImpl) Shutdown(code int, reason string) { - g.log.Info("Shutdown started", "code", code, "reason", reason) +func (g *GrafanaServerImpl) Shutdown(reason string) { + g.log.Info("Shutdown started", "reason", reason) + g.shutdownReason = reason // call cancel func on root context g.shutdownFn() // wait for child routines - if err := g.childRoutines.Wait(); err != nil && err != context.Canceled { - g.log.Error("Server shutdown completed", "error", err) + g.childRoutines.Wait() +} + +func (g *GrafanaServerImpl) Exit(reason error) { + // default exit code is 1 + code := 1 + + if reason == context.Canceled && g.shutdownReason != "" { + reason = fmt.Errorf(g.shutdownReason) + code = 0 } + + g.log.Error("Server shutdown", "reason", reason) + os.Exit(code) } func (g *GrafanaServerImpl) writePIDFile() { From 23655315b877c18d0a73e0669e4b8cfea39a2255 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Torkel=20=C3=96degaard?= Date: Wed, 2 May 2018 18:10:21 +0200 Subject: [PATCH 3/6] fix: fixed race condition between http.Server ListenAndServe & Shutdown, now service crash during startup correctly closes http server every time --- pkg/api/http_server.go | 2 ++ pkg/cmd/grafana-server/main.go | 2 +- pkg/cmd/grafana-server/server.go | 34 ++++++++++++++++++++++++-------- 3 files changed, 29 insertions(+), 9 deletions(-) diff --git a/pkg/api/http_server.go b/pkg/api/http_server.go index 2cf8f3796ae..f30e5602484 100644 --- a/pkg/api/http_server.go +++ b/pkg/api/http_server.go @@ -71,6 +71,8 @@ func (hs *HTTPServer) Run(ctx context.Context) error { // handle http shutdown on server context done go func() { <-ctx.Done() + // Hacky fix for race condition between ListenAndServe and Shutdown + time.Sleep(time.Millisecond * 100) if err := hs.httpSrv.Shutdown(context.Background()); err != nil { hs.log.Error("Failed to shutdown server", "error", err) } diff --git a/pkg/cmd/grafana-server/main.go b/pkg/cmd/grafana-server/main.go index 23cac74ccdb..02ea7a7f8f1 100644 --- a/pkg/cmd/grafana-server/main.go +++ b/pkg/cmd/grafana-server/main.go @@ -86,7 +86,7 @@ func main() { go listenToSystemSignals(server) - err := server.Start() + err := server.Run() trace.Stop() log.Close() diff --git a/pkg/cmd/grafana-server/server.go b/pkg/cmd/grafana-server/server.go index aff40111ae9..43f6bed1646 100644 --- a/pkg/cmd/grafana-server/server.go +++ b/pkg/cmd/grafana-server/server.go @@ -54,18 +54,19 @@ func NewGrafanaServer() *GrafanaServerImpl { } type GrafanaServerImpl struct { - context context.Context - shutdownFn context.CancelFunc - childRoutines *errgroup.Group - log log.Logger - cfg *setting.Cfg - shutdownReason string + context context.Context + shutdownFn context.CancelFunc + childRoutines *errgroup.Group + log log.Logger + cfg *setting.Cfg + shutdownReason string + shutdownInProgress bool RouteRegister api.RouteRegister `inject:""` HttpServer *api.HTTPServer `inject:""` } -func (g *GrafanaServerImpl) Start() error { +func (g *GrafanaServerImpl) Run() error { g.loadConfiguration() g.writePIDFile() @@ -129,8 +130,24 @@ func (g *GrafanaServerImpl) Start() error { } g.childRoutines.Go(func() error { + // Skip starting new service is we are shutting down + // Ccan happen when service crash during startup + if g.shutdownInProgress { + return nil + } + err := service.Run(g.context) - g.log.Info("Stopped "+reflect.TypeOf(service).Elem().Name(), "reason", err) + + // If error is not canceled then the service crashed + if err != context.Canceled { + g.log.Error("Stopped "+reflect.TypeOf(service).Elem().Name(), "reason", err) + } else { + g.log.Info("Stopped "+reflect.TypeOf(service).Elem().Name(), "reason", err) + } + + // Mark that we are in shutdown mode + // So more services are not started + g.shutdownInProgress = true return err }) } @@ -159,6 +176,7 @@ func (g *GrafanaServerImpl) loadConfiguration() { func (g *GrafanaServerImpl) Shutdown(reason string) { g.log.Info("Shutdown started", "reason", reason) g.shutdownReason = reason + g.shutdownInProgress = true // call cancel func on root context g.shutdownFn() From e3ea6c683c928bcb84c5710b447b4b8f1ec087ae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Torkel=20=C3=96degaard?= Date: Wed, 2 May 2018 18:27:54 +0200 Subject: [PATCH 4/6] fix: comment spell fix --- pkg/cmd/grafana-server/server.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/cmd/grafana-server/server.go b/pkg/cmd/grafana-server/server.go index 43f6bed1646..e03018a4167 100644 --- a/pkg/cmd/grafana-server/server.go +++ b/pkg/cmd/grafana-server/server.go @@ -130,8 +130,8 @@ func (g *GrafanaServerImpl) Run() error { } g.childRoutines.Go(func() error { - // Skip starting new service is we are shutting down - // Ccan happen when service crash during startup + // Skip starting new service when shutting down + // Can happen when service stop/return during startup if g.shutdownInProgress { return nil } From b5e70d4607996120e78eca384ce5eb3643b6ff04 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Torkel=20=C3=96degaard?= Date: Wed, 2 May 2018 19:00:16 +0200 Subject: [PATCH 5/6] fix: removed unused channel --- pkg/cmd/grafana-server/main.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/cmd/grafana-server/main.go b/pkg/cmd/grafana-server/main.go index 02ea7a7f8f1..c7ea6bb432b 100644 --- a/pkg/cmd/grafana-server/main.go +++ b/pkg/cmd/grafana-server/main.go @@ -39,7 +39,6 @@ var enterprise string var configFile = flag.String("config", "", "path to config file") var homePath = flag.String("homepath", "", "path to grafana install/home path, defaults to working directory") var pidFile = flag.String("pidfile", "", "path to pid file") -var exitChan = make(chan int) func main() { v := flag.Bool("v", false, "prints current version and exits") From c40a50829d622f38c51462e13daec5d92bd75326 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Torkel=20=C3=96degaard?= Date: Wed, 2 May 2018 21:30:15 +0200 Subject: [PATCH 6/6] fix: removed manully added http server from inject graph as it is now a self registered service --- pkg/cmd/grafana-server/server.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/cmd/grafana-server/server.go b/pkg/cmd/grafana-server/server.go index e03018a4167..0f364076a15 100644 --- a/pkg/cmd/grafana-server/server.go +++ b/pkg/cmd/grafana-server/server.go @@ -88,7 +88,6 @@ func (g *GrafanaServerImpl) Run() error { serviceGraph.Provide(&inject.Object{Value: g.cfg}) serviceGraph.Provide(&inject.Object{Value: dashboards.NewProvisioningService()}) serviceGraph.Provide(&inject.Object{Value: api.NewRouteRegister(middleware.RequestMetrics, middleware.RequestTracing)}) - serviceGraph.Provide(&inject.Object{Value: api.HTTPServer{}}) // self registered services services := registry.GetServices()