snap: add support for `snap advise-snap --from-apt` #5122

mvo5 · 2018-05-01T21:14:47Z

This adds support for interacting with the new apt 1.6+ hooks.

It implements the org.debian.apt.hooks.install.fail hook and
will suggest a snap if there is one available with the given
name.

E.g.:

Reading package lists... Done
Building dependency tree
Reading state information... Done

No apt package "aws-cli", but there is a snap with that name.
Try "snap install aws-cli"

E: Unable to locate package aws-cli

This will also need a spread test for the end-to-end integration test.

This adds support for interacting with the new apt 1.6+ hooks. It implements the `org.debian.apt.hooks.install.fail` hook and will suggest a snap if there is one available with the given name. E.g.: ``` Reading package lists... Done Building dependency tree Reading state information... Done No apt package "aws-cli", but there is a snap with that name. Try "snap install aws-cli" E: Unable to locate package aws-cli ```

Conan-Kudo · 2018-05-02T21:49:50Z

Hmm, that'd be interesting to have as a DNF hook too...

mvo5 · 2018-05-03T12:07:22Z

@Conan-Kudo I would love to add one to dnf as well, do you have a pointer how to interact with dnf?

Conan-Kudo · 2018-05-03T12:57:09Z

@mvo5: Here's the DNF plugin API: http://dnf.readthedocs.io/en/latest/api_plugins.html

Conan-Kudo · 2018-05-03T13:00:16Z

@mvo5 And here are several examples of different plugins:

zyga · 2018-05-04T08:58:54Z

It looks like the hook fails when snapd is being removed (or has been removed):

+ quiet apt-get remove -y --purge -y snapd
quiet: apt-get remove -y --purge -y snapd
quiet: exit status 100. Output follows:
Reading package lists...
Building dependency tree...
Reading state information...
The following package was automatically installed and is no longer required:
  grub-pc-bin
Use 'sudo apt autoremove' to remove it.
error: json: cannot unmarshal object into Go struct field .packages of type string
The following packages will be REMOVED:
  snapd*
