[05:09] morning [05:30] PR snapd#7621 closed: spdx: add EPL-2.0 to licenses [05:46] mvo: hey [05:46] hey mborzecki [06:01] PR snapd#7609 closed: snap-recovery: remove "usedPartitions" from sfdisk.Create() [06:01] PR snapd#7620 closed: snap-install: add ext4,vfat creation support [06:02] 7613 is ready for review now, pretty small and hopefully simple :) [06:09] mvo: so are we going with snap-recovery instead of recovery-tool, or will the former just stay an internal/testing thing for now? [06:13] mborzecki: hope I understand the question but snap-recovery is the new name of recovery-tool, recovery-tool is a bit too generic (iirc that was what was the outcome of the code review earlier) [06:13] mvo: ah ok, missed that [06:14] mborzecki: there should be no recovery-tool anymore unless I overlooked something :) [06:20] PR snapd#7627 closed: overlord: add checks for bootvars in TestRemodelSwitchToDifferentKernel === pstolowski|afk is now known as pstolowski [07:02] morning === pedronis_ is now known as pedronis [07:09] hey pstolowski [07:18] re [07:18] hey guys [07:18] feeling somewhat better, working from office now [07:19] resuming work on /etc -> /var move [07:26] PR snapd#7628 opened: gadget: helper for volume compatibility checks [07:27] mvo: ^^ [07:27] pstolowski: zyga: hey [07:28] mborzecki: nice one [07:28] zyga: hey ! glad to hear that you feel better [07:28] mvo: thanks [07:29] mborzecki: hi, btw now that claudio changes have landed #7509 should be unblocked ? [07:29] PR #7509: gadget, snap/pack: perform extended validation of gadget metadata and contents [07:29] pedronis: yes [07:41] mvo: check this out [07:41] https://www.irccloud.com/pastebin/Kpa4yYyI/ [07:44] zyga: hm maybe snapd.failure.service ran and exited between the is-active calls? [07:44] that's a good hunch [07:44] perhaps it should stay-after-exit? [07:44] or leave a trace that the test can reliably check for [07:45] PR snapd#7629 opened: snap-recovery: create filesystems as defined in the gadget [08:00] osx jobs fail on travis? [08:02] mborzecki: I didn't see any failures, what did you get? [08:02] zyga: jobn fails, 10 minutes with no output, as if nothing got really executed [08:03] hum [08:03] I've submitted some changes so I will likely see that soon [08:03] but no output is a big hard to debug [08:03] macos has abandoned bash and switched to zsh [08:03] maybe that's a factor? [08:04] really? [08:04] wonder why [08:05] gpl [08:05] they never upgraded past gpl 2.0 licensed version [08:05] there's nothing gpl 3.0 on macos [08:07] duh [08:07] at least they didn't go with tcsh :P [08:07] mash :) [08:10] mvo: so I'm ready to submit seed/seed20 code and the basics of using it for firstboot, but I need reviews otherwise it will just be huge diffs [08:10] mvo: question on your policy branch https://github.com/snapcore/snapd/pull/7615/files#r336367270 [08:11] PR #7615: policy: implement CanRemove policy for the snapd type [08:16] I'll have a break and then switch to doing reviews [08:19] zyga: sure, I have a look [08:19] pedronis: will review your PR in a little bit [08:25] 👋 [08:25] just to note I'm not working today :-) [08:25] did I say note? I meant 'gloat' [08:31] Chipaca: are you okay? [08:44] zyga: what's the status of #7547 ? does it need Jamie's review? [08:44] PR #7547: many: use a dedicated named cgroup hierarchy for tracking [08:44] pedronis: I asked for someone from security (jamie or alex) to review it [08:44] jamie had a look and mentioned the idea is sound (on IRC) [08:44] but there's no full review from him yet [08:45] I think it would be good to have one though it's all entirely behind a feature flag so we might land it without a review assuming we get one later [08:45] I have more code depending on this landing so moving it would unblock me [09:01] mborzecki: reviewed https://github.com/snapcore/snapd/pull/7628#pullrequestreview-303746745 [09:01] PR #7628: gadget: helper for volume compatibility checks [09:01] thx [09:02] thanks zyga ! [09:11] mvo: small review for you https://github.com/snapcore/snapd/pull/7626#pullrequestreview-303758751 [09:11] PR #7626: [RFC] managers: add remodel undo test for new required snaps case [09:15] zyga: happy to discuss this in a bit, fwiw we use similar style in snapstate and wrappers, it seems to be ok if "f() (err error)" style is used - or am I misreading/misunderstanding? [09:16] mvo: it's still easy to define another err [09:16] mvo: I know we are using this style [09:16] but if we are going to rely on it I think it's a bit too fragile [09:17] zyga: it's a bit more subtle [09:18] zyga: you mean if someone defines does "return err2" ? [09:18] the return do an assignment afaiu [09:18] yeah, that is my understanding as well, so I wonder if I miss something [09:18] zyga: see, this code https://play.golang.org/p/gzUa03a0ET7 [09:18] mvo: I could be wrong but last time I was doing it I had unexpected results from foo, err := ... style code even if err was defined [09:19] zyga: this case will work, I can add some test for this later [09:19] zyga: but in the middle of a review right now so I don't want to switch too much context :) [09:20] zyga: aha, pedronis was kind enough to add the pastebin [09:20] eh, play [09:22] zyga: is the this bit of the spec: A "return" statement that specifies results sets the result parameters before any deferred functions are executed. [09:22] https://golang.org/ref/spec#Return_statements [09:25] pedronis: that's interesting, I didn't know that [09:25] I was trying to craft something nefarious in the playground but could not [09:27] zyga: we could add a comment to this kind of code to ensure ppl are not nervous about it [09:28] there are still some ways to get this idiom wrong: [09:29] not defining err in the return type, but have some early err := but using the idiom anyway somehow [09:29] some path that errors but returns nil to the caller (but still hope to get the undoing working) [09:38] mvo: thanks for the review, I asked a question about one of your questions :) [09:45] I'll be back soon, [09:45] it's so cold in the office nowadays [09:45] I should turn more computers on [09:46] (heating is on but the office has weaker heat insulation compared to the rest of the house) [10:00] pedronis: thanks, I replied [10:13] yay, got the cloud image firstboot working end-to-end with pre-baked image \o/ [10:15] now onto open points.. system key, gadget configure hook.. [10:15] pstolowski: nice milestone :) [10:20] mvo: couple of comments in 7626 [10:20] pedronis: thank you [10:20] pstolowski: great [10:28] PR snapd#7548 closed: tests: update snap logs to match for multiple lines for "running" [10:29] pedronis: thanks for your suggestions, I will update the undo PR accordingly, much simpler this way :) [10:33] PR snapd#7630 opened: overlord/devicestate: check snap handler for gadget remodel compatibility [10:33] mvo: pedronis: ^^ [11:11] PR snapcraft#2754 closed: meta: support the case of a plug without a default provider [11:13] mborzecki: thanks, looking [11:13] mborzecki: https://github.com/snapcore/snapd/pull/7630#pullrequestreview-303818422 [11:13] PR #7630: overlord/devicestate: check snap handler for gadget remodel compatibility [11:14] mvo: is https://github.com/snapcore/snapd/pull/7613 already minimal or would merging master shrink it? [11:15] PR #7613: snap-recovery: add minimal binary so that we can use spread on it [11:16] mborzecki: can you do a pass over https://github.com/snapcore/snapd/pull/7614 as well [11:16] PR #7614: cmd/snap-confine: implement snap-device-helper internally [11:17] zyga: its already minimal, I merged master this morning [11:17] mvo: thanks, I'll review it then [11:17] mvo: I only asked because it said it is based on something and I wasn't sure [11:18] zyga: ta [11:37] * pstolowski lunch [11:41] * zyga break as well [11:41] mvo: I'll finish the review soon [11:45] zyga: no worries, I have lunch now === ricab is now known as ricab|lunch [11:58] mvo: https://github.com/snapcore/snapd/pull/7629#pullrequestreview-303823599 :) [11:58] PR #7629: snap-recovery: create filesystems as defined in the gadget [12:01] PR snapd#7628 closed: gadget: helper for volume compatibility checks [12:03] LXD test is failing a few times today [12:11] pedronis: updated #7509 [12:11] PR #7509: gadget, snap/pack: perform extended validation of gadget metadata and contents [12:14] jdstrand: hi, I made a meta comment in the system-backup PR, you probably know but you have quite few snapd PRs asking your review [12:15] pedronis: thanks, yes, going to get through as many as I can today [12:18] off to pick up the kids, or i'm gonna be late for standup [12:21] mvo: hey, others said they would bring this up in your standup, but fyi bug #1848567 [12:21] Bug #1848567: autogenerated per-snap snap-update-ns apparmor profile may contain many duplicate mount rules causing excessive parser memory usage [12:22] oh [12:22] zyga: oh, did you not see backscroll from after you eod'd? [12:22] * jdstrand gathers [12:22] nope [12:22] I didn't read any [12:24] zyga: https://paste.ubuntu.com/p/NKFGsZtBZN/ [12:25] jdstrand: thanks [12:25] jdstrand: I think there is an "easy" way to solve this [12:25] jdstrand: I can give it a try [12:25] jdstrand: we can de-duple in the apparmor spec [12:25] *de-dupe [12:26] jdstrand: and simply change how we emit lines for the offending rules [12:26] zyga: please only dedupe in snap-update-ns profiles though [12:26] jdstrand: I'll try [12:26] jdstrand: though [12:26] jdstrand: I think it'd be good to do globally [12:26] zyga: I do not [12:26] jdstrand: we can have some code that emits # line if line was already added [12:26] jdstrand: why? [12:26] jdstrand: doing it in a specific profile would create a special case [12:27] jdstrand: so more complexity to test [12:28] zyga: snap-update-ns is auto-generated so we need to make sure it is clean. the handcrafted snippets will have few duplicates and removing them will have essentially no performance gain but cause confusion in debugging and auditing [12:28] jdstrand: it depends on how it is done, we can keep the duplicated lines, just comment them out [12:28] I agree with jdstrand that it will get confusing for debugging [12:28] is there documentation somewhere about the acls I can give to `snapcraft export-login`? [12:28] pedronis: it's hard to say if it is confusing before we see it [12:29] zyga: heh [12:29] you are copying something into something else [12:29] except removing some bits [12:29] zyga: please note the problem is that the parser doesn't (currently) have a cheap dedupe for mount rules. all others are dedupes fast. we just need to make sure the auto-generated profiles doesn't balloon [12:29] depending on mixing of things [12:29] that sounds confusing [12:29] a priori [12:29] pedronis: I think you are confused by what I'm trying to do [12:30] jdstrand: sure, I understand the problem [12:30] zyga: I expect every line from a template to end up in the final profile atm [12:30] it sounds you want to change that [12:30] zyga: the commented out idea is better than removing, but we don't want that. why have 17000 commented out lines? [12:31] jdstrand: for the audit you wanted? [12:31] pedronis: I don't want to change anything related to the template [12:31] pedronis: or parse anything for that matter [12:31] zyga: you are changing the relation between template content and output [12:32] zyga: I am talking about *auto-generated* rules. for templated policy with snippets, we should have everything [12:32] pedronis: yes, but that is unavoidable, we must change something to get less output [12:32] zyga: yes, but only for autogenerated stuff [12:32] jdstrand: sure, but I never intended to change the template here [12:32] pedronis: yes, that's what I was proposing [12:33] anyway, I think code speaks clearer than english [12:33] zyga: so I'm not confused, but both me and jdstrand are saying no to that [12:33] I will have a look [12:33] and we can discuss it then [12:33] zyga: but you did say that you would dedupe snap.* profiles, not just snap-update-ns.*. snap.* aren't autogenerated [12:33] zyga: please discuss with mvo how to prioritize this [12:33] jdstrand: my idea is to dedupe snippets [12:33] jdstrand: nothing more [12:33] jdstrand: this idea is super simple, it only depends on how we send mount rules as snippets [12:33] jdstrand: instead of sending one big block of unique text [12:33] zyga: snippets are also not autogenerated rules [12:34] jdstrand: we can send each mount rule [12:34] jdstrand: snippets are just that, the contets is auto generated or not [12:34] zyga: or rather, the ones in interfaces/builtins are not autogenerated [12:34] jdstrand: if we change the content permissions to generate each mount rule as a snippet [12:34] (actually we may already have deduplication in the snippets) [12:34] zyga: there is a difference between snapd calculating individual rules and adding them and snapd splatting out templated snippets [12:34] jdstrand: so instead of sending one, say, ten line snippet [12:35] jdstrand: we can send ten snippets [12:35] jdstrand: and if the next directory rule sends the 9 snippets that are the same [12:35] jdstrand: and one that is different [12:35] jdstrand: we end up with just 11 snippets now [12:35] jdstrand: as I said above, I don't intend to parse or split snippets that are provided to the spec [12:36] jdstrand: just be smarter about the snippets that we add to the spec in this case [12:36] zyga: there are implementation ideas that can be done to dedupe. that is fine. I'm saying that at the end of it all, we should have deduped snap-update-ns policy, but snap.* policy is character by character identical [12:36] to what we have now [12:37] jdstrand: I think I see your point but I don't think we should be this strict about this, this kind of approach introduces special cases that are error prone. I don't think we have any duplication in snap profiles today. Having said that I would not like to if-then-else this special case [12:38] zyga: it doesn't feel that hard. add a flag if and set it to true if generating snap-update-ns [12:38] jdstrand: btw, how did you run into this? [12:38] jdstrand: is there a simple test where we can measure the memory used by apparmor parser? [12:39] jdstrand: agreed [12:39] zyga: people (including myself) were seeing OOMs in Ubuntu vms [12:39] zyga: did you read the bug? :) [12:40] jdstrand: I didn't notice the -f mem thing there, I read the summary only [12:40] jdstrand: I will look at adding a test that checks if the memory used by apparmor parser on a specific profile is not unreasonable [12:40] (though as with all "benchmark" style tests, it's not a clear-cut what is reasonable) [12:40] zyga: btw, there is duplication in the snap.* profiles today, but it is small and unimportant for performance and memory [12:41] zyga: that will be difficult and dependent on how many cpus you have [12:41] jdstrand: I would imagine a test that loads one specific profile should not depend on that [12:41] jdstrand: (load profile one by one, check that memory used by apparmor parser is << than some sane value) [12:42] (with the right arguments to force skipping cache to see the actual cost) [12:42] zyga: one profile no, but that is artificial with pstolowski's patches to load all of the snaps profiles with one parser invocation. it might be ok, but I don't know what a good number is. it is definitely less than 1.5G :) [12:42] jdstrand: btw, why is apparmor parser not de-duplicating mount rules? [12:43] zyga: that is in the bug :) [12:43] jdstrand: yeah, for sure, I need to see [12:43] zyga: it is a bug in apparmor_parser. it should be [12:43] I see [12:43] ok [12:43] mvo: can you triage https://bugs.launchpad.net/snapd/+bug/1848567 :) [12:43] Bug #1848567: autogenerated per-snap snap-update-ns apparmor profile may contain many duplicate mount rules causing excessive parser memory usage [12:43] zyga: but even if that was fixed, 17000 auto-generated rules need to be fixed [12:44] jdstrand: it looked good when we were writing it ;) [12:44] jdstrand: precise and all that [12:44] but yeah [12:44] that is impossible to read, audit and too much disk [12:44] mborzecki: I don't understand why we need volumes for classic gadgets now in #7509 [12:44] PR #7509: gadget, snap/pack: perform extended validation of gadget metadata and contents [12:44] jdstrand: perhaps we should not be this precise [12:44] zyga: well, I think things are pretty 'ok' with a single content interface. but the pattern in the filed is multiple content interfaces like with gnome-calculator [12:45] mborzecki: commented there [12:45] zyga: we are precise for a reason. just dedupe: we are then precise and small [12:46] the file ends up being <700 lines (with comments) after deduped. that is totally fine [12:46] (for gnome-calculator that is) [12:46] memory is inline with all our othe profiles [12:47] time to compile too [12:48] it's a cheap win with no production risk since the rules loaded into the kernel are exactly the same as the 17000 line profile [12:48] jdstrand: because of apparmor parser DFA miminalization? [12:50] zyga: yeah [12:51] jdstrand: well looks like you got your bug discussed before I even had a chance to mention it :-) [12:52] zyga: there is an expensive dedupe stage and a cheap one. the cheap one is missing for mount in the parser. [12:52] yeah, I think I understand how the parser is ending up working this way [12:52] zyga: the idea is, do the cheap one then the expensive. [12:52] state machine transformations are expensive, especially the minimalization that results in exponential states [12:53] ijohnson: heh, it was good. now people know where everyone stands and the discussion in the standup can perhaps be more productive, shorter and about resourcing [12:53] +1 [12:54] oh btw I still have a todo to look into stopping pivot_root for containerd, is that still necessary/relevant (cc joedborg) [12:54] pedronis: heh, right, leftover from the initial version of the pr [12:54] jdstrand: ^ [12:55] ijohnson: hi, I did a pass on the disabled services PRs [12:55] ijohnson: I’ve tried with setting `no_pivot` to true but am still getting the app armor violations on the file system [12:55] ijohnson: kjackal is who wanted it (but I'm sure joedborg is interested too), but yes, kjackal saw prometheus had a similaar issue. I'll let him comment further [12:55] ijohnson: if you don't mind that is [12:56] * kjackal reading [12:58] zyga: fyi, sha256sum for big vs deduped cache files: https://paste.ubuntu.com/p/YFCkGZysk2/ [12:58] jdstrand: how did you dedupe? [12:58] pedronis: I saw that thanks [12:58] zyga: it is in the bug ;) I sort -u then put the preamble lines and the } in the right spot [12:59] jdstrand: +1 [12:59] thanks [12:59] yeah, this is expected [12:59] * jdstrand nods [12:59] ijohnson: joedborg is probably the best to comment on this. The only thing I can say for sure is that we see issues with a number of workloads some of which are probably due to patches mising from containerd. [12:59] just wanted to prove that the kernel blob is the same. no reason to change our rules [13:00] jdstrand, kjackal, joedborg: ack I'll still take a look at it then. btw which snap is this for, the kubernetes-worker snap? I can't seem to find it on github anymore, I thought it was somewhere in https://github.com/charmed-kubernetes/bundle [13:01] ijohnson: https://github.com/charmed-kubernetes/snap-kubernetes-worker is kubernetes-worker [13:01] thanks jdstrand [13:01] ijohnson: https://github.com/ubuntu/microk8s/ is microk8s [13:01] ah thanks jdstrand [13:01] ijohnson: (though I don't know where kjackal's strict mode branch is currently living) [13:02] otoh [13:02] It is here https://github.com/ubuntu/microk8s/tree/feature/strict-v3 [13:03] thanks everybody, will try to look into this soon [13:03] zyga: so, for the benchmark, /usr/bin/time -f "wall: %E, mem: %M Kb" apparmor_parser -QTK -o benchmark.cache profile [13:03] zyga: actually, just to be 100% consistent: /usr/bin/time -f "wall: %E, mem: %M Kb" apparmor_parser -j1 -QTK -o benchmark.cache profile [13:04] PR snapd#7562 closed: tests: check the apparmor_parser when the file exists on snap-confine test [13:05] zyga: where profile is something akin to what gnome-calculator has (eg, multiple content plugs). maybe put memory at under 50000 and time at under 1 second? [13:05] jdstrand: thank you, I'll make sure this is tested [13:06] zyga: you might have to play with those thresholds. the deduped profile was ~32M here and < .5 seconds [13:06] here [13:06] jdstrand: I think I will put the threshold at 512M [13:06] if we needed more mem we will fail on some supported systems [13:06] zyga: maybe make sure it is < 1000? line? (it was 662 here) [13:07] but I need to sit down and code it first [13:07] most likely next week as we won't release over weekend anyway [13:07] zyga: soryy, 50000 was in k. that is 50M [13:08] zyga: 'time' uses 'k' but then I used 'm' in later comments. 50M as the upper limit initially sounds good [13:08] * jdstrand tests on arm64 and armhf real quick [13:09] jdstrand: I'm only grumpy that nobody raised this during 19.10 beta [13:09] jdstrand: we could have fixed it for that release [13:09] jdstrand: but that's why I want to do a test [13:09] zyga: it isn't a 19.10 thing [13:09] jdstrand: so that next time we won't be burned by accident [13:09] jdstrand: oh I know [13:09] but it JUST SHIPPED and nobody noticed before :) [13:09] zyga: oh I see what you mean [13:09] zyga: yes, well, at least it was found by us rather than the field blowing up [13:10] jdstrand: well, the field may still blow up :) [13:10] zyga: but, for sure classic distro is feeling this and don't know it [13:11] zyga: most desktop system probably have 1.5G ram and swap these days, and it is desktop system that hit the pathological multiple content interface case [13:11] jdstrand: yeah [13:11] zyga: so OOM won't usually be hit unless other things are going on [13:11] jdstrand: if you want to review it today I may look at a quick PR after standup [13:12] jdstrand: if you are fine waiting it will be done next week [13:12] but, they could crawl with swap and be a bit slow [13:12] zyga: yes. I am off Mon and Tue. 2.43 material is fine [13:12] jdstrand: oh [13:12] hmmm [13:12] zyga: I'm going to try to get through all the PR reviews today [13:12] perhaps a quick iteration today is better [13:12] well, I'll leave that to you [13:13] otherwise I will resume reviews [13:13] and try to finish a bugfix for apparmor in another area [13:13] (the one with the include file) [13:13] zyga: 2.43 wasn't even branched yet as of... yesterday? wed? [13:13] I don't know the status of the release [13:13] sorry === kristijanzic[m] is now known as KristijanZic [13:14] that's fine. next week is fine. please discuss priority in standup (and feel free to relay I'm reviewing today and off Mon and Tue if that is useful) [13:14] zyga: ^ [13:14] ok [13:15] zyga: a not nearly as important bug that I can't recall if I forwarded to you: https://bugs.launchpad.net/snapd/+bug/1848516 [13:15] Bug #1848516: snap connections/interfaces shows dropped interfaces as connected after refresh [13:15] zyga: oh, I did. I was really surprised remove/install still showed it as connected :) [13:17]