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.

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

snap: implement new snap reboot command #9021

Merged
merged 21 commits into from
Sep 10, 2020
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
4469021
snap: implement new `snap reboot` command
mvo5 Jul 16, 2020
b0e0833
Merge remote-tracking branch 'upstream/master' into snap-cmd-reboot
bboozzoo Jul 28, 2020
1a5f7fe
cmd/snap: tweak reboot command help and errors, extend unit tests
bboozzoo Jul 28, 2020
2e4100f
snap: remove XXX comment that was discussed in the PR
mvo5 Aug 11, 2020
bab0ca2
snap: improve `snap reboot` output message (thanks to Ian)
mvo5 Aug 25, 2020
83ea27a
Merge branch 'api-system-post-reboot' into snap-cmd-reboot
mvo5 Sep 3, 2020
c98ad20
snap: make <label>/reboot-OPTIONS no longer required
mvo5 Sep 3, 2020
105ede5
client: implement DoSystemReboot()
mvo5 Sep 3, 2020
4485991
snap: make `snap reboot` use client.DoSystemReboot
mvo5 Sep 3, 2020
5a34eff
tests: update uc20-recovery test to match new "snap reboot" output
mvo5 Sep 3, 2020
5a99a04
Merge remote-tracking branch 'upstream/master' into snap-cmd-reboot
mvo5 Sep 7, 2020
c876309
snap: improve tests/output of snap reboot cmd
mvo5 Sep 7, 2020
4509e86
Merge branch 'systems-reboot-api-client-side' into snap-cmd-reboot
mvo5 Sep 7, 2020
9da1973
Merge branch 'systems-reboot-api-client-side' into snap-cmd-reboot
mvo5 Sep 7, 2020
688b602
Merge remote-tracking branch 'upstream/master' into snap-cmd-reboot
mvo5 Sep 8, 2020
421e70a
snap: use correct client.RebootToSystem API
mvo5 Sep 10, 2020
a37c40d
snap: improve long reboot doc
mvo5 Sep 10, 2020
4a9c98e
client,snap: rework error formating of reboot command (thanks to Samu…
mvo5 Sep 10, 2020
e88e6d5
snap: fix silly typo
mvo5 Sep 10, 2020
29d87b0
snap: add missing i18n.G() (thanks Pawel)
mvo5 Sep 10, 2020
5b57db1
tests: fix silly typo
mvo5 Sep 10, 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
2 changes: 1 addition & 1 deletion cmd/snap/cmd_help.go
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ var helpCategories = []helpCategory{
}, {
Label: i18n.G("Other"),
Description: i18n.G("miscellanea"),
Commands: []string{"version", "warnings", "okay", "ack", "known", "model", "create-cohort", "recovery"},
Commands: []string{"version", "warnings", "okay", "ack", "known", "model", "create-cohort", "recovery", "reboot"},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We probably need to rethink "other" at some point and maybe add "Device Management", "Device" or "Ubuntu Core" or something what then has "model", "recovery", "reboot"

}, {
Label: i18n.G("Development"),
Description: i18n.G("developer-oriented features"),
Expand Down
119 changes: 119 additions & 0 deletions cmd/snap/cmd_reboot.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
// -*- Mode: Go; indent-tabs-mode: t -*-

/*
* Copyright (C) 2020 Canonical Ltd
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License version 3 as
* published by the Free Software Foundation.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*
*/

package main

import (
"fmt"

"github.com/jessevdk/go-flags"

"github.com/snapcore/snapd/i18n"
)

type cmdReboot struct {
clientMixin
Positional struct {
Label string
} `positional-args:"true"`

RunMode bool `long:"run"`
InstallMode bool `long:"install"`
RecoverMode bool `long:"recover"`
}

var shortRebootHelp = i18n.G("Reboot into selected system and mode")
var longRebootHelp = i18n.G(`
The reboot command reboots the system into a particular mode of the selected
recovery system.
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should also explain what happens if mode or label are omitted

`)

func init() {
addCommand("reboot", shortRebootHelp, longRebootHelp, func() flags.Commander {
return &cmdReboot{}
}, map[string]string{
// TRANSLATORS: This should not start with a lowercase letter.
"run": i18n.G("Boot into run mode"),
// TRANSLATORS: This should not start with a lowercase letter.
"install": i18n.G("Boot into install mode"),
// TRANSLATORS: This should not start with a lowercase letter.
"recover": i18n.G("Boot into recover mode"),
}, []argDesc{
{
// TRANSLATORS: This needs to begin with < and end with >
name: i18n.G("<label>"),
// TRANSLATORS: This should not start with a lowercase letter.
desc: i18n.G("The recovery system label"),
},
})
}

func (x *cmdReboot) modeFromCommandline() (string, error) {
var mode string

for _, arg := range []struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice.

enabled bool
mode string
}{
{x.RunMode, "run"},
{x.RecoverMode, "recover"},
{x.InstallMode, "install"},
} {
if !arg.enabled {
continue
}
if mode != "" {
return "", fmt.Errorf("Please specify a single mode")
Copy link
Contributor

Choose a reason for hiding this comment

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

i18n?

}
mode = arg.mode
}

return mode, nil
}

func (x *cmdReboot) Execute(args []string) error {
if len(args) > 0 {
return ErrExtraArgs
}

mode, err := x.modeFromCommandline()
if err != nil {
return err
}

if err := x.client.DoSystemReboot(x.Positional.Label, mode); err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this has a new name now, no?

if x.Positional.Label != "" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

feels like we should move the error formatting logic into client itself, so here we just use what it returns, it means we get a less repetitive error

return fmt.Errorf("cannot reboot into system %q: %v", x.Positional.Label, err)
}
return fmt.Errorf("cannot reboot: %v", err)
}

