=== chihchun is now known as chihchun_afk === chihchun_afk is now known as chihchun [04:08] PR snapcraft#2354 closed: Preflight missing multipass === chihchun is now known as chihchun_afk [06:14] morning [06:22] Hey [06:22] mborzecki: how are you [06:23] zyga|afk: hey, feeling better today [07:08] zyga|afk: noticed that removable-media allows acessing/wiritng to /mnt, i was always under the impression that only {/run}/media is allowed there [07:10] mvo: morning [07:10] mborzecki: good morning! hope you feel better [07:10] zyga|afk: hey, anything new from the aa bug? [07:11] mvo: yes, way better today [07:14] mborzecki: great! [07:21] zyga|afk: when you're here, can you take a look at https://forum.snapcraft.io/t/the-removable-media-interface/7910 I've added /mnt there since we support it, but did not mention anything about mount propagation since the interface does not allow mounting anyway === chihchun_afk is now known as chihchun === pstolowski|afk is now known as pstolowski [07:58] hey [07:59] pstolowski: hey [07:59] mborzecki: hey, how are you, feeling better? [07:59] pstolowski: yeah, thanks [08:01] zyga|afk: woah, I got "cannot create temporary directory for /var/lib/snapd mount point: Permission denied" now on first try of mvo5/run-fontconfig-2.36 [08:05] popey: i took your advice and made a screenshot https://snapcraft.io/go [08:07] PR snapd#6223 closed: cmd/libsnap: move apparmor-support to libsnap [08:07] mwhudson: nice, should include go env output too [08:08] mborzecki: you think? i most of it is pretty boring [08:08] +think [08:08] mvo: hi [08:09] mwhudson: maybe go env GOROOT GOTOOLDIR? [08:09] mvo: anything I can help with? [08:10] mborzecki: yeah, might make sense [08:10] the whole process is a bit tedious tbh, but if i redo it for some reason :) [08:11] mwhudson: that's asciinema right? [08:11] pedronis: good morning - we are still struggling with the apparmor failure in 2.36. I did some work on the --trace-exec in 6185, please have a look but I can do more just got distracted because of the aa issue [08:11] mborzecki: yes, plus hacking to smooth out the typing [08:11] pedronis: but 6185 should be cleaner than before [08:11] mwhudson: I love that! [08:11] mvo: ok, do we need to have a chat about the aa issues with zyga as well? [08:12] mwhudson: mhm, very nice! [08:12] pedronis: maybe, I think he had some new ideas since last night, but I don't know more details yet [08:12] ok [08:13] mwhudson: I hope it's not so big as to trigger this though ;) https://gitlab.gnome.org/GNOME/gnome-software/issues/532 [08:13] popey: heh no [08:13] pedronis: 5845 needs a second review (the files interface) but thats probably in good hands with jamie and zyga [08:14] pstolowski: are you blocked atm? my and mvo days are a bit full of meetings and we have the 2.36 troubles [08:14] pstolowski: I still would like the 3 of us to talk about hot plug next step this week tough, but tomorrow might be better [08:30] pedronis: not really, it can wait till tomorrow (not sure i can get 2nd review of the hotplug disconnect branchs from mvo under these circumstances anyway). i've also been working on one of the 2.36 blockers [08:32] pstolowski: ok, btw I answered in https://bugs.launchpad.net/snapd/+bug/1777121 it makes sense to pick it up, make a card and work on it when you have time [08:33] Bug #1777121: Remove is called after snap services are stopped [08:33] pedronis: great, i'll take it [09:05] pstolowski: can I help somehow with that panic? [09:06] pedronis: no, thanks, i think that's fixed, except cannot land due to other random test failures [09:07] pedronis: the panic is interessting, mocking the usr.lib.snapd.snap-confine.real makes the problem go away - it almost looks like there might be a connection to the other bug we are looking at [09:07] Yep [09:07] Hello [09:07] pstolowski: could it be that setup profiles for some reason looks at the real /snap/core/current/... usr.lib.snapd.snap-confine.real and dies there? [09:07] Sorry for starting late. Daughter woes [09:07] hey zyga|afk [09:07] zyga|afk: hey [09:08] I’m taking bit out but heading home now [09:08] I will read backlog [09:08] mvo: no, it's not looking at real one; it's for sure looking at /tmp/.../check-xyz/snap/core/ (i had debug logs confirming this) [09:08] pstolowski: ok [09:09] it's curious why it wasn't failing before though, not sure what changed. or if we were plain lucky before [09:11] Hahhahhahh [09:11] You will have fun knowing why [09:11] Man [09:12] Sorry still not at my kb === chihchun is now known as chihchun_afk [09:14] There [09:14] Solved it [09:16] zyga|afk: can't wait to hear the details [09:16] zyga|afk: does it also shed some light on the other issue ? [09:17] can't wait to that as well :) [09:20] mvo: is there any other 2.36 issue i can help with? [09:21] pstolowski: 6185 needs a second review, we could pull this into 2.36 [09:21] pstolowski: but it first needs a merge into master :) [09:36] mvo, zyga|afk https://github.com/snapcore/snapd/pull/6219 is green now ; needs 2nd review [09:36] PR #6219: overlord/tests: fix panic in managers test [09:37] now sure how relevant it is with zyga|afk's findings about setup-profiles [09:38] pstolowski: I'm looking at the exact error now, I have it in a VM and it seems to be reliable to reproduce [09:38] mvo: is it cosmic? [09:38] re [09:38] pstolowski: 16.04 === zyga|afk is now known as zyga [09:38] give me a moment please [09:41] so, the 1000ft version is that we never ever mocked security backends [09:42] and that's not good [09:42] and stuff didn't blow up because we 100% ignored all errors [09:42] so running overlord tests would really run apparmor parser [09:42] reload udev [09:42] and all the other stuff [09:43] including setting up all snaps on manager startup [09:43] including initializing the apparmor backend [09:43] errors:[]state.taskError{state.taskError{task:"Setup snap \"some-snap\" (40) security profiles", error:"cannot setup apparmor for snap \"core\": cannot create host snap-confine apparmor configuration: cannot compute snap-confine profile: cannot open apparmor profile for vanilla snap-confine: open /tmp/check-2391705272889139391/162/snap/core/1/etc/apparmor.d/usr.lib.snapd.snap-confine: no such file or directory" [09:43] and setting up snap-confine profile when core was being setup [09:43] zyga: so yes :) [09:43] so [09:43] the reason that pawel's branch fixed the panic [09:44] is that it made part of the apparmor.Backend.Setup code happy [09:44] zyga: where? I'm quite sure we tried to mock aa parser [09:44] I realised while outside that the only reason this can have an effect [09:44] is that we ran with the full backend [09:44] pedronis: managers_test [09:44] it just spawns the full interface manager and carries on [09:44] ms.aa = testutil.MockCommand(c, "apparmor_parser", "") [09:44] etc [09:45] zyga: I think this is understood now, thank you! the next question is why http://paste.ubuntu.com/p/fv5fsh3PDy/ makes the error to setup the aa profile basicly go away [09:45] [09:45] zyga: we do mock apparmor_parser though (at least in these tests i worked on) [09:45] maybe it doesn't work, but for sure it was trying [09:45] that's only part of the story, we should not run those tests with real backends [09:45] zyga: ? [09:45] they are meant to be quite integrationy [09:45] that seems a bit of a broad statement [09:46] then we need much more preparation in that phase, right now a small tweak in backend needs to be reflected in extra setup or mocking in the overlord test [09:46] pedronis: the error we get in the test is actually that it can't open the core template profile, so this happens before aa_parser (just for context) [09:46] anyway, please let me explore [09:46] yep [09:46] so we need to either: [09:46] mvo: yes, so we have some code that doesn't respect dirs root [09:46] 1) prepare a full blown environemnt that each backend expects [09:46] or soemthing [09:47] I'm just trying to counter that it wasn't trying to mock things [09:47] pedronis: it does respect them, thats the problem, there is nothing there so it fails to open the profile [09:47] pedronis: yeah, that is correct - we do mock stuff [09:47] 2) ask each backend to mock itself properly so that it can kind of run but have no real effect [09:47] 3) not run with real backends at that stage [09:47] we mock things in the wrong place, the overlord doens't know how to mock apparmor [09:47] mocking one command is not enough [09:48] zyga: assume I understand what you are saying [09:48] the responsibility of knowing how to mock is in the backend itself, not in a test far far away [09:50] zyga: is there a clean way to turn off the backends? would those tests still pass as they are? [09:50] e.g. calling backend.Initialize from apparmor interrogates the system about nfs and overlayfs [09:50] that's not mocked in any way [09:50] pedronis: sure, we do that in the interface manager tests [09:50] pedronis: I was surprised this is not done so [09:51] partially the question is what do we want to do in those tests [09:51] are they integration tests? [09:51] are they tests for the overlord? [09:51] zyga: the name tells you what they try to do [09:51] they try to test the integration of more than one manager [09:51] if they are integration tests then running with real managers, even if their activity is mocked feels wasteful since we don't observe what they do [09:51] pedronis: that's fine [09:52] keeping the manager is ok [09:52] zyga: anyway I told you a very actionable thing, is there a clean way to turn off the backends? would those tests still pass as they are? [09:52] but if we don't check what is the impact of apparmor or seccomp part of the interface manager, perhaps we should not use real backends [09:52] you answered the first bit [09:52] let's see about the 2nd, no? [09:52] yes, checking that now [09:52] hold on [09:52] I think this is interessting and we need to look at this but it does not (afaict) help with the apparmor_parser getting killed issue we have in 2.36 [09:53] mvo: does it get killed on some distros or all distros? [09:53] when does it get killed? [09:53] pedronis: so far I only saw it on 16.04 [09:53] I mean in which tests [09:53] pedronis: it gets killed when it tries to setup the snap-confine security profiles on the hosts [09:53] in a spread test? unit test? [09:53] randomly? [09:54] pedronis: I see it in tests/main/layout and tests/main/parallel-install-layout - spread tests [09:54] ok, spread test [09:54] pedronis: let me try to find out if it happens in more [09:54] anywy, yes then is not related [09:54] by def [09:55] pedronis: making the error from a notify log to a real error changes the outcome for some reason, its super strange but with http://paste.ubuntu.com/p/kksmSpN95H/ I cannot reproduce the issue anymore [09:56] mvo: there are probably two calls? and the first fails benigly and the 2nd days? [09:56] that would explain why that would make a difference [09:57] pedronis: also failure seems slightly random, in GH we see it also in parallel-install-interfaces-content and snap-env but all 16.04 [09:57] mvo: there is one more place [09:57] mvo: actually two, you just have an older patch [09:58] mvo: the key place is actually in interface manager initialize when we redo security when system key changes, this is still ignored in the older copy you have [09:58] zyga: ok, where can I find the correct one? [09:59] zyga: what is super strange is that literally on the first try of run-fontconfig-2.36 I hit the bug [09:59] zyga: then I added the diff and run again and had ~4-5 runs since and nothing triggers it === chihchun_afk is now known as chihchun [10:08] pedronis: all tests pass [10:08] zyga: good, can you propose something that removes the bad mocking and does the right mocking? so we can look at it [10:08] yep [10:09] doing that now [10:12] mvo: I added a couple of questions to #6185 [10:13] pedronis: thank you, checking [10:13] PR #6185: snap: add new `snap run --trace-exec` call [10:14] mvo: sorry, missed your question [10:14] mvo: there's a patch with logging -> errors on GH but I found one more place where that happens, haven't pushed that part [10:20] pedronis: https://github.com/snapcore/snapd/pull/6226 [10:20] PR #6226: overlord: mock security backends for testing [10:20] let's see how this runs [10:20] PR snapd#6226 opened: overlord: mock security backends for testing (2.36) [10:22] zyga: thank you! [10:23] mvo: re errors vs logging, I checked my patches from last night and they are all up to date [10:23] sorry for the noise, still drinking my first coffee [10:24] zyga: no worries [10:24] what remains a bit of a mystery is the no-output error from apparmor parser [10:24] zyga: I reverted to see if I get the failure without the change or if I'm hunting a ghost [10:24] cookl [10:25] zyga: yeah, especially since we set SNAPD_DEBUG afaik in the tests so we should see output [10:25] I restarted my tests, if I can get it to happen again I'd like to look [10:26] I wonder if it is possible that mocked no-op apparmor_parser is _somehow_ leaking from unit tests into the state of the machine where they execute [10:30] mvo: I will now look at the profile compatibility issue on leap [10:30] mvo: I think we should not compile snap-confine profile if snap-confine in the distribution has disabled apparmor [10:31] zyga: MockCommand is based on setting PATH so it should't go further than unit tests [10:31] pedronis: yeah but maybe some magic sequence of bugs or something [10:31] mvo: alternatively we can look at apparmor parser version and skip it this way, it should be the same outcome [10:31] s/be/have/ [10:32] zyga: first run without the diff that makes it a real error and I hit the issue again [10:32] zyga: "Nov 28 10:32:00 nov281009-194489 snapd[29337]: backend.go:312: cannot create host snap-confine apparmor configuration: cannot reload snap-confine apparmor profile: cannot load apparmor profiles: signal: terminated [10:32] " [10:32] Nov 28 10:32:00 nov281009-194489 snapd[29337]: apparmor_parser output: [10:33] can you check one thing [10:33] if you have shell still [10:33] I do [10:33] look at what apparmor_parser is [10:33] is it the real deal [10:33] also look at journalctl [10:33] zyga: https://paste.ubuntu.com/p/BvBvkxP4Mn/ [10:34] if the parser is loading stuff into the kernel it will trigger an audit event [10:34] so you may match the timestamp above [10:34] to an audit even [10:34] (looks allright) [10:35] zyga: https://paste.ubuntu.com/p/KWYSKPVgJY/ - looks like audit is not showing that the snap-cofine profile is loaded [10:36] indeed [10:36] anything around the timestamp of [10:36] "Nov 28 10:32:00 nov281009-194489 [10:37] zyga: https://paste.ubuntu.com/p/6wbYy8psFb/ [10:38] hum [10:38] as if nothing had happened [10:38] zyga: I will try to run the apparmor_parser manual now [10:38] mvo: crazy idea, apparmo_parser -> apparmor_parser.real, log all invocations [10:39] can you reliably reproduce it? [10:39] which test was it that failed now [10:39] zyga: the test is pretty random [10:39] zyga: but without your diff I hit it 2/2 so far [10:39] zyga: today [10:39] zyga: via the mvo5/run-fontconfig-2.36 [10:40] zyga: I think plain upstream/release/2.36 will also work, takes ~100 tests and then it happens [10:42] mvo: I will try your branch now [10:45] zyga: silly question, what is our cache dir again? [10:45] which cache? [10:46] apparmor? [10:46] zyga: apparmor cache for the re-exec thing [10:46] we use /var/cache/apparmor [10:46] for all our profiles [10:46] we no longer use any other paths [10:46] zyga: thanks [10:47] note: we still use the other place for that single profile that ships with the package [10:47] zyga: and running it by hand - works :( [10:47] yeah, I ran it by hand once in a debug session [10:47] it's maddening [10:47] wish I had a rollback machine to get a few seconds into the past [10:47] mvo: so FYI, /etc/apparmor.d/cache/ [10:47] because cache is in etc [10:49] pstolowski: mvo: I created a meeting for tomorrow, let me know if it doesn't work [10:50] pedronis: thanks, it's fine [10:50] pedronis: thank you [11:04] brb, it's super cold in the office today [11:05] zyga: see you - I add the wrapper thing now === cpaelzer_ is now known as cpaelzer [11:14] back [11:14] mvo: https://github.com/snapcore/snapd/pull/6226 is green! [11:14] PR #6226: overlord: mock security backends for testing (2.36) [11:14] pedronis: can you look please, [11:14] I think this is the dealbreaker [11:14] though it doesn't explain why things fail in non-unit tests [11:15] those also passed on 1st run :) [11:15] virtually unheard of :) [11:16] zyga: do we have other places outside of overlord that do overlord.New ? and need the same mocking [11:16] I think daemon might [11:17] I checked all of overlord but indeed, daemon might [11:17] looking now [11:17] zyga: +1 with this kind of questions [11:21] pedronis: yes, adding the same treatment to daemon now [11:22] zyga: devicestate/firstboot_test.go seems also to create a full overlord [11:22] for some tests [11:23] I added a printf that logs the added backends, I'll check that we are good across the tree [11:24] actually, it seems daemon did this already [11:24] * zyga double checks [11:25] though not for all suites [11:30] zyga: oh, fun [11:30] mvo: any luck with apparmor_parser? [11:31] zyga: not yet [11:31] zyga: still running [11:31] zyga: green is also annoying - why is it green, it should fail with the same appamor issue [11:32] zyga: I mean, it should fail with permission denied at a random place when the apparmor_profile canot be (re)loaded [11:33] yes :/ [11:33] odd observation [11:33] go test [11:33] I see stdout [11:33] go test ./... [11:33] I don't see stdout! [11:36] zyga: I remember I discussed that with john a while ago, it was strange go testrunner setup iirc [11:44] everything is mocked now, let's do a master version of that [11:47] PR snapd#6227 opened: overlord,daemon: mock security backends for testing [11:49] zyga: with wrapper -> no error [11:49] seriously [11:49] how can that be!?! [11:50] maybe just back luck? [11:50] *bad [11:51] zyga: maybe, I run it again [11:51] zyga: the mock security backends got cherry-picked, right? [11:51] mvo: meaning? [11:51] I opened a 2.36 and master PRs [11:51] same patch [11:51] zyga: I mean, between 2.36 and master it can be chrry picked? [11:51] yes [11:51] zyga: great [11:51] two PRs are in flight now [11:52] zyga: ok [11:52] zyga: did you restart the 2.36? [11:52] yes [11:52] well [11:52] I pushed to include daemon mocking and devicestate mocking [11:52] so same patch in both places, one was new one was force pushed over the old one [11:53] zyga: ok [11:58] PR snapd#6228 opened: snapstate,overlord: update fontconfig caches with overlord mocking (2.36) [12:01] zyga: second run with wrapper starts now [12:08] mvo: I'm fixing leap bug [12:08] mvo: I'm a bit unsure if we should land https://github.com/snapcore/snapd/pull/6221 [12:08] PR #6221: interfaces: return security setup errors (2.36) <⛔ Blocked> [12:08] the risk is breaking snapd startup [12:09] zyga: got the error: https://paste.ubuntu.com/p/r54VmSGf37/ [12:09] zyga: yeah, this is why I set it to blocked [12:09] perhaps we should still ignore this error: https://github.com/snapcore/snapd/blob/master/overlord/ifacestate/helpers.go#L187 [12:09] looking [12:09] zyga: I think this needs some more work, breaking on startup is not ideal [12:10] I checked my PR, it's not blocking startup [12:10] zyga: hm, actually the real error is earlier, let me dig some more [12:10] memory of what I pushed last evening is rusty [12:10] mvo: is that the only invocation? [12:10] or last one [12:11] zyga: the last one, a gazillon before [12:11] aha [12:11] zyga: I thik `--replace --write-cache -O no-expr-simplify --cache-loc=/var/cache/apparmor /var/lib/snapd/apparmor/profiles/snap-confine.core.6022` is the failing one but the script needs to be smarter [12:11] maybe patch the wrapper to log the error too [12:11] zyga: yeah [12:11] cool [12:11] suggestion [12:11] when it fails copy the non -- arguments to /tmp/WAT/ [12:11] for inspection [12:11] fingers crossed [12:14] pstolowski, mvo: shall we close https://github.com/snapcore/snapd/pull/6219 [12:15] PR #6219: overlord/tests: fix panic in managers test <⛔ Blocked> [12:18] closed [12:18] PR snapd#6219 closed: overlord/tests: fix panic in managers test <⛔ Blocked> [12:19] pstolowski: thank you for writing that, without that PR I would never think that that code is using apparmor backend [12:21] yeah, thanks pstolowski and zyga for this one - now we just need to figure the appamor thing out to make me truly happy [12:21] yep, that an interesting one ;) [12:38] mvo: the branches are green [12:38] shall we merge https://github.com/snapcore/snapd/pull/6226 [12:38] PR #6226: overlord,daemon: mock security backends for testing (2.36) [12:38] and https://github.com/snapcore/snapd/pull/6227 [12:38] PR #6227: overlord,daemon: mock security backends for testing [12:46] pedronis: ^ ? [12:48] one sec [12:49] zyga: did you rebase it? [12:49] pedronis: the one on master, yes [12:50] well seems you force pushed the one on 2.36 as well [12:50] yes, with daemon and devicestate changes [12:50] I wanted one patch for master [12:53] zyga: do we need the devicestate changes? it doesn't seem to use the interface manager [12:54] pedronis: it spawns the overlord, that initializes apparmor backend doing some work [12:54] where? [12:54] just creating the overlord is enough, then that adds the interface manager, that does the rest [12:55] I tested this with a printf next to AddBackend in the interface manager initialization function [12:55] (printing each security backend being added) [12:55] zyga: it creates only a Mock overlord [12:55] we do that all over tha place [12:55] so we need to know because there might be more [12:55] Mock is safe [12:55] let me double check [12:56] I don't see a overlord.New there [12:56] but maybe I'm missing something [12:57] zyga: I'm talking devicestate_test.go to be clear [12:57] firstboot_test.go does create a full overlord [12:58] testing... [12:58]