Skip to content

Commit

Permalink
o/state: record change-update notices on change status updates
Browse files Browse the repository at this point in the history 
* o/state: address review comments (thanks @benhoyt)
	* panic when addNotice fails to signal an internal programmer error
	* better comments

* o/state: skip alternating change-update notices

* o/state: address review comments (thanks @olivercalder and @benhoyt)

* o/state: save change notice status to disk (thanks @pedronis)

* o/state: add omitempty to last-recorded-notice-status change state (thanks @pedronis)

Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com>
  • Loading branch information
@ZeyadYasser @Meulengracht
ZeyadYasser authored and Meulengracht committed Feb 27, 2024
1 parent bebd832 commit 360fec7
Show file tree
Hide file tree
Showing 4 changed files with 261 additions and 14 deletions.
86 changes: 86 additions & 0 deletions overlord/snapstate/snapstate_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10000,3 +10000,89 @@ func (s *snapmgrTestSuite) TestRefreshInhibitProceedTime(c *C) {
expectedRefreshInhibitProceedTime := refreshInhibitedTime.Add(snapstate.MaxInhibition)
c.Check(snapst.RefreshInhibitProceedTime(s.state), Equals, expectedRefreshInhibitProceedTime)
}

func (s *snapmgrTestSuite) TestChangeStatusRecordsChangeUpdateNotice(c *C) {
st := s.state
st.Lock()
defer st.Unlock()

chg := st.NewChange("refresh", "")

s.o.TaskRunner().AddHandler("fake-task", func(task *state.Task, tomb *tomb.Tomb) error { return nil }, nil)

var prev *state.Task
addTask := func(name string) {
t := st.NewTask(name, "")
chg.AddTask(t)
if prev != nil {
t.WaitFor(prev)
}
prev = t
}

for i := 0; i < 5; i++ {
addTask("fake-task")
}

s.settle(c)

// Check notice is recorded on change status updates
notices := s.state.Notices(nil)
c.Assert(notices, HasLen, 1)
n := noticeToMap(c, notices[0])
c.Check(n["type"], Equals, "change-update")
c.Check(n["key"], Equals, chg.ID())
c.Check(n["last-data"], DeepEquals, map[string]any{"kind": "refresh"})
// Default -> Doing -> Done
c.Check(n["occurrences"], Equals, 3.0)
}

func (s *snapmgrTestSuite) TestChangeStatusUndoRecordsChangeUpdateNotice(c *C) {
st := s.state
st.Lock()
defer st.Unlock()

chg := st.NewChange("refresh", "")

s.o.TaskRunner().AddHandler("fake-task", func(task *state.Task, tomb *tomb.Tomb) error { return nil }, nil)

var prev *state.Task
addTask := func(name string) {
t := st.NewTask(name, "")
chg.AddTask(t)
if prev != nil {
t.WaitFor(prev)
}
prev = t
}

for i := 0; i < 5; i++ {
addTask("fake-task")
}
addTask("error-trigger")
for i := 0; i < 5; i++ {
addTask("fake-task")
}

s.settle(c)

// Check notice is recorded on change status updates
notices := s.state.Notices(nil)
c.Assert(notices, HasLen, 1)
n := noticeToMap(c, notices[0])
c.Check(n["type"], Equals, "change-update")
c.Check(n["key"], Equals, chg.ID())
c.Check(n["last-data"], DeepEquals, map[string]any{"kind": "refresh"})
// Default -> Doing -> Undo -> Abort -> Undo -> Error
c.Check(n["occurrences"], Equals, 6.0)
}

// noticeToMap converts a Notice to a map using a JSON marshal-unmarshal round trip.
func noticeToMap(c *C, notice *state.Notice) map[string]any {
buf, err := json.Marshal(notice)
c.Assert(err, IsNil)
var n map[string]any
err = json.Unmarshal(buf, &n)
c.Assert(err, IsNil)
return n
}
55 changes: 41 additions & 14 deletions overlord/state/change.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -139,16 +139,17 @@ const (
// while the individual Task values would track the running of
// the hooks themselves.
type Change struct {
state *State
id string
kind string
summary string
status Status
clean bool
data customData
taskIDs []string
ready chan struct{}
lastObservedStatus Status
state *State
id string
kind string
summary string
status Status
clean bool
data customData
taskIDs []string
ready chan struct{}
lastObservedStatus Status
lastRecordedNoticeStatus Status

spawnTime time.Time
readyTime time.Time
Expand Down Expand Up @@ -184,6 +185,8 @@ type marshalledChange struct {

SpawnTime time.Time `json:"spawn-time"`
ReadyTime *time.Time `json:"ready-time,omitempty"`

LastRecordedNoticeStatus Status `json:"last-recorded-notice-status,omitempty"`
}

// MarshalJSON makes Change a json.Marshaller
Expand All @@ -204,6 +207,8 @@ func (c *Change) MarshalJSON() ([]byte, error) {

SpawnTime: c.spawnTime,
ReadyTime: readyTime,

LastRecordedNoticeStatus: c.lastRecordedNoticeStatus,
})
}

