Minor tweaks from review.

client/checks.go

@@ -39,8 +39,8 @@ type ChecksActionOptions struct {
 	Names []string
 }
 
-// CheckActionResults holds the results of a check action.
-type CheckActionResults struct {
+// ChecksActionResult holds the results of a check action.
+type ChecksActionResult struct {
 	Changed []string `json:"changed"`
 }
 
@@ -129,24 +129,22 @@ func (client *Client) Checks(opts *ChecksOptions) ([]*CheckInfo, error) {
 	return checks, nil
 }
 
-// Start starts the checks named in opts.Names.
-func (client *Client) StartChecks(opts *ChecksActionOptions) (results *CheckActionResults, err error) {
-	results, err = client.doMultiCheckAction("start", opts.Names)
-	return results, err
+// StartChecks starts the checks named in opts.Names.
+func (client *Client) StartChecks(opts *ChecksActionOptions) (*ChecksActionResult, error) {
+	return client.doMultiCheckAction("start", opts.Names)
 }
 
-// Stop stops the checks named in opts.Names.
-func (client *Client) StopChecks(opts *ChecksActionOptions) (results *CheckActionResults, err error) {
-	results, err = client.doMultiCheckAction("stop", opts.Names)
-	return results, err
+// StopChecks stops the checks named in opts.Names.
+func (client *Client) StopChecks(opts *ChecksActionOptions) (*ChecksActionResult, error) {
+	return client.doMultiCheckAction("stop", opts.Names)
 }
 
 type multiCheckActionData struct {
 	Action string   `json:"action"`
 	Checks []string `json:"checks"`
 }
 
-func (client *Client) doMultiCheckAction(actionName string, checks []string) (results *CheckActionResults, err error) {
+func (client *Client) doMultiCheckAction(actionName string, checks []string) (*ChecksActionResult, error) {
 	action := multiCheckActionData{
 		Action: actionName,
 		Checks: checks,
@@ -166,6 +164,7 @@ func (client *Client) doMultiCheckAction(actionName string, checks []string) (re
 		return nil, err
 	}
 
+	var results *ChecksActionResult
 	err = resp.DecodeResult(&results)
 	if err != nil {
 		return nil, err

client/checks_test.go

@@ -63,7 +63,7 @@ func (cs *clientSuite) TestChecksGet(c *check.C) {
 
 func (cs *clientSuite) TestStartChecks(c *check.C) {
 	cs.rsp = `{
-	    "result": {"changed": ["chk1", "chk2"]},
+		"result": {"changed": ["chk1", "chk2"]},
 		"status": "OK",
 		"status-code": 200,
 		"type": "sync"
@@ -88,7 +88,7 @@ func (cs *clientSuite) TestStartChecks(c *check.C) {
 
 func (cs *clientSuite) TestStopChecks(c *check.C) {
 	cs.rsp = `{
-	    "result": {"changed": ["chk1"]},
+		"result": {"changed": ["chk1"]},
 		"status": "OK",
 		"status-code": 200,
 		"type": "sync"

docs/reference/cli-commands.md

@@ -706,9 +706,9 @@ The `replan` command starts, stops, or restarts services that have changed, so t
 Usage:
   pebble replan [replan-OPTIONS]
 
-The replan command starts, stops, or restarts services that have changed,
-so that running services exactly match the desired configuration in the
-current plan.
+The replan command starts, stops, or restarts services and checks that have
+changed, so that running services and checks exactly match the desired
+configuration in the current plan.
 
 [replan command options]
       --no-wait    Do not wait for the operation to finish but just print the

internals/cli/cmd_replan.go

@@ -20,11 +20,11 @@ import (
 	"github.com/canonical/pebble/client"
 )
 
-const cmdReplanSummary = "Ensure running services match the current plan"
+const cmdReplanSummary = "Ensure running services and checks match the current plan"
 const cmdReplanDescription = `
-The replan command starts, stops, or restarts services that have changed,
-so that running services exactly match the desired configuration in the
-current plan.
+The replan command starts, stops, or restarts services and checks that have
+changed, so that running services and checks exactly match the desired
+configuration in the current plan.
 `
 
 type cmdReplan struct {

internals/cli/cmd_start-checks_test.go

@@ -31,7 +31,7 @@ func (s *PebbleSuite) TestStartChecks(c *check.C) {
 		body := DecodedRequestBody(c, r)
 		c.Check(body, check.DeepEquals, map[string]interface{}{
 			"action": "start",
-			"checks": []interface{}{"chk1", "chk2"},
+			"checks": []interface{}{"chk1", "chk2", "chk3"},
 		})
 
 		fmt.Fprintf(w, `{
@@ -41,7 +41,7 @@ func (s *PebbleSuite) TestStartChecks(c *check.C) {
  }`)
 	})
 
-	rest, err := cli.ParserForTest().ParseArgs([]string{"start-checks", "chk1", "chk2"})
+	rest, err := cli.ParserForTest().ParseArgs([]string{"start-checks", "chk1", "chk2", "chk3"})
 	c.Assert(err, check.IsNil)
 	c.Assert(rest, check.HasLen, 0)
 	c.Check(s.Stdout(), check.Equals, "Checks started: chk1, chk2\n")

internals/cli/cmd_stop-checks_test.go

@@ -31,7 +31,7 @@ func (s *PebbleSuite) TestStopChecks(c *check.C) {
 		body := DecodedRequestBody(c, r)
 		c.Check(body, check.DeepEquals, map[string]interface{}{
 			"action": "stop",
-			"checks": []interface{}{"chk1", "chk2"},
+			"checks": []interface{}{"chk1", "chk2", "chk3"},
 		})
 
 		fmt.Fprintf(w, `{
@@ -41,7 +41,7 @@ func (s *PebbleSuite) TestStopChecks(c *check.C) {
  }`)
 	})
 
-	rest, err := cli.ParserForTest().ParseArgs([]string{"stop-checks", "chk1", "chk2"})
+	rest, err := cli.ParserForTest().ParseArgs([]string{"stop-checks", "chk1", "chk2", "chk3"})
 	c.Assert(err, check.IsNil)
 	c.Assert(rest, check.HasLen, 0)
 	c.Check(s.Stdout(), check.Equals, "Checks stopped: chk1, chk2\n")

internals/daemon/api_checks.go

@@ -26,7 +26,7 @@ import (
 type checkInfo struct {
 	Name      string `json:"name"`
 	Level     string `json:"level,omitempty"`
-	Startup   string `json:"startup,omitempty"`
+	Startup   string `json:"startup"`
 	Status    string `json:"status"`
 	Failures  int    `json:"failures,omitempty"`
 	Threshold int    `json:"threshold"`

internals/overlord/checkstate/manager.go

@@ -39,14 +39,13 @@ const (
 
 // CheckManager starts and manages the health checks.
 type CheckManager struct {
-	state *state.State
+	state   *state.State
+	planMgr *planstate.PlanManager
 
 	failureHandlers []FailureFunc
 
 	checksLock sync.Mutex
 	checks     map[string]CheckInfo
-
-	planMgr *planstate.PlanManager
 }
 
 // FailureFunc is the type of function called when a failure action is triggered.
@@ -390,9 +389,6 @@ type checker interface {
 // StartChecks starts the checks with the specified names, if not already
 // running, and returns the checks that did need to be started.
 func (m *CheckManager) StartChecks(checks []string) ([]*plan.Check, error) {
-	m.state.Lock()
-	defer m.state.Unlock()
-
 	currentPlan := m.planMgr.Plan()
 
 	// If any check specified is not in the plan, return an error.
@@ -402,6 +398,9 @@ func (m *CheckManager) StartChecks(checks []string) ([]*plan.Check, error) {
 		}
 	}
 
+	m.state.Lock()
+	defer m.state.Unlock()
+
 	var started []*plan.Check
 	for _, name := range checks {
 		check := currentPlan.Checks[name] // We know this is ok because we checked it above.
@@ -430,9 +429,6 @@ func (m *CheckManager) StartChecks(checks []string) ([]*plan.Check, error) {
 // StopChecks stops the checks with the specified names, if currently running,
 // and returns the checks that did need to be stopped.
 func (m *CheckManager) StopChecks(checks []string) ([]*plan.Check, error) {
-	m.state.Lock()
-	defer m.state.Unlock()
-
 	currentPlan := m.planMgr.Plan()
 
 	// If any check specified is not in the plan, return an error.
@@ -441,6 +437,10 @@ func (m *CheckManager) StopChecks(checks []string) ([]*plan.Check, error) {
 			return nil, fmt.Errorf("cannot find check %q in plan", name)
 		}
 	}
+
+	m.state.Lock()
+	defer m.state.Unlock()
+
 	var stopped []*plan.Check
 	for _, name := range checks {
 		check := currentPlan.Checks[name] // We know this is ok because we checked it above.
@@ -459,13 +459,15 @@ func (m *CheckManager) StopChecks(checks []string) ([]*plan.Check, error) {
 			continue
 		}
 		change := m.state.Change(info.ChangeID)
-		change.Abort()
+		if change != nil {
+			change.Abort()
+			stopped = append(stopped, check)
+		}
 		// We pass in the current number of failures so that it remains the
 		// same, so that people can inspect what the state of the check was when
 		// it was stopped. The status of the check will be "inactive", but the
 		// failure count combined with the threshold will give the full picture.
 		m.updateCheckInfo(check, "", info.Failures)
-		stopped = append(stopped, check)
 	}
 
 	return stopped, nil