From 74ddc06ce1c649ba97221524cbf92cf8a61eaf49 Mon Sep 17 00:00:00 2001 From: Ben Hoyt Date: Thu, 28 Apr 2022 17:11:20 +1200 Subject: [PATCH 1/4] Add "start-time" field to services API Shows as follows in the CLI: $ pebble services Service Startup Current Start Time test2 enabled active today at 17:08 NZST $ pebble services --abs-time Service Startup Current Start Time test2 enabled active 2022-04-28T17:08:45+12:00 --- client/services.go | 8 +++++--- cmd/pebble/cmd_services.go | 13 ++++++++++--- cmd/pebble/cmd_services_test.go | 18 +++++++++--------- internal/daemon/api_services.go | 11 ++++++++--- internal/overlord/servstate/handlers.go | 3 +++ internal/overlord/servstate/manager.go | 10 ++++++---- internal/overlord/servstate/manager_test.go | 3 +++ 7 files changed, 44 insertions(+), 22 deletions(-) diff --git a/client/services.go b/client/services.go index 09e8ddf7..7358a2bc 100644 --- a/client/services.go +++ b/client/services.go @@ -20,6 +20,7 @@ import ( "fmt" "net/url" "strings" + "time" ) type ServiceOptions struct { @@ -80,9 +81,10 @@ type ServicesOptions struct { // ServiceInfo holds status information for a single service. type ServiceInfo struct { - Name string `json:"name"` - Startup ServiceStartup `json:"startup"` - Current ServiceStatus `json:"current"` + Name string `json:"name"` + Startup ServiceStartup `json:"startup"` + Current ServiceStatus `json:"current"` + StartTime time.Time `json:"start-time"` } // ServiceStartup defines the different startup modes for a service. diff --git a/cmd/pebble/cmd_services.go b/cmd/pebble/cmd_services.go index 20f8c17e..3189a778 100644 --- a/cmd/pebble/cmd_services.go +++ b/cmd/pebble/cmd_services.go @@ -24,6 +24,7 @@ import ( type cmdServices struct { clientMixin + timeMixin Positional struct { Services []string `positional-arg-name:""` } `positional-args:"yes"` @@ -59,14 +60,20 @@ func (cmd *cmdServices) Execute(args []string) error { w := tabWriter() defer w.Flush() - fmt.Fprintln(w, "Service\tStartup\tCurrent") + fmt.Fprintln(w, "Service\tStartup\tCurrent\tStart Time") for _, svc := range services { - fmt.Fprintf(w, "%s\t%s\t%s\n", svc.Name, svc.Startup, svc.Current) + startTime := "-" + if !svc.StartTime.IsZero() { + startTime = cmd.fmtTime(svc.StartTime) + } + fmt.Fprintf(w, "%s\t%s\t%s\t%s\n", svc.Name, svc.Startup, svc.Current, startTime) } return nil } func init() { - addCommand("services", shortServicesHelp, longServicesHelp, func() flags.Commander { return &cmdServices{} }, nil, nil) + addCommand("services", shortServicesHelp, longServicesHelp, + func() flags.Commander { return &cmdServices{} }, + timeDescs, nil) } diff --git a/cmd/pebble/cmd_services_test.go b/cmd/pebble/cmd_services_test.go index fa691cff..c59cffb0 100644 --- a/cmd/pebble/cmd_services_test.go +++ b/cmd/pebble/cmd_services_test.go @@ -43,10 +43,10 @@ func (s *PebbleSuite) TestServices(c *check.C) { c.Assert(err, check.IsNil) c.Assert(rest, check.HasLen, 0) c.Check(s.Stdout(), check.Equals, ` -Service Startup Current -svc1 enabled inactive -svc2 enabled inactive -svc3 enabled backoff +Service Startup Current Start Time +svc1 enabled inactive - +svc2 enabled inactive - +svc3 enabled backoff - `[1:]) c.Check(s.Stderr(), check.Equals, "") } @@ -60,18 +60,18 @@ func (s *PebbleSuite) TestServicesNames(c *check.C) { "type": "sync", "status-code": 200, "result": [ - {"name": "bar", "current": "active", "startup": "disabled"}, + {"name": "bar", "current": "active", "startup": "disabled", "start-time": "2022-04-28T17:05:23+12:00"}, {"name": "foo", "current": "inactive", "startup": "enabled"} ] }`) }) - rest, err := pebble.Parser(pebble.Client()).ParseArgs([]string{"services", "foo", "bar"}) + rest, err := pebble.Parser(pebble.Client()).ParseArgs([]string{"services", "foo", "bar", "--abs-time"}) c.Assert(err, check.IsNil) c.Assert(rest, check.HasLen, 0) c.Check(s.Stdout(), check.Equals, ` -Service Startup Current -bar disabled active -foo enabled inactive +Service Startup Current Start Time +bar disabled active 2022-04-28T17:05:23+12:00 +foo enabled inactive - `[1:]) c.Check(s.Stderr(), check.Equals, "") } diff --git a/internal/daemon/api_services.go b/internal/daemon/api_services.go index 25308de2..b5276477 100644 --- a/internal/daemon/api_services.go +++ b/internal/daemon/api_services.go @@ -20,6 +20,7 @@ import ( "net/http" "sort" "strings" + "time" "github.com/canonical/pebble/internal/overlord/servstate" "github.com/canonical/pebble/internal/overlord/state" @@ -27,9 +28,10 @@ import ( ) type serviceInfo struct { - Name string `json:"name"` - Startup string `json:"startup"` - Current string `json:"current"` + Name string `json:"name"` + Startup string `json:"startup"` + Current string `json:"current"` + StartTime *time.Time `json:"start-time,omitempty"` // pointer as omitempty doesn't work with time.Time directly } func v1GetServices(c *Command, r *http.Request, _ *userState) Response { @@ -48,6 +50,9 @@ func v1GetServices(c *Command, r *http.Request, _ *userState) Response { Startup: string(svc.Startup), Current: string(svc.Current), } + if !svc.StartTime.IsZero() { + info.StartTime = &svc.StartTime + } infos = append(infos, info) } return SyncResponse(infos) diff --git a/internal/overlord/servstate/handlers.go b/internal/overlord/servstate/handlers.go index bbf81701..933d8be5 100644 --- a/internal/overlord/servstate/handlers.go +++ b/internal/overlord/servstate/handlers.go @@ -92,6 +92,7 @@ type serviceData struct { backoffTime time.Duration resetTimer *time.Timer restarting bool + startTime time.Time restarts int } @@ -366,6 +367,7 @@ func (s *serviceData) startInternal() error { return fmt.Errorf("cannot start service: %w", err) } logger.Debugf("Service %q started with PID %d", serviceName, s.cmd.Process.Pid) + s.startTime = time.Now() s.resetTimer = time.AfterFunc(s.config.BackoffLimit.Value, func() { logError(s.backoffResetElapsed()) }) // Start a goroutine to wait for the process to finish. @@ -424,6 +426,7 @@ func (s *serviceData) exited(exitCode int) error { s.manager.servicesLock.Lock() defer s.manager.servicesLock.Unlock() + s.startTime = time.Time{} if s.resetTimer != nil { s.resetTimer.Stop() } diff --git a/internal/overlord/servstate/manager.go b/internal/overlord/servstate/manager.go index d1ba8a70..2e22d846 100644 --- a/internal/overlord/servstate/manager.go +++ b/internal/overlord/servstate/manager.go @@ -235,10 +235,11 @@ func (m *ServiceManager) Ensure() error { } type ServiceInfo struct { - Name string - Startup ServiceStartup - Current ServiceStatus - Restarts int + Name string + Startup ServiceStartup + Current ServiceStatus + StartTime time.Time + Restarts int } type ServiceStartup string @@ -300,6 +301,7 @@ func (m *ServiceManager) Services(names []string) ([]*ServiceInfo, error) { default: info.Current = StatusError } + info.StartTime = s.startTime info.Restarts = s.restarts } services = append(services, info) diff --git a/internal/overlord/servstate/manager_test.go b/internal/overlord/servstate/manager_test.go index 3455c1c6..242ab447 100644 --- a/internal/overlord/servstate/manager_test.go +++ b/internal/overlord/servstate/manager_test.go @@ -753,6 +753,7 @@ services: } func (s *S) TestServices(c *C) { + started := time.Now() services, err := s.manager.Services(nil) c.Assert(err, IsNil) c.Assert(services, DeepEquals, []*servstate.ServiceInfo{ @@ -775,6 +776,8 @@ func (s *S) TestServices(c *C) { services, err = s.manager.Services(nil) c.Assert(err, IsNil) + c.Assert(services[1].StartTime.After(started) && services[1].StartTime.Before(started.Add(5*time.Second)), Equals, true) + services[1].StartTime = time.Time{} c.Assert(services, DeepEquals, []*servstate.ServiceInfo{ {Name: "test1", Current: servstate.StatusInactive, Startup: servstate.StartupEnabled}, {Name: "test2", Current: servstate.StatusActive, Startup: servstate.StartupDisabled}, From 482091b38a627eb42a2b2a27b05305a2b6e37211 Mon Sep 17 00:00:00 2001 From: Ben Hoyt Date: Thu, 28 Apr 2022 17:12:48 +1200 Subject: [PATCH 2/4] Remove unused "restarts" field (was internal only) --- internal/overlord/servstate/handlers.go | 2 -- internal/overlord/servstate/manager.go | 1 - 2 files changed, 3 deletions(-) diff --git a/internal/overlord/servstate/handlers.go b/internal/overlord/servstate/handlers.go index 933d8be5..fce67886 100644 --- a/internal/overlord/servstate/handlers.go +++ b/internal/overlord/servstate/handlers.go @@ -93,7 +93,6 @@ type serviceData struct { resetTimer *time.Timer restarting bool startTime time.Time - restarts int } func (m *ServiceManager) doStart(task *state.Task, tomb *tomb.Tomb) error { @@ -600,7 +599,6 @@ func (s *serviceData) backoffTimeElapsed() error { switch s.state { case stateBackoff: - s.restarts++ err := s.startInternal() if err != nil { return err diff --git a/internal/overlord/servstate/manager.go b/internal/overlord/servstate/manager.go index 2e22d846..8a10f0bd 100644 --- a/internal/overlord/servstate/manager.go +++ b/internal/overlord/servstate/manager.go @@ -302,7 +302,6 @@ func (m *ServiceManager) Services(names []string) ([]*ServiceInfo, error) { info.Current = StatusError } info.StartTime = s.startTime - info.Restarts = s.restarts } services = append(services, info) } From 389a8d0ffdd12eb4c098cf02b0ec74ad67993dca Mon Sep 17 00:00:00 2001 From: Ben Hoyt Date: Thu, 22 Sep 2022 17:35:49 +1200 Subject: [PATCH 3/4] Change start-time to current-since per discussion with Gustavo --- client/services.go | 8 ++--- client/services_test.go | 5 +-- cmd/pebble/cmd_services.go | 10 +++--- cmd/pebble/cmd_services_test.go | 6 ++-- internal/daemon/api_services.go | 12 +++---- internal/overlord/servstate/handlers.go | 33 +++++++++++-------- internal/overlord/servstate/manager.go | 36 +++++++++++---------- internal/overlord/servstate/manager_test.go | 4 +-- 8 files changed, 61 insertions(+), 53 deletions(-) diff --git a/client/services.go b/client/services.go index 7358a2bc..35c9f997 100644 --- a/client/services.go +++ b/client/services.go @@ -81,10 +81,10 @@ type ServicesOptions struct { // ServiceInfo holds status information for a single service. type ServiceInfo struct { - Name string `json:"name"` - Startup ServiceStartup `json:"startup"` - Current ServiceStatus `json:"current"` - StartTime time.Time `json:"start-time"` + Name string `json:"name"` + Startup ServiceStartup `json:"startup"` + Current ServiceStatus `json:"current"` + CurrentSince time.Time `json:"current-since"` } // ServiceStartup defines the different startup modes for a service. diff --git a/client/services_test.go b/client/services_test.go index 9d246464..51eec13a 100644 --- a/client/services_test.go +++ b/client/services_test.go @@ -17,6 +17,7 @@ package client_test import ( "encoding/json" "net/url" + "time" "gopkg.in/check.v1" @@ -87,7 +88,7 @@ func (cs *clientSuite) TestServicesGet(c *check.C) { cs.rsp = `{ "result": [ {"name": "svc1", "startup": "enabled", "current": "inactive"}, - {"name": "svc2", "startup": "disabled", "current": "active"} + {"name": "svc2", "startup": "disabled", "current": "active", "current-since": "2022-04-28T17:05:23Z"} ], "status": "OK", "status-code": 200, @@ -101,7 +102,7 @@ func (cs *clientSuite) TestServicesGet(c *check.C) { c.Assert(err, check.IsNil) c.Assert(services, check.DeepEquals, []*client.ServiceInfo{ {Name: "svc1", Startup: client.StartupEnabled, Current: client.StatusInactive}, - {Name: "svc2", Startup: client.StartupDisabled, Current: client.StatusActive}, + {Name: "svc2", Startup: client.StartupDisabled, Current: client.StatusActive, CurrentSince: time.Date(2022, 4, 28, 17, 5, 23, 0, time.UTC)}, }) c.Assert(cs.req.Method, check.Equals, "GET") c.Assert(cs.req.URL.Path, check.Equals, "/v1/services") diff --git a/cmd/pebble/cmd_services.go b/cmd/pebble/cmd_services.go index 3189a778..a4a927f9 100644 --- a/cmd/pebble/cmd_services.go +++ b/cmd/pebble/cmd_services.go @@ -60,14 +60,14 @@ func (cmd *cmdServices) Execute(args []string) error { w := tabWriter() defer w.Flush() - fmt.Fprintln(w, "Service\tStartup\tCurrent\tStart Time") + fmt.Fprintln(w, "Service\tStartup\tCurrent\tSince") for _, svc := range services { - startTime := "-" - if !svc.StartTime.IsZero() { - startTime = cmd.fmtTime(svc.StartTime) + since := "-" + if !svc.CurrentSince.IsZero() { + since = cmd.fmtTime(svc.CurrentSince) } - fmt.Fprintf(w, "%s\t%s\t%s\t%s\n", svc.Name, svc.Startup, svc.Current, startTime) + fmt.Fprintf(w, "%s\t%s\t%s\t%s\n", svc.Name, svc.Startup, svc.Current, since) } return nil } diff --git a/cmd/pebble/cmd_services_test.go b/cmd/pebble/cmd_services_test.go index c59cffb0..96b9961e 100644 --- a/cmd/pebble/cmd_services_test.go +++ b/cmd/pebble/cmd_services_test.go @@ -43,7 +43,7 @@ func (s *PebbleSuite) TestServices(c *check.C) { c.Assert(err, check.IsNil) c.Assert(rest, check.HasLen, 0) c.Check(s.Stdout(), check.Equals, ` -Service Startup Current Start Time +Service Startup Current Since svc1 enabled inactive - svc2 enabled inactive - svc3 enabled backoff - @@ -60,7 +60,7 @@ func (s *PebbleSuite) TestServicesNames(c *check.C) { "type": "sync", "status-code": 200, "result": [ - {"name": "bar", "current": "active", "startup": "disabled", "start-time": "2022-04-28T17:05:23+12:00"}, + {"name": "bar", "current": "active", "startup": "disabled", "current-since": "2022-04-28T17:05:23+12:00"}, {"name": "foo", "current": "inactive", "startup": "enabled"} ] }`) @@ -69,7 +69,7 @@ func (s *PebbleSuite) TestServicesNames(c *check.C) { c.Assert(err, check.IsNil) c.Assert(rest, check.HasLen, 0) c.Check(s.Stdout(), check.Equals, ` -Service Startup Current Start Time +Service Startup Current Since bar disabled active 2022-04-28T17:05:23+12:00 foo enabled inactive - `[1:]) diff --git a/internal/daemon/api_services.go b/internal/daemon/api_services.go index b5276477..4df44e36 100644 --- a/internal/daemon/api_services.go +++ b/internal/daemon/api_services.go @@ -28,10 +28,10 @@ import ( ) type serviceInfo struct { - Name string `json:"name"` - Startup string `json:"startup"` - Current string `json:"current"` - StartTime *time.Time `json:"start-time,omitempty"` // pointer as omitempty doesn't work with time.Time directly + Name string `json:"name"` + Startup string `json:"startup"` + Current string `json:"current"` + CurrentSince *time.Time `json:"current-since,omitempty"` // pointer as omitempty doesn't work with time.Time directly } func v1GetServices(c *Command, r *http.Request, _ *userState) Response { @@ -50,8 +50,8 @@ func v1GetServices(c *Command, r *http.Request, _ *userState) Response { Startup: string(svc.Startup), Current: string(svc.Current), } - if !svc.StartTime.IsZero() { - info.StartTime = &svc.StartTime + if !svc.CurrentSince.IsZero() { + info.CurrentSince = &svc.CurrentSince } infos = append(infos, info) } diff --git a/internal/overlord/servstate/handlers.go b/internal/overlord/servstate/handlers.go index fce67886..f1d74139 100644 --- a/internal/overlord/servstate/handlers.go +++ b/internal/overlord/servstate/handlers.go @@ -81,18 +81,18 @@ const ( // serviceData holds the state and other data for a service under our control. type serviceData struct { - manager *ServiceManager - state serviceState - config *plan.Service - logs *servicelog.RingBuffer - started chan error - stopped chan error - cmd *exec.Cmd - backoffNum int - backoffTime time.Duration - resetTimer *time.Timer - restarting bool - startTime time.Time + manager *ServiceManager + state serviceState + config *plan.Service + logs *servicelog.RingBuffer + started chan error + stopped chan error + cmd *exec.Cmd + backoffNum int + backoffTime time.Duration + resetTimer *time.Timer + restarting bool + currentSince time.Time } func (m *ServiceManager) doStart(task *state.Task, tomb *tomb.Tomb) error { @@ -282,6 +282,13 @@ func (s *serviceData) transition(state serviceState) { // transitionRestarting changes the service's state and also sets the restarting flag. func (s *serviceData) transitionRestarting(state serviceState, restarting bool) { + // Update current-since time if derived status is changing. + oldStatus := stateToStatus(s.state) + newStatus := stateToStatus(state) + if oldStatus != newStatus { + s.currentSince = time.Now() + } + s.state = state s.restarting = restarting } @@ -366,7 +373,6 @@ func (s *serviceData) startInternal() error { return fmt.Errorf("cannot start service: %w", err) } logger.Debugf("Service %q started with PID %d", serviceName, s.cmd.Process.Pid) - s.startTime = time.Now() s.resetTimer = time.AfterFunc(s.config.BackoffLimit.Value, func() { logError(s.backoffResetElapsed()) }) // Start a goroutine to wait for the process to finish. @@ -425,7 +431,6 @@ func (s *serviceData) exited(exitCode int) error { s.manager.servicesLock.Lock() defer s.manager.servicesLock.Unlock() - s.startTime = time.Time{} if s.resetTimer != nil { s.resetTimer.Stop() } diff --git a/internal/overlord/servstate/manager.go b/internal/overlord/servstate/manager.go index 8a10f0bd..f16e68b7 100644 --- a/internal/overlord/servstate/manager.go +++ b/internal/overlord/servstate/manager.go @@ -235,11 +235,10 @@ func (m *ServiceManager) Ensure() error { } type ServiceInfo struct { - Name string - Startup ServiceStartup - Current ServiceStatus - StartTime time.Time - Restarts int + Name string + Startup ServiceStartup + Current ServiceStatus + CurrentSince time.Time } type ServiceStartup string @@ -290,18 +289,8 @@ func (m *ServiceManager) Services(names []string) ([]*ServiceInfo, error) { info.Startup = StartupEnabled } if s, ok := m.services[name]; ok { - switch s.state { - case stateInitial, stateStarting, stateRunning: - info.Current = StatusActive - case stateTerminating, stateKilling, stateStopped: - // Already set to inactive above, but it's nice to be explicit for each state - info.Current = StatusInactive - case stateBackoff: - info.Current = StatusBackoff - default: - info.Current = StatusError - } - info.StartTime = s.startTime + info.Current = stateToStatus(s.state) + info.CurrentSince = s.currentSince } services = append(services, info) } @@ -311,6 +300,19 @@ func (m *ServiceManager) Services(names []string) ([]*ServiceInfo, error) { return services, nil } +func stateToStatus(state serviceState) ServiceStatus { + switch state { + case stateStarting, stateRunning: + return StatusActive + case stateTerminating, stateKilling, stateStopped: + return StatusInactive + case stateBackoff: + return StatusBackoff + default: // stateInitial (should never happen) and stateExited + return StatusError + } +} + // DefaultServiceNames returns the name of the services set to start // by default. func (m *ServiceManager) DefaultServiceNames() ([]string, error) { diff --git a/internal/overlord/servstate/manager_test.go b/internal/overlord/servstate/manager_test.go index 242ab447..b4b4f4d4 100644 --- a/internal/overlord/servstate/manager_test.go +++ b/internal/overlord/servstate/manager_test.go @@ -776,8 +776,8 @@ func (s *S) TestServices(c *C) { services, err = s.manager.Services(nil) c.Assert(err, IsNil) - c.Assert(services[1].StartTime.After(started) && services[1].StartTime.Before(started.Add(5*time.Second)), Equals, true) - services[1].StartTime = time.Time{} + c.Assert(services[1].CurrentSince.After(started) && services[1].CurrentSince.Before(started.Add(5*time.Second)), Equals, true) + services[1].CurrentSince = time.Time{} c.Assert(services, DeepEquals, []*servstate.ServiceInfo{ {Name: "test1", Current: servstate.StatusInactive, Startup: servstate.StartupEnabled}, {Name: "test2", Current: servstate.StatusActive, Startup: servstate.StartupDisabled}, From 17a32c7d0b11a01fcee913d7ff8e0a5efdf91c02 Mon Sep 17 00:00:00 2001 From: Ben Hoyt Date: Thu, 22 Sep 2022 17:48:04 +1200 Subject: [PATCH 4/4] Tweak cmd test to handle non-abs-time case --- cmd/pebble/cmd_services_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/pebble/cmd_services_test.go b/cmd/pebble/cmd_services_test.go index 96b9961e..84e97d8f 100644 --- a/cmd/pebble/cmd_services_test.go +++ b/cmd/pebble/cmd_services_test.go @@ -33,7 +33,7 @@ func (s *PebbleSuite) TestServices(c *check.C) { "type": "sync", "status-code": 200, "result": [ - {"name": "svc1", "current": "inactive", "startup": "enabled"}, + {"name": "svc1", "current": "inactive", "startup": "enabled", "current-since": "2022-04-28T17:05:23+12:00"}, {"name": "svc2", "current": "inactive", "startup": "enabled"}, {"name": "svc3", "current": "backoff", "startup": "enabled"} ] @@ -44,7 +44,7 @@ func (s *PebbleSuite) TestServices(c *check.C) { c.Assert(rest, check.HasLen, 0) c.Check(s.Stdout(), check.Equals, ` Service Startup Current Since -svc1 enabled inactive - +svc1 enabled inactive 2022-04-28 svc2 enabled inactive - svc3 enabled backoff - `[1:])