cmd/snap-recovery-chooser: add recovery chooser #8305

bboozzoo · 2020-03-20T10:18:35Z

This is a 2nd version of #8191. The PR implements a recovery chooser that acts as a simple proxy between snapd and the UI tool, currently console-conf. The chooser obtains a list of systems over snapd API endpoint, passes that to the UI process over stdin, then waits for the process to finish and print out the choice to its stdout. The coupling allows us to decouple the UI tool from snapd, where eventually the tool could be part of one of the snaps and run confined.

The PR requires changes from #8298.

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

Add a recovery chooser host, which talks to snapd and invokes the actual UI component from snapd internal tools. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

…overy-chooser-rfc

bboozzoo · 2020-03-20T14:43:20Z

The actual communication method needs more investigation. Looks like urwid library used by console-conf has some opinions about stdin, that are no longer true when we use a pipe there.

pedronis · 2020-03-23T09:20:12Z

The actual communication method needs more investigation. Looks like urwid library used by console-conf has some opinions about stdin, that are no longer true when we use a pipe there.

@bboozzoo let me know if you want to chat about that

bboozzoo · 2020-03-24T11:21:48Z

I was able to fix the issues in console-conf, the the 'blocked' is dropped now.

anonymouse64

LGTM, but I would like to see the env var renamed and potentially also a check on the existence of the file marker before we do anything as a sanity check (but maybe I'm wrong about the purpose of the file marker)

anonymouse64 · 2020-03-30T21:13:10Z

cmd/snap-recovery-chooser/main.go

+	}
+
+	// for local testing
+	if os.Getenv("USE_STDOUT") != "" {


could we prefix this env var with SNAPPY_TESTING or something like that to make it clear this is only for tests and shouldn't be relied on by anybody else?

anonymouse64 · 2020-03-30T21:30:56Z

cmd/snap-recovery-chooser/main_test.go

+	})
+
+	c.Assert(s.markerFile, testutil.FileAbsent)
+


unnecessary whitespace ?

Suggested change

anonymouse64 · 2020-03-30T21:38:08Z

cmd/snap-recovery-chooser/main.go

+
+func chooser(cli *client.Client) error {
+	// consume the trigger file
+	defer cleanupTriggerMarker()


should we also actually check that there is a marker file before we run (and then delete it if there is)?

I think it's unlikely this binary will ever be used except by something which sees the trigger file and then triggers this to run, but still perhaps it's a good idea?

…overy-chooser-rfc

…rtup Forward the response from the UI back to snapd. Also look at SNAPPY_TESTING instead of USE_STDOUT. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

anonymouse64 · 2020-03-31T14:31:44Z

cmd/snap-recovery-chooser/main.go

@@ -119,6 +137,15 @@ func cleanupTriggerMarker() error {
 }

 func chooser(cli *client.Client) error {
+	snappyTesting := os.Getenv("SNAPPY_TESTING") != ""


won't this change the behavior during spread tests generally? Or perhaps this binary will only ever be used in spread in an artificial way?

ah, didn't notice setting this top level in spread.yaml 😞

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

anonymouse64

Still LGTM with the actual call to the snapd API at the end

zyga

Looks really nice.

I left a few comments about packaging and some questions here and there.

cmd/snap-recovery-chooser/main.go

zyga · 2020-04-01T08:19:54Z

cmd/snap-recovery-chooser/main.go

+func outputForUI(out io.Writer, sys *ChooserSystems) error {
+	enc := json.NewEncoder(out)
+	if err := enc.Encode(sys); err != nil {
+		return fmt.Errorf("cannot serialize chooser options: %v", err)


For consistency between the message and the type.

Suggested change

return fmt.Errorf("cannot serialize chooser options: %v", err)

return fmt.Errorf("cannot serialize chooser systems: %v", err)

zyga · 2020-04-01T08:22:20Z

cmd/snap-recovery-chooser/main.go

+}
+
+func chooser(cli *client.Client) error {
+	snappyTesting := os.Getenv("SNAPPY_TESTING_USE_STDOUT") != ""


We have those "getenv bool" helpers. This looks like a place to use one.

the SNAPPY_ prefix is historical, we use SNAPD_ these days, please let's not use it for new things

I'll go with SNAPD_TESTING_USE_STDOUT.

You could use osutil.GetenvBool() here which would make this a bit more natural, the current code will behave unexpectedly when using "SNAPPY_TESTING_USE_STDOUT=false" (but I guess zyga said this already).

We could do something like https://paste.ubuntu.com/p/sXqWJkT7wp/ and check the outputForUI this way (or keeping in a separate test with something like https://paste.ubuntu.com/p/s86HQY8zfn/)

zyga · 2020-04-01T08:25:38Z

cmd/snap-recovery-chooser/main.go

+		return fmt.Errorf("UI process failed: %v", err)
+	}
+
+	logger.Noticef("got response: %+v", response)


What does %+v do?

%v the value in a default format when printing structs, the plus flag (%+v) adds field names

zyga · 2020-04-01T08:32:55Z

cmd/snap-recovery-chooser/main_test.go

+			})
+			c.Assert(err, IsNil)
+		default:
+			c.Fatalf("expected to get 1 requests, now on %d", n+1)


Nitpick, the number here is zero-based, which might be surprising if the test ever fails and someone has to debug it.

It's a pattern we use throughout the client/snap command tests. Still, the message should say at most 2 requests.., so fixed that.

zyga · 2020-04-01T08:37:05Z

cmd/snap-recovery-chooser/main_test.go

+}
+
+func (s *mockedClientCmdSuite) TestMainChooserStdout(c *C) {
+	os.Setenv("SNAPPY_TESTING_USE_STDOUT", "1")


This makes me wonder, should the side that reads this be using snapdenv?

The flag is only used in the snap-recovery-chooser binary.

if it's very specific to testing this code I would say no. For the moment the main candidates for snapdenv are things used by more than one package, or well advertised to users

zyga · 2020-04-01T08:41:27Z

packaging/ubuntu-16.04/snapd.install

@@ -9,6 +9,7 @@ usr/bin/snapd /usr/lib/snapd/
 usr/bin/snap-seccomp /usr/lib/snapd/
 usr/bin/snap-bootstrap /usr/lib/snapd/
 usr/bin/snap-preseed /usr/lib/snapd/
+usr/bin/snap-recovery-chooser /usr/lib/snapd/


The DM in me would say, please write a manual page.

No user is expected to ever run that themselves.

Then it should be in libexecdir

Oh, wait, it is. I take that back :)

