snap: implement new `snap reboot` command #9021

mvo5 · 2020-07-16T15:56:29Z

The new snap reboot command will reboot into the given recovery
system with the given mode.

The new `snap reboot` command will reboot into the given recovery system with the given mode.

zyga

LGTM

tests/core/uc20-recovery/task.yaml

cmd/snap/cmd_reboot.go

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

pedronis

This needs a bit more work, in terms of what combination of action and system (current vs not) are possible and when the system should have a default.

As we also discussed, probably adding a dedicated API variant action: "reboot" of the API makes also sense.

cmd/snap/cmd_reboot.go

pedronis · 2020-08-06T15:05:46Z

cmd/snap/cmd_reboot.go

+		return err
+	}
+
+	if err := x.client.DoSystemAction(x.Positional.Label, action); err != nil {


For run label can only be the current system, so it could also be omitted. Same for recover.

In general the label can be left out and default to current.

pedronis · 2020-08-06T15:08:33Z

cmd/snap/cmd_reboot.go

+	}
+
+	if err := x.client.DoSystemAction(x.Positional.Label, action); err != nil {
+		return fmt.Errorf("cannot reboot into system %q: %v", x.Positional.Label, err)


we maybe want a client.RebootSystem(label, mode) method, that should map to a different variant of the API that has action: "reboot"

the action: "do" setup was mostly meant for the chooser, where possibly some actions might not be reboots

pedronis · 2020-08-12T10:12:06Z

@mvo5 about default behavior, two remarks:

we agreed to make it so that recovery is not permanent, so basically we need to change the boot env back to run at some point
if that's the case I think "snap reboot" without any option if we support it should simply reboot what is current, without changing the boot env first (another reason to have a dedicated action)

Basically, in run mode, "snap reboot" reboots the run mode, in recovery mode once the first remark is implemented it goes back to run mode.

anonymouse64 · 2020-08-12T10:35:10Z

we agreed to make it so that recovery is not permanent, so basically we need to change the boot env back to run at some point

@pedronis this is on my TODO, I was waiting for some more of my things to land before proposing this, but I think I can make this change, I was going to change the bootenv at the end of the seeding task so after seeding is done (when a user could login) any reboot will go back to run mode

pedronis · 2020-08-12T12:23:34Z

@pedronis this is on my TODO, I was waiting for some more of my things to land before proposing this, but I think I can make this change, I was going to change the bootenv at the end of the seeding task so after seeding is done (when a user could login) any reboot will go back to run mode

is login only possible after seeding? (me doesn't remember the details)

anonymouse64 · 2020-08-12T12:39:01Z

is login only possible after seeding? (me doesn't remember the details)

ah you're right that login is actually possible before seeding, however you don't see the message telling you that you can now login until after seeding is done. I still think it's reasonable to do this in the seeding task of recover, but perhaps we should sync shortly on this if you think now is a good time to make that change

mvo5 · 2020-09-07T10:35:57Z

cmd/snap/cmd_help.go

@@ -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"},


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"

mvo5 · 2020-09-07T11:11:18Z

client/systems.go

@@ -103,3 +103,25 @@ func (client *Client) DoSystemAction(systemLabel string, action *SystemAction) e
 	}
 	return nil
 }
+
+// DoSystemReboot issues a request to reboot into the given label/mode
+func (client *Client) DoSystemReboot(systemLabel, mode string) error {


Should this be called just "SystemReboot()"?

Mabye RebootToSystem? also the doc comment needs to be expanded to explain what happens if label and/or mode are empty?

pedronis

some detail comments

pedronis · 2020-09-08T11:38:01Z

cmd/snap/cmd_reboot.go

+		return err
+	}
+
+	if err := x.client.DoSystemReboot(x.Positional.Label, mode); err != nil {


this has a new name now, no?

pedronis · 2020-09-08T11:39:40Z

cmd/snap/cmd_reboot.go

+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.


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

pedronis · 2020-09-08T11:43:30Z

cmd/snap/cmd_reboot.go

+	}
+
+	if err := x.client.DoSystemReboot(x.Positional.Label, mode); err != nil {
+		if x.Positional.Label != "" {


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

…ele)

this has changed quite a bit since review time

pedronis

thank you, this probably needs a master merge now that uc20-recovery was maybe fixed

pedronis · 2020-09-10T08:55:55Z

cmd/snap/cmd_reboot.go

+When called without a system label and without a mode it will just
+trigger a regular reboot.
+
+When called without a system -abel but with a mode it will use the


s/-abel/label/

stolowski

Thank, looks very nice! Two questions.

stolowski · 2020-09-10T09:16:58Z

cmd/snap/cmd_reboot.go

+func (x *cmdReboot) modeFromCommandline() (string, error) {
+	var mode string
+
+	for _, arg := range []struct {


stolowski · 2020-09-10T09:22:04Z

cmd/snap/cmd_reboot.go

+			continue
+		}
+		if mode != "" {
+			return "", fmt.Errorf("Please specify a single mode")


stolowski · 2020-09-10T09:25:43Z

cmd/snap/cmd_reboot.go

+	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")


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

A good question, happy about ideas :)

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".

snap: implement new snap reboot command

4469021

The new `snap reboot` command will reboot into the given recovery system with the given mode.

zyga previously approved these changes Jul 17, 2020

View reviewed changes

tests/core/uc20-recovery/task.yaml Outdated Show resolved Hide resolved

cmd/snap/cmd_reboot.go Outdated Show resolved Hide resolved

bboozzoo reviewed Jul 21, 2020

View reviewed changes

cmd/snap/cmd_reboot.go Outdated Show resolved Hide resolved

cmd/snap/cmd_reboot.go Outdated Show resolved Hide resolved

cmd/snap/cmd_reboot.go Outdated Show resolved Hide resolved

bboozzoo added 2 commits July 28, 2020 08:04

Merge remote-tracking branch 'upstream/master' into snap-cmd-reboot

b0e0833

cmd/snap: tweak reboot command help and errors, extend unit tests

1a5f7fe

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

pedronis self-requested a review August 5, 2020 17:37

pedronis requested changes Aug 6, 2020

View reviewed changes

snap: remove XXX comment that was discussed in the PR

2e4100f

snap: improve snap reboot output message (thanks to Ian)

bab0ca2

mvo5 mentioned this pull request Aug 25, 2020

daemon: add /v2/systems "reboot" action API #9210

Merged

pedronis added the UC20 label Sep 2, 2020

pedronis added this to the 2.47 milestone Sep 2, 2020

mvo5 added 6 commits September 3, 2020 10:41

Merge branch 'api-system-post-reboot' into snap-cmd-reboot

83ea27a

snap: make <label>/reboot-OPTIONS no longer required

c98ad20

client: implement DoSystemReboot()

105ede5

snap: make snap reboot use client.DoSystemReboot

4485991

tests: update uc20-recovery test to match new "snap reboot" output

5a34eff

Merge remote-tracking branch 'upstream/master' into snap-cmd-reboot

5a99a04

mvo5 commented Sep 7, 2020

View reviewed changes

snap: improve tests/output of snap reboot cmd

c876309

mvo5 commented Sep 7, 2020

View reviewed changes

mvo5 mentioned this pull request Sep 7, 2020

client: implement RebootToSystem #9291

Merged

mvo5 added 2 commits September 7, 2020 18:00

Merge branch 'systems-reboot-api-client-side' into snap-cmd-reboot

4509e86

Merge branch 'systems-reboot-api-client-side' into snap-cmd-reboot

9da1973

mvo5 closed this Sep 8, 2020

mvo5 reopened this Sep 8, 2020

Merge remote-tracking branch 'upstream/master' into snap-cmd-reboot

688b602

pedronis self-requested a review September 8, 2020 11:09

pedronis reviewed Sep 8, 2020

View reviewed changes

mvo5 added 3 commits September 10, 2020 07:10

snap: use correct client.RebootToSystem API

421e70a

snap: improve long reboot doc

a37c40d

client,snap: rework error formating of reboot command (thanks to Samu…

4a9c98e

…ele)

pedronis self-requested a review September 10, 2020 08:30

pedronis approved these changes Sep 10, 2020

View reviewed changes

snap: fix silly typo

e88e6d5

stolowski approved these changes Sep 10, 2020

View reviewed changes

mvo5 added 2 commits September 10, 2020 11:42

snap: add missing i18n.G() (thanks Pawel)

29d87b0

tests: fix silly typo

5b57db1

mvo5 merged commit 93efba1 into snapcore:master Sep 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

snap: implement new `snap reboot` command #9021

snap: implement new `snap reboot` command #9021

mvo5 commented Jul 16, 2020

zyga left a comment

pedronis left a comment

pedronis Aug 6, 2020

pedronis Aug 6, 2020

pedronis commented Aug 12, 2020 •

edited

anonymouse64 commented Aug 12, 2020

pedronis commented Aug 12, 2020

anonymouse64 commented Aug 12, 2020

mvo5 Sep 7, 2020

mvo5 Sep 7, 2020

pedronis Sep 7, 2020

pedronis left a comment

pedronis Sep 8, 2020

pedronis Sep 8, 2020

pedronis Sep 8, 2020

pedronis left a comment

pedronis Sep 10, 2020

stolowski left a comment

stolowski Sep 10, 2020

stolowski Sep 10, 2020

stolowski Sep 10, 2020

mvo5 Sep 10, 2020

stolowski Sep 10, 2020

snap: implement new snap reboot command #9021

snap: implement new snap reboot command #9021

Conversation

mvo5 commented Jul 16, 2020

zyga left a comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis commented Aug 12, 2020 • edited

anonymouse64 commented Aug 12, 2020

pedronis commented Aug 12, 2020

anonymouse64 commented Aug 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stolowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snap: implement new `snap reboot` command #9021

snap: implement new `snap reboot` command #9021

pedronis commented Aug 12, 2020 •

edited