[05:04] morning [05:10] PR snapd#7323 closed: features, overlord: make parallel-installs exported, export flags on startup [05:33] School morning [05:33] zyga: hey, yeah, school morning [05:42] I will be around shortly [05:42] Walking back from school [05:45] mvo: hey [05:46] hey mborzecki ! welcome back [06:05] PR snapd#7384 opened: HACKING.md: clarify where "make fmt" is needed [06:08] PR snapd#7382 closed: osutil: make flock test more robust [06:10] hey mvo, thank you for merging that [06:10] mvo: btw, can you look at the corresponding non-test code [06:10] zyga: good morning! thanks for creating it :) [06:10] mvo: I was puzzled by how that can ever happen [06:11] mvo: it looks like a bug in the compiler or something in our understanding [06:11] mvo: as that function cannot return nil, nil, can it? [06:11] mborzecki: 1st day of school and temperature drops by 10 degrees [06:11] zyga: NewFileLock, no, that looks impossible, we are explicitly about the "nil" there. [06:12] mvo: right? I was worried when I saw that because it feels we are missing something important [06:16] zyga: yeah, its very confusing [06:16] perhaps go gc bug? [06:23] arch package updated [06:25] PR snapd#7385 opened: tests/cross/go-build: use go list rather than shell trickery [06:25] super simple PR ^^ [06:29] zyga: I looked at this flock issue again, I think you fixed it. the issue is that "err = lock.TryLock()" may return nil - if there is lock. but then err, Equals, osutil.ErrALreadyLocked will trigger a err->Error I think [06:29] oh [06:29] actually, yeah [06:29] zyga: which causes the crash, your code now first assigns err and then checks for non-nil so this error goes away [06:29] actually [06:29] no wait [06:29] hmm [06:30] so are you saying that due to the raceiness [06:30] the error can be nil [06:30] zyga: correct [06:30] and then Equals would panic? [06:30] zyga: yes, I think so, equal will try to make it an error [06:30] zyga: I have not looked at the details but I think so [06:30] that's certainly plausible and more realistic that a gc bug :) [06:30] zyga: I think "c.Assert(nil, Equals, osutil.Err..") should behave the same [06:30] nice, you made my mind more at ease [06:31] oh, good idea [06:31] I'll chcek [06:31] zyga: yeah, thanks for fixing this one! it is really an interessting one :) [06:31] indeed [06:31] https://www.irccloud.com/pastebin/z0RHTYJO/ [06:31] you are spot on [06:31] yay [06:31] that's so unexpected btw [06:31] zyga: agreed [06:32] zyga: ErrorMatches will DTRT - except it matches instead of equals :/ [06:35] off to the school with kids, back in ~1.5h [06:36] mborzecki: good luck :) [06:36] hey sil2100 :) [06:37] zyga: oh no, I think this error happens in the "diff" code we added a while ago :( so gopkg.in seems to got broken by this [06:37] zyga: that bug is on me I think :( [06:38] ahhh [06:38] I recall this [06:38] well, one less but two more :D [06:38] zyga: yeah, there is also the circular bug in github.com/kr/pretty - makes me wonder if we should simply revert that diff code [06:40] the diff on error is somewhat useful, I wonder how hard would it be to fix it instead [06:40] but also worried if we use old version or if upstream is dead [06:40] zyga: I fix it, thats trivial [06:41] zyga: I'm just a bit sad that its "1 added, 2 new bugs introduced" [06:41] yeah, no disagreement [06:41] "what's under this rock over here, let me turn it over" [06:51] niemeyer: hey, good morning! if you have some spare cycles, I have a very simple gocheck PR (https://github.com/go-check/check/pull/114) that would be great to get merged. I feel bad because its an issue introduced with a PR that I originally proposed :( [06:51] PR go-check/check#114: checkers: fix misleading error for c.Check(nil, Equals, "foo") [07:00] mborzecki: can you please check if github.com/sanity.io/litter is available in fedora as a package? I wonder if we can switch away from kr/pretty as its unmaintained (and there is this cyclic references bug that can cause hangs) [07:01] PR snapd#7384 closed: HACKING.md: clarify where "make fmt" is needed === pstolowski|afk is now known as pstolowski [07:02] mornings [07:03] hello pawel [07:03] pstolowski: feels good to send kids to school again, no? :) [07:04] hi zyga [07:05] hey, good morning [07:06] zyga: at GUADEC, there was a request for a snapd feature that might be quite difficult to implement [07:06] tell us! [07:07] zyga: essentially they want to move the org.freedesktop.portal.Flatpak.Spawn portal interface from Flatpak to xdg-desktop-portal, and have it do something sensible for snaps [07:07] zyga: haha, indeed, i just got back from there [07:07] this is essentially a way for a confined process to spawn a second process with even stricter confinement: https://github.com/flatpak/flatpak/blob/master/data/org.freedesktop.portal.Flatpak.xml [07:09] the idea being to allow applications that confine portions of their code to continue doing so when put in Flatpak/snap confinement (e.g. web browsers) [07:15] jamesh: hmm [07:16] what is the interface like? [07:16] the XML file I linked to has documentation for the interface [07:17] the main features are (a) pass file descriptors to spawned process, (b) optionally block network or file system access, (c) expose a restricted set of files to the spawned subprocess [07:18] yeah, reading that now [07:18] It's basically something to replace the fork/unshare/exec code you might find in multi-process apps [07:18] hmmm [07:18] some of the concepts don't map cleanly [07:18] what is a "sandbox" in flatpak term? [07:19] as in flag value 4 [07:19] spawn in a sandbox [07:19] I need to look that up. [07:19] is there a demo app for this or a production app that would use it? [07:19] I think the big one at the moment is webkit-gtk [07:19] I think it's doable but not small or on or our roadmap for this cycle so it's hard for me to say if we can do it [07:20] jamesh: some things there look vert flatpack specific, like ~/.var/app/$APP_ID/sandbox [07:21] I'm sure it's all doable but we'd have to think about how to construct that [07:21] jamesh: that dbus api is for the user session? [07:21] zyga: right. I think the idea is to get something that could provide equivalent functionality [07:21] jamesh: so you spawn it as yourself essentially, right? [07:21] the xdg-desktop-portal version wouldn't necessarily be identical [07:21] I see [07:21] yes. This is a user session API, same as the other portal services [07:21] well, I'm open to this for sure [07:22] it would be nice to explore this if we have agreement / time to do it [07:27] zyga: so I think "fully sandboxed" means only access to the runtime, app data and files specifically exposed in ~/.var/app/$APP_ID/sandbox [07:28] so snap-wise, that would be no home interface, and only access to a small subset of ~/snap/$snap_name [07:28] almost like "all interfaces disconnected" [07:29] possibly, yeah. [07:31] zyga: If we could get this all working and it was adopted by upstreams, it would probably remove the need for the allow-sandbox flag on our browser-support interface [07:32] jamesh: indeed, that's pretty interesting as an evolution of the concept "we package stuff as-is" to "we provide a new layer to apps" [07:35] zyga: in the current release of GNOME, the file manager makes use of bubblewrap to confine thumbnailer subprocesses. So a big part of this is to allow this model to continue in the new world [07:35] (since you can't run bubblewrap in Flatpak's bubblewrap based sandbox either) [07:59] mborzecki: hey [07:59] mborzecki: I updated https://github.com/snapcore/snapd/pull/7218/files [07:59] it's just a new spread test [07:59] PR #7218: tests: measure behavior of the device cgroup [07:59] could you please have a look, make sure to read both commit messages [08:01] mvo: Thanks, will have a look [08:01] niemeyer: should be super trivial, let me know if I can help in any way [08:05] mvo: It's in, thanks again! [08:05] niemeyer: \o/ [08:19] Bug #1842282 opened: can't add channel information as install downloaded snap [08:32] re [08:35] mvo: there's apackage of github.com/sanity.io/liter in fedora, it's version 1.1.0 if that makes any difference [08:36] mborzecki: thank you [08:37] mvo: hmm https://github.com/sanity.io/litter is a 404 for me? [08:38] mborzecki: https://github.com/sanity-io/litter [08:41] duh [08:41] ok, works for me :P [08:42] mvo: do you remember that other lib? https://github.com/davecgh/go-spew [08:43] mvo: looks like litter is packaged for debian too, so we can switch to either one [08:54] mborzecki: spew is available for centos/fedora too? cool [08:55] mvo: for centos/amzn we use a bundled pacakge [08:55] s/use/build/ [08:57] mvo: I have an on going diff sitting on my disk for a while around that area of go-check, btw.. [08:58] mvo: It improves a bit the way the diff is displayed in some cases [08:58] mvo: But I've hit an issue with kr's diff lib.. it broke down in some nested data [08:59] niemeyer: yes, we have a fix for this in kr/pretty [08:59] mvo: Oh? [08:59] niemeyer: it breaks on cyclic data [08:59] mvo: Was that submitted upstream? [08:59] niemeyer: https://github.com/kr/pretty/pull/58 [08:59] PR kr/pretty#58: Fix infinite recursion when diffing structs with cycles [09:00] niemeyer: also https://github.com/kr/pretty/pull/44 (which looks similar) [09:00] PR kr/pretty#44: Detect cyclic references in Fdiff() [09:00] Oh, nice [09:00] niemeyer: I can't reach him though :/ maybe he replies to twitter pings :) [09:00] this is open for some time [09:02] I can understand him :) [09:02] We should just fork temporarily while this is sorted [09:04] niemeyer: works for me [09:32] People : hi ! I run a lxd cluster on three virtual machines under ubuntu-18.04 lts. As you might know, lxd is shipped via snap. There seem to be a problem with the way updates are run. Indeed, the three machines are all the same considering installed os version and hardware. They, of course, have all their time maintained via ntp. Error occur when all three decide that it is time to update lxd : since the processes are ran at the same time, this [09:32] results in a crash of the overall cluster and require intervention to have all three systems in a production state. Question is : how does snap decide it is time to update what is installed ? If it is done internally, wouldn't it be fine to add some "salt" to the decided time so that everything does not start at the same time everywhere ? [09:39] geodb27: updates shouldn't run at the same time, as there is a random element to the time [09:40] geodb27: additionally, you can schedule when updates happen, so if you have a requirement for them not to happen simultaneously that is something you can configure [09:41] geodb27: if you have debug logs on snapd will log when it will update next, but debug logs are very chatty so most people don't [09:41] geodb27: 'snap refresh --time' will print when the next refresh will happen (mostly) [09:42] geodb27: 'mostly' because right after snapd starts, and until the first time it checks, the next refresh time isn't set [09:42] Thanks for you answer Chipaca. I'll check how it is done on the different machines. [09:42] geodb27: as to how to set the refresh timer, https://snapcraft.io/docs/keeping-snaps-up-to-date [09:44] Well, the problem seems somewhere else. I'll have to wait for the next lxd's snap to check what goes wrong. [09:44] * zyga finishes calls and jumps into the fray :) [09:45] What is sure is that a lxd cluster is in an "undefined" state after a snap update while a single one runs fine. [09:47] geodb27: i'm not sure what that means :-| maybe stgraber can shed some light? [09:47] zyga: Thanks for reviewing #7366 - I've found a (I believe) satisfactory solution to only exposing appropriate gpg-agent functionality. I'd be quite glad if you could give it another look in the near future [09:47] PR #7366: interfaces/gpg-keys: Allow access to gpg-agent and creation of lockfiles [09:48] ppd1990: sure, thank you for iterating [09:48] I'll look now :) [09:49] ppd1990: you mean the S.gpg-agent.extra socket? [09:49] zyga: Right [09:49] ppd1990: that sounds very much better, I will defer the final review to jdstrand but I think that's no longer -0.5 on my end [09:49] ppd1990: please update the PR with that [09:50] make sure to update the summary of the interface as well, it's just text but it matters for review clarity [09:50] Indeed Chipaca. I'll look forward to fill in an issue on https://discuss.linuxcontainers.org/ when I face the same problem. Thanks for your kind answers anyway :-) [09:50] zyga: Will do. Any opinion on it being necessary to "bend" the GPG binary in the snap to this socket? Is is somehow possible to "layout" this socket into the snap namespace instead of S.gpg-agent? [09:51] ppd1990: not yet, not for anything in /shome [09:51] (I meant /home/) [09:51] ppd1990: I think if there's a workable way to point gpg there that's enough [09:53] zyga: Fair enough. If it works, it works :) Shall we update the interface description to include "signing and encrypting"? [09:53] mhm [09:53] plas [09:53] *please [09:53] gosh, I cannot type today [09:53] PR snapd#7386 opened: many: refactor boot/boottest and move to bootloader/mockbootloader [09:54] * Chipaca gives zyga a keyboard that does auto-correct on the fly [09:54] Chipaca: thakns ;) [09:55] (that was deliberate) [09:55] * Chipaca pushes an update to the auto-correct snap to mess with zyga [09:56] mborzecki: https://github.com/snapcore/snapd/pull/7218 should now pass [09:56] PR #7218: tests: measure behavior of the device cgroup [09:57] Chipaca: brexit news is surprising [09:57] Chipaca: at this rate I would not be surprised in anything [09:57] zyga: which part of it? [09:58] Chipaca: the whole suspension of parliament [09:58] zyga: and the "if you vote this way you'll be fired"? [09:58] tyranny is fun [09:59] game of thrones, except some people are living in it and are on the frontline [10:00] "this was not meant as a tutorial!?!" [10:04] zyga: about https://github.com/snapcore/snapd/pull/7385#discussion_r319827158 i did the change and it looks a bit awkward, since it's a single go build -o /dev/null for all 5 binaries (in theory all binaries are written to /dev/null) [10:05] PR #7385: tests/cross/go-build: use go list rather than shell trickery [10:05] aw, I see [10:05] I forgot the binary aspect of it [10:05] normally I was just "go building" everything [10:05] but I didn't mind the binaries [10:06] so what is this doing? testing that we can build? [10:06] fwiw i think go build should erorr out there [10:06] zyga: yes, it's cross compiling those binaries [10:07] yeah, +1 [10:07] zyga: can you leave a +1 in the PR? it's green so we'll land it ;P [10:08] sure, done! [10:08] thanks! [10:08] PR snapd#7385 closed: tests/cross/go-build: use go list rather than shell trickery [10:23] Chipaca: I commented on #7386, we don't have any mock* package atm [10:23] PR #7386: many: refactor boot/boottest and move to bootloader/mockbootloader [10:23] fair [10:23] * Chipaca adds more mock packages to fix that issue [10:23] Chipaca: silly you :) [10:24] :-) [10:25] zyga: updated #7302 [10:25] PR #7302: cmd/snap-confine: add support for parallel instances of classic snaps <⛔ Blocked> [10:37] mborzecki: thanks, I'll look shortly [10:37] mborzecki: 19.04 ha some more cgroup wonkiness [10:37] my test is failing there, I think you were right, the /proc/PID/cgroup format changerd [10:37] *changed [10:37] I'll finish and check that out soon [11:08] Chipaca: +1, with comments on something related but prexisting [11:23] PR snapd#7387 opened: packaging/fedora, tests/lib/prepare-restore: helper tool for packing sources for RPM [11:24] zyga: support script for packit ^^ [11:25] looking [11:29] https://github.com/snapcore/snapd/pull/7387#pullrequestreview-282543875 [11:29] PR #7387: packaging/fedora, tests/lib/prepare-restore: helper tool for packing sources for RPM [11:32] * zyga goes for a walk with the dog [11:33] mborzecki: I'll review mount ns for classic snaps PR when I'm back [11:48] zyga: I updated #7366. Thanks again for the guidance [11:48] PR #7366: interfaces/gpg-keys: Allow access to gpg-agent and creation of lockfiles === ricab is now known as ricab|lunch [12:25] pedronis: thanks! I'll fix those in a followup [12:26] PR snapd#7386 closed: many: refactor boot/boottest and move to bootloader/bootloadertest [12:27] zyga, did you see this one? https://paste.ubuntu.com/p/MngRDC5nQm/ [13:00] re [13:00] cachio: what about it? [13:01] mvo: needing to re-login to chrome, i'm omw [13:03] 2fa [13:34] PR snapd#7388 opened: boot, bootloader, o/snapstate: boot env manip goes in bootloader [13:36] pedronis, zyga, ^ includes (as separate commits) the cleanups you'd requested from previous PRs [13:38] Chipaca: we need to talk [13:38] pedronis: ok === ricab|lunch is now known as ricab [13:48] mvo: about the world clocks, you'll also need an extension called "panel world clock (lite)" [13:49] PR snapd#7388 closed: boot, bootloader, o/snapstate: boot env manip goes in bootloader [13:50] jamesh, hey [13:51] jacekn, https://github.com/snapcore/snapd/pull/7361#issuecomment-526330249 [13:51] PR #7361: tests: enabling ubuntu 19.10-64 on spread.yaml [13:51] cmatsuoka: ta [13:51] jamesh, https://github.com/snapcore/snapd/pull/7361#issuecomment-526330249 [13:51] jamesh, any idea about this problem on eoan? [13:55] PR snapd#7389 opened: cmd/libsnap: make feature flag enum 1< [14:04] pedronis: do we have a convetion for constants vs variables? (i think no, but thought to check) [14:05] pedronis: nm [14:08] Chipaca: in which sense? [14:09] pedronis: there was a constant defined for "snap_mode", and I wondered if we wanted it to be obvious it was a constant in usage [14:09] pedronis: but then i realised it was not actually used anywhere [14:10] Chipaca: go doesn't have special naming convention for constants afaik [14:10] pedronis: so the problem disappeared with a nice round "pop!" [14:10] pedronis: :) [14:13] PR snapd#7390 opened: tests: unit test for a refresh failing on configure hook [14:22] PR snapcraft#2696 opened: shell_utils: Add SNAPCRAFT_DEBUG_SCRIPTS env variable [14:40] PR snapd#7391 opened: boot, bootloader, o/devicestate: boot env manip goes in boot [14:40] pedronis: ^^^ [14:49] mvo, hey, this is the error on pi2 https://paste.ubuntu.com/p/zzQ8Q73SY6/ [14:50] when we try to create a user [15:00] cachio_: that error looks like /var/lib/extrausers isn't writable [15:02] PR snapd#7389 closed: cmd/libsnap: make feature flag enum 1< [15:05] cachio_: what Chipaca said, can you ssh into this machine and touc a file in /var/lib/extrausers ? maybe something "funny" is going on with the writable magic [15:08] Chipaca: so I keep poking at ondras LK pr - I played with your idea to split the types and have a "lk" and "lkPrepareImage" and that makes the code easier but it feels a bit strange. I was wondering if we should have a "func PrepareImage(gadgetDir)" on the bootloader interface (and kill ConfigFile there which does not make sense in a world of partitions). wdyt? [15:09] mvo: would the PrepareImage(gadgetDir) on bootloader avoid all the ifs? [15:10] Chipaca: I am exploring this, the way image.go works probably also needs tweaking [15:10] Chipaca: anyway, if its not something you looked at (yet) I keep poking, was just wondering if you had thoughts :) [15:11] Chipaca: https://github.com/snapcore/snapd/pull/7320 needs some attention [15:11] PR #7320: snap/pack, cmd_pack: 'snap pack --check-skeleton' checks interfaces [15:11] mvo: if it avoids the ifs i'm fine with it :-) i'd check with pedronis wrt core20 but i think bootloader will be growing something similar anyway? not sure [15:11] zyga: ack [15:12] Chipaca: it seems conceptually clean so I think its worth exploring, I look at the image bits now and then I will chat with pedronis if this makes sense [15:13] mvo: if not, if the bootloader really needs different behaviour depending on whether it's in prepare or regular mode, then it's fine for it to be two separate things. Alternatively it might just be parametrised if the ifs are just to determine a string [15:13] or, you know, subclassed if it's somewhere in the middle :-) [15:14] and so it rains [15:14] welcome to september kids [15:14] Chipaca: yeah, I have sometihng without ifs now but I'm still not happy, it feels conceptually not great [15:14] mvo: 7286 is close but I left a comment there [15:14] mvo: what does PrepareImage do? [15:14] Chipaca: https://github.com/snapcore/snapd/compare/master...mvo5:lk-2?expand=1 fwiw - based on the earlier ideas [15:15] cachio: can you look at 7218 again please [15:15] sure [15:15] pedronis: it would do the job of the current InstallBootConfig and setting the boot vars. its mostly there because with LK runtime and image-building time is conceptually different. the image-build needs to write to a file and the runtime to a partition. so I was thinking it would be nice to have a clear interface for this [15:16] cachio: https://github.com/snapcore/snapd/pull/7361 seems to be more complex, do you think we can close it and open dedicated PRs for address book interface and test issue, go fmt test issue and then the remaining 19.10 mechanics [15:16] PR #7361: tests: enabling ubuntu 19.10-64 on spread.yaml [15:17] cachio: especially since the interface test may take more iteration [15:17] mvo: ok, it will need some mediation for Core 20 though, because is not all a booloader problem [15:18] pedronis: yeah, let me experiment a little bit and then I can write something up. I don't want to waste your time with something that may not be great in the end [15:18] (s/great/helpful/) [15:18] zyga, ok, what about moving 19.10 to manual [15:19] cachio: that's okay too, if that helps you out [15:19] mvo: I am also touching all that code, [15:19] cachio: or remove the tests from 19.10 [15:19] then we have the system in the spread yaml nad aI can create both changes [15:19] well touching soon [15:19] cachio: anything that is focused on one task and can land fast is good in my book [15:19] ok, in that case I'll remove those failing tests from 19.10 [15:20] so we can go forward [15:20] zyga, thanks [15:20] that sounds good, thank you [15:23] mvo: my worry is that not all the bootvars will be a task of the bootloader anymore [15:23] so we cannot move the task completely there [15:25] mvo: so the right interface in image is probably something like boot.PrepareImage() but exactly what it needs to take is not super clear atm [15:25] of course internally it would call bootloader bits as needed [15:26] pedronis: right, I can add it to boot.PrepareImage, we probably still need something on the bootloader interface to setup a subset of the bootvars [15:26] pedronis: what's your opinon on using a separate bootloader instead? [15:27] ? [15:27] ah, missing context [15:27] pedronis: Chipaca: so I keep poking at ondras LK pr - I played with your idea to split the types and have a "lk" and "lkPrepareImage" and that makes the code easier but it feels a bit strange. I was wondering if we should have a "func PrepareImage(gadgetDir)" on the bootloader interface (and kill ConfigFile there which does not make sense in a world of partitions). wdyt? [15:27] have bootloader that we use only in prepare-image ? [15:28] doesnt' sound a great idea to me [15:28] yeah as mvo says it feels weird :-) [15:28] it makes the code more organized though but still feels weird [15:28] pedronis: it started because currently almost everything in lk is "if inPrepareImage { one thing } else { some other thing }" [15:29] well [15:29]