${libexecdir}/snapd is /usr/lib/snapd on Ubuntu, isn't it?

zyga · 2020-04-01T08:43:26Z

packaging/debian-sid/not-installed

+usr/bin/snap-bootstrap
+usr/bin/snap-recovery-chooser


Do you need to handle this in packaging/snapd.mk, namely to remove it like so:

ifeq ($(with_core_bits),0) # Remove core 20 recovery chooser install:: rm -f $(DESTDIR)$(bindir)/snap-recovery-chooser endif

We just don't build it there as it has no use outside of Ubuntu package.

Ah, indeed. I was under the impression we build all go binaries. This is fine though I would prefer to have it for completeness, with the hope that this makefile can be used in all packaging at some point.

bboozzoo · 2020-04-02T06:19:58Z

The PR in subiquity canonical/subiquity#655 has landed.

bboozzoo · 2020-04-02T13:09:41Z

And additional fixes in canonical/subiquity#680 have landed too. We should probably merge this PR and do any tweaks in follow ups.

mvo5

This looks fine, I have some nitpicks inside but all followup material

mvo5 · 2020-04-02T14:48:22Z

cmd/snap-recovery-chooser/main.go

+	}
+	// TODO:UC20 update once upstream console-conf has the right command
+	// line switches
+	return exec.Command(tool, "--recovery-chooser-mode"), nil


The upstream console-conf changes have landed, so is this still a valid todo?

mvo5 · 2020-04-02T14:51:31Z

cmd/snap-recovery-chooser/main.go

+	// reuse stderr
+	cmd.Stderr = os.Stderr
+	// the chooser may be invoked via console-conf service which uses
+	// KillMethod=process, so make sure the UI process dies when we die


Nice! Also thanks for the comment.

mvo5 · 2020-04-02T14:56:51Z

cmd/snap-recovery-chooser/main.go