Expand Down Expand Up @@ -233,6 +238,7 @@ func (c *Change) UnmarshalJSON(data []byte) error {
if unmarshalled.ReadyTime != nil {
c.readyTime = *unmarshalled.ReadyTime
}
c.lastRecordedNoticeStatus = unmarshalled.LastRecordedNoticeStatus
return nil
}

Expand Down Expand Up @@ -423,12 +429,33 @@ func (c *Change) Status() Status {
panic(fmt.Sprintf("internal error: cannot process change status: %v", statusStats))
}

// addNotice records an occurrence of a change-update notice for this change.
// The notice key is set to the change ID.
func (c *Change) addNotice() error {
opts := &AddNoticeOptions{
Data: map[string]string{"kind": c.Kind()},
}
_, err := c.state.AddNotice(nil, ChangeUpdateNotice, c.id, opts)
return err
}

func shouldSkipChangeUpdateNotice(old, new Status) bool {
// Skip alternating Doing->Do->Doing and Undoing->Undo->Undoing notices
return (old == new) || (old == DoingStatus && new == DoStatus) || (old == UndoingStatus && new == UndoStatus)
}

func (c *Change) notifyStatusChange(new Status) {
if c.lastObservedStatus == new {
return
if c.lastObservedStatus != new {
c.state.notifyChangeStatusChangedHandlers(c, c.lastObservedStatus, new)
c.lastObservedStatus = new
}
if !shouldSkipChangeUpdateNotice(c.lastRecordedNoticeStatus, new) {
// NOTE: Implies State.writing()
if err := c.addNotice(); err != nil {
logger.Panicf(`internal error: failed to add "change-update" notice on status change: %v`, err)
}
c.lastRecordedNoticeStatus = new
}
c.state.notifyChangeStatusChangedHandlers(c, c.lastObservedStatus, new)
c.lastObservedStatus = new
}

// SetStatus sets the change status, overriding the default behavior (see Status method).
Expand Down
129 changes: 129 additions & 0 deletions overlord/state/change_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
package state_test

import (
"encoding/json"
"errors"
"fmt"
"sort"
Expand All @@ -44,6 +45,15 @@ func (cs *changeSuite) TestNewChange(c *C) {
chg := st.NewChange("install", "summary...")
c.Check(chg.Kind(), Equals, "install")
c.Check(chg.Summary(), Equals, "summary...")

// Check notice is recorded on change spawn
notices := st.Notices(nil)
c.Assert(notices, HasLen, 1)
n := noticeToMap(c, notices[0])
c.Check(n["type"], Equals, "change-update")
c.Check(n["key"], Equals, chg.ID())
c.Check(n["last-data"], DeepEquals, map[string]any{"kind": "install"})
c.Check(n["occurrences"], Equals, 1.0)
}

func (cs *changeSuite) TestReadyTime(c *C) {
Expand Down Expand Up @@ -1450,3 +1460,122 @@ func (cs *changeSuite) TestIsWaitingUndoMultipleDependencies(c *C) {
t4.SetStatus(state.UndoneStatus)
c.Check(chg.Status(), Equals, state.WaitStatus)
}

func (cs *changeSuite) TestChangeStatusRecordsChangeUpdateNotice(c *C) {
st := state.New(nil)
st.Lock()
defer st.Unlock()

chg := st.NewChange("change", "...")

t1 := st.NewTask("task1", "...")
t2 := st.NewTask("task2", "...")
t2.WaitFor(t1)
t3 := st.NewTask("task3", "...")
t3.WaitFor(t2)

chg.AddTask(t1)
chg.AddTask(t2)
chg.AddTask(t3)

// Verify that change status is alternating Doing -> Do -> Doing
t1.SetStatus(state.DoingStatus)
c.Assert(chg.Status(), Equals, state.DoingStatus)
t1.SetStatus(state.DoneStatus)
c.Assert(chg.Status(), Equals, state.DoStatus)

t2.SetStatus(state.DoingStatus)
c.Assert(chg.Status(), Equals, state.DoingStatus)
t2.SetStatus(state.DoneStatus)
c.Assert(chg.Status(), Equals, state.DoStatus)

t3.SetStatus(state.DoingStatus)
c.Assert(chg.Status(), Equals, state.DoingStatus)
t3.SetStatus(state.DoneStatus)
c.Assert(chg.Status(), Equals, state.DoneStatus)

// Check notice is recorded on change status updates and ignores
// the alternating status
notices := st.Notices(nil)
c.Assert(notices, HasLen, 1)
n := noticeToMap(c, notices[0])
c.Check(n["type"], Equals, "change-update")
c.Check(n["key"], Equals, chg.ID())
c.Check(n["last-data"], DeepEquals, map[string]any{"kind": "change"})
// Default -> Doing -> Done
c.Check(n["occurrences"], Equals, 3.0)
}

func (cs *changeSuite) TestChangeStatusUndoRecordsChangeUpdateNotice(c *C) {
st := state.New(nil)
st.Lock()
defer st.Unlock()

chg := st.NewChange("change", "...")

t1 := st.NewTask("task1", "...")
t2 := st.NewTask("task2", "...")
t2.WaitFor(t1)
t3 := st.NewTask("task3", "...")
t3.WaitFor(t2)

chg.AddTask(t1)
chg.AddTask(t2)
chg.AddTask(t3)

// Verify that change status is alternating Doing -> Do -> Doing
t1.SetStatus(state.DoingStatus)
c.Assert(chg.Status(), Equals, state.DoingStatus)
t1.SetStatus(state.DoneStatus)
c.Assert(chg.Status(), Equals, state.DoStatus)

t2.SetStatus(state.DoingStatus)
c.Assert(chg.Status(), Equals, state.DoingStatus)
t2.SetStatus(state.DoneStatus)
c.Assert(chg.Status(), Equals, state.DoStatus)

// Trigger an error and abort change
chg.Abort()
t3.SetStatus(state.ErrorStatus)
c.Assert(chg.Status(), Equals, state.UndoStatus)

// Verify that change status is alternating Undo -> Undoing -> Undo
t2.SetStatus(state.UndoingStatus)
c.Assert(chg.Status(), Equals, state.UndoingStatus)
t2.SetStatus(state.UndoneStatus)
c.Assert(chg.Status(), Equals, state.UndoStatus)

t1.SetStatus(state.UndoingStatus)
c.Assert(chg.Status(), Equals, state.UndoingStatus)
t1.SetStatus(state.UndoneStatus)
c.Assert(chg.Status(), Equals, state.ErrorStatus)

// Check notice is recorded on change status updates and ignores
// the alternating status
notices := st.Notices(nil)
c.Assert(notices, HasLen, 1)
n := noticeToMap(c, notices[0])
c.Check(n["type"], Equals, "change-update")
c.Check(n["key"], Equals, chg.ID())
c.Check(n["last-data"], DeepEquals, map[string]any{"kind": "change"})
// Default -> Doing -> Undo -> Undoing -> Error
c.Check(n["occurrences"], Equals, 5.0)
}

func (cs *changeSuite) TestChangeLastRecordedNoitceStatusPersisted(c *C) {
st := state.New(nil)
st.Lock()
defer st.Unlock()

chg := st.NewChange("change", "summary...")
chg.SetStatus(state.DoingStatus)

data, err := json.Marshal(chg)
c.Assert(err, IsNil)

var chgData map[string]any
err = json.Unmarshal(data, &chgData)
c.Assert(err, IsNil)
obtainedStatus := state.Status(chgData["last-recorded-notice-status"].(float64))
c.Check(obtainedStatus, Equals, state.DoingStatus)
}
5 changes: 5 additions & 0 deletions overlord/state/state.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -343,6 +343,11 @@ func (s *State) NewChange(kind, summary string) *Change {
id := strconv.Itoa(s.lastChangeId)
chg := newChange(s, id, kind, summary)
s.changes[id] = chg
// Add change-update notice for newly spawned change
// NOTE: Implies State.writing()
if err := chg.addNotice(); err != nil {
logger.Panicf(`internal error: failed to add "change-update" notice for new change: %v`, err)
}
return chg
}

Expand Down

0 comments on commit 360fec7

Please sign in to comment.