switch {
case x.Positional.Label != "" && mode != "":
fmt.Fprintf(Stdout, "Reboot into %q %q mode.\n", x.Positional.Label, mode)
case x.Positional.Label != "":
fmt.Fprintf(Stdout, "Reboot into %q.\n", x.Positional.Label)
case mode != "":
fmt.Fprintf(Stdout, "Reboot into %q mode.\n", mode)
default:
fmt.Fprintf(Stdout, "Reboot\n")
Copy link
Contributor

Choose a reason for hiding this comment

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

The reboot is not immediate right? If so, should we say "Reboot ... requested" or something like this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A good question, happy about ideas :)

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not super important to be clear... My suggestion is to just append "requested" to all messages, e.g. "Reboot into %q mode requested".

}

return nil
}
159 changes: 159 additions & 0 deletions cmd/snap/cmd_reboot_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,159 @@
// -*- Mode: Go; indent-tabs-mode: t -*-

/*
* Copyright (C) 2020 Canonical Ltd
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License version 3 as
* published by the Free Software Foundation.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*
*/

package main_test

import (
"fmt"
"io/ioutil"
"net/http"
"strings"

. "gopkg.in/check.v1"

snap "github.com/snapcore/snapd/cmd/snap"
)

func (s *SnapSuite) TestRebootHelp(c *C) {
msg := `Usage:
snap.test reboot [reboot-OPTIONS] [<label>]

The reboot command reboots the system into a particular mode of the selected
recovery system.

[reboot command options]
--run Boot into run mode
--install Boot into install mode
--recover Boot into recover mode

[reboot command arguments]
<label>: The recovery system label
`
s.testSubCommandHelp(c, "reboot", msg)
}