+}
+
+func cleanupTriggerMarker() error {
+	err := os.Remove(defaultMarkerFile)


(nitpick^2) I think slightly more go-ish would be:

if err := os.Remove(defaultMarkerFile); err != nil && !os.IsNotExit(err) { return err } return nil

but I already feel bad writing this comment as it's sooooooo much a nitpick

This and some other tweaks are done in #8415

mvo5 · 2020-04-02T14:57:48Z

cmd/snap-recovery-chooser/main.go

+}
+
+func chooser(cli *client.Client) error {
+	snappyTesting := os.Getenv("SNAPPY_TESTING_USE_STDOUT") != ""


You could use osutil.GetenvBool() here which would make this a bit more natural, the current code will behave unexpectedly when using "SNAPPY_TESTING_USE_STDOUT=false" (but I guess zyga said this already).

mvo5 · 2020-04-02T14:59:46Z

cmd/snap-recovery-chooser/main.go

+	}
+
+	// for local testing
+	if snappyTesting {


I'm not at the point yet where I see how this is used but it makes me wonder if for testing we could have something like a "testingUI" that capture stdin and gives canned replies

When testing locally it's sometimes useful to do:

$ USE_STDOUT=1 snap-recovery-chooser | console-conf --recovery-chooser-mode # or $ USE_STDOUT=1 snap-recovery-chooser > canned-input $ console-conf --recovery-chooser-mode < canned-input

bboozzoo added 3 commits March 19, 2020 16:09

packaging/ubuntu-16.04: pick up snap-recovery-chooser

e17e50c

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

packaging/debian,ubuntu-14.04: drop snap-recovery-chooser

de093f2

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

cmd/snap-recovery-chooser: add a recovery chooser

dac9333

Add a recovery chooser host, which talks to snapd and invokes the actual UI component from snapd internal tools. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

bboozzoo added the UC20 label Mar 20, 2020

Merge remote-tracking branch 'upstream/master' into bboozzoo/snap-rec…

ce88a51

…overy-chooser-rfc

bboozzoo added the ⛔ Blocked label Mar 20, 2020

bboozzoo removed the ⛔ Blocked label Mar 24, 2020

bboozzoo mentioned this pull request Mar 24, 2020

console_conf: implement UC20 recovery chooser canonical/subiquity#655

Merged

anonymouse64 self-requested a review March 26, 2020 16:22

anonymouse64 approved these changes Mar 30, 2020

View reviewed changes

bboozzoo added 2 commits March 31, 2020 14:30

Merge remote-tracking branch 'upstream/master' into bboozzoo/snap-rec…

69bf196

…overy-chooser-rfc

cmd/snap-recovery-chooser: respond back to snapd, check marker on sta…

6de5968

…rtup Forward the response from the UI back to snapd. Also look at SNAPPY_TESTING instead of USE_STDOUT. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

anonymouse64 reviewed Mar 31, 2020

View reviewed changes

cmd/snap-recovery-chooser: use SNAPPY_TESTING_USE_STDOUT for local tests

f0b324b

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

anonymouse64 approved these changes Mar 31, 2020

View reviewed changes

zyga reviewed Apr 1, 2020

View reviewed changes

mvo5 added this to the 2.45 milestone Apr 2, 2020

mvo5 approved these changes Apr 2, 2020

View reviewed changes

mvo5 merged commit c998d9a into snapcore:master Apr 2, 2020

bboozzoo mentioned this pull request Apr 3, 2020

cmd/snap-recovery-chooser: tweaks #8415

Merged

	return fmt.Errorf("cannot serialize chooser options: %v", err)
	return fmt.Errorf("cannot serialize chooser systems: %v", err)

		usr/bin/snap-bootstrap
		usr/bin/snap-recovery-chooser

cmd/snap-recovery-chooser: add recovery chooser #8305

cmd/snap-recovery-chooser: add recovery chooser #8305

Conversation

bboozzoo commented Mar 20, 2020 • edited by anonymouse64

bboozzoo commented Mar 20, 2020

pedronis commented Mar 23, 2020

bboozzoo commented Mar 24, 2020

anonymouse64 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

anonymouse64 left a comment

Choose a reason for hiding this comment

zyga 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

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

bboozzoo Apr 1, 2020 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zyga Apr 1, 2020 • edited

Choose a reason for hiding this comment

bboozzoo Apr 1, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bboozzoo commented Apr 2, 2020 • edited

bboozzoo commented Apr 2, 2020

mvo5 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bboozzoo commented Mar 20, 2020 •

edited by anonymouse64

bboozzoo Apr 1, 2020 •

edited

zyga Apr 1, 2020 •

edited

bboozzoo Apr 1, 2020 •

edited

bboozzoo commented Apr 2, 2020 •

edited