[00:03] er does cmd/snap progress reporting essentially assume that any task where total > 1 is downloading something? [00:03] that sounds like a chipaca question [00:54] behold my snazzy progress bar! https://asciinema.org/a/aTbeNl5BnF2jtbWlXZT8ed7eE === chihchun_afk is now known as chihchun [05:35] Hello [06:07] morning [06:08] hey mborzecki :-) [06:08] how are you doing>? [06:08] zyga: hey [06:08] zyga: fine, 'connections' landed yesterday :) [06:09] I saw, nice work :) [06:10] zyga: hm need to figure out why the spread test fails in #6485 [06:10] PR #6485: interfaces/seccomp: regenerate changed profiles only <⛔ Blocked> [06:11] zyga: the small s-c patches are waiting for perdronis right? [06:11] mborzecki: and jdstrand [06:11] mborzecki: I'm making breakfast for kids now [06:37] re [06:37] mborzecki: looks l like core18 + snapd not working [06:38] mborzecki: did you try to get a debug shell? [06:38] though given that this is prepare it may be broken too [06:38] zyga: which pr? [06:38] perhaps we need to adjust that code thttps://github.com/snapcore/snapd/pull/6485 [06:38] PR #6485: interfaces/seccomp: regenerate changed profiles only <⛔ Blocked> [06:38] so that we bail out early if snap is not on path [06:39] zyga: stuck looking at some review now [06:39] ok [06:39] zyga: but that's interesting === pstolowski|afk is now known as pstolowski [08:03] mornings [08:05] good morning pstolowski [08:15] Hey Paweł, Michael [08:18] pstolowski, mvo: morning guys [08:19] hey zyga and mborzecki ! [08:24] 6492 needs a second review (its on the 2.38 critical path) [08:25] pedronis: 6356 looks good to me, unless you want to do something with it I will merge it [08:26] PR snapd#6560 closed: cmd/snap-confine: drop uid from random /tmp name [08:26] PR snapd#6561 closed: cmd/snap-confine: chown private /tmp to root.root [08:32] mvo: it's fine to merge for me, but make sure people are aware you would like follow ups [08:34] pedronis: I will wait for john - followups are not strictly needed, its mostly ideas/suggestions nothing strictly required [08:34] ok [08:37] 5.0 kernel is already in testing repo in arch, hope we don't make any assumptions about kernel versions in our code [08:41] mborzecki: we compare it in a couple of places to see if it's higher than some version [08:42] but usually there's a distro check as well [08:42] mborzecki: I see code like in apparmor backend.go seccomp backend.go and sanity/version.go [08:42] they should be fine afaict [08:42] yeah [08:44] mborzecki: when we compare we use the semantic versions compare that follows the debian rules which the kernel honors currently so we should be good [08:44] mvo: you left some optional comments also in #6322 for pstolowski [08:45] PR #6322: overlord/hookstate: apply pending transaction changes onto temporary configuration for snapctl get [08:47] pedronis: i noted these comments and will address in a followup, these were cosmetics [08:48] Hey, with dog at vet, I’ll start around 10 today [08:49] Thanks for merging mvo [08:51] how is mvo merged? [08:52] Mvo is very dedicated, one could say merged with is duties ;-) [08:54] haha [09:31] pedronis: I addressed the feedback in 6381, the one question is if I should remove "setDeviceFromModelAssertion" again from that PR and move it into a later one [09:36] PR snapd#6539 closed: cmd, daemon: split out the common bits of mapLocal and mapRemote [09:47] 6492 needs a second review [09:48] Chipaca: I will merge 6356, there are some nitpicks/ideas in my review but feel free to ignore or do in a followup [09:48] hmm [09:48] * Chipaca looks [09:48] mvo: ok :-) [09:48] Chipaca: for some reason i think you might like this https://asciinema.org/a/aTbeNl5BnF2jtbWlXZT8ed7eE [09:48] zyga: you mentioned you want to re-review 6549, could you please make it a priority? I would like to finalize 2.38~pre1 this morning [09:49] mwhudson: nice! [09:50] mwhudson: how much would that break if the snap progress bar took up more lines? [09:50] Chipaca: not at all, it's pinging v2/changes/$id [09:50] mwhudson: ah, it's doing its own thing? ok [09:50] jdstrand: I'm unable to pause them due to this issue https://github.com/canonical-websites/build.snapcraft.io/issues/623 [09:51] mwhudson: using ansimeter? [09:51] Chipaca: no, urwid has it's own process bar thingy [09:51] mvo: ack [09:51] mvo: let me look now [09:51] wow, it's windy today [09:51] Chipaca: you can tell it's not ansimeter becuase it's not reporting time remaining :) [09:51] ah and it says MiB [09:51] ok [09:51] mwhudson: nice indeed [09:52] mwhudson: basically, you can have more than 1 thing in Doing [09:52] Chipaca: wait.go / ansimeter seems to assume that anything that has total > 1 is downloading something [09:52] PR snapd#6356 closed: overlord/snapstate: during refresh, re-refresh on epoch bump [09:52] mwhudson: and currently we just report the first one [09:52] which is 'fun' [09:52] Chipaca: ah! [09:52] did I mention that 6492 needs a second review ? *please* ? [09:52] i guess it wouldn't be so hard to cope with that in my code [09:52] Chipaca: is this something that happens in practice? [09:53] mwhudson: that's why if you install something that needs to download a content snap, it'll stay in 'connecting' for ages [09:53] ah ha [09:53] mvo: i'll do it [09:53] so if i was refreshing to a version of subiquity that depended on core18, for example... [09:53] Chipaca: thank you! [09:54] mvo: I can look as well after apparmor review [09:54] zyga: appreicated, but if john looks that is enough, its relatively small and already has a review :) thanks still! [09:54] ok [09:57] mvo: in 6492 you say you "added the check for .Active", but I don't see it [09:57] mvo: ah, i see the last commit now, ok [09:58] mvo: we don't block disabling snapd though [09:59] mvo: so you could have snapd installed and disabled [09:59] mvo: (add TypeSnapd to snapstate's canDisable) [10:00] mvo: why am i not writing this on github [10:05] PR snapd#6562 opened: timings: base API for recording timings in state [10:05] Chipaca: lots of things break, we don't check for Active core either [10:05] pedronis: core _is_ in the list that can't be disabled [10:05] Chipaca: is it? [10:05] []snap.Type{snap.TypeGadget, snap.TypeKernel, snap.TypeOS} [10:06] it wasn't for a long time [10:06] core is TypeOS [10:06] we don't check TypeBase [10:06] anyway sounds a different problem [10:06] we do need to switch to a type [10:06] for snapd [10:06] pedronis: ? [10:06] snapd is TypeSnapd already [10:07] Chipaca: it isn't [10:07] Chipaca: look at snapcraft.yaml [10:07] dang [10:07] we _have_ TypeSnapd [10:07] or do snap info snapd [10:07] Chipaca: we do [10:08] but the store and snapcraft weren't on the plan [10:08] are we dum [10:08] :-( [10:08] we r dum [10:08] sounds we need to push [10:10] Chipaca: I created a card for mvo === cpaelzer_ is now known as cpaelzer [10:13] oh no! i have only one PR up! [10:13] * Chipaca strives to fix this [10:13] Chipaca: you can write docs instead [10:13] pedronis: :-) [10:14] pedronis: I also have a followup to the snapinfo pr that just landed, that i'm running spread on locally now that it's rebased [10:16] PR snapd#6492 closed: snapstate: restart into the snapd snap on classic [10:30] wonder whether we should really fail to initialize seccomp backend when snap-seccomp version-info does nto match the patter or is otherwise invalid [10:31] mborzecki: we control both sides, no? [10:31] pedronis: yes, so it's unlinkely to happen [10:32] pedronis: however unlikely, should this fail, backend will not initialize and snapd won't start [10:33] mborzecki: we have many (slightly unlikely) things that will make snapd not start [10:33] ok, so maybe not a problem after all [10:33] especially since both ends are in sync [10:34] I mean, if we didn't control the other side, I would agree [10:34] that being robust is more important [10:34] but if we do, something is off anyway [10:34] agreed [10:48] PR snapd#6563 opened: ctlcmd/tests: tests tweaks (followup to #6322) [10:50] mvo: ^ trivial per your comment to 6322 [10:55] pstolowski: thanks, I have a look in a wee bit [11:10] jdstrand: reviewed https://github.com/snapcore/snapd/pull/6549#pullrequestreview-210608273 [11:10] PR #6549: apparmor: support AppArmor 2.13 [11:14] zyga: I answered some questions, please let me know if its sufficient for merge or if you would prefer more info (it seems the open issues can be done in a followup maybe?) [11:14] looking [11:15] mvo: replied [11:15] mvo: can we add the snapd part and then merge it? [11:16] mvo: unless you think my questions about profileIsRemovableOnCoreSetup warrant more time [11:17] zyga: good question, I'm not super happy with the function but I played around and all my attempts to change it just made it different but not "better" [11:17] mvo: I'd split it to spearate checks per line [11:17] with comment that explains what is being matched [11:18] unlike one very long line [11:18] zyga: if you have an idea, could you please do it and add a comment what it would look like? [11:18] sure [11:18] doing that now [11:20] ta [11:28] mvo: I tried and none of my ideas were any better [11:28] mvo: I'll defer to you in case you want to land and release === sparkieg` is now known as sparkiegeek [11:34] zyga: thank you, I had the same experience, might be worthwhile to add a note in the PR (or a FIXME in the code) that it would be nice to find something nicer looking [11:35] zyga: but otherwise I think we are good to merge and can polish in a followup :) (sorry, a bit in OCD mode to get a first 2.38~pre1 out) [11:35] mvo: there's always .1 [11:35] ;-) [11:35] (right?) [11:37] zyga: *cough* [11:37] zyga: in this case a ~pre2 ,) [11:38] I, for one, embrace the trigger happy merge because we need to get faster, not slower [11:42] damn spread snap-seccomp spread test, it's passing on arch nowm but failing on ubuntu still for some reason [11:43] mborzecki: hmm? [11:43] and debian sid doesn't build because we're patching the source code [11:43] zyga: tests/regression/lp-1805838 is failing with the seccomp branch, even with the fixes i added [11:44] oh [11:44] that's serious [11:53] yes, we do need to patch the patches [11:53] at least now they are in the same tree [11:54] mborzecki: what PR is that? [11:54] mvo: #6485 [11:54] PR #6485: interfaces/seccomp: regenerate changed profiles only <⛔ Blocked> [11:55] mvo: no worries, i'll update those, but it took a while to figure out what's happening there [11:55] mborzecki: thank you! [11:55] PR snapd#6549 closed: apparmor: support AppArmor 2.13 [11:56] btw. fedora packaging does sed to place that libseccomp-go import, we should probably use the same patch as for debian there instead [11:58] mvo: do we need to keep the configure hook in core in sync with the code in snapd? (it's mostly not used on classic), and if you get a new core you get also a new snapd [11:58] mvo: Q is about this: https://github.com/snapcore/core/pull/102 [11:58] PR core#102: hooks: add force_turbo to PI_CONFIG_KEYS [12:00] pedronis: I think we don't, we do it all internally now, don't we? [12:00] mvo: yes, that's why the question, I wonder if we can drop it now completely? or make it empty? [12:01] mvo: we needed for a while for reverts, during the transition [12:02] pedronis: yeah, I think making it empty is the right move [12:02] pedronis: lets add a card, should be trivial [12:02] dammit, power outage here [12:12] mvo: https://trello.com/c/IakOtfSS/183-replace-hooks-configure-in-core-with-an-empty-one [12:12] brb [12:13] pedronis: ta [12:14] pedronis: https://github.com/snapcore/snapd/blob/release/2.38/packaging/ubuntu-16.04/changelog has the 2.38~pre1 changelog [12:15] mvo: thx, will look at in a little bit [12:15] pedronis: no rush, its a ~pre1 [12:29] PR snapd#6564 opened: cmd/snap, tests: refactor info to unify handling of 'direct' snaps [12:32] mvo: I looked at the new things since the last list, I was aware of most of them, I also double checked a couple of snap-confine stuff [12:33] pstolowski: any storms cooming your way today? [12:34] mborzecki: nope [12:34] mvo: zyga: i've updated the seccomp patch, please take a look if that's fine by you or should i add a new one on top of the current series [12:34] mborzecki: sure, give me a sec to look [12:34] pstolowski: well, the season has already started :P === ricab is now known as ricab|lunch [12:46] mborzecki: in that case it's a maintenance in the nearby area [12:58] mborzecki: hmmm [12:59] mborzecki: snap-seccomp version must go into system key, otherwise snapd update with new snap-seccomp won't trigger a regeneration [13:00] zyga: build id will change [13:00] oh, w8 [13:00] hm [13:00] oh, perhaps that is sufficient? [13:01] nvm, need to look at it again [13:01] mborzecki: I mentioned something related in the PR [13:03] off to pick up the kids [13:08] mborzecki: review sent [13:29] PR snapd#6324 closed: Fixing socket permissions on 4.15 kernels [13:37] zyga: I think I answered your questions in PR 6549 [13:37] PR #6549: apparmor: support AppArmor 2.13 [13:40] mvo, zyga: fwiw, I rewrote that function a bunch of times too. see my comment on why I landed on what I did [13:45] joedborg: ok, can you at least make a fix to it and say that it 'architectures: [ all ]' (it doesn't ship any binaries). that will make it easier to deal with reviews, but you should also request classic confinement in the forum since nothing passes review atm [13:46] okay jdstrand [13:46] jdstrand: I asked a question there === ricab|lunch is now known as ricab [13:55] jdstrand: thank you, looking [13:56] pedronis: answered [13:57] jdstrand: thank you for the answers [13:57] jdstrand: note that some of my questions and the rfc branch was trigged by my unfamiliarity with 2.13 [13:57] I rewrote this a bunch of ways and like mvo couldn't come up with something better (in my mind). I picked one; classic bikeshed [13:57] happy to do a followup PR if there is something clearly better [13:58] jdstrand: tumbleweed is using 2.13 now so I'm learing about it [13:58] zyga: it really isn't that different [13:58] .features file is fun [13:58] lisp but with braces instead of parentheses ;) [13:59] zyga: the only thing that 2.13 is different about is it creates a hash of the features and kernel abi as a subdir of the cache dir and puts everything in there [13:59] jdstrand: oh, interesting, so 2.13 already has cache populated with snapd stuff [13:59] zyga: the unified directory is not dictated by 2.13. that is just something Debian did, but anyone could have [14:00] jdstrand: unified directory? [14:00] do you mean /var/cache/apparmor? [14:00] zyga: if tumbleweed has 2.13 and is generating snap policy, I think you'll find that you cannot remove snaps on tumbleweed right now [14:00] jdstrand: on TW /var/cache/apparmor/$cache.0/ has stuff like snap-confine.core.1234 [14:00] oh! [14:00] jdstrand: fun [14:00] * zyga tries [14:00] zyga: yes, /var/cache/apparmor [14:02] zyga: the cache location is still chosen by the distro. in Debian and soon to be ubuntu 19.04, it is /var/cache/apparmor. it might be that elsewhere. [14:02] cannot remove vlc on TW https://www.irccloud.com/pastebin/7HWJOWvZ/ [14:02] standup time [14:02] but I will dig in a moment [14:02] zyga: the choosing of the cache location is separate from the forest behavior is all I'm saying. the fact the Debian combined /etc/apparmor.d/cache and /var/cache/apparmor into on unified dir is separate from 2.13 [14:03] Chipaca: ^ is this expected? [14:03] jdstrand: ah, I see now [14:03] jdstrand: thank you for this [14:03] np [14:03] jdstrand: I am iterating on responses and patches based on your feedback, thank you for that as well [14:03] zyga: no, that's not expected [14:04] zyga: I maintain that it is an isolated changed in just Unload that breaking out a bunch of code and added machinery is probably not worth it [14:04] zyga: but I'll let others decide that [14:04] adding* [14:05] zyga: what does ‘find /var/snap/vlc -ls’ print out [14:06] zyga: note that the vls removal is a different error than what I saw with 2.13 apparmor on Ubuntu. It seems you are hitting a different error earlier than when snapd tries to remove the cache files but can't [14:06] vlc* [14:06] zyga@yantra:~> sudo find /var/snap/vlc -ls [14:06] 2234362 4 drwxr-xr-x 3 root root 4096 mar 5 15:01 /var/snap/vlc [14:06] 2367702 4 drwxr-xr-x 2 root root 4096 lis 6 22:06 /var/snap/vlc/555 [14:07] ack [14:07] zyga: so the question is why did os.RemoveAll fail to remove that [14:08] Chipaca: note that this is reproducible! [14:08] undo works [14:08] so ... good? [14:08] https://www.irccloud.com/pastebin/qdZMzJ6Q/ [14:08] zyga: undo of remove does not work [14:08] it's interesting that we snap disconnect [14:08] why do we do that? CC pstolowski [14:09] zyga: because of hooks that may run on the other end of connection [14:09] aha [14:10] (interface hooks) [14:10] we should be able to optimize away the ones that don't have any [14:10] zyga: +1 [14:10] zyga: so the question is why do you have revision 555 there [14:10] dunno [14:10] (also maybe we should be more resilient about this) [14:10] why cannot snapd remove it? [14:10] zyga: because it doesn't try to [14:11] it doesn't expect that to be there [14:11] aaah [14:11] zyga: actually we probably don't create hook tasks if not needed already [14:11] I see [14:11] zyga: it removes all the revisions, and then the parent [14:11] pstolowski: I'm pretty sure disconnecting stuff from vlc is not needed when you remove the snap [14:11] and all the connections go to core [14:11] zyga: vlc has mpris which doesn't go to core, fwiw [14:12] Chipaca: yes but mpris is not connected [14:15] https://www.irccloud.com/pastebin/tJtbkHRy/ [14:15] Chipaca: it's from october [14:15] but this is my dev machine so anything is possible [14:15] zyga: snap list --all ? [14:16] tried it, not there [14:22] I can't find an obvious way of repro'ing it [14:22] maybe there's a cleanup missing [14:22] ? [14:22] no worries, we know it may happen [14:22] shall I report it for tracking: [14:22] that is, a refresh or install that fails and doesn't un-mkdir? [14:22] ? [14:28] hah! [14:28] zyga: yes indeed we do! [14:28] zyga: o/s/b/link.go, updateCurrentSymlinks [14:28] zyga: happily does a MkdirAll(snap.DataDir()) [14:28] zyga: and then more stuff [14:28] zyga: and that more stuff can fail [14:29] zyga: and there is no clean up of the mkdir on fail [14:29] good [14:29] I'm happy we're happy now :) [14:29] * Chipaca looks around [14:29] we are? [14:29] shallow eyeballs all that stuff [14:39] PR snapd#6565 opened: snap: remove obsolete license-* fields in the yaml [14:40] zyga: i'll push a pr in a bit [14:40] first i need to go move my car [14:40] oh! mvo: tomorrow I need to go to do an anual paperwork thing for kid #2 (kid #1's is in a couple of weeks), probably won't make it to the standup even [14:41] i'll file for a half-day later [14:42] jdstrand: hi, could you adjust the summaries in #5644 , then it could be landed as per agreement [14:42] PR #5644: interfaces: add audio-playback/audio-record and make pulseaudio manually connect [14:42] assuming the tests follow-up [14:43] pedronis: so, I was wanting to ask you about that. I did make some changes before your last comment ot make changes [14:43] pedronis: what is it you are looking for exactly? [14:43] (and yes, when tests can be written, I'll write them) [14:44] jdstrand: https://github.com/snapcore/snapd/pull/5644/files#r212674777 [14:44] PR #5644: interfaces: add audio-playback/audio-record and make pulseaudio manually connect [14:45] pedronis: oh, that change. ack. I thought this was something else [14:45] Chipaca: thanks [14:45] Chipaca: no worries [14:46] jdstrand: to be fair, I see we use your spelling quite a bit [14:47] well, not quite a bit [14:47] but in a few cases [14:49] degville: interestingly there are a few things that are document for snapcraft.yaml but not snap.yaml :) [14:49] degville: https://forum.snapcraft.io/t/snapcraft-app-and-service-metadata/8335 [14:50] pedronis: yeah, I've been pinged about updates to snapcraft.yaml but didn't think to push them back to the snap.yaml doc. [14:50] Lunch [14:50] Chipaca: thank you! :-) [14:51] jdstrand: pulseaudio is interesting because the summary in the code talks about operating as the service, but the doc doesn't https://forum.snapcraft.io/t/the-pulseaudio-interface/7906 [14:52] jdstrand: while I would, sort of, expect the reverse [14:54] jdstrand: I mean the simple summary to explain the plug/consumer POV, the "extended" docs to explain that the slot can be implemented by apps [14:55] zyga: pedronis: under the assumption that snapd and snap-seccomp are in sync, i think we don't need to include snap-seccomp version in version-info, just the library version and the hash are enough [14:55] mborzecki: what about system-key? [14:55] mborzecki: I agree with your statement [14:55] mborzecki: but system-key correctness is essential for correctness [14:55] zyga: we still need that, i.e. put version-info in system-key [14:56] ? [14:56] zyga: what i mean we don't necessarily need to include the compiler version in the output [14:56]