Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

o/devicestate: cleanup system actions supported by recover mode #8686

Merged
merged 8 commits into from
May 20, 2020
Merged

o/devicestate: cleanup system actions supported by recover mode #8686

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
0955d89
o/devicestate: tweak the list of actions supported by recovery mode
bboozzoo May 14, 2020
11ae8eb
daemon: recover to recover is no longer supported
bboozzoo May 14, 2020
ac040e2
tests/core/uc20-recovery: update system actions in recover mode check
bboozzoo May 18, 2020
17bda66
o/devicestate: restore and update same-mode system action unit test
bboozzoo May 19, 2020
c0a9eef
o/devicestate: cleanup common test scenarios, check reinstall from di…
bboozzoo May 19, 2020
e2421c9
tests/core/uc20-recovery: update test
bboozzoo May 19, 2020
bf59e74
tests/core/uc20-recovery: verify system actions in run/recovery diffe…
bboozzoo May 19, 2020
b0be1ee
Merge remote-tracking branch 'upstream/master' into bboozzoo/what-see…
bboozzoo May 19, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 4 additions & 3 deletions daemon/api_systems_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -349,9 +349,10 @@ func (s *apiSuite) TestSystemActionRequestWithSeeded(c *check.C) {
},
{
// from recover mode -> recover mode is no-op
currentMode: "recover",
actionMode: "recover",
comment: "recover mode to recover mode",
currentMode: "recover",
actionMode: "recover",
expUnsupported: true,
comment: "recover mode to recover mode",
},
{
// from install mode -> install mode is no-no
Expand Down
22 changes: 19 additions & 3 deletions overlord/devicestate/devicemgr.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -772,6 +772,10 @@ var currentSystemActions = []SystemAction{
{Title: "Recover", Mode: "recover"},
{Title: "Run normally", Mode: "run"},
}
var recoverSystemActions = []SystemAction{
{Title: "Reinstall", Mode: "install"},
{Title: "Run normally", Mode: "run"},
}

func systemFromSeed(label string) (*System, error) {
s, err := seed.Open(dirs.SnapSeedDir, label)
Expand Down Expand Up @@ -864,6 +868,19 @@ func isCurrentSystem(current *seededSystem, other *System) bool {
current.BrandID == other.Brand.AccountID()
}

func currentSystemActionsForMode(mode string) []SystemAction {
switch mode {
case "install":
// there should be no current system in install mode
return nil
case "run":
return currentSystemActions
case "recover":
return recoverSystemActions
}
return nil
}

// Systems list the available recovery/seeding systems. Returns the list of
// systems, ErrNoSystems when no systems seeds were found or other error.
func (m *DeviceManager) Systems() ([]*System, error) {
Expand Down Expand Up @@ -891,7 +908,7 @@ func (m *DeviceManager) Systems() ([]*System, error) {
}
if currentSys != nil && isCurrentSystem(currentSys, system) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still feel this should actually be integrated back in systemFromSeed by passing all the needed info in, but can be done when moving it to systems.go

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed, we'll do this after #8689 lands.

system.Current = true
system.Actions = currentSystemActions
system.Actions = currentSystemActionsForMode(m.systemMode)
}
systems = append(systems, system)
}
Expand All @@ -915,8 +932,7 @@ func (m *DeviceManager) RequestSystemAction(systemLabel string, action SystemAct
return fmt.Errorf("cannot load seed system: %v", err)
}
if currentSys != nil && isCurrentSystem(currentSys, system) {
system.Current = true
system.Actions = currentSystemActions
system.Actions = currentSystemActionsForMode(m.systemMode)
}

var sysAction *SystemAction
Expand Down
76 changes: 44 additions & 32 deletions overlord/devicestate/devicestate_systems_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,8 +337,8 @@ func (s *deviceMgrSystemsSuite) TestRequestSameModeSameSystem(c *C) {

label := s.mockedSystemSeeds[0].label

happyModes := []string{"run", "recover"}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't we have just moved recover to sadModes, are we not testing that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, silly me. I'll restore the test.

sadModes := []string{"install"}
happyModes := []string{"run"}
sadModes := []string{"install", "recover"}

for _, mode := range append(happyModes, sadModes...) {
c.Logf("checking mode: %q", mode)
Expand Down Expand Up @@ -393,6 +393,24 @@ func (s *deviceMgrSystemsSuite) TestRequestNotYetSeeded(c *C) {
}
}

func (s *deviceMgrSystemsSuite) testRequestModeWithRestart(c *C, toModes []string, label string) {
for _, mode := range toModes {
c.Logf("checking mode: %q", mode)
err := s.mgr.RequestSystemAction(label,
devicestate.SystemAction{Mode: mode})
c.Assert(err, IsNil)
m, err := s.bootloader.GetBootVars("snapd_recovery_mode", "snapd_recovery_system")
c.Assert(err, IsNil)
c.Check(m, DeepEquals, map[string]string{
"snapd_recovery_system": label,
"snapd_recovery_mode": mode,
})
c.Check(s.restartRequests, DeepEquals, []state.RestartType{state.RestartSystemNow})
s.restartRequests = nil
s.bootloader.BootVars = map[string]string{}
}
}

func (s *deviceMgrSystemsSuite) TestRequestModeRunInstallForRecover(c *C) {
// we are in recover mode here
devicestate.SetSystemMode(s.mgr, "recover")
Expand All @@ -414,21 +432,7 @@ func (s *deviceMgrSystemsSuite) TestRequestModeRunInstallForRecover(c *C) {
})
s.state.Unlock()

for _, mode := range []string{"install", "run"} {
c.Logf("checking mode: %q", mode)
err := s.mgr.RequestSystemAction(s.mockedSystemSeeds[0].label,
devicestate.SystemAction{Mode: mode})
c.Assert(err, IsNil)
m, err := s.bootloader.GetBootVars("snapd_recovery_mode", "snapd_recovery_system")
c.Assert(err, IsNil)
c.Check(m, DeepEquals, map[string]string{
"snapd_recovery_system": s.mockedSystemSeeds[0].label,
"snapd_recovery_mode": mode,
})
c.Check(s.restartRequests, DeepEquals, []state.RestartType{state.RestartSystemNow})
s.restartRequests = nil
s.bootloader.BootVars = map[string]string{}
}
s.testRequestModeWithRestart(c, []string{"install", "run"}, s.mockedSystemSeeds[0].label)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice

}

func (s *deviceMgrSystemsSuite) TestRequestModeInstallRecoverForCurrent(c *C) {
Expand All @@ -450,21 +454,7 @@ func (s *deviceMgrSystemsSuite) TestRequestModeInstallRecoverForCurrent(c *C) {
})
s.state.Unlock()

for _, mode := range []string{"install", "recover"} {
c.Logf("checking mode: %q", mode)
err := s.mgr.RequestSystemAction(s.mockedSystemSeeds[0].label,
devicestate.SystemAction{Mode: mode})
c.Assert(err, IsNil)
m, err := s.bootloader.GetBootVars("snapd_recovery_mode", "snapd_recovery_system")
c.Assert(err, IsNil)
c.Check(m, DeepEquals, map[string]string{
"snapd_recovery_system": s.mockedSystemSeeds[0].label,
"snapd_recovery_mode": mode,
})
c.Check(s.restartRequests, DeepEquals, []state.RestartType{state.RestartSystemNow})
s.restartRequests = nil
s.bootloader.BootVars = map[string]string{}
}
s.testRequestModeWithRestart(c, []string{"install", "recover"}, s.mockedSystemSeeds[0].label)
}