func (s *SnapSuite) TestRebootHappy(c *C) {

for _, tc := range []struct {
cmdline []string
expectedEndpoint string
expectedJSON string
expectedMsg string
}{
{
cmdline: []string{"reboot"},
expectedEndpoint: "/v2/systems",
expectedJSON: `{"action":"reboot","mode":""}`,
expectedMsg: `Reboot`,
},
{
cmdline: []string{"reboot", "--recover"},
expectedEndpoint: "/v2/systems",
expectedJSON: `{"action":"reboot","mode":"recover"}`,
expectedMsg: `Reboot into "recover" mode.`,
},
{
cmdline: []string{"reboot", "20200101"},
expectedEndpoint: "/v2/systems/20200101",
expectedJSON: `{"action":"reboot","mode":""}`,
expectedMsg: `Reboot into "20200101".`,
},
{
cmdline: []string{"reboot", "--recover", "20200101"},
expectedEndpoint: "/v2/systems/20200101",
expectedJSON: `{"action":"reboot","mode":"recover"}`,
expectedMsg: `Reboot into "20200101" "recover" mode.`,
},
} {

n := 0
s.ResetStdStreams()

s.RedirectClientToTestServer(func(w http.ResponseWriter, r *http.Request) {
switch n {
case 0:
c.Check(r.Method, Equals, "POST")
c.Check(r.URL.Path, Equals, tc.expectedEndpoint, Commentf("%v", tc.cmdline))
c.Check(r.URL.RawQuery, Equals, "")
body, err := ioutil.ReadAll(r.Body)
c.Check(err, IsNil)
c.Check(string(body), Equals, tc.expectedJSON+"\n")
fmt.Fprintln(w, `{"type": "sync", "result": {}}`)
default:
c.Fatalf("expected to get 1 requests, now on %d", n+1)
}

n++
})

// The server side will work out if the request is valid
rest, err := snap.Parser(snap.Client()).ParseArgs(tc.cmdline)
c.Assert(err, IsNil)
c.Assert(rest, DeepEquals, []string{})
c.Check(s.Stdout(), Equals, tc.expectedMsg+"\n", Commentf("%v", tc.cmdline))
c.Check(s.Stderr(), Equals, "")
}
}

func (s *SnapSuite) TestRebootUnhappy(c *C) {
s.RedirectClientToTestServer(func(w http.ResponseWriter, r *http.Request) {
c.Fatalf("server should not be hit in this test")
})

var tc = []struct {
args []string
errStr string
}{
{
args: []string{"reboot", "--run", "--recover", "20200101"},
errStr: "Please specify a single mode",
},
{
args: []string{"reboot", "--unknown-mode", "20200101"},
errStr: "unknown flag `unknown-mode'",
},
}

for _, t := range tc {
_, err := snap.Parser(snap.Client()).ParseArgs(t.args)
c.Check(err, ErrorMatches, t.errStr, Commentf(strings.Join(t.args, " ")))
}
}

func (s *SnapSuite) TestRebootAPIFail(c *C) {
n := 0
s.RedirectClientToTestServer(func(w http.ResponseWriter, r *http.Request) {
switch n {
case 0:
c.Check(r.Method, Equals, "POST")
c.Check(r.URL.Path, Equals, "/v2/systems/20200101")
c.Check(r.URL.RawQuery, Equals, "")
w.WriteHeader(404)
fmt.Fprintln(w, `{"type": "error", "status-code":404, "result": {"message":"requested system does not exist"}}`)
default:
c.Fatalf("expected to get 1 requests, now on %d", n+1)
}

n++
})
_, err := snap.Parser(snap.Client()).ParseArgs([]string{"reboot", "--recover", "20200101"})
c.Assert(err, ErrorMatches, `cannot reboot into system "20200101": cannot request system action: requested system does not exist`)
c.Check(s.Stdout(), Equals, "")
c.Check(s.Stderr(), Equals, "")
}
11 changes: 3 additions & 8 deletions tests/core/uc20-recovery/task.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,9 @@ execute: |
# with spread
mount -o bind "$PWD/mock-shutdown" /usr/sbin/shutdown

# request a recovery mode
# reboot to recovery mode
echo "Request rebooting into recovery mode"
# response contains maintenance: {"maintenance":{"message":"system is restarting","kind":"system-restart"}}
test-snapd-curl.curl -X POST --data '{"action":"do","mode":"recover"}' \
-s --unix-socket /run/snapd.socket "http://localhost/v2/systems/$label" | \
MATCH '"system-restart"'
snap reboot --recover "$label" | MATCH 'Reboot into ".*" "recover" mode'

# snapd schedules a slow timeout and an immediate one, however it is
# scheduled asynchronously, try keep the check simple
Expand Down Expand Up @@ -146,9 +143,7 @@ execute: |
mount -o bind "$PWD/mock-shutdown" /usr/sbin/shutdown

# request going back to run mode
test-snapd-curl.curl -X POST --data '{"action":"do","mode":"run"}' \
-s --unix-socket /run/snapd.socket "http://localhost/v2/systems/$label" | \
MATCH '"system-restart"'
snap reboot --run "$label" | MATCH 'Reboot into ".*" "run" mode."
# XXX: is this a race between spread seeing REBOOT and machine rebooting?

# see earlier note about shutdown
Expand Down