0 upgraded, 0 newly installed, 1 to remove and 6 not upgraded.
After this operation, 57.6 MB disk space will be freed.
(Reading database ... 
(Reading database ... 5%
(Reading database ... 10%
(Reading database ... 15%
(Reading database ... 20%
(Reading database ... 25%
(Reading database ... 30%
(Reading database ... 35%
(Reading database ... 40%
(Reading database ... 45%
(Reading database ... 50%
(Reading database ... 55%
(Reading database ... 60%
(Reading database ... 65%
(Reading database ... 70%
(Reading database ... 75%
(Reading database ... 80%
(Reading database ... 85%
(Reading database ... 90%
(Reading database ... 95%
(Reading database ... 100%
(Reading database ... 101755 files and directories currently installed.)

Removing snapd (1337.2.32.6) ...

Processing triggers for man-db (2.8.3-2) ...

(Reading database ... 
(Reading database ... 5%
(Reading database ... 10%
(Reading database ... 15%
(Reading database ... 20%
(Reading database ... 25%
(Reading database ... 30%
(Reading database ... 35%
(Reading database ... 40%
(Reading database ... 45%
(Reading database ... 50%
(Reading database ... 55%
(Reading database ... 60%
(Reading database ... 65%
(Reading database ... 70%
(Reading database ... 75%
(Reading database ... 80%
(Reading database ... 85%
(Reading database ... 90%
(Reading database ... 95%
(Reading database ... 100%
(Reading database ... 101676 files and directories currently installed.)

Purging configuration files for snapd (1337.2.32.6) ...

Stopping snap-core-4588.mount

Stopping unit snap-core-4588.mount

Waiting until unit snap-core-4588.mount is stopped [attempt 1]

snap-core-4588.mount is stopped.

Removing snap core and revision 4588

Removing snap-core-4588.mount

Final directory cleanup

Discarding preserved snap namespaces

umount: /run/snapd/ns/: not mounted.

Removing extra snap-confine apparmor rules

Removing snapd cache

Removing snapd state

E: Could not read response to hello message from hook [ ! -f /usr/bin/snap ] || /usr/bin/snap advise-snap --from-apt || true: Connection reset by peer
E: Could not read message separator line after handshake from [ ! -f /usr/bin/snap ] || /usr/bin/snap advise-snap --from-apt || true: Connection reset by peer

zyga

LGTM, I added a few comments but that's all nitpicks I think.

zyga · 2018-05-04T09:00:48Z

cmd/snap/cmd_advise.go

+}
+
+func readRpc(r *bufio.Reader) (*jsonRPC, error) {
+	line, _, err := r.ReadLine()


Is it guaranteed to be on a single line?

zyga · 2018-05-04T09:10:21Z

cmd/snap/cmd_advise.go

+	}
+	fd, err := strconv.Atoi(sockFd)
+	if err != nil {
+		return err


Nitpick, errors from atoi were a bit horrible last time I looked and it would be even less clear what went wrong if this were to happen. How about:

return fmt.Errorf("expected APT_HOOK_SOCKET to be a decimal integer, found %q", sockFd)

zyga · 2018-05-04T09:11:08Z

cmd/snap/cmd_advise.go

+
+	conn, err := net.FileConn(f)
+	if err != nil {
+		return fmt.Errorf("cannot connect to %q: %v", sockFd, err)


Nitpick: I'd probably use fd rather than sockFd here since we're "done" handling that layer (string vs int).

zyga · 2018-05-04T09:15:07Z

cmd/snap/cmd_advise.go

+func adviceViaAptHook() error {
+	sockFd := os.Getenv("APT_HOOK_SOCKET")
+	if sockFd == "" {
+		return fmt.Errorf("cannot find APT_HOOK_SOCKET env")


Should we just return nil here? Will this be the case when older apt is used and there's just no socket RPC to do.

Apt added support for the new kind of hooks at the same time as the json rpc was added. So if there is no APT_HOOK_SOCKET env but the hook is called something is wrong (on the apt side).

zyga · 2018-05-04T09:17:16Z

packaging/ubuntu-16.04/snapd.install

@@ -18,6 +18,8 @@ data/udev/rules.d/66-snapd-autoimport.rules /lib/udev/rules.d
 data/info /usr/lib/snapd/
 # polkit actions
 data/polkit/io.snapcraft.snapd.policy /usr/share/polkit-1/actions/
+# apt hook
+data/apt/20snapd.conf /etc/apt/apt.conf.d/


I wish apt would pick hooks from /lib as this is yet another thing that needs to be in /etc

Since this is a new feature, can we influence apt to do the reasonable thing?

mvo5 · 2018-05-07T07:03:46Z

As for the remove failure - I think we need a fix in apt for this or we need to make the shell code talk to the socket but that would be pretty hard because we cannot really depend on much when apt is in a minimal chroot.

bboozzoo

Some nitpicks, overall LGTM.

bboozzoo · 2018-05-07T07:50:12Z

cmd/snap/cmd_advise.go

-	Format  string `long:"format" default:"pretty" choice:"pretty" choice:"json"`
-	Command bool   `long:"command"`
+	Format string `long:"format" default:"pretty" choice:"pretty" choice:"json"`
+	// Command makes advice try to find snaps that provide this command


s/advice/advise/

bboozzoo · 2018-05-07T07:50:29Z

cmd/snap/cmd_advise.go

+	// Command makes advice try to find snaps that provide this command
+	Command bool `long:"command"`
+
+	// FromApt tells advice that it got started from an apt hook


s/advice/advise/

bboozzoo · 2018-05-08T08:04:59Z

@Conan-Kudo I looked a bit at dnf plugins, but it's not clear how the functionality proposed in this patch could be utilized. i hoped we could use dnf.Plugin.resolved but as far as I understand after going through the documentation (haven't looked at dnf source code yet), the method is called when transaction is successfully resolved. At least, there does not appear to be a way to access the packages that could not be resolved. I'm open to any advice. Do you think we will need to have the dnf plugin API extended a bit?

Conan-Kudo · 2018-05-08T12:47:56Z

@bboozzoo I think it's possible if you file an RFE for it.

Conan-Kudo · 2018-05-09T17:11:12Z

@bboozzoo So I've had a chat with @dmach, and he might be able to help figure out what's needed to extend DNF to make this possible.

zyga · 2018-05-14T10:03:23Z

@mvo5 this branch is failing and I don't see a clear way to undo that. Can you please comment about what you plan to do with it?

mvo5 · 2018-05-15T09:59:16Z

@zyga We need a fix in apt for this. Once that has landed we can go forward with the PR.

mvo5 · 2018-05-15T16:15:09Z

Setting to blocked because we need a fix in apt for this to work.

mvo5 · 2018-06-11T12:13:39Z

Blocked because of https://bugs.launchpad.net/ubuntu/+source/apt/+bug/1776218

codecov-io · 2018-07-19T22:03:36Z

Codecov Report

Merging #5122 into master will decrease coverage by 0.04%.
The diff coverage is 46.55%.

@@            Coverage Diff             @@
##           master    #5122      +/-   ##
==========================================
- Coverage   79.05%   79.01%   -0.05%     
==========================================
  Files         514      514              
  Lines       38943    39001      +58     
==========================================
+ Hits        30787    30816      +29     
- Misses       5678     5692      +14     
- Partials     2478     2493      +15

Impacted Files	Coverage Δ
cmd/snap/cmd_advise.go	`57.14% <46.55%> (-9.04%)`	⬇️
overlord/hookstate/hookmgr.go	`74.39% <0%> (+0.96%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dd8d998...f79d7b9. Read the comment docs.

mvo5 · 2018-07-20T07:57:03Z

This is no longer blocked, the apt in the distro is fixed now.

mvo5 force-pushed the apt-hook branch from 38ce7b4 to 012d51b Compare May 2, 2018 06:42

zyga approved these changes May 4, 2018

View reviewed changes

address review feedback

b23a307

bboozzoo approved these changes May 7, 2018

View reviewed changes

mvo5 added 2 commits May 15, 2018 11:57

Merge remote-tracking branch 'upstream/master' into apt-hook

746826c

cmd_advise.go: s/advice/advise/g

c7edc6c

mvo5 added the ⛔ Blocked label May 15, 2018

mvo5 added 2 commits July 19, 2018 19:06

Merge remote-tracking branch 'upstream/master' into apt-hook

3fb2307

tests: ensure apt is the latest version

4a245af

mvo5 force-pushed the apt-hook branch from 27e1391 to 4a245af Compare July 19, 2018 21:36

mvo5 removed the ⛔ Blocked label Jul 20, 2018

mvo5 added this to the 2.35 milestone Jul 23, 2018

mvo5 added 2 commits July 23, 2018 10:20

Merge remote-tracking branch 'upstream/master' into apt-hook

2a160fb

tests: add apt hook integration test

f79d7b9

mvo5 merged commit 2b3d77e into canonical:master Jul 23, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

snap: add support for `snap advise-snap --from-apt` #5122

snap: add support for `snap advise-snap --from-apt` #5122

mvo5 commented May 1, 2018

Conan-Kudo commented May 2, 2018

mvo5 commented May 3, 2018

Conan-Kudo commented May 3, 2018

Conan-Kudo commented May 3, 2018

zyga commented May 4, 2018

zyga left a comment

zyga May 4, 2018

zyga May 4, 2018

zyga May 4, 2018

zyga May 4, 2018

mvo5 May 7, 2018

zyga May 4, 2018

mvo5 May 7, 2018

zyga May 9, 2018

mvo5 commented May 7, 2018

bboozzoo left a comment

bboozzoo May 7, 2018

bboozzoo May 7, 2018

bboozzoo commented May 8, 2018

Conan-Kudo commented May 8, 2018

Conan-Kudo commented May 9, 2018

zyga commented May 14, 2018

mvo5 commented May 15, 2018

mvo5 commented May 15, 2018

mvo5 commented Jun 11, 2018

codecov-io commented Jul 19, 2018 •

edited

Loading

mvo5 commented Jul 20, 2018

snap: add support for snap advise-snap --from-apt #5122

snap: add support for snap advise-snap --from-apt #5122

Conversation

mvo5 commented May 1, 2018

Conan-Kudo commented May 2, 2018

mvo5 commented May 3, 2018

Conan-Kudo commented May 3, 2018

Conan-Kudo commented May 3, 2018

zyga commented May 4, 2018

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

mvo5 commented May 7, 2018

bboozzoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bboozzoo commented May 8, 2018

Conan-Kudo commented May 8, 2018

Conan-Kudo commented May 9, 2018

zyga commented May 14, 2018

mvo5 commented May 15, 2018

mvo5 commented May 15, 2018

mvo5 commented Jun 11, 2018

codecov-io commented Jul 19, 2018 • edited Loading

Codecov Report

mvo5 commented Jul 20, 2018

snap: add support for `snap advise-snap --from-apt` #5122

snap: add support for `snap advise-snap --from-apt` #5122

codecov-io commented Jul 19, 2018 •

edited

Loading