[00:08] not finished, will carry on tomorrow [00:08] off to zzz :-) [00:54] PR snapd#4835 opened: tests: add bionic system to google backend === thresh_ is now known as thresh [05:27] Good morning [06:07] morning [07:15] re! [07:15] kids are handled, wife at work, dog is sleeping (again) and I can finally get to work :) [07:19] zyga-ubuntu: hey [07:19] hey :-) [07:21] zyga-ubuntu: so the kernel bug with dangling symlinks is real, quite a lot of these came up in your pr [07:21] yes, it's real [07:21] now it's unclear if it is related to the issues we saw [07:21] or if it is just a harmless issue [07:22] zyga-ubuntu: does it also appear in debian sid, or is this part of apparmor ubuntu specific? [07:22] I don't know [07:23] my gut feeling is that it's not specific to ubuntu [07:25] oops, I managed to get an appointment with a doctor at 11 [07:36] good morning mvo [07:36] zyga-ubuntu: hey! good morning [07:39] mvo: hey [07:39] hey mborzecki [07:39] time for 2.32~rc2! [07:43] zyga-ubuntu: sorry for being a pain, what is the (rough) status of the layout fix we need for 2.32? [07:44] no change since last week, I know what the problem is and I worked on the first of the two problems (symlink problem) but put this on hold earlier this week [07:44] I'm adding unit tests [07:44] though I think I will be changing that code a little as well [07:44] zyga-ubuntu: thanks! [07:44] so the status is: known bugs, in progress [07:44] zyga-ubuntu: could we disable parts of the functionality to get there quicker? or is that a) stupid b) not-needed c) all of the above? [07:45] it's not worth it, we need the experimental feature flag anyway [07:45] zyga-ubuntu: aha, ok [07:45] the symlink bug is "don't make symlinks if one is in place and has good value" [07:45] the writability leak problem is "don't write over things that are not tmpfs" [07:46] hm the opensuse kernel that we use has apparmor disabled apparently [07:46] uname shows 4.15.7-x86_64-linode102, linode specific kernel? [07:47] yes, apparently [07:47] anyone working on the vet failures on 18.04? if not I will do now [07:48] damn, kernel-default-4.4.27-2.1.x86_64 this is installed, but it's running a different kernel, pfff [07:48] mvo: i can take a look [07:49] mborzecki: its about the vet failures in the test log of 4835 [07:50] there are some people who want to contribute to the gentoo overlay [07:50] https://github.com/zyga/gentoo-snappy/issues [07:50] some PRs and issues [07:50] PR snapd#4803 closed: snap-confine, snap-seccomp: utilize new seccomp logging features - 2.32 [07:50] PR snapd#4831 closed: tests: force profile re-generation via system-key [07:51] PR snapd#4828 closed: many: support holding refreshes by setting refresh.hold (2.32) [07:52] zyga-ubuntu: how safe is 4830? [07:53] zyga-ubuntu: I am inclinded to merge [07:53] I don't know of any issues [07:53] zyga-ubuntu: this runs in edge for some time, right? [07:53] yes [07:53] I think it's safe [07:53] also on reverts [07:53] since reverting brings the reverted snap-confine [07:53] and its own profile and startup behavior [07:54] the only potential downside is if we made a mistake in the apparmor profiles [07:54] ok [07:54] but nobody complained so far [07:54] PR snapd#4830 closed: many: generate and use per-snap snap-update-ns profile (2.32) [07:55] mvo: hmmmm [07:55] https://github.com/snapcore/snapd/pull/4765 [07:55] PR #4765: interfaces: harden snap-update-ns profile [07:55] actually, I'm drunk [07:55] since that PR you just merged is just the split of the profiles [07:55] this is the hardening [07:56] and this is the part when I meant that maybe we missed something [07:56] the first PR is safe [07:56] mvo: this PR is the interesting one that jamie wanted to see. [07:56] man, I thought we merged it already [07:57] and is full of policy [07:57] mvo: can you review it? [07:58] zyga-ubuntu: yes, in a bit, let me go over the essential bits for 2.32 first [07:58] mvo: I think this is also for 2.32 but I understand if you lack it [07:58] NACK it [07:58] (silly spellchecker on macOS) [08:00] moin moin [08:01] hey kalikiana [08:03] PR snapd#4836 opened: tests: force profile re-generation via system-key === pstolowski|afk is now known as pstolowski [08:06] morning! [08:06] kalikiana: pstolowski: morning guys [08:07] woow, we use testable examples in snap/info_test.go, never seen it being used before [08:07] mborzecki: we want 4826 - right? [08:07] mvo: for 2.32? [08:07] mborzecki: I mean we want it for 2.32 [08:07] mborzecki: yes [08:07] pstolowski: good morning! [08:08] mvo: yes, it'd be nice to include it, it's a bugfix :) [08:08] pedronis: 4825 looks almost ready, just one question from gustavo left afaict? [08:08] mborzecki: cool, lets see if we can find a second reviewer [08:15] zyga-ubuntu: do we know how the feature flag will look like? environment key? [08:16] Core setting [08:16] snap set core experimental.layouts true [08:16] hmm vet does not handle struct methods defined in export_test.go files [08:19] zyga-ubuntu: do you have code for this or shall I add some? asking because I think with that in place ~rc2 would be ready [08:20] zyga-ubuntu: I can work on it [08:29] https://github.com/golang/go/issues/23701 [08:33] mvo: I don't have that yet [08:33] so if you want to pick it up, thanks please do [08:33] mborzecki: hm, this reads as if the issue should be fixed? do we just need a golang update in bionic? [08:34] go version go1.10 linux/amd64 locally and i can still see this [08:34] zyga-ubuntu: yeah, I want 2.32~rc2 today if layout have bugs thats ok, we need one more upload for 2.32(-final) anyway [08:34] ack [08:34] mborzecki: hm, does that have https://go-review.googlesource.com/c/go/+/92215 ? [08:36] mvo: yes, the commit is in 1.10 branch [08:37] :( [08:37] ok [08:37] mborzecki: if you have a fix for the other vet issues, please push [08:41] PR snapd#4837 opened: many: go vet cleanups [08:41] mvo: ^^ [08:41] mborzecki: \o/ [08:42] as for the bug that's supposed to be resolved in 1.10, i can still reproduce it with the sample that's provided in the original ticket on both 1.10 from golang.org download and 1.10 from arch packages [08:43] mborzecki: ok, so we need to reopen upstream? and/or find a local workaround :/ [08:44] and/or disable vet on go1.10 (which would suck) [08:45] i have a sort of a workaround but it's labor intensive :) [08:45] basically changing the struct methods to simple funcs [08:46] and update all call sites [08:47] mborzecki: hm, that sounds slightly nasty [08:50] moin moin [08:51] * Chipaca realises he still has time for more coffee before official start-of-day [08:52] mvo: it isn't that bad https://paste.ubuntu.com/p/ZPgmsKpM6s/ [08:53] mvo: i can push that too [08:53] mborzecki: yeah, that looks reasonable [08:53] mborzecki: unfortunate but reasonable [08:53] Chipaca: hey! good morning! [08:54] Chipaca: morning, more coffee yes, that's a splendid idea :P [08:54] elopio: Hi! I see you did initial snap packaging for keybase client. Are there any blockers on the way, anyway I could help get that finished ? [08:55] * Chipaca hugs elopio [08:56] * Chipaca hugs his coffee, also [08:58] mvo: pushed the workarounds [09:02] need to run an errand, bb in ~1.5h [09:27] * zyga-ubuntu -> doctor [09:27] ttyl [09:40] PR snapd#4838 opened: snapstate: put layout feature behind feature flag [09:57] mvo: commented, thank you for writing that [09:57] * zyga-ubuntu waits at the doctors [09:58] This will take a while (hopefully not too long) [10:04] zyga-ubuntu: thank you [10:08] zyga-ubuntu: heh, spread tests> probably, I guess the spread run will be very unhappy! [10:08] zyga-ubuntu: good catch [10:08] mvo: can you remind me what advise-snap's supposed to do without --command? [10:09] Chipaca: advice on snap name [10:09] mvo: what's the apt equivalent of that one? [10:09] Chipaca: so that "apt install spotify" could say something nice like "The spotify snap is available at version 2.0, see snap info spotify for additional versions" [10:09] ah [10:09] a'ight [10:09] Chipaca: its a bit under speced right now [10:09] Chipaca: we also probably want mispell there [10:09] (which should be trivial) [10:11] speling is trivial indeed [10:17] we really struggle with 'temporarily' [10:17] so far i've found temporarely and temporariy [10:23] Chipaca: *cough* s/we/I/ [10:24] mvo: I will miss standup. I need to do an X-ray now :-( [10:25] Not sure how soon I will be back today [10:25] I can work on reviews from my phone [10:26] zyga-ubuntu: uhh, good luck [10:27] yay, found a bug in go-udev [10:27] pstolowski: that was quick [10:30] zyga-ubuntu: it's a small project and just a few functions really. and fix is trival thankfully [10:37] huh, there's a new rpi [10:38] Yep [10:38] And it has a heat spreader on the chip [10:38] And does PoE with a hat [10:38] That latter thing makes it really interesting but there is no information about the availability of the hat yet [10:39] I didn't know Poe used a hat [10:39] * Chipaca imagines ravens with trilbies [10:40] anyway, bbiab [10:51] Chipaca it is a very stylish poe [10:51] * zyga-ubuntu waits for X-ray [10:55] PR snapd#4839 opened: errtracker: respect the /etc/whoopsie configuration [11:02] mvo: reviewed [11:03] mvo, it looks like the root reason for the bug we chatted around yesterday is device cgroups: https://forum.snapcraft.io/t/race-condition-between-snapd-and-nm-when-providing-permissions/4451/3 [11:03] zyga-ubuntu, ^^ [11:03] Hey [11:03] zyga-ubuntu, o/ [11:03] I’m AFK so sorry if I don’t respond quickly [11:04] Let me read the forum thread [11:04] thanks [11:04] So a quick question [11:04] What created the additional device nodes [11:05] zyga-ubuntu, a script that needs to set some GPIO to power on the device. That takes a few seconds [11:05] It feels like we are missing an udev rule that adds tags such devices [11:05] Is it udev tagged by any part of network manager interface? [11:05] the udev rule is fine, it tags /dev/tty* [11:06] it is in the ppp interface, used by NM [11:06] Can you please paste the udev profile generates by snapd for this snap [11:06] sure [11:06] Thanks [11:06] * zyga-ubuntu only has a phone with him [11:07] zyga-ubuntu, https://pastebin.canonical.com/p/jGBrPdkZZq/ [11:08] Aww [11:08] My token is at home [11:08] Is it sensitive? [11:08] zyga-ubuntu, nah, not really: https://paste.ubuntu.com/p/sGWRcj7Ts4/ [11:09] who creates the device cgroup, snap-confine? [11:09] Yes [11:10] it would make sense that it adds only the present devices [11:10] and if a new one appears, apparmor rules are fine thanks to udev, but not the device cgroup [11:10] which is not updated [11:11] re [11:11] (well, not thanks to udev, it is just that apparmor has a tty*) [11:13] Re [11:13] Well it should be updated [11:13] Look at those rules [11:13] We run a tool to change the device cgroup as devices come and go [11:14] Unless we are hit by a bug [11:14] We renamed that tool now [11:15] hm then a bug must be [11:15] which is that tool? [11:15] Can you confirm it exists in your system with the given path you see in the udev rule [11:15] Read the rule please [11:15] It is raining [11:15] And typing on a phone is not easy [11:16] sure :) [11:16] ok, /usr/lib/snapd/snappy-app-dev [11:19] Yes [11:19] zyga-ubuntu, mvo is it possible that core in candidate has been recently upgraded? now I cannot reproduce the issue anymore, although it is a race so I am not 100% sure [11:20] abeato: candidate got updated on monday [11:20] abeato: from 2.31.1 to 2.31.2 [11:20] hm... [11:20] abeato: http://people.canonical.com/~mvo/core-changes/html/candidate/2.31.1r4110_2.31.2r4206.html [11:21] * abeato trying to reproduce again [11:23] abeato: do you see this on core or on classic [11:23] core [11:23] I see [11:23] Hmm hmm [11:23] Well [11:23] If it shows up please tell us [11:23] yup [11:24] To the best of our knowledge the system supports devices that are added after startup [11:24] One possible thing to check for would be a race during cgroup construction [11:24] That may indeed be buggy [11:24] I would have to look at the code to confirm this [11:25] Perhaps the tool should be grabbing the per-snap lock [11:25] As that would probably rule out the race [11:25] I can control when the device is created, will tell you if I can reproduce that way in 1min [11:25] So to be clear [11:26] If the udev event fires at the same moment as the first process of that snap [11:26] We may have an issue in that specific case [11:26] Ok [11:26] I have my X-ray but results won’t be available till 4PM, I’m heading home [11:30] zyga-ubuntu, definitely I can reproduce (ping me when you are back again) [11:31] Do can you reproduce that and capture udev logs as things happen? [11:31] yes [11:31] Please add all findings to the forum thread [11:32] will do, thanks [11:32] And can you look at the resulting cgroup and ensure the devices are not there [11:32] I wonder if we can make the small helper tool log something [11:32] that's how I checked that I reproduced it :) [11:33] the new devices are not in devices.list [11:33] (and I got the EPERM too) [11:33] Ok [11:34] Which device is that? The serial port? [11:34] /dev/ttyACM0 from a modem [11:35] abeato: is there any indication in dmesg that the port may go away and appear again? [11:35] The rule looks ol [11:35] Can you check one more thing [11:35] Run the tool as udev would [11:35] mborzecki, it appears and it is kept there [11:36] Pass the right tings (cannot tell you from the top of my head) [11:36] And see if it gets added [11:36] sure, let me try that [11:39] zyga-ubuntu, https://paste.ubuntu.com/p/hZFQ5XS62g/ :/ [11:40] Woah [11:40] That looks like a bug [11:40] Is the tool anywhere in the filesystem? [11:40] /lib/udev/snappy-app-dev [11:40] bad path [11:40] zyga-ubuntu: just a quick update before I bail, I have a patch thats is a wip, I should have something for you to test tomorrow (today?) sometime [11:41] zyga-ubuntu, so there is a bad path in the udev rule, that's it [11:41] That is great news jjohansen. Thank you for getting this so quickly! I can test the patch when you have it [11:42] Mvo: we need a small fix for what abeato just found [11:42] abeato: which version is that? [11:42] zyga-ubuntu, 4209 [11:43] And in snap —version parlance? [11:43] zyga-ubuntu: oh, what fix is needed? [11:43] jjohansen: is there any indication that the bug may have other side effects? [11:43] zyga-ubuntu, https://paste.ubuntu.com/p/tWkMJRP84Y/ [11:43] mvo: udev rules use a wrong path to the helper tool [11:44] Woah [11:44] mvo, /usr/lib/snapd/snappy-app-dev -> /lib/udev/snappy-app-dev [11:44] abeato your snapd is wonky [11:44] Can you reproduced that on vanilla 2.31.2 [11:44] I will be home soon [11:44] ~40 minutes maybe [11:45] abeato: this is slightly strange, the snapd version is pretty out of sync with the snap version [11:45] (That not super soon though) [11:45] zyga-ubuntu, mvo well, it is the core from candidate, no hacks I promish [11:45] That is cery ul ujęły [11:45] Unlikely [11:46] Did you push a locally built snapd over? [11:46] abeato: when I grep snappy-app-dev in master I get only a compat symlink hit, I'm a bit puzzled why its looking in /usr/lib/snapd/snappy-app-dev, is that in the udev rule? or where do you see it? [11:47] It is in the udev rule but it looks like the snapd there is old or at least based on an old dirty build [11:48] zyga-ubuntu, hm, wait, I remember I pushed one snapd and mounted it on boot to measure a performance issue [11:48] let me remove that [11:48] Thank you! [11:56] PR snapd#4840 opened: many: add `core.problem-reports.disabled` option [11:56] heh, go vet is flaky in 1.10 and even more so in master, this is what I get with master atm: https://paste.ubuntu.com/p/B2vc9F3CGr/ using a test case https://github.com/golang/go/issues/23701#issuecomment-363187224 [11:57] mwhudson: hm, do you have an opinion about go vet in bionic? we have some problems with 1.10 -^ [11:57] mborzecki: maybe the default of 1.10 is a bit too aggressive :/ [11:58] there will be an update to 1.10.1 though? [11:58] i think i'd prefer a slightly broken version if that helps from getting stuck with 1.9 for the next 5 years [11:59] mborzecki: I guess, worst case is that we disable vet on 1.10 for now [11:59] mvo: the workarounds are there, we can revert them once we know it's fixed again [11:59] zyga-ubuntu, mvo after reverting the snapd version I see the right path in /etc/udev/rules.d/70-snap.network-manager.rules, so this was my fault. Sorry for the noise [12:01] mborzecki: aha, ok. I thought you mentioned there are additional errors, am I outdated :) ? [12:01] abeato: no worries, thanks for reporting it, better this way than if we let a bug escape :) [12:02] at least I've learned a bit about device cgroups ;) [12:02] mvo: i mean i tried go master, built it and was able to reproduced the problem using a test case posted in the original ticket, got a nice backtrace from go internals :P at least 1.10 did not panic [12:04] abeato: thank you for confirming that! [12:04] mvo: alarm is over :) [12:05] zyga-ubuntu, thank you in fact [12:08] and I'm back [12:08] resuming work on fixes [12:08] mborzecki: ok [12:08] zyga-ubuntu: thank you for handling it! [12:26] PR snapcraft#1983 closed: snap: update revision of patchelf to use [12:54] PR snapd#4841 opened: snap/pack, cmd/snap: add `snap pack --check-skeleton` [13:39] Chipaca: would you mind if I bother you with a symlinkat review later today? [13:39] niemeyer: that's the project I was referring to: https://github.com/pilebones/go-udev [13:39] pstolowski: Yeah, that's one I looked into as well, given my browsing history [13:39] pstolowski: Looking for others now [13:40] zyga-ubuntu: yes, I'd be very upset with myself if you asking me for a review bothered me [13:43] niemeyer: wrt the 'halp' pr (#4809) your comments on 'snap pack' seem to imply you want a change of behaviour of snap pack [13:43] PR #4809: cmd/snap: tweak and polish help strings [13:43] Chipaca: Hmm.. it wasn't intentional at least [13:43] Chipaca: What did I miss? [13:43] i won't be working on that pr until this evening so there's no rush, but [13:43] niemeyer: 'snap pack' already defaults both arguments to '.' [13:43] Chipaca: Huh, ok [13:44] Chipaca: So what's the delta with try? [13:44] Chipaca: That it doens't look for "prime"? [13:45] niemeyer: essentially yes, but also it only sets it to . if there's a meta/snap.yaml in . [13:45] and it only sets it to prime if there's a snapcraft.yaml in one of the usual places [13:45] Chipaca: Okay, I indeed missed those details.. so +1 on your suggestion [13:45] (curiously it doesn't look for meta/snap.yaml in the prime directory, but that's a tweak) [13:46] Chipaca: I think we should ignore snapcraft.yaml and just look for meta/snap.yaml as you suggest [13:46] niemeyer: it's still slightly awkard that it'll happily drop a snap inside the snap (and there are snaps with snaps in them in the store, so it's a real problem) [13:46] (in prime, in .) [13:46] Chipaca: Is there a way to make snap pack ignore those? [13:47] * niemeyer --help [13:47] niemeyer: not currently; snapcraft has an ignore list, but not snap pack [13:48] OTOH most people will be using it via snapcraft so it'll be fine :-) [13:48] anyway, glad that's cleared up, i'll tweak the text and the behaviour (but possibly in a followup) [13:48] this evening though; now to the cache [13:49] hm, maybe, maybe target should default to the parent dir of snap-dir [13:49] Chipaca: or soem /tmp location [13:50] mborzecki: it might not be the best example, but dpkg-buildpackage plops the debs in .. [13:50] Chipaca: Yep, -e works [13:50] Chipaca: Although it's completely undocumented apparently [13:50] Chipaca: -e [13:50] Chipaca: I suggest using that for any *.snap in the root directory [13:50] niemeyer: fair 'nuff [13:51] Chipaca: I've seen actual snaps shipping the prior revision of their own snaps [13:51] niemeyer: IKR [13:51] Chipaca: Built with snapcraft [13:51] So it looks like a common issue [13:52] In the rare circumstances where people do want to ship snaps, they can just put it in a subdir [13:53] niemeyer: https://i.imgflip.com/26d5u0.jpg [13:53] Yeah :) [13:53] * Chipaca forgets the whole conversation and moves to the hash cache stash [13:57] mvo, I have tried the upgrade test issue with the last bionic image in google and I see the same problem [13:57] all the snaps appear as broken [13:58] mvo, any idea? [13:58] I could get a debug session with the error [14:03] my / has inode number 2 [14:03] how [14:03] * Chipaca doesn't really care and gets back to work [14:04] cachio: yeah, happy to get into a debug session [14:04] cachio: what did you run to trigger it? [14:05] mvo, https://github.com/snapcore/snapd/pull/4835 [14:05] PR #4835: tests: add bionic system to google backend [14:05] this branch [14:05] mvo, but using this image [14:05] ubuntu-os-cloud-devel/daily-ubuntu-1804-bionic-v20180307 [14:05]