[06:16] morning [08:02] good morning [08:02] mborzecki: a lower priority thing I sent last week: https://github.com/snapcore/snapd/pull/6251 - the refactoring you asked for [08:02] PR #6251: cmd/snap-confine: refactor calling snapd tools into helper module [08:03] mborzecki: I'd like to mainly land the 2.36 branches: https://github.com/snapcore/snapd/pull/6245 and https://github.com/snapcore/snapd/pull/6235 [08:03] PR #6245: interfaces/backends: detect too old apparmor_parser (2.36) [08:03] PR #6235: overlord,apparmor: new syskey behaviour + non-ignored snap-confine profile errors (2.36) [08:03] I will focus on https://github.com/snapcore/snapd/pull/6190, sorting out my travel [08:03] PR #6190: overlord/configstate,features: expose features to snapd tools <⛔ Blocked> [08:03] I can assist in any reviews === pstolowski|afk is now known as pstolowski [08:05] mornings [08:07] hey pawel [08:07] I guess this morning is just us three, no? [08:15] i think so, yes [08:18] zyga: pstolowski: morning guys [08:18] :-) [08:18] the week of short standups [08:19] hey mborzecki [08:19] zyga: https://github.com/systemd/systemd/issues/10872#issuecomment-443504757 i'm building it right now [08:19] looking :) [08:19] yeah, i'll likely skip today's, my daughter has a dentist apointment at 15:20 [08:20] hmmm [08:20] pstolowski: ok [08:20] pstolowski: I'll bug you for some reviews [08:20] since it's just the three of us [08:21] sure [08:40] good morning everyone [08:46] hey dot-tobias [09:05] zyga: thanks for the review of #6180 ; i'm not sure what to do about public SnapGlobal/Explicit tbh [09:05] PR #6180: snap/info: bind global plugs/slots to implicit hooks [09:06] pstolowski: I tried myself [09:06] and I found two ways out [09:06] maybe three if we rewrite tests :) [09:06] number one is to write a new DeepEquals that ignores private fields [09:06] I think it is interesting in principle [09:06] the second idea is to keep this as is [09:06] I think that's what we should do [09:07] I would change the name of the variables a little, I wasn't sure what to call them [09:07] ideas welcome [09:08] zyga: i don't think DeepEquals should ever ignore private feels, it feels like it's no longer deep equals ;) [09:08] *private fields [09:08] it would be some new comparator, sure [09:09] but it would help in cases like that [09:09] PublicEquals [09:09] or something ish [09:11] degville: another thing I found valuable today https://www.youtube.com/watch?v=vtIzMaLkCaM :-) [09:11] zyga: i'd keep it as is and continue in a followup [09:11] ^ recommend watching that [09:11] pstolowski: yes but please before landing, let's rename the new variables [09:11] they don't feel good (sorry for being vague) [09:12] zyga: i mean yes, sure, changing names is fine [09:12] maybe a 3 minute brainstorm with mborzecki could help [09:12] hm? [09:12] mborzecki: we need two variable names [09:12] mborzecki: one will tell you that a hook was explicitly defined in yaml, rather than being found in a specific directory on disk [09:13] mborzecki: another will tell you that a specific plug or slot affects all apps and hooks in a snap, rather than being associated with just a subset of them [09:13] mborzecki: how would you name those two? [09:13] Is it possible to detect within my snap if a required interface is already connected? A service in my snap errors out right after the snap is installed, because I have to manually connect the interface. [09:14] zyga: is that in 6180? [09:15] mborzecki: yes [09:15] aha [09:17] dot-tobias: I beileve so [09:17] dot-tobias: but only in a hook [09:17] dot-tobias: I don't believe this is possible from a snap in general, that is, go and ask if something is connected yet [09:21] zyga: Ok, I'll test the hook route. Thought about parsing snapctl interfaces -i my-snap-name from inside the service, but that seemed overkill. [09:21] dot-tobias: please ask pstolowski as well [09:21] I think this is useful to have [09:22] pstolowski: what exactly SnapGlobal means there? [09:24] mborzecki: it means that given plug/slot is defined at the top-level in the snap yaml [09:26] hmm perhaps TopLevel would be a better name? [09:26] I was thinking about scope but that doesn't work as well, I like TopLevel [09:32] TopLevel sounds ok, but I'm looking at the original code and it looks a bit confusing [09:33] pstolowski: do i read it right, if you have top level slots in the snap, and say have, one of the slots listed under an app too, then it's no longer automatically bound to all other apps/hooks? [09:34] mborzecki: yes, that's the semantics [09:34] mborzecki: plugs and slots are defined implictly by mentioning them in the abbreviated format [09:34] yep [09:34] mborzecki: such plugs and slots are bound to the apps and hooks that mention them [09:34] mborzecki: interfaces may be also defined or expanded in the top level [09:35] unless they are associated with a specific app or plug they become global/toplevel and apply to everything [09:35] maybe we should call it what it is [09:35] privilege spearation [09:35] privilege separation: true, applies to subset [09:35] no, applies to all executable code [09:52] zyga: what do you think about TopLevel name? [09:53] yeah, let's go with top level for now [09:53] Ideally we'd find a name that makes it useful and not just an implementation detail [09:53] but let's not make perfect the enemy of the good [09:53] enough to move forward [10:03] https://github.com/snapcore/snapd/pull/6253 needs a 2nd review [10:03] PR #6253: Members of canonical LP group should pass CLA check [10:23] zyga: thanks [10:31] guys, quick question [10:32] I'm working on https://github.com/snapcore/snapd/pull/6190/files [10:32] PR #6190: overlord/configstate,features: expose features to snapd tools <⛔ Blocked> [10:32] there are some low hanging fruit inside [10:32] for instance stuff like https://github.com/snapcore/snapd/pull/6190/commits/f8f2f3b389b920f299f8994a7e4fb96a02c14a19 [10:32] shall I pop that out to a new branch? [10:40] zyga: yeah, looks useful [10:57] mborzecki: in that case -> https://github.com/snapcore/snapd/pull/6255 [10:57] PR #6255: testutil: add File{Present,Absent} checkers [10:58] PR snapd#6255 opened: testutil: add File{Present,Absent} checkers [11:02] zyga: i think the fontconfig fix is insufficient for fedora [11:02] oh? [11:02] tell more please [11:02] it made positive effect on all suse versions I tried [11:02] zyga: they use --with-cache-dir=/usr/lib/fontconfig/cache [11:02] (42.3, 15 and tw) [11:02] heh [11:03] is that when we tell mvo on monday ;) [11:03] why /usr/lib?!? [11:03] zyga: beats me [11:03] file a bug please [11:03] not on fedora [11:03] on snapd for now [11:04] i was chasing down that denial, and couldn't trigger it on f29 cloud image, so went on digging and found this :/ arch uses /var/cache/fontconfig fwik [11:04] brb, let me make coffee [11:04] mborzecki: desktop is such a fractured thing [11:04] so wait [11:04] on fedora /usr/lib/fonconfig/cache [11:04] are files there written at runtime [11:04] or shipped via packages? [11:04] brb [11:05] written when fc-cache is invoked [11:06] also, fc-cache appears to be tricky for i686 and x86_64, on fedora fc-cache is a script that calls fc-cache-32 and fc-cache-64 [11:06] i recall someone mentioning that the cache files are actually a memory dump of a struct or somesuch [11:09] yeah, fontconfig.i686 ships fc-cache-32 [11:10] maybe we need a helper to run these tools after all [11:10] yes, they are [11:10] I think [11:10] instead of shipping our own builds of fc-cache [11:10] we should instead run the cache from the distro [11:10] and only do this for classic confined snap [11:10] for strictly confined snaps we should do what we did now [11:10] that is, provide our own cache [11:10] but the approach for strict and classic must differ [11:18] zyga: i think the trouble is that the distro may not ship fc-cache for particular fontconfig version in the core snap [11:18] mborzecki: but that version is rarely used in practice [11:19] otherwise you would see an improvement [11:19] right? [11:19] you are really seeing a snap using your fontconfig [11:19] straight from the distro === geodb27_ is now known as geodb27 [11:29] zyga: my cache, but not my lib version (unless someone tweaked how the snap is built) [11:29] so wait [11:29] how can both be true [11:29] if a program uses fontconfig [11:29] it is either from core [11:29] from the snap itself (equivalent) [11:30] or from the native host libs [11:31] zyga: yeah, so unless tweaked, the snap ends up with whatever libfontconfig was in 16.04, right? that's for both confined and classic [11:31] of 18.04 [11:31] yeah [11:31] but classic snaps can be hand made [11:31] can have any layout inside [11:31] yes, that's what i mean by tweaking the build [11:31] including vanilla linker [11:32] so [11:32] my point is that there's either libs from the host used [11:32] which imply *cache* from the host [11:32] or libs from the snap/core [11:32] which imply cache compatible with that [11:32] there's no third cache [11:32] aah, i see what you mean, the core lib was rebuilt with cache in /var/cache/fontconfig [11:51] mborzecki, pstolowski: could you please review https://github.com/snapcore/snapd/pull/6255 [11:51] PR #6255: testutil: add File{Present,Absent} checkers [11:51] zyga: nice! that will be pretty useful, looking [11:52] pstolowski: and if you can, I need 2nd review on https://github.com/snapcore/snapd/pull/6244 [11:52] PR #6244: release: detect too old apparmor_parser [12:05] pstolowski: Is it possible to detect within my snap's application code if a required interface is already connected? snapctl only supports config and service related commands [12:09] dot-tobias: hmm, not really, you could store this fact somewhere in interface hooks as connections happen (but then you also need to do the opposite on disconnect) [12:11] degville: can you remind me where was the doc for interface hooks? [12:12] pstolowski: https://forum.snapcraft.io/t/interface-hooks/8214 [12:12] dot-tobias: ^ [12:13] pstolowski: Thanks! Sounds like a good solution for now. I just want to prevent log bloat since my snap's services try to run various interface-dependent commands on app start, but adding exception handling just for this one moment where the snap's plugs are not yet connected is a bit much. [12:13] degville: thanks! is this going to be merged with the main docs? [12:13] dot-tobias: obviously, if you could simply probe and it's cheap, than it's an option too [12:14] pstolowski: it's currently linked to from the general hooks page (https://docs.snapcraft.io/supported-snap-hooks/3795) [12:17] ack, thanks [12:35] re [12:35] back to coding [12:58] pstolowski: can you take a look at https://github.com/snapcore/snapd/pull/6246 ? [12:58] PR #6246: spread: show AVC audits when debugging, start auditd on Fedora [13:10] off to pick up the kids [13:15] mborzecki, pstolowski: can you please review the 2.36 PRs https://github.com/snapcore/snapd/milestone/21 [13:16] PR snapd#6253 closed: Members of canonical LP group should pass CLA check [13:43] brb [13:55] zyga, mborzecki i'm gonna miss the standup [13:56] ack [14:18] zyga: about cgroups v1 https://github.com/systemd/systemd/issues/10969#issuecomment-442357207 ;) [14:23] see ;-) [14:23] zyga: https://github.com/snapcore/snapd/pull/6185 has 2 +1s, i'm thinking squash merge? [14:23] PR #6185: snap: add new `snap run --trace-exec` call [14:23] looking now [14:24] yes [14:24] + on squashing it [14:27] PR snapd#6185 closed: snap: add new `snap run --trace-exec` call [14:28] zyga: i'll open a 2.36 PR [14:29] wait [14:29] please merge my two PRs [14:29] otherwise 2.36 is red [14:30] PR snapd#6256 opened: snap: add new `snap run --trace-exec` call (2.36) [14:30] aah ok, looking [14:32] zyga: #6235 has conflicts now [14:32] PR #6235: overlord,apparmor: new syskey behaviour + non-ignored snap-confine profile errors (2.36) [14:33] PR snapd#6245 closed: interfaces/backends: detect too old apparmor_parser (2.36) [14:45] grumpy about random failures [14:46] https://github.com/snapcore/snapd/pull/6255 red for 3rd time in a row [14:46] PR #6255: testutil: add File{Present,Absent} checkers [14:51] ok, left a note under https://github.com/systemd/systemd/issues/10872 [14:52] mmm [14:52] hmm [14:52] so does it work? [14:52] mborzecki: btw, I need to show you that part in libmount where I think the bug is as well, there's a lock missing [14:52] maybe we are seeing a pair of bugs [14:53] mborzecki: will you be around today or are you wrapping up now? [14:53] zyga: you think bugs are like sith? [14:53] mborzecki: like binary star systems ;) [14:53] they always come in piarr [14:53] *pairs [14:53] mborzecki: I need to break for lunch now [14:53] mborzecki: will you be here in an hour/ [14:54] i'll probably be around later too, need to take kids to the scouts ~5pm [14:55] ok, ttyl [14:59] zyga: now that my cla check PR was merged the other PR failed cla-check with a KeyError [14:59] but the cla_check PR passed CI... is the LP API just flaky? [15:14] kenvandine: dunno [15:14] kenvandine: can you run the check locally? [15:14] or handle the api key [15:14] er [15:14] or handle the key error [15:14] or maybe there's a cache a [15:29] zyga: it works locally :/ [15:33] * cachio lunch [15:33] hmm [15:56] eh [15:56] more failures [16:03] need coffee [16:09] going to make coffee :) [16:20] mborzecki: https://twitter.com/Arrfab/status/1069623805520355329 [16:24] mborzecki, zyga should I create a new gce image for centos? [16:24] or we are ok [16:26] Son_Goku: yay! [16:26] cachio: yes, please [16:28] mborzecki, ok [16:28] cachio: i think it's best if you create a new image under a separate name and only switch if the whole suite passes [16:29] cachio: if there are issues with the spread run, i can take a look tomorrow and fix stuff :) [16:46] re [16:46] sorry, had to do homework with kids [16:46] back now [16:46] cachio: I think a new image will save us time on upgrades so yeah [16:46] I agrew with what mborzecki said :) [16:47] FUUUK [16:47] why does 6255 fail all the time [16:49] zyga, mborzecki ok, I'll create the new image and make some runs to validate it [16:49] thanks [16:49] zyga: because... if you decompose the 2 as 1 + 1, and do 6(5+1)(5+1) you get 666!!!!!!! evil! [16:49] that made no sense at all, sorry [16:50] roadmr: ha, I wound not be suprirsed by now :) [16:50] I hate that our test suite has some flaky components [16:50] and as we add tests and distributions to run against [16:50] the chance of landing a trivial branch is very low [16:50] and any attempt takes an hour [16:50] cannot have velocity with roadblocks like that :/ [16:54] https://people.neilon.software/ :D [16:56] zyga: can you find yourself there? I did find myself :/ [16:56] I'd be the extreme underestimator [16:57] hehe :) the icons are fantastic btw [17:01] zyga: you should have written better tests! :-P [17:02] sergiusens: our tests are notoriously flaky :/ [17:02] many are racy [17:02] some leak stuff but we have no way to tell yet [17:02] but at this scale [17:02] it's not even that [17:02] we bit the bullet and stopped development for a while to get all that sorted [17:02] simple things like archive issues [17:02] yes, leaking and test cross contamination is hard [17:03] sergiusens: we wanted to do that for a few months but it is unreasonable to do [17:03] at least, if you want speed [17:03] sergiusens: I want speed, I get asked to focus on a feature I need to do [17:03] so I do what I'm told [17:03] and restart those nasty tests [17:03] sergiusens: I don't disagree [17:03] I just don't get that choice [17:04] we added it as a roadmap item, "migrate to spread", so it was accounted for, we still have a list of things that did not make it, but we are in a better place now [17:05] sergiusens: I hope next cycle we can do that [17:05] spread doesn't work if it must be 100% green across ~5K tests that can almsot each fail on network [17:05] we either need a smarter UI / runner that can retrigger a single test [17:05] or change how we land things [17:05] zyga: you should do it during the year crossing cycle, it is shorter and it is nicer to to these sort of things as the end of year holidays come closer [17:06] last two weeks was "red in 2.36" mode [17:06] where nothing in a release branch could land [17:06] sergiusens: this cycle is packed [17:06] won't queeze more [17:06] we can do some perf work on snapd though [17:06] that's valuable [17:06] but equally frustrating to land :) [17:06] zyga: next year... [17:06] unless I hack on spread and snapd during weekends and holidays [17:06] next year, yeah [17:07] I mean, we always fix the test suite [17:07] but it's not really done with the effort required [17:07] mainly about making the failure causes fixed [17:07] (leaking tests) [17:07] and racy tests [17:07] we have so many racy tests that it's not fun [17:08] this with the requirement to get two reviews means that stuff lingers [17:08] first until green [17:08] then on iteration [17:08] then on making that green [17:08] then on more reviews [17:15] roadmr: hi! when convenient, can you pull in r1165? (not urgent) [17:16] mborzecki: if still around, could you review https://github.com/snapcore/snapd/pull/6251 [17:16] PR #6251: cmd/snap-confine: refactor calling snapd tools into helper module [17:16] jdstrand: sure thing! because $REASONS we're on a merging moratorium until tomorrow but I'll merge that tomorrow [17:16] roadmr: np, thanks again [17:16] happy to help :) [17:18] pstolowski: if still around, can you please review https://github.com/snapcore/snapd/pull/6235 [17:18] PR #6235: overlord,apparmor: new syskey behaviour + non-ignored snap-confine profile errors (2.36) [17:18] zyga: looking [17:18] thank you! [17:19] pstolowski: FYI: the master version in https://github.com/snapcore/snapd/pull/6233 has more tests [17:20] PR #6233: overlord: don't write system key if security setup fails [17:24] jdstrand: hey, do you have time to review https://github.com/snapcore/snapd/pull/6244 [17:24] PR #6244: release: detect too old apparmor_parser [17:24] we're very short on reviewers this week [17:35] seb128: fyi, I think you might find this helpful: https://forum.snapcraft.io/t/notifications-for-out-of-date-stage-packages/5161/7 [17:35] zyga: i'm slightly confused by the delta between #6233 and #6235 - e.g. writeSystemKey vs shouldWriteSystemKey, an extensive comment missing in 6235> [17:35] PR #6233: overlord: don't write system key if security setup fails [17:35] PR #6235: overlord,apparmor: new syskey behaviour + non-ignored snap-confine profile errors (2.36) [17:35] PR snapd#6255 closed: testutil: add File{Present,Absent} checkers [17:36] pstolowski: yeah, some things changed as I enabled testing [17:36] I can backport the whole lot, just the 2.36 branch is the raw essence of the thing [17:36] and the master branch has more stuff to run unit and spread tests [17:37] zyga: i see, ok [17:37] i thought you forgot to cherry pick something [17:37]