[06:00] morning [06:44] (answer: snapd in the core snap is newer than snapd in bioinic today) [07:08] good morning [07:09] mwhudson: can you give me the denial please [07:09] mwhudson: grep for DENIED in dmesg [07:09] zyga-ubuntu: mornings [07:09] zyga-ubuntu: i think it's the more or less known thing about apparmor and overlayfs not being friends [07:10] mwhudson: ah, so it's still the version witohut overrlay support? [07:10] mwhudson: can you pastebin /var/lib/snapd/system-key [07:10] zyga-ubuntu: i'm going to wait for 2.32 until i get bothersome about it [07:10] zyga-ubuntu: don't have a vm up know [07:10] that's ok [07:10] I thought that patch was out already [07:11] zyga-ubuntu: i think xenial is on 2.23.1 or something [07:12] hm that's what xenial-proposed has anyway [07:12] so does the stable core snap have snapd from xenial-proposed in it? [07:12] that seems surprising [07:12] * zyga-ubuntu doesn't know [07:12] I was a little detached yesterday [07:30] PR snapd#4844 opened: overlord/snapstate: allow core defaults configuration via 'system' key === pstolowski|afk is now known as pstolowski [07:36] mornings [07:37] pstolowski: hey [07:38] pstolowski: can you take a look at #4826? [07:38] PR #4826: wrappers: services which are socket or timer activated should not be started during boot [07:53] mborzecki: yep [07:59] moin moin === _cpaelzer is now known as cpaelzer [08:15] pstolowski: thanks for the review [08:16] np [08:18] PR snapd#4826 closed: wrappers: services which are socket or timer activated should not be started during boot [08:37] PR snapd#4845 opened: snap, store: store version numbers in the commands database [08:38] mborzecki: could you please create a 2.32 version of 4826 (if there isn't one already)? [08:39] mvo: coming up [08:39] ta [08:49] PR snapd#4846 opened: [2.32] wrappers: services which are socket or timer activated should not be started during boot [08:50] mvo: ^^ [08:53] mborzecki: ta [08:53] *yawn* [08:53] just five more minutes [08:58] hello [08:59] pedronis: hi [08:59] Chipaca: hi, I need to update a PR after let me know when you want to chat about puritan [09:00] pedronis: knee-deep in the hash cache mash atm [09:00] Chipaca: I saw you discussing that, did you notice that some of those helpers are used when there's no state [09:01] StateCache need to be optional [09:01] pedronis: yes I did [09:01] I mean snapasserts exist to hold stuff at the intersection of snap files and assertions (but not needing state) [09:02] or at least not requiring state [09:02] * Chipaca nods [09:04] Chipaca: are you looking also into the download case? I though the idea was to fix firstboot first [09:04] store has its own ways to keep state at distance [09:04] pedronis: firstboot first, but with my eye on download to make the followup easy (or trivial) [09:05] easier sounds fair, trivial unlikely [09:05] it's all a bit messy though [09:05] pedronis: did you see the thing about the extra extra hashing during download delta? [09:05] yes, I was aware of that [09:05] we also check the hash of the delta itself [09:06] i guess i'd forgotten :-) [09:06] that's fine [09:06] seems a bit orthogonal [09:06] except that we should have picked a different hash for those [09:06] anyhoo, that's what i'm in [09:06] no sense being super expensive [09:06] given that we need to recheck the whole thing anyhow [09:06] but it's as it is [09:07] pedronis: we can fix that though :-) [09:07] yes, it's fixable [09:07] not today [09:07] also, store is a mess, and daemon is a mess [09:07] ¯\_(ツ)_/¯ [09:07] store was worse [09:07] oh, totally [09:07] baby steps [09:07] it's improving (slowly) [09:08] daemon, no, I don't think we rearchitected much of it yet [09:08] let me know if I can help, also happy to review [09:09] * pedronis back to his high prio stuff [09:09] PR snapd#4847 opened: osutil: add symlinkat(2) [09:20] grumble, the PR that had a consistently failing spread test now passed once I added a some debug stanza :/ I'm quite sure there is something fragile after the change but who knows now [09:23] pedronis: i'm feeling rather dumb here, but I can't figure out where the second hashing in firstboot is happening [09:24] pedronis: derive side info does one, but the second one...? [09:24] it'd be so nice if i didn't have to fire up a vm with fedora/suse just to check what `rpm -E %configure` expands to [09:24] Chipaca: verify-snap [09:24] pedronis: that's not run for firstboot [09:24] ? [09:24] pedronis: firstboot does InstallPath [09:24] pedronis: which means the snap is local, so gets no validate-snap task [09:25] really [09:25] I'm confused [09:25] that's how i read it, but see: dumb [09:25] I thought we always did that [09:25] if we don't there is nothing to fix [09:25] but might be problematic for other reasons [09:25] * Chipaca takes the rest of the day off [09:25] :-) [09:26] that's a recent chnage [09:27] pedronis: 2016 [09:27] nothing to do afaict [09:28] now I'll discover it's by me [09:28] PR snapd#4846 closed: [2.32] wrappers: services which are socket or timer activated should not be started during boot [09:29] pedronis: you'd only discover that if you went looking [09:30] pedronis: I say don't [09:30] it's by me [09:30] so we never did double hashing [09:30] so the symlink fix should make all the difference we can make [09:31] I mean at firstboot [09:31] * Chipaca too [09:32] mmh [09:32] yes, avoiding the copy is as good as it gets [09:33] unless we go back to the cheating script [09:33] same also for core btw [09:33] pedronis: well, core links [09:33] pedronis: the symlink only happens if the link fails [09:33] yea, there's not win to be had on core [09:33] ah. yeah. [09:34] but [09:34] i do feel that if we had a central place to do hashing this would've not taken so long to figure out [09:34] but, not work for today [09:35] not sure how we can achieve that [09:35] unless you mean one codeline, not conceptually [09:35] magic ヽ(｀Д´)⊃━☆ﾟ. * ･｡ﾟ, [09:36] wishful thinking mostly [09:36] we would need unify DeriveSideInfo and doValidateSnap, seems worthwhile but not sure how to attack that [09:37] it's probably possible, bit of a code pretzel though [09:37] * Chipaca nods [09:37] meanwhile I'll do the cheap thing and skip the extra hash in download after successful applyDelta [09:37] then that'll stop bugging me [09:39] pedronis: can we talk about puritan in ~2h [09:40] yes [09:46] mborzecki: can you please look at 4847 again [09:46] I added one more syscall [09:56] mborzecki: go vet is fully happy now again on 1.10, right? [09:57] mvo: yes, it's passing [09:57] mvo: cachio's bionic PR failed in InstallDate() unit test, probably outdated mksquashfs with the bug that you fixed [10:00] sergiusens, mvo I see that https://github.com/snapcore/snapcraft/commit/10509d590658f1c417b91d66a356700483997d75 has landed now, is there any plan to rename base-18 to core18 soon? [10:08] mborzecki: yeah, the issue here is that squashfs has not transitioned to bionic yet iirc the autopkgtest for lxd failed [10:08] ackk: yes, we want to rename it soon, right now a snapcraft issue prevents us from building, this will get fixed this week, then we will do the rename (once we can build again) [10:09] mvo, ah, cool, thanks [10:10] pstolowski: hi, I think I still owe you a bunch of reviews, I will try to have a reviews day tomorrow [10:12] pedronis: hi! no worries, and thanks in advance! I think you can focus on #4358 for now and ignore the rest [10:12] PR #4358: interfaces: interface hooks implementation [10:12] as they are stacked on top of 4358 [10:15] PR snapd#4848 opened: many: cherry-pick relevent `go vet` 1.10 fixes to 2.32 [10:18] mvo: you'll also want to cherry pick 19a1dc929b89bf8dd5d6ad22bb36942b5a2508c4 [10:19] PR snapcraft#1994 closed: docker: add the architecture [10:19] mvo: for bubonic i mean [10:19] bah, for bubonic to pass [10:19] bionic* [10:20] Chipaca: thank you [10:20] there's a go 1.10's behaviour change that breaks our tests (explained in that commit) [10:20] Chipaca: bubonic ;) [10:21] Chipaca: its not in master yet, is it? [10:21] mvo: no [10:21] hope it's not about the plague [10:21] Chipaca, mborzecki lol [10:21] mvo: it's pushed to #4835 [10:21] PR #4835: tests: add bionic system to google backend [10:22] but that fails to be green because the google archive mirror doesn't have the new bionic squashfstools [10:22] Chipaca: we need to enable bionic-proposed there [10:22] Chipaca: but I can cherry-pick into my 2.32 pr [10:22] * Chipaca nods [10:22] this is why i mention it :-) [10:22] ta [10:24] Chipaca: can you take a look at #4841? [10:24] PR #4841: snap/pack, cmd/snap: add `snap pack --check-skeleton` [10:25] hey stgraber ! the autopkgtests for squashfs-tools fail in the lxd testing right now (http://people.canonical.com/~ubuntu-archive/proposed-migration/update_excuses.html#squashfs-tools), is there a know issue with autopkgtest and lxd? the failures do not look related to the squashfs change [10:25] mborzecki: I can do 4831, and 4861, but not 4841 sorry [10:26] * Chipaca lies [10:27] at least 4861 gives the nice tatooine-ish 404 :P [10:28] PR snapcraft#1996 closed: tests: add SNAPCRAFT_KEEP_DATA to debug [10:28] mvo, the problem of enabling bionic-proposed is that I need to doit suring the prepare, is it ok? [10:29] #4829 and #4849 also needs reviews [10:29] PR #4829: store: Sections and WriteCatalogs need to strictly send device auth only if the device has a custom store [10:29] PR #4849: many: propagate contexts enough to be able to mark store operations done from the Ensure loop [10:29] PR snapd#4849 opened: many: propagate contexts enough to be able to mark store operations done from the Ensure loop [10:30] mvo, it will be temporal until is it enabled by default [10:32] cachio: yeah, I think thats ok [10:34] mvo: how would you feel about sliding 4834 into .32? [10:34] #4834 [10:34] PR #4834: snap/squashfs: when installing from seed, try symlink before cp [10:35] PR snapd#4834 closed: snap/squashfs: when installing from seed, try symlink before cp [10:36] Chipaca: let me look [10:36] zyga-ubuntu ping [10:36] Chipaca: absolutely [10:36] mvo: cd7a41b921446bff4ad02ceacf1811c1a6319cc7 fwiw [10:36] Chipaca: I cherry-pick [10:37] Hey ondra [10:37] zyga-ubuntu hey, I have asked this before, but when using layouts, what was the process to read original file? [10:38] Chipaca: ta [10:41] ondra: you can attempt to use hostfs [10:43] zyga-ubuntu any how to do that? [10:48] Chipaca: having early lunch, we can chat in ~40 ? [10:49] pedronis: sure [10:50] ondra: re, sorry, I was afk [10:50] ondra: so, you can look at the file in /var/lib/snapd/hostfs/$F [10:50] ondra: but you will need an interface for t hat [10:50] PR snapd#4850 opened: many: fix shellcheck warnings in bionic [10:51] zyga-ubuntu which interface is that? [10:51] none exists now [10:51] mvo: the spellcheck check is also broken in xenial, fwiw, we just don't run it [10:51] it would depend on $F for sure [10:51] zyga-ubuntu :) [10:51] as otherwise it would let you read any file [10:51] zyga-ubuntu right [10:52] Chipaca: spellcheck or shellcheck? I don't mind so much about the spellcheck, we might as well disable it and only run it via a nightly job [10:52] shellcheck [10:52] ah [10:52] sorry [10:52] :-) [10:52] oh, ok [10:52] sheepcheck [10:52] * Chipaca shellchecks his spellchecker [10:52] shellshock [10:52] zyga-ubuntu wouldn't be easier to allow hook to run before bind mount, and then we can rely on existing permissions? [10:52] ah wait, yes, shellcheck, we're talking of the same thing [10:52] ondra: no, it's not easy in any way [10:53] zyga-ubuntu right [10:53] ondra: and it would still need an interface [10:53] mvo: shellcheck does not run as part of tests; it's not installed on the images [10:53] ondra: as this way you could read abritrary files [10:53] ondra: our current design assumes that applications cannot observe pre-layout filesystem at any stage [10:53] so no partial state is expsed [10:53] zyga-ubuntu ok, so hostfs it is then [10:53] as our apparmor profile assumes the layout is in place [10:53] and grants permissions to the places affected by the layout [10:53] zyga-ubuntu ah, OK that makes sense [10:54] zyga-ubuntu: regarding #4571 we could move configure.ac/autogen.sh to the top of snapd tree [10:54] PR #4571: data, cmd, packaging: use autotools to generate artifacts under data [10:54] ondra: my advice is to open a thread to request an interface for reading hostfs file (specific file) so that we can grant this [10:54] mvo: what do you think about mborzecki's proposal? [10:54] I'm okay but we had a discussion with that and this is how we ended up with "autotools is hidden in cmd/" [10:55] zyga-ubuntu OK will do [11:00] zyga-ubuntu, that should be fixed? https://travis-ci.org/snapcore/snapd/builds/353519499#L5247 [11:00] zyga-ubuntu, or it is still in progress [11:00] cachio: no, not yet. I'm working on that now [11:01] zyga-ubuntu, ah, ok, thanks [11:04] zyga-ubuntu: I think to think but to me autotools is just something I dislike whereever it lifes [11:04] zyga-ubuntu: I mean "I need to think" [11:04] mvo: any ideas as to what to do then? [11:04] should we move away from autotools [11:04] or just use it consistently [11:06] zyga-ubuntu: sorry, I know my comment was not helpful. but yeah, either consistently or switch but switching must be worth it, mesons seems to be popular but I'm not sure its sufficiently better [11:07] (to justify the cost) [11:08] mvo: xenial version of meson is outdated, and afaik it's not available in trusty at all [11:11] zyga-ubuntu mvo any idea what could be causing "run hook "post-refresh": execv failed: No such file or directory" [11:11] hmmm, [11:12] ondra: what does the hook look like? [11:12] zyga-ubuntu https://pastebin.ubuntu.com/p/w22S3q9Y2D/ [11:12] zyga-ubuntu :) [11:13] zyga-ubuntu I trimmed it down, as I can't figure out what is the problem [11:13] so you refresh a snap and the hook fails to run? [11:13] is it reproducible? [11:14] zyga-ubuntu I have install hook which runs just fine I did sym link as I usually do and it failed to run [11:14] zyga-ubuntu install works [11:14] symlink? [11:14] zyga-ubuntu happening all the time [11:14] zyga-ubuntu that was before [11:14] pstolowski: ^ wanna look? [11:14] zyga-ubuntu so I replaced sym link with file, and still same [11:16] ondra: can you share that snap? [11:16] pstolowski yeah preparing to share it [11:22] pstolowski zyga-ubuntu damn, I did clean build and now it works, and I did same before [11:22] do you have the old snap? [11:22] so can't reproduce it now [11:22] maybe look inside [11:23] zyga-ubuntu so I was using snap try [11:23] zyga-ubuntu and without rebuild I was modifying prime/meta/hook [11:25] PR snapd#4848 closed: many: cherry-pick relevent `go vet` 1.10 fixes to 2.32 === bashfulrobot_ is now known as bashfulrobot === Kamilion|ZNC is now known as Kamilion === diddledan_ is now known as diddledan === mborzeck1 is now known as mborzecki === wiking_ is now known as wiking [11:30] Chipaca: I'm back [11:31] pedronis: wb :) [11:31] pedronis: so, if I understand your point of view, using puritan for everything stringish isn't a sustainable practice [11:31] zyga-ubuntu pstolowski http://people.canonical.com/~okubik/git_2.7.4_amd64.snap [11:32] zyga-ubuntu so I'm using snap try, so unsquash it first [11:32] Chipaca: indeed [11:32] ondra: could this be a missing -x on the hook script? [11:32] zyga-ubuntu pstolowski so now I have different problem, it will install, but if I try to run I get execv failed: No such file or directory [11:32] pedronis: OTOH my point of view is that unless we do something for every stringish thinig, we're going to eventually miss one we shouldn't [11:33] zyga-ubuntu pstolowski do $ snap try <>; it will run, call $ snap try <> on same directory and it will break [11:33] pedronis: would you be happier with using a lot more types, for example? [11:33] Chipaca: no in general [11:34] Chipaca: we need tooling and a way to mark things [11:34] Chipaca: I find needing a special type for snap-id very suspect [11:34] it feels very cargo cultish [11:34] pedronis: when you say tooling, do you mean a static checker, or do you mean a json decoder replacement? [11:34] whatever works [11:34] ondra: what core are you running with? [11:35] Chipaca: atm we are just making a bit of code uglier without clear path forward [11:35] pstolowski 16-2.31.2+git610.7a79b84 (4243) [11:36] pedronis: what would you do? [11:36] Chipaca: also whatevedr is problematic with the store is likely problematic with snap.yaml [11:36] as well [11:36] Chipaca: I don't know, I'm not even sure I agree with your premise [11:36] that we really need to worry about everything [11:37] but if we do need to worry about everything the current approach alone doesn't cut it [11:38] pedronis: because of the lack of a static checker? [11:38] ondra: ok, same core here. I un-suqashed it, did snap try twice, it worked. can you do 'snap changes' to find changes correspondin to your snap-try commands, and then 'snap change ' for both to see more info about the failure? [11:38] Chipaca: it seems we need to mark all structs with json or yaml tags as either risky or not and then decide what to do with each [11:39] zyga-ubuntu: mvo: tried having just cmd/configure.ac and doing AC_CONFIG_FILES([../data/Makefile ..]) but that doesn't work, the resulting makefiles do some silly thing and try to cd one level above snapd checkout [11:39] though sometimes we use map[string]interface{} directly, maybe not on input [11:39] not sure [11:40] mborzecki: thank you for confirming that [11:41] zyga-ubuntu: mvo: just moving configure.ac and autogen.sh to the top level dir (plus adjusting paths) is enough, it will clutter the directory once you autoreconf but at least both cmd and data are built using autohell [11:41] mborzecki: yeah, [11:41] you have my +1 but convince gustavo as he was opposing it before [11:42] pedronis: that's a mountain of work, but it does seem the logical conclusion [11:42] zyga-ubuntu: we could easily say with the hack that's right now in the PR :P works too, and does not clutter the directory [11:42] Chipaca: well, the non logical solution is say we need to care about everything but then leaving it up to reviewers [11:43] Chipaca: could we do a silly hack with reflection and field tags perhaps? [11:44] pstolowski https://paste.ubuntu.com/p/7J8yPTrmBG/ [11:44] * cachio afk [11:45] Chipaca: then have an unmarshaller that wraps json and goes to inspect all the strings to clean them up, unless there's a tag that says to leave the string as it is [11:45] pedronis: ^^ [11:45] there are probably a few possible approaches [11:46] there are also concerns about runtime speed [11:46] also personally don't think we should write a full new json decoder [11:48] agreed, i'm not propsing to write a json decoder, just sanitize the structs when they come out of the decoder somehow, in a separate call perhaps [11:48] pstolowski here you have whole test https://paste.ubuntu.com/p/2q7PH8cqk4/ [11:48] pstolowski from snap remove [11:49] Chipaca: now we also need to understand what everything means, I suppose you mean that for example store generated error messages are also a concern? [11:49] pstolowski exactly same error I was getting before about configure hook [11:49] Chipaca: I mean is our attacker a user of the store, or the store? [11:49] pstolowski need to run for lunch, but will be available later [11:50] (there are blurry lines there but it still make a difference where to spend our energy) [11:50] pedronis: if it's the store itself i think things fall apart [11:51] pedronis: but, the problem is bugs [11:51] I understand [11:51] but there are all sorts of bugs [11:51] pedronis: I mean, #1750527 is the bug [11:51] Bug #1750527: dashboard does not validate text fields [11:52] pedronis: even with that change, it's not clear to me everything textish from the store is validated [11:52] Chipaca: I'm not arguing for doing nothing, but I'm also not happy to do something cargo cultish and unbounded [11:53] pedronis: I understand, but if the alternative to cargo culting something is having to make a decision about every single field, cargo culting starts to look good [11:53] Chipaca: you are saying we should use the store data without knowing what it is / what it is about [11:54] pedronis: "is this something that can be edited by third parties, and is it going to be shown to the user" seemed to be the question [11:54] the first half we can probably answer easily [11:55] Chipaca: and snap-id is dubious? [11:55] anyway at this point as I said even if we continue to use string for some things [11:55] as I said we need to worry about all the json and all the yaml [11:55] I don't think snap-id is dubious, no [11:55] but you made a puritan.SimpleString [11:56] but if you make everything not be a string, you can write a really silly static checker [11:56] the alternative is tagging, which isn't hard (but I hadn't thought of it in this context) [11:56] I'm skeptical of that [11:57] (the silly part) [11:58] pedronis: it'd be only a little more work than the check in daemon for commands [11:58] ? [11:58] we define local types sometimes to parse json [11:59] ok [11:59] pedronis: so what do we do [12:00] it seems like we need to introduce safejson and safeyaml, and not use json and yaml, as thin wrappers and go from there [12:01] with a silly check that we don't use the other diretly in non-tests [12:01] pedronis: what are safejson and safeyaml? [12:01] Chipaca: packages with a subset of what is offered by json and yaml [12:02] we can probably then make them have different behavior at test time vs runtime [12:02] and at test time they could check the rules that we make for target types === tyhicks` is now known as tyhicks === tyhicks is now known as Guest65850 [12:05] Chipaca: it seems the only approach to make sure we cover all the ground (I'm a bit worried about the messiness of dealing with our various uses of json statically) [12:05]