[06:27] good morning [06:32] hey pedronis :) [06:59] hi [07:02] hey pedronis === pstolowski|afk is now known as pstolowski [07:07] mornings [07:09] hey pstolowski [07:13] PR snapd#7329 opened: snap: explicitly forbid trying to parallel install from seed [07:14] mvo: ^ pstolowski: ^ [07:14] pstolowski: hi [07:14] pedronis: ta [07:15] ok [07:19] mvo: what about https://github.com/snapcore/snapd/pull/7321 [07:19] PR #7321: tests: restore dpkg selections after upgrade-from-2.15 test [07:19] I'm not sure how many reviews we need now [07:20] PR snapd#7328 closed: tests: pass --remove to userdel on core [07:25] zyga: its fine from my POV, its not perfect [07:25] mvo: yeah, I'd like to land it [07:25] I mean, you pointed out that the tool has flaws [07:25] but its fine IMO [07:25] mvo: yeah [07:25] mvo: do we need zero, one or two reviews on top of this state? [07:28] zyga: so far this has zero reviews… :( let me review it [07:28] zyga: having one review from !us would be nice I think [07:29] yeah, I feel the same way [07:29] it's unclear what the rules say when two people work on something and agree [07:29] zyga: lets not fuss too much, its a tiny PR and affects only tests. I'm sure if e.g. pstolowski can have a quick look it can land :) [07:30] +1 [07:32] seems i missed my 2 minute window to review this ;) [07:38] ah, it wasa about 7321, looking at it [07:44] brb [07:46] one comment there, +1 [07:51] re [07:51] pstolowski: replied [07:52] mvo: I archive 2.40 and created the 2.42 lane, I also made cards for the UC20 things I'm working on [07:54] PR snapd#7321 closed: tests: restore dpkg selections after upgrade-from-2.15 test [07:58] pedronis: excellent, thank you [08:38] ullo ullo [08:39] i just popped in to make sure y'all were aware today's a bank holiday so that's why i'm not here [08:39] pedronis, zyga: when we optimize apparmor backend to load multiple profiles in one go we're going to loose the granularity of error handling we have now for the apparmor_parser invocation. at the moment regenerateAllSecurityProfiles just logs an error for a given snap and carries on with the rest (we can still have such per-snap policy for intermediate errors before aa parser is invoked). any toughts on that? [08:39] hey Chipaca [08:42] pedronis, zyga unless we run apparmor parser with --vebose flag and go into business of parsing it's output ('Replacement succeeded for "snap....' gets printed for every successfull profile) [08:51] mvo, morning! would it be possible to merge https://github.com/snapcore/snapd/pull/7252 now? Also, what about https://github.com/snapcore/snapd/pull/7173 ? Is retriggering the tests needed there? [08:51] PR #7252: interfaces/network-manager: allow using org.freedesktop.DBus.ObjectManager [08:51] PR #7173: interfaces: support Tegra display drivers [08:52] abeato: in a meeting, will check in ~1h when they are over [08:52] mvo, ack, thanks [08:55] pstolowski: interesting [08:55] pstolowski: on failure, we can re-run per profile and see what really failed [08:58] zyga: yeah but we need to find out which one failed (for which snap), just log this snap and don't consider entire operation as failed [09:02] pstolowski: yeah but when the batch fails you can re-try non-batch to reliably get information about those that really failed [09:02] pstolowski: alternatively [09:02] pstolowski: we can batch only inside one snap [09:03] pstolowski: that's less generic but better in terms of our understanding what to do [09:03] pstolowski: because when things fail, we need to change at the snap-granularity anyway [09:04] zyga: we already pass all profiles of given snap to aa parser (if that's what you mean). but re-trying one-by-one is an interesting idea [09:05] pstolowski: if it's already per-snap then retrying is not of much use [09:05] pstolowski: because it partially failed anyway [09:05] pstolowski: another idea: do it in stages: first compile then load [09:05] pstolowski: if compilation failed nothing in the system is broken [09:05] pstolowski: it's easier to reason about the state [09:06] pstolowski: you can pass a flag to the parser to let it just compile all profiles [09:06] pstolowski: then you can pass a different flag to ask it to just load binary profiles [09:08] -d, --debug [09:08] Given once, only checks the profiles to ensure syntactic correctness. [09:08] zyga: ^ yeah, interesting [09:11] pstolowski: rather than debug I'd try something like --skip-kernel-load [09:11] zyga: aha! thanks [09:11] pstolowski: you can use other options to capture the compiled profile into memory [09:11] pstolowski: or to disk [09:26] Morning all [09:30] hey niemeyer ! welcome back [09:30] Thanks! Glad to be back [09:30] How're things going? [09:38] PR snapd#7252 closed: interfaces/network-manager: allow using org.freedesktop.DBus.ObjectManager [09:40] niemeyer: going well, thank you! [09:40] I see mup is still going strong [09:41] niemeyer: some annoying test hickups that we are fighting but its getting better [09:41] niemeyer: yeah, mup was doing great last week iirc [09:41] mup: hey, still fine? [09:41] mvo: I apologize, but I'm pretty strict about only responding to known commands. [09:41] * mvo hugs mup [09:42] Nice [10:33] mvo: found the bug :) [10:33] mvo: whee [10:33] core is good [10:33] zyga: tell me more! [10:33] mvo: the logic in reset_all_snap was silly wrong [10:34] I'll send a PR in a moment [10:34] zyga: ta [10:45] mvo: two bugs actually [10:45] in other news, shell is tricky [10:52] pfft [11:14] fired one more run to be sure, brb [11:14] mvo: tl;dr; grep -w is a deception [11:14] mvo: - is a word separator [11:14] mvo: dont-remove-this-snap-core18 matches against grep -w core18 [11:25] zyga: woah, nice find [11:32] mvo: https://github.com/snapcore/snapd/pull/7330 [11:32] PR #7330: tests: fix removal of snaps on ubuntu-core [11:32] PR snapd#7330 opened: tests: fix removal of snaps on ubuntu-core [11:38] PR snapd#7331 opened: tests: move interfaces-contacts-service to /tmp [11:40] ohh, a new PR label [11:40] PR snapd#7332 opened: tests: remove trailing spaces from shell scripts [11:41] mvo, it looks like tests for https://github.com/snapcore/snapd/pull/7173 finally passed :) [11:41] PR #7173: interfaces: support Tegra display drivers [11:41] I need to do some reviews while tests are churning [11:49] PR snapd#7333 opened: tests: fix system version check on listing test for external backend [11:51] wow, only one test not passing cleanly in main on core16! [11:51] and a simple one at that [12:15] unit test failure? [12:15] https://www.irccloud.com/pastebin/AQb5iDB8/ [12:15] and another one on snap_daemon user [12:15] https://www.irccloud.com/pastebin/5rYhzSZG/ [12:15] mvo: is that still expected?^ [12:15] or should I get more recent master [12:32] PR snapd#7334 opened: tests: remove locally installed revisions of core === ricab is now known as ricab|bbl [12:38] zyga: the daemon-user in tumbleweed? [12:38] mvo: that is 16.04 [12:38] mvo: not TW [12:38] mvo: group removal failed because there's a user using it as primary group [12:39] zyga: let me check [12:39] mvo: offtopic: core18 does not leak any more mount points as far as my tests are showing [12:39] mvo: I'm running a more complete pass but this is fantastic, it's not buggy anymore : [12:40] mvo: this means both core and classic are clean, only two PRs to review to get there [12:42] zyga: cool [12:42] zyga: where is the userdel failure? [12:43] zyga: I mean, what PR failed ? I want to check the log [12:43] mvo: I restarted it already [12:43] mvo: sorry, :/ === pedronis_ is now known as pedronis [12:43] and then the test suite will be 100% robust? :-) [12:45] jamesh: no :) [12:45] jamesh: but that aspect will [12:45] jamesh: so my test that measures it can be enabled again [12:46] jamesh: without acting like a mount leak detector [12:47] zyga: fyi, I'm in Greece for GUADEC until Thursday (and then flying home until Friday evening). So I may be slow to respond to activity on my PRs [12:48] jamesh: enjoy Guadec, I'd love to visit Greece for one :) [12:48] I'm doing a pass over older PRs, including many that you sent, sorry for the enormous lag [12:49] zyga: no problem. At some point I'd like to go over the work I need to complete for 20.04 [12:50] dbus activation is still the big one (which hopefully is getting close to unblocked) [12:50] there will probably be some xdg-desktop-portal related changes too though [12:50] jamesh: any interesting portal work that would be of interest for us to integrate with? [12:50] * zyga has some back pain today, should move to laptop [12:51] zyga: mostly things that were brainstormed almost a year ago [12:51] jamesh: as in, they are coming now? [12:51] after a year of being under development? [12:52] I want to give xdg-desktop-portal more information about snaps, which will probably involve adding a hidden "snap" command that takes a pid for a confined app and spits out some machine readable data about the snap [12:52] a year of having other priorities [12:52] jamesh: oh neat [12:52] jamesh: would it be like snap info --pid=123 [12:52] that then matches that pid to a snap and gives you that kind of information [12:52] jamesh: my question is mainly: do you need more than what snap info would say [12:53] in particular, I want snap name, desktop file ID, possibly presence of network plug [12:53] maybe some information about readable/writable paths [12:53] mmm [12:53] jamesh: what kind of writable paths do you mean? precise rules or something high-level? [12:54] zyga: the main use would be to identify cases where files do not need to be proxied through xdg-document-portal [12:54] oh, I see [12:54] so it's alright if it isn't complete [12:54] jamesh: would it be ok if it was just a query per path? [12:55] jamesh: snap internal is-accessible --pid=123 /path/to/file [12:55] zyga: perhaps. I need to get up to speed with that code base again [12:55] ok [12:55] thanks for sharing, this is interesting [12:56] pstolowski: ime you don't want to run apparmor_parser so much on all profiles as much as run it only once at the end [12:57] zyga: also: what is the best cross platform way to detect if a pid is a snap app? [12:57] zyga: the current code is checking AppArmor labels, but that obviously doesn't work for the non AA distros [12:57] pstolowski: ie, snap.foo.bar has 3 interfaces. apparmor_parser it once after all 3 interfaces are connected instead of each time after one is connectedc [12:57] the aim is to have something quick to check before shelling out to the "snap internal" command we eventually write [12:58] jamesh: the freezer cgroup atm [12:58] jamesh: that's a good question [12:58] jamesh: I'd like that to be an API of snapd, not something that is determined externally [12:58] jamesh: because e.g. the freezer or apparmor are internal implementation detail [12:58] jamesh: and that will change [12:58] jamesh: unless we can come up with a robust way to make that without making any problems [12:58] zyga: the flatpak method is to check for a special file in the root directory of the process's mount namespace [12:58] jamesh: perhaps /proc/PID/root/meta/snap.yaml presence? [12:59] jamesh: that would be reliable today (except for classic snaps) [12:59] and yes, we should have an api [12:59] jdstrand: ^ I might even be happy with that being a stable way to check [12:59] regardless of an API [12:59] zyga: treating classic snaps as unconfined is fine for the purposes of xdg-desktop-portal [12:59] they effectively are anyway [12:59] jdstrand: right. this is kind of already done for auto-connect on install (landed recently, edge only atm) [13:00] pstolowski: the same goes for snap-seccomp [13:00] * zyga goes to a meeting [13:00] pstolowski: oh! 'kind of'? [13:00] jdstrand: the case i'm trying to cover now is where we regenerate all profiles on startup when system key or templates changed [13:00] jdstrand: 'kind of' in a sense that it's only for auto-connect, not connect in general [13:01] pstolowski: but that includes auto-connect via snap decl too, right? [13:01] (it would be weird if it didn't, but thought I'd ask) [13:02] pstolowski: what about app snap refresh? [13:02] jdstrand: yes, precisely that - when you install a snap, and slots get automatically connected [13:02] pstolowski: is refresh a special case for install? [13:03] otp [13:03] pstolowski: feel free to ping me after [13:07] PR snapd#7331 closed: tests: move interfaces-contacts-service to /tmp [13:11] jdstrand: yes, refresh as well, it's special case for install indeed [13:15] jdstrand: but there is atm no improvement if you manually snap connect multiple plugs/slots in one go. this could benefit from similiar optimization (as you suggested; and similiar to auto-connect), the framework for that is already in place and we can tackle it at some point [13:16] pstolowski: oh, I didn't know you could do more than one in one go :) indeed, that could be done. I think the key is once per command, not per snap or per batch of snaps and then you get reasonable optimizing with good error reporting [13:17] pstolowski: doing more than one profile at a time only saves you a fork/exec/tiny overhead which is probably not worth losing the error handling [13:17] at least note atm [13:17] not* [13:17] pstolowski: and this was done for snap-seccomp too? [13:19] jdstrand: yes, this was done on higher level than specific backends, we basically run setup-profiles just once at the right moment [13:19] pstolowski: woo nice! :) [13:21] jdstrand: hmm re apparmor parser, my tests on pi3 suggest we will benefit a lot (3x faster) by running aa parser once over multiple snaps' profiles, did i miss something? see https://forum.snapcraft.io/t/apparmor-parser-performance-on-pi3/12909 [13:22] (re error handling i'm sure we can overcome this, per zyga's suggestions) [13:23] pstolowski: so apparmor can do some parallelizing and things for sure... but it seems your --cache-loc is wrong [13:24] pstolowski: it should be /var/cache/apparmor, not /etc/apparmor.d/cache [13:24] the cache's should be invalidated [13:24] (though) [13:24] jdstrand: I'll be talking to the freedesktop-sdk folk tomorrow. Is there anything newer than what you wrote at https://forum.snapcraft.io/t/base-runtime-freedesktop-sdk-runtime-19-08/11153/40?u=jamesh that I should be aware of? [13:25] jamesh: unfortunately not. since the work is unplanned, it is falling behind some other stuff that is planned [13:25] fair enough [13:26] jamesh: but, there is a clear path forward [13:26] it's 'just work' [13:26] getting to that point is half the battle :-) [13:26] (in terms of implementaion. there are a few process details to sort out, but that isn't a big deal) [13:26] jamesh: ha, yes :) [13:27] mvo: FYI https://github.com/snapcore/snapd/pull/7316#pullrequestreview-279586840 [13:27] PR #7316: tests: add unit tests for cmd_whoami [13:28] * zyga goes to rest to avoid the back pain [13:28] zyga: thanks, looking [13:31] zyga: nice, thank you! [13:32] pstolowski: your use of snap.* is not accounting for snap-update-ns.*, no? did you find those aren't getting regenerated with ifacemgr? [13:32] PR snapd#7173 closed: interfaces: support Tegra display drivers [13:33] jdstrand: indeed, looks like i used the wrong cache path. anyway, that shouldn't matter for the test due to --skip-read-cache, as i was testing the pessimistic scenario only [13:34] jdstrand: and you're right i haven't tested snap-update-ns* profile (it is covered by snapd code, so code is fine) [13:35] I think what is happening here is with the single call you are benefiting from parallel processing [13:35] whereas otherwise it is completely serial [13:36] gimme a second and I can confirm that [13:36] PR snapd#7330 closed: tests: fix removal of snaps on ubuntu-core [13:37] right, a pi3 has 4 cpus that can be used [13:37] and so apparmor will used -j4 [13:39] right, if I use --max-jobs=1 then I get roughly the same performance as serial [13:39] PR snapcraft#2684 closed: meta: decouple DesktopFile logic [13:39] 33s vs 35s on amd64 [13:43] jdstrand: ah, that means it would help on faster machines (originally the problem was reported with fast pc & lots of snaps https://forum.snapcraft.io/t/snapd-causing-lengthy-boot-time/10466), but not on low-end ones that some customers have... [13:44] pstolowski: I commented: https://forum.snapcraft.io/t/apparmor-parser-performance-on-pi3/12909/2 [13:45] ps [13:45] jdstrand: thank you, that's useful feedback [13:46] pstolowski: let me dig up one more tidbit. I recall we had perf bugs in Ubuntu when we took all the CPUs and choked boot [13:46] mvo: another failure, daemon user present, test failed to add it [13:47] on actual single core HW you will also see I/O saturation that you wont if you use a multicore device and just limit cores [13:47] pstolowski: also, you may find this handy: getconf _NPROCESSORS_ONLN [13:49] jdstradn, pstolowski: fwiw the big problem I remember from a customer device about this is that it takes _ages_ on a low-end ARM CPU to finish the first boot because there's a lot of snaps and a lot of service/commands with a lot of plugs [13:49] zyga: yeah, I pushed a pr to get to the bottom of this [13:49] jdstrand: sorry ^ [13:49] zyga: its very strange [13:49] pstolowski: right, I remember now. we could make the parser even faster by using a -j value higher than _NPROCESSORS_ONLN, and that was the problem. In Ubuntu, we use _NPROCESSORS_ONLN. *maybe* you want to use _NPROCESSORS_ONLN-1? (you'd want to measure, etc) [13:50] note that device only has one CPU core [13:50] ijohnson: that is already going to be helped tremendously by what pstolowski already did. run the parser once per command rather than once per interface per command [13:50] jdstrand: interesting. we may try to be conservative and paranoid there to start with [13:51] right okay, I missed part of this conversation so I wasn't sure what was being discussed right now :-) I'm sure they will be happy to hear that [13:51] btw [13:51] * jdstrand hugs pstolowski :) [13:52] pstolowski: ok cool. I forgot that the rpi3 had so many cores. parallelizing defintely helps with more cores :) [13:54] we should really measure on actual HW that was designed as single core though ... faking it like that will make you miss a lot of aspects [13:55] ijohnson, yes, I am happy to hear that :D [13:55] (though if the focus is really only on CPU processing thats probably okay ... but for overall performance mesuaring IO should also come into play) [13:55] jdstrand: thanks for looking at this! [13:57] ogra: well, note, on a single core it defaults to -j1. apparmor will use -jNUMBER OF CORES by default. My question is do we want to make that 'minus 1' or something [13:57] no one is suggesting a -j higher than the number of cores (I advocated agaisnt it :) [13:57] what would minus 1 achieve ? dynamic picking ? [13:57] no no [13:58] oh, you mean "one less" [13:58] I'm saying 1 core, -j1. 2 cores, -j1, 3 cores, -j2, etc [13:58] yeah [13:58] yes [13:58] got it, sorry, slow today [13:58] ie, leave at least one for other stuff [13:59] which is probably quite reasonable for a refresh core scenario [13:59] if we can we should surely do that since we might have something running in parallel (we have no real control about systemd here i guess) [13:59] but I'll let others decide [14:00] jdstrand, or maybe change the nice value for the process, as another option? [14:00] oh, we can do that. calculate the cores (see aforementioned getconf command), add --max-jobs [14:00] abeato: that is an available option as well [14:08] * zyga has lots of back pain and stops to do any reviews too [14:08] sorry folks, need to break and get better somehow [14:12] * ijohnson relocates, back in ~15 min [14:12] mvo: I got this: https://api.travis-ci.org/v3/job/576683798/log.txt do I need to merge master again? [14:13] PR snapd#7334 closed: tests: remove locally installed revisions of core [14:14] pedronis: no, this is still under investigation :( [14:14] pedronis: its strange, let me add some more debug [14:18] mvo, hey [14:18] hey cachio [14:18] mvo, taking a look to the error on core18 I see this [14:18] https://paste.ubuntu.com/p/9hzczyDQjf/ [14:18] 2.41. instead of 2.41~ [14:18] is it a bug? [14:19] or we will use dot [14:19] just to see if I need to change the script [14:19] PR snapd#7336 opened: tests: add debug section to interfaces-contacts-service [14:19] cachio: thats a bug [14:19] cachio: the edge is the correct version [14:20] cachio: its just the version number though that is wrong in beta, its the right snapd [14:20] cachio: with the next beta/candidate the version shoud be fine [14:20] mvo, perfect [14:20] thanks! [14:21] cachio: once you are happy with the beta I can push a new version. but we need to make sure of course that it includes all fixes you have for 2.41 [14:22] mvo, sure, I'll push a fix now [14:22] mvo, that + the fix for listing test should be enough [14:22] then I have another one but so far I cound't reproduce it [14:25] cachio: thanks, let me tag the listing one as 2.41 [14:25] PR snapd#7332 closed: tests: remove trailing spaces from shell scripts [14:25] Thank you mvo [14:31] PR snapd#7337 opened: tests: re-enable mount-ns test on classic [14:39] mvo: all stuff fails on travis [14:39] E: Failed to fetch http://apt.postgresql.org/pub/repos/apt/dists/xenial-pgdg/main/binary-i386/Packages Writing more data than expected (788347 > 787357) [14:39] 269 [14:39] zyga: yeah, I noticed - I don't think we can do much, this is in the travis setup itself [14:39] correct :/ [14:39] zyga: I wonder if #travis is a thing to report issues [14:40] zyga: but I guess nowdays #twitter is the thing :P [14:40] I'm sure they know [14:40] yeah [14:40] where the kids and crazy world leaders (or is that that same thing) are [14:40] mvo: random simple patch from my pile https://github.com/snapcore/snapd/pull/7338 [14:40] PR #7338: tests: move restore-project-each code to existing function [14:40] PR snapd#7338 opened: tests: move restore-project-each code to existing function [14:42] PR snapd#7339 opened: tests: don't look for lxcfs in mountinfo [14:45] mvo: cna we unblock https://github.com/snapcore/snapd/pull/7196 [14:45] PR #7196: packaging: use passthrough for type:snapd <⛔ Blocked> === ricab|bbl is now known as ricab [15:32] cachio: https://github.com/snapcore/snapd/pull/7110 is green and can land? [15:32] PR #7110: tests: new test to check the output after refreshing/reverting core [15:36] pstolowski: please hold on [15:36] ok [15:36] PR snapd#7340 opened: tests: adding support for arm devices on ubuntu-core-device-reg test