func (s *deviceMgrSystemsSuite) TestRequestModeErrInBoot(c *C) {
Expand Down Expand Up @@ -523,3 +513,25 @@ func (s *deviceMgrSystemsSuite) TestRequestModeForNonCurrent(c *C) {
c.Assert(err, Equals, devicestate.ErrUnsupportedAction)
c.Check(s.restartRequests, HasLen, 0)
}

func (s *deviceMgrSystemsSuite) TestRequestInstallForOther(c *C) {
devicestate.SetSystemMode(s.mgr, "run")
// non run modes use modeenv
modeenv := boot.Modeenv{
Mode: "run",
}
err := modeenv.WriteTo("")
c.Assert(err, IsNil)

s.state.Lock()
s.state.Set("seeded-systems", []devicestate.SeededSystem{
{
System: s.mockedSystemSeeds[0].label,
Model: s.mockedSystemSeeds[0].model.Model(),
BrandID: s.mockedSystemSeeds[0].brand.AccountID(),
},
})
s.state.Unlock()
// reinstall from different system seed is ok
s.testRequestModeWithRestart(c, []string{"install"}, s.mockedSystemSeeds[1].label)
}
9 changes: 6 additions & 3 deletions tests/core/uc20-recovery/task.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@ debug: |
if [ -e systems.json ]; then
cat systems.json
fi
if [ -e system-info ]; then
cat system-info
fi
if [ -e /tmp/mock-shutdown.calls ]; then
cat /tmp/mock-shutdown.calls
fi
Expand All @@ -39,7 +42,7 @@ execute: |
test -n "$label"
# make sure that the seed exists
test -d "/var/lib/snapd/seed/systems/$label"
jq -r .result.systems[0].actions[].mode < systems.json | MATCH 'recover'
jq -r .result.systems[0].actions[].mode < systems.json | sort | tr '\n' ' ' | MATCH 'install recover run'

# keep a copy of the systems dump for later reference
cp systems.json /writable/systems.json.run
Expand Down Expand Up @@ -92,8 +95,8 @@ execute: |
test -e /host/ubuntu-data/systems.json.run

test-snapd-curl.curl -s --unix-socket /run/snapd.socket http://localhost/v2/systems > systems.json
# systems list should be identical
diff -up /host/ubuntu-data/systems.json.run systems.json
jq -r .result.systems[0].actions[].mode < systems.json | sort | tr '\n' ' ' | MATCH 'install run'

label="$(cat /host/ubuntu-data/systems.label)"
test -n "$label"
# seed in mounted in recover mode too
Expand Down