feat: include check name in notice data for perform-check and recover…

…-check (#444)

For notices of kind `change-updated`, the notice data will include the
`kind` field that is standard for these notices, but also when `kind` is
`perform-check` or `recover-check`, it will also include `check-name`,
set to the name of the check that is being performed or recovered.

This is the Pebble portion of
[OP046](https://docs.google.com/document/d/13ItH8l5ZSmmv9WqZXpqjV8EifZU5e3zxINiNLubnC68/edit).
It is used by Juju ([PR](juju/juju#17655)) and
ops ([PR](canonical/operator#1281)).

---------

Co-authored-by: Ben Hoyt <benhoyt@gmail.com>

internals/overlord/checkstate/manager_test.go

@@ -250,6 +250,7 @@ func (s *ManagerSuite) TestFailures(c *C) {
 	c.Assert(check.ChangeID, Equals, originalChangeID)
 
 	// Should have called failure handler and be unhealthy after 3 failures (threshold)
+	c.Assert(changeData(c, s.overlord.State(), check.ChangeID), DeepEquals, map[string]string{"check-name": "chk1"})
 	check = waitCheck(c, s.manager, "chk1", func(check *checkstate.CheckInfo) bool {
 		return check.Failures == 3 && check.ChangeID != originalChangeID
 	})
@@ -278,6 +279,7 @@ func (s *ManagerSuite) TestFailures(c *C) {
 	c.Assert(check.Threshold, Equals, 3)
 	c.Assert(notifies.Load(), Equals, int32(1))
 	c.Assert(lastTaskLog(s.overlord.State(), check.ChangeID), Equals, "")
+	c.Assert(changeData(c, s.overlord.State(), check.ChangeID), DeepEquals, map[string]string{"check-name": "chk1"})
 }
 
 func (s *ManagerSuite) TestFailuresBelowThreshold(c *C) {
@@ -571,3 +573,14 @@ func lastTaskLog(st *state.State, changeID string) string {
 	}
 	return logs[len(logs)-1]
 }
+
+func changeData(c *C, st *state.State, changeID string) map[string]string {
+	st.Lock()
+	defer st.Unlock()
+
+	chg := st.Change(changeID)
+	var data map[string]string
+	err := chg.Get("notice-data", &data)
+	c.Assert(err, IsNil)
+	return data
+}

internals/overlord/checkstate/request.go

@@ -37,7 +37,9 @@ func performCheckChange(st *state.State, config *plan.Check) (changeID string) {
 	task := st.NewTask(performCheckKind, summary)
 	task.Set(checkDetailsAttr, &checkDetails{Name: config.Name})
 
-	change := st.NewChange(performCheckKind, task.Summary())
+	change := st.NewChangeWithNoticeData(performCheckKind, task.Summary(), map[string]string{
+		"check-name": config.Name,
+	})
 	change.Set(noPruneAttr, true)
 	change.AddTask(task)
 
@@ -55,7 +57,9 @@ func recoverCheckChange(st *state.State, config *plan.Check, failures int) (chan
 	task := st.NewTask(recoverCheckKind, summary)
 	task.Set(checkDetailsAttr, &checkDetails{Name: config.Name, Failures: failures})
 
-	change := st.NewChange(recoverCheckKind, task.Summary())
+	change := st.NewChangeWithNoticeData(recoverCheckKind, task.Summary(), map[string]string{
+		"check-name": config.Name,
+	})
 	change.Set(noPruneAttr, true)
 	change.AddTask(task)
 

internals/overlord/state/change.go

@@ -17,6 +17,7 @@ package state
 import (
 	"bytes"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"sort"
 	"strings"
@@ -428,7 +429,16 @@ func (c *Change) addNotice() error {
 	opts := &AddNoticeOptions{
 		Data: map[string]string{"kind": c.Kind()},
 	}
-	_, err := c.state.AddNotice(nil, ChangeUpdateNotice, c.id, opts)
+	var extraData map[string]string
+	err := c.Get("notice-data", &extraData)
+	if err == nil {
+		for k, v := range extraData {
+			opts.Data[k] = v
+		}
+	} else if !errors.Is(err, ErrNoState) {
+		return fmt.Errorf("cannot get notice data from change %s: %w", c.ID(), err)
+	}
+	_, err = c.state.AddNotice(nil, ChangeUpdateNotice, c.id, opts)
 	return err
 }
 

internals/overlord/state/change_test.go

@@ -51,6 +51,19 @@ func (cs *changeSuite) TestNewChange(c *C) {
 	c.Check(n["occurrences"], Equals, 1.0)
 }
 
+func (cs *changeSuite) TestNewChangeWithExtraNoticeData(c *C) {
+	st := state.New(nil)
+	st.Lock()
+	defer st.Unlock()
+
+	st.NewChangeWithNoticeData("perform-check", "...", map[string]string{"check-name": "c"})
+
+	notices := st.Notices(nil)
+	c.Assert(notices, HasLen, 1)
+	n := noticeToMap(c, notices[0])
+	c.Check(n["last-data"], DeepEquals, map[string]any{"kind": "perform-check", "check-name": "c"})
+}
+
 func (cs *changeSuite) TestReadyTime(c *C) {
 	st := state.New(nil)
 	st.Lock()

internals/overlord/state/state.go

@@ -333,11 +333,22 @@ func (s *State) Cache(key, value interface{}) {
 
 // NewChange adds a new change to the state.
 func (s *State) NewChange(kind, summary string) *Change {
+	return s.NewChangeWithNoticeData(kind, summary, nil)
+}
+
+// NewChangeWithNoticeData adds a new change to the state, adding in any provided notice data.
+func (s *State) NewChangeWithNoticeData(kind, summary string, noticeData map[string]string) *Change {
 	s.writing()
 	s.lastChangeId++
 	id := strconv.Itoa(s.lastChangeId)
 	chg := newChange(s, id, kind, summary)
 	s.changes[id] = chg
+
+	// Set this before calling addNotice as that needs to use it.
+	if len(noticeData) > 0 {
+		chg.Set("notice-data", noticeData)
+	}
+
 	// Add change-update notice for newly spawned change
 	// NOTE: Implies State.writing()
 	if err := chg.addNotice(); err != nil {

internals/overlord/state/state_test.go

@@ -354,6 +354,36 @@ func (ss *stateSuite) TestNewChangeAndChanges(c *C) {
 	c.Check(st.Change("no-such-id"), IsNil)
 }
 
+func (ss *stateSuite) TestNewChangeWithNoticeData(c *C) {
+	st := state.New(nil)
+	st.Lock()
+	defer st.Unlock()
+
+	extraData := map[string]string{"foo": "bar"}
+	chg := st.NewChangeWithNoticeData("perform-check", "...", extraData)
+
+	chgs := st.Changes()
+	c.Check(chgs, HasLen, 1)
+
+	var data map[string]string
+	err := chg.Get("notice-data", &data)
+	c.Assert(err, IsNil)
+	c.Check(data, DeepEquals, extraData)
+}
+
+func (ss *stateSuite) TestNewChangeWithNilNoticeData(c *C) {
+	st := state.New(nil)
+	st.Lock()
+	defer st.Unlock()
+
+	st.NewChange("replan", "...")
+	st.NewChangeWithNoticeData("replan", "...", nil)
+
+	for _, chg := range st.Changes() {
+		c.Assert(chg.Has("notice-data"), Equals, false)
+	}
+}
+
 func (ss *stateSuite) TestNewChangeAndCheckpoint(c *C) {
 	b := new(fakeStateBackend)
 	st := state.New(b)