Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
errtracker: include bits of snap-confine apparmor profile #3421
Conversation
zyga
requested a review
from
mvo5
Jun 1, 2017
zyga
added
the
Critical
label
Jun 1, 2017
chipaca
approved these changes
Jun 1, 2017
Does what it says on the tin.
I'm not sure if it does already, but if not, maybe also include the output of dpkg-query -W -f '${Status}' snapd?
| +// https://forum.snapcraft.io/t/test-failures-with-cannot-create-lock-directory-run-snapd-lock/390/ | ||
| +func detectPartialDpkgUpdate() string { | ||
| + _, err := os.Stat(snapConfineProfileDpkgNew) | ||
| + if err == nil { |
|
Interesting, yes, I like both suggestions. @mvo5 how do you feel about the dpkg-query? |
codecov-io
commented
Jun 1, 2017
•
Edited 1 time
-
codecov-io
Jun 1, 2017
-
Jun 1, 2017
Codecov Report
@@ Coverage Diff @@
## master #3421 +/- ##
==========================================
+ Coverage 77.51% 77.52% +0.01%
==========================================
Files 371 371
Lines 25537 25544 +7
==========================================
+ Hits 19796 19804 +8
Misses 3986 3986
+ Partials 1755 1754 -1
Continue to review full report at Codecov.
|
| +// | ||
| +// This probe is here to aid in resolving the following issue: | ||
| +// https://forum.snapcraft.io/t/test-failures-with-cannot-create-lock-directory-run-snapd-lock/390/ | ||
| +func detectPartialDpkgUpdate() string { |
mvo5
Jun 1, 2017
Collaborator
(nitpick) the name is not ideal, as far as dpkg is concerned its not a partial update, it did what it is supposed to do. Maybe detectStaleSnapConfineApparmorConffile or something?
zyga
Jun 1, 2017
Contributor
AFAIR this is also happening when a partial update (broken mid way, before snapd is configured) happens so this is why I called it like that. I can rename it to something neutral like detectSnapConfineDpkgNew
| +// https://forum.snapcraft.io/t/test-failures-with-cannot-create-lock-directory-run-snapd-lock/390/ | ||
| +func detectPartialDpkgUpdate() string { | ||
| + if osutil.FileExists(snapConfineProfileDpkgNew) { | ||
| + return "dpkg-new file present" |
mvo5
Jun 1, 2017
Collaborator
Maybe the md5 hash of the file instead of this string? This way we might get a clue if all files are modified in the same way and we can compare with the hash of previous deb packages etc.
| @@ -119,6 +135,7 @@ func Report(snap, errMsg, dupSig string, extra map[string]string) (string, error | ||
| "CoreSnapdBuildID": coreBuildID, | ||
| "Date": timeNow().Format(time.ANSIC), | ||
| "Snap": snap, | ||
| + "PartialDpkgUpdate": detectPartialDpkgUpdate(), |
mvo5
Jun 1, 2017
Collaborator
StaleSnapdApparmorConffile maybe? Also we should not add it if its empty I think.
zyga
Jun 1, 2017
Contributor
I think we can do two things:
- show the flag that .dpkg-new is present
- carry md5 of the actual profile
WDYT?
zyga
changed the title from
errtracker: include hints of partial dpkg update in error reports
to
errtracker: include bits of snap-confine apparmor profile
Jun 1, 2017
mvo5
reviewed
Jun 1, 2017
Very nice! two tiny suggestions and then we should squash it and cherry pick in 2.26
| + if err != nil { | ||
| + return "" | ||
| + } | ||
| + return fmt.Sprintf("%x", sha1.Sum(profileText)) |
mvo5
Jun 1, 2017
Collaborator
If we use md5 here we can more easily compare with the dpkg metadata. This way we can look at e.g. dpkg -s snapd and immediately see the md5sums of the conffiles without having to unpack the debs.
| @@ -119,6 +130,8 @@ func Report(snap, errMsg, dupSig string, extra map[string]string) (string, error | ||
| "CoreSnapdBuildID": coreBuildID, | ||
| "Date": timeNow().Format(time.ANSIC), | ||
| "Snap": snap, | ||
| + "SnapConfineCurrent": snapConfineProfileDigest(""), |
mvo5
Jun 1, 2017
Collaborator
Maybe SnapConfineApparmorProfileCurrent to avoid possible confusion with the hash of the actual snap-confine binary? Same below.
zyga
Jun 1, 2017
Contributor
Done, I kept it brief for golang reflow reasons but I called it this way before :)
zyga
Jun 1, 2017
Contributor
Done, I kept it brief for golang reflow reasons but I called it this way before :)
All checks have passed
mvo5
merged commit 8911fd2
into
snapcore:master
Jun 1, 2017
1 of 7 checks passed
added a commit
to zyga/snapd
that referenced
this pull request
Jun 1, 2017
zyga commentedJun 1, 2017
•
Edited 1 time
-
zyga
Jun 1, 2017
This patch extends the error report with information about the used
snap-confine apparmor profile (as expressed by the text of the file) as
well as hint of partial dpkg update.
Partial or failed updates can leave the old (previous) apparmor profile
of snap-confine around (since it is tagged as a conffile) and
subsequently cause any snap execution to fail with access to
"/run/snapd/lock" directory.
To test the theory we will now measure the hashes of the current and any
.dpkg-new versions of the apparmor profile of snap-confine and attach
them to the report.
Signed-off-by: Zygmunt Krynicki zygmunt.krynicki@canonical.com