| mborzecki | morning | 05:09 |
|---|---|---|
| mup | PR snapd#7621 closed: spdx: add EPL-2.0 to licenses <Created by ralight> <Closed by pedronis> <https://github.com/snapcore/snapd/pull/7621> | 05:30 |
| mborzecki | mvo: hey | 05:46 |
| mvo | hey mborzecki | 05:46 |
| mup | PR snapd#7609 closed: snap-recovery: remove "usedPartitions" from sfdisk.Create() <Created by mvo5> <Merged by mvo5> <https://github.com/snapcore/snapd/pull/7609> | 06:01 |
| mup | PR snapd#7620 closed: snap-install: add ext4,vfat creation support <Created by mvo5> <Merged by mvo5> <https://github.com/snapcore/snapd/pull/7620> | 06:01 |
| mvo | 7613 is ready for review now, pretty small and hopefully simple :) | 06:02 |
| mborzecki | 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:09 |
| mvo | 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 |
| mborzecki | mvo: ah ok, missed that | 06:13 |
| mvo | mborzecki: there should be no recovery-tool anymore unless I overlooked something :) | 06:14 |
| mup | PR snapd#7627 closed: overlord: add checks for bootvars in TestRemodelSwitchToDifferentKernel <Simple 😃> <Created by mvo5> <Merged by bboozzoo> <https://github.com/snapcore/snapd/pull/7627> | 06:20 |
| === pstolowski|afk is now known as pstolowski | ||
| pstolowski | morning | 07:02 |
| === pedronis_ is now known as pedronis | ||
| mvo | hey pstolowski | 07:09 |
| zyga | re | 07:18 |
| zyga | hey guys | 07:18 |
| zyga | feeling somewhat better, working from office now | 07:18 |
| zyga | resuming work on /etc -> /var move | 07:19 |
| mup | PR snapd#7628 opened: gadget: helper for volume compatibility checks <Remodel :train:> <Created by bboozzoo> <https://github.com/snapcore/snapd/pull/7628> | 07:26 |
| mborzecki | mvo: ^^ | 07:27 |
| mborzecki | pstolowski: zyga: hey | 07:27 |
| mvo | mborzecki: nice one | 07:28 |
| mvo | zyga: hey ! glad to hear that you feel better | 07:28 |
| zyga | mvo: thanks | 07:28 |
| pedronis | mborzecki: hi, btw now that claudio changes have landed #7509 should be unblocked ? | 07:29 |
| mup | PR #7509: gadget, snap/pack: perform extended validation of gadget metadata and contents <Created by bboozzoo> <https://github.com/snapcore/snapd/pull/7509> | 07:29 |
| mborzecki | pedronis: yes | 07:29 |
| zyga | mvo: check this out | 07:41 |
| zyga | https://www.irccloud.com/pastebin/Kpa4yYyI/ | 07:41 |
| mborzecki | zyga: hm maybe snapd.failure.service ran and exited between the is-active calls? | 07:44 |
| zyga | that's a good hunch | 07:44 |
| zyga | perhaps it should stay-after-exit? | 07:44 |
| zyga | or leave a trace that the test can reliably check for | 07:44 |
| mup | PR snapd#7629 opened: snap-recovery: create filesystems as defined in the gadget <Created by mvo5> <https://github.com/snapcore/snapd/pull/7629> | 07:45 |
| mborzecki | osx jobs fail on travis? | 08:00 |
| zyga | mborzecki: I didn't see any failures, what did you get? | 08:02 |
| mborzecki | zyga: jobn fails, 10 minutes with no output, as if nothing got really executed | 08:02 |
| zyga | hum | 08:03 |
| zyga | I've submitted some changes so I will likely see that soon | 08:03 |
| zyga | but no output is a big hard to debug | 08:03 |
| zyga | macos has abandoned bash and switched to zsh | 08:03 |
| zyga | maybe that's a factor? | 08:03 |
| mborzecki | really? | 08:04 |
| mborzecki | wonder why | 08:04 |
| zyga | gpl | 08:05 |
| zyga | they never upgraded past gpl 2.0 licensed version | 08:05 |
| zyga | there's nothing gpl 3.0 on macos | 08:05 |
| mborzecki | duh | 08:07 |
| mborzecki | at least they didn't go with tcsh :P | 08:07 |
| zyga | mash :) | 08:07 |
| pedronis | 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 |
| zyga | mvo: question on your policy branch https://github.com/snapcore/snapd/pull/7615/files#r336367270 | 08:10 |
| mup | PR #7615: policy: implement CanRemove policy for the snapd type <Created by mvo5> <https://github.com/snapcore/snapd/pull/7615> | 08:11 |
| pedronis | I'll have a break and then switch to doing reviews | 08:16 |
| mvo | zyga: sure, I have a look | 08:19 |
| mvo | pedronis: will review your PR in a little bit | 08:19 |
| Chipaca | 👋 | 08:25 |
| Chipaca | just to note I'm not working today :-) | 08:25 |
| Chipaca | did I say note? I meant 'gloat' | 08:25 |
| zyga | Chipaca: are you okay? | 08:31 |
| pedronis | zyga: what's the status of #7547 ? does it need Jamie's review? | 08:44 |
| mup | PR #7547: many: use a dedicated named cgroup hierarchy for tracking <Created by zyga> <https://github.com/snapcore/snapd/pull/7547> | 08:44 |
| zyga | pedronis: I asked for someone from security (jamie or alex) to review it | 08:44 |
| zyga | jamie had a look and mentioned the idea is sound (on IRC) | 08:44 |
| zyga | but there's no full review from him yet | 08:44 |
| zyga | 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 |
| zyga | I have more code depending on this landing so moving it would unblock me | 08:45 |
| zyga | mborzecki: reviewed https://github.com/snapcore/snapd/pull/7628#pullrequestreview-303746745 | 09:01 |
| mup | PR #7628: gadget: helper for volume compatibility checks <Remodel :train:> <Created by bboozzoo> <https://github.com/snapcore/snapd/pull/7628> | 09:01 |
| mborzecki | thx | 09:01 |
| mvo | thanks zyga ! | 09:02 |
| zyga | mvo: small review for you https://github.com/snapcore/snapd/pull/7626#pullrequestreview-303758751 | 09:11 |
| mup | PR #7626: [RFC] managers: add remodel undo test for new required snaps case <Created by mvo5> <https://github.com/snapcore/snapd/pull/7626> | 09:11 |
| mvo | 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:15 |
| zyga | mvo: it's still easy to define another err | 09:16 |
| zyga | mvo: I know we are using this style | 09:16 |
| zyga | but if we are going to rely on it I think it's a bit too fragile | 09:16 |
| pedronis | zyga: it's a bit more subtle | 09:17 |
| mvo | zyga: you mean if someone defines does "return err2" ? | 09:18 |
| pedronis | the return do an assignment afaiu | 09:18 |
| mvo | yeah, that is my understanding as well, so I wonder if I miss something | 09:18 |
| pedronis | zyga: see, this code https://play.golang.org/p/gzUa03a0ET7 | 09:18 |
| zyga | 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:18 |
| mvo | zyga: this case will work, I can add some test for this later | 09:19 |
| mvo | zyga: but in the middle of a review right now so I don't want to switch too much context :) | 09:19 |
| mvo | zyga: aha, pedronis was kind enough to add the pastebin | 09:20 |
| mvo | eh, play | 09:20 |
| pedronis | 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 |
| pedronis | https://golang.org/ref/spec#Return_statements | 09:22 |
| zyga | pedronis: that's interesting, I didn't know that | 09:25 |
| zyga | I was trying to craft something nefarious in the playground but could not | 09:25 |
| mvo | zyga: we could add a comment to this kind of code to ensure ppl are not nervous about it | 09:27 |
| pedronis | there are still some ways to get this idiom wrong: | 09:28 |
| pedronis | not defining err in the return type, but have some early err := but using the idiom anyway somehow | 09:29 |
| pedronis | some path that errors but returns nil to the caller (but still hope to get the undoing working) | 09:29 |
| pedronis | mvo: thanks for the review, I asked a question about one of your questions :) | 09:38 |
| zyga | I'll be back soon, | 09:45 |
| zyga | it's so cold in the office nowadays | 09:45 |
| zyga | I should turn more computers on | 09:45 |
| zyga | (heating is on but the office has weaker heat insulation compared to the rest of the house) | 09:46 |
| mvo | pedronis: thanks, I replied | 10:00 |
| pstolowski | yay, got the cloud image firstboot working end-to-end with pre-baked image \o/ | 10:13 |
| pstolowski | now onto open points.. system key, gadget configure hook.. | 10:15 |
| zyga | pstolowski: nice milestone :) | 10:15 |
| pedronis | mvo: couple of comments in 7626 | 10:20 |
| mvo | pedronis: thank you | 10:20 |
| pedronis | pstolowski: great | 10:20 |
| mup | PR snapd#7548 closed: tests: update snap logs to match for multiple lines for "running" <Created by sergiocazzolato> <Merged by pedronis> <https://github.com/snapcore/snapd/pull/7548> | 10:28 |
| mvo | pedronis: thanks for your suggestions, I will update the undo PR accordingly, much simpler this way :) | 10:29 |
| mup | PR snapd#7630 opened: overlord/devicestate: check snap handler for gadget remodel compatibility <Remodel :train:> <Created by bboozzoo> <https://github.com/snapcore/snapd/pull/7630> | 10:33 |
| mborzecki | mvo: pedronis: ^^ | 10:33 |
| mup | PR snapcraft#2754 closed: meta: support the case of a plug without a default provider <Created by sergiusens> <Merged by sergiusens> <https://github.com/snapcore/snapcraft/pull/2754> | 11:11 |
| mvo | mborzecki: thanks, looking | 11:13 |
| zyga | mborzecki: https://github.com/snapcore/snapd/pull/7630#pullrequestreview-303818422 | 11:13 |
| mup | PR #7630: overlord/devicestate: check snap handler for gadget remodel compatibility <Remodel :train:> <Created by bboozzoo> <https://github.com/snapcore/snapd/pull/7630> | 11:13 |
| zyga | mvo: is https://github.com/snapcore/snapd/pull/7613 already minimal or would merging master shrink it? | 11:14 |
| mup | PR #7613: snap-recovery: add minimal binary so that we can use spread on it <Created by mvo5> <https://github.com/snapcore/snapd/pull/7613> | 11:15 |
| zyga | mborzecki: can you do a pass over https://github.com/snapcore/snapd/pull/7614 as well | 11:16 |
| mup | PR #7614: cmd/snap-confine: implement snap-device-helper internally <Created by zyga> <https://github.com/snapcore/snapd/pull/7614> | 11:16 |
| mvo | zyga: its already minimal, I merged master this morning | 11:17 |
| zyga | mvo: thanks, I'll review it then | 11:17 |
| zyga | mvo: I only asked because it said it is based on something and I wasn't sure | 11:17 |
| mvo | zyga: ta | 11:18 |
| * pstolowski lunch | 11:37 | |
| * zyga break as well | 11:41 | |
| zyga | mvo: I'll finish the review soon | 11:41 |
| mvo | zyga: no worries, I have lunch now | 11:45 |
| === ricab is now known as ricab|lunch | ||
| zyga | mvo: https://github.com/snapcore/snapd/pull/7629#pullrequestreview-303823599 :) | 11:58 |
| mup | PR #7629: snap-recovery: create filesystems as defined in the gadget <Created by mvo5> <https://github.com/snapcore/snapd/pull/7629> | 11:58 |
| mup | PR snapd#7628 closed: gadget: helper for volume compatibility checks <Remodel :train:> <Created by bboozzoo> <Merged by bboozzoo> <https://github.com/snapcore/snapd/pull/7628> | 12:01 |
| zyga | LXD test is failing a few times today | 12:03 |
| mborzecki | pedronis: updated #7509 | 12:11 |
| mup | PR #7509: gadget, snap/pack: perform extended validation of gadget metadata and contents <Created by bboozzoo> <https://github.com/snapcore/snapd/pull/7509> | 12:11 |
| pedronis | 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:14 |
| jdstrand | pedronis: thanks, yes, going to get through as many as I can today | 12:15 |
| mborzecki | off to pick up the kids, or i'm gonna be late for standup | 12:18 |
| jdstrand | mvo: hey, others said they would bring this up in your standup, but fyi bug #1848567 | 12:21 |
| mup | Bug #1848567: autogenerated per-snap snap-update-ns apparmor profile may contain many duplicate mount rules causing excessive parser memory usage <aa-parser> <AppArmor:New> <snapd:New> <https://launchpad.net/bugs/1848567> | 12:21 |
| zyga | oh | 12:22 |
| jdstrand | zyga: oh, did you not see backscroll from after you eod'd? | 12:22 |
| * jdstrand gathers | 12:22 | |
| zyga | nope | 12:22 |
| zyga | I didn't read any | 12:22 |
| jdstrand | zyga: https://paste.ubuntu.com/p/NKFGsZtBZN/ | 12:24 |
| zyga | jdstrand: thanks | 12:25 |
| zyga | jdstrand: I think there is an "easy" way to solve this | 12:25 |
| zyga | jdstrand: I can give it a try | 12:25 |
| zyga | jdstrand: we can de-duple in the apparmor spec | 12:25 |
| zyga | *de-dupe | 12:25 |
| zyga | jdstrand: and simply change how we emit lines for the offending rules | 12:26 |
| jdstrand | zyga: please only dedupe in snap-update-ns profiles though | 12:26 |
| zyga | jdstrand: I'll try | 12:26 |
| zyga | jdstrand: though | 12:26 |
| zyga | jdstrand: I think it'd be good to do globally | 12:26 |
| jdstrand | zyga: I do not | 12:26 |
| zyga | jdstrand: we can have some code that emits # line if line was already added | 12:26 |
| zyga | jdstrand: why? | 12:26 |
| zyga | jdstrand: doing it in a specific profile would create a special case | 12:26 |
| zyga | jdstrand: so more complexity to test | 12:27 |
| jdstrand | 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 |
| zyga | jdstrand: it depends on how it is done, we can keep the duplicated lines, just comment them out | 12:28 |
| pedronis | I agree with jdstrand that it will get confusing for debugging | 12:28 |
| Saviq | is there documentation somewhere about the acls I can give to `snapcraft export-login`? | 12:28 |
| zyga | pedronis: it's hard to say if it is confusing before we see it | 12:28 |
| pedronis | zyga: heh | 12:29 |
| pedronis | you are copying something into something else | 12:29 |
| pedronis | except removing some bits | 12:29 |
| jdstrand | 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 |
| pedronis | depending on mixing of things | 12:29 |
| pedronis | that sounds confusing | 12:29 |
| pedronis | a priori | 12:29 |
| zyga | pedronis: I think you are confused by what I'm trying to do | 12:29 |
| zyga | jdstrand: sure, I understand the problem | 12:30 |
| pedronis | zyga: I expect every line from a template to end up in the final profile atm | 12:30 |
| pedronis | it sounds you want to change that | 12:30 |
| jdstrand | zyga: the commented out idea is better than removing, but we don't want that. why have 17000 commented out lines? | 12:30 |
| zyga | jdstrand: for the audit you wanted? | 12:31 |
| zyga | pedronis: I don't want to change anything related to the template | 12:31 |
| zyga | pedronis: or parse anything for that matter | 12:31 |
| pedronis | zyga: you are changing the relation between template content and output | 12:31 |
| jdstrand | zyga: I am talking about *auto-generated* rules. for templated policy with snippets, we should have everything | 12:32 |
| zyga | pedronis: yes, but that is unavoidable, we must change something to get less output | 12:32 |
| pedronis | zyga: yes, but only for autogenerated stuff | 12:32 |
| zyga | jdstrand: sure, but I never intended to change the template here | 12:32 |
| zyga | pedronis: yes, that's what I was proposing | 12:32 |
| zyga | anyway, I think code speaks clearer than english | 12:33 |
| pedronis | zyga: so I'm not confused, but both me and jdstrand are saying no to that | 12:33 |
| zyga | I will have a look | 12:33 |
| zyga | and we can discuss it then | 12:33 |
| jdstrand | zyga: but you did say that you would dedupe snap.* profiles, not just snap-update-ns.*. snap.* aren't autogenerated | 12:33 |
| pedronis | zyga: please discuss with mvo how to prioritize this | 12:33 |
| zyga | jdstrand: my idea is to dedupe snippets | 12:33 |
| zyga | jdstrand: nothing more | 12:33 |
| zyga | jdstrand: this idea is super simple, it only depends on how we send mount rules as snippets | 12:33 |
| zyga | jdstrand: instead of sending one big block of unique text | 12:33 |
| jdstrand | zyga: snippets are also not autogenerated rules | 12:33 |
| zyga | jdstrand: we can send each mount rule | 12:34 |
| zyga | jdstrand: snippets are just that, the contets is auto generated or not | 12:34 |
| jdstrand | zyga: or rather, the ones in interfaces/builtins are not autogenerated | 12:34 |
| zyga | jdstrand: if we change the content permissions to generate each mount rule as a snippet | 12:34 |
| zyga | (actually we may already have deduplication in the snippets) | 12:34 |
| jdstrand | zyga: there is a difference between snapd calculating individual rules and adding them and snapd splatting out templated snippets | 12:34 |
| zyga | jdstrand: so instead of sending one, say, ten line snippet | 12:34 |
| zyga | jdstrand: we can send ten snippets | 12:35 |
| zyga | jdstrand: and if the next directory rule sends the 9 snippets that are the same | 12:35 |
| zyga | jdstrand: and one that is different | 12:35 |
| zyga | jdstrand: we end up with just 11 snippets now | 12:35 |
| zyga | jdstrand: as I said above, I don't intend to parse or split snippets that are provided to the spec | 12:35 |
| zyga | jdstrand: just be smarter about the snippets that we add to the spec in this case | 12:36 |
| jdstrand | 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 |
| jdstrand | to what we have now | 12:36 |
| zyga | 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:37 |
| jdstrand | zyga: it doesn't feel that hard. add a flag if and set it to true if generating snap-update-ns | 12:38 |
| zyga | jdstrand: btw, how did you run into this? | 12:38 |
| zyga | jdstrand: is there a simple test where we can measure the memory used by apparmor parser? | 12:38 |
| pedronis | jdstrand: agreed | 12:39 |
| jdstrand | zyga: people (including myself) were seeing OOMs in Ubuntu vms | 12:39 |
| jdstrand | zyga: did you read the bug? :) | 12:39 |
| zyga | jdstrand: I didn't notice the -f mem thing there, I read the summary only | 12:40 |
| zyga | 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 |
| zyga | (though as with all "benchmark" style tests, it's not a clear-cut what is reasonable) | 12:40 |
| jdstrand | zyga: btw, there is duplication in the snap.* profiles today, but it is small and unimportant for performance and memory | 12:40 |
| jdstrand | zyga: that will be difficult and dependent on how many cpus you have | 12:41 |
| zyga | jdstrand: I would imagine a test that loads one specific profile should not depend on that | 12:41 |
| zyga | jdstrand: (load profile one by one, check that memory used by apparmor parser is << than some sane value) | 12:41 |
| zyga | (with the right arguments to force skipping cache to see the actual cost) | 12:42 |
| jdstrand | 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 |
| zyga | jdstrand: btw, why is apparmor parser not de-duplicating mount rules? | 12:42 |
| jdstrand | zyga: that is in the bug :) | 12:43 |
| zyga | jdstrand: yeah, for sure, I need to see | 12:43 |
| jdstrand | zyga: it is a bug in apparmor_parser. it should be | 12:43 |
| zyga | I see | 12:43 |
| zyga | ok | 12:43 |
| zyga | mvo: can you triage https://bugs.launchpad.net/snapd/+bug/1848567 :) | 12:43 |
| mup | Bug #1848567: autogenerated per-snap snap-update-ns apparmor profile may contain many duplicate mount rules causing excessive parser memory usage <aa-parser> <AppArmor:New> <snapd:New> <https://launchpad.net/bugs/1848567> | 12:43 |
| jdstrand | zyga: but even if that was fixed, 17000 auto-generated rules need to be fixed | 12:43 |
| zyga | jdstrand: it looked good when we were writing it ;) | 12:44 |
| zyga | jdstrand: precise and all that | 12:44 |
| zyga | but yeah | 12:44 |
| jdstrand | that is impossible to read, audit and too much disk | 12:44 |
| pedronis | mborzecki: I don't understand why we need volumes for classic gadgets now in #7509 | 12:44 |
| mup | PR #7509: gadget, snap/pack: perform extended validation of gadget metadata and contents <Created by bboozzoo> <https://github.com/snapcore/snapd/pull/7509> | 12:44 |
| zyga | jdstrand: perhaps we should not be this precise | 12:44 |
| jdstrand | 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:44 |
| pedronis | mborzecki: commented there | 12:45 |
| jdstrand | zyga: we are precise for a reason. just dedupe: we are then precise and small | 12:45 |
| jdstrand | the file ends up being <700 lines (with comments) after deduped. that is totally fine | 12:46 |
| jdstrand | (for gnome-calculator that is) | 12:46 |
| jdstrand | memory is inline with all our othe profiles | 12:46 |
| jdstrand | time to compile too | 12:47 |
| jdstrand | 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 |
| zyga | jdstrand: because of apparmor parser DFA miminalization? | 12:48 |
| jdstrand | zyga: yeah | 12:50 |
| ijohnson | jdstrand: well looks like you got your bug discussed before I even had a chance to mention it :-) | 12:51 |
| jdstrand | zyga: there is an expensive dedupe stage and a cheap one. the cheap one is missing for mount in the parser. | 12:52 |
| zyga | yeah, I think I understand how the parser is ending up working this way | 12:52 |
| jdstrand | zyga: the idea is, do the cheap one then the expensive. | 12:52 |
| zyga | state machine transformations are expensive, especially the minimalization that results in exponential states | 12:52 |
| jdstrand | 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 |
| ijohnson | +1 | 12:53 |
| ijohnson | oh btw I still have a todo to look into stopping pivot_root for containerd, is that still necessary/relevant (cc joedborg) | 12:54 |
| mborzecki | pedronis: heh, right, leftover from the initial version of the pr | 12:54 |
| ijohnson | jdstrand: ^ | 12:54 |
| pedronis | ijohnson: hi, I did a pass on the disabled services PRs | 12:55 |
| joedborg | ijohnson: I’ve tried with setting `no_pivot` to true but am still getting the app armor violations on the file system | 12:55 |
| jdstrand | 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 |
| jdstrand | ijohnson: if you don't mind that is | 12:55 |
| * kjackal reading | 12:56 | |
| jdstrand | zyga: fyi, sha256sum for big vs deduped cache files: https://paste.ubuntu.com/p/YFCkGZysk2/ | 12:58 |
| zyga | jdstrand: how did you dedupe? | 12:58 |
| ijohnson | pedronis: I saw that thanks | 12:58 |
| jdstrand | zyga: it is in the bug ;) I sort -u then put the preamble lines and the } in the right spot | 12:58 |
| zyga | jdstrand: +1 | 12:59 |
| zyga | thanks | 12:59 |
| zyga | yeah, this is expected | 12:59 |
| * jdstrand nods | 12:59 | |
| kjackal | 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 |
| jdstrand | just wanted to prove that the kernel blob is the same. no reason to change our rules | 12:59 |
| ijohnson | 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:00 |
| jdstrand | ijohnson: https://github.com/charmed-kubernetes/snap-kubernetes-worker is kubernetes-worker | 13:01 |
| ijohnson | thanks jdstrand | 13:01 |
| jdstrand | ijohnson: https://github.com/ubuntu/microk8s/ is microk8s | 13:01 |
| joedborg | ah thanks jdstrand | 13:01 |
| jdstrand | ijohnson: (though I don't know where kjackal's strict mode branch is currently living) | 13:01 |
| jdstrand | otoh | 13:02 |
| kjackal | It is here https://github.com/ubuntu/microk8s/tree/feature/strict-v3 | 13:02 |
| ijohnson | thanks everybody, will try to look into this soon | 13:03 |
| jdstrand | zyga: so, for the benchmark, /usr/bin/time -f "wall: %E, mem: %M Kb" apparmor_parser -QTK -o benchmark.cache profile | 13:03 |
| jdstrand | 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:03 |
| mup | PR snapd#7562 closed: tests: check the apparmor_parser when the file exists on snap-confine test <Simple 😃> <Created by sergiocazzolato> <Merged by sergiocazzolato> <https://github.com/snapcore/snapd/pull/7562> | 13:04 |
| jdstrand | 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 |
| zyga | jdstrand: thank you, I'll make sure this is tested | 13:05 |
| jdstrand | zyga: you might have to play with those thresholds. the deduped profile was ~32M here and < .5 seconds | 13:06 |
| jdstrand | here | 13:06 |
| zyga | jdstrand: I think I will put the threshold at 512M | 13:06 |
| zyga | if we needed more mem we will fail on some supported systems | 13:06 |
| jdstrand | zyga: maybe make sure it is < 1000? line? (it was 662 here) | 13:06 |
| zyga | but I need to sit down and code it first | 13:07 |
| zyga | most likely next week as we won't release over weekend anyway | 13:07 |
| jdstrand | zyga: soryy, 50000 was in k. that is 50M | 13:07 |
| jdstrand | 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:08 | |
| zyga | jdstrand: I'm only grumpy that nobody raised this during 19.10 beta | 13:09 |
| zyga | jdstrand: we could have fixed it for that release | 13:09 |
| zyga | jdstrand: but that's why I want to do a test | 13:09 |
| jdstrand | zyga: it isn't a 19.10 thing | 13:09 |
| zyga | jdstrand: so that next time we won't be burned by accident | 13:09 |
| zyga | jdstrand: oh I know | 13:09 |
| zyga | but it JUST SHIPPED and nobody noticed before :) | 13:09 |
| jdstrand | zyga: oh I see what you mean | 13:09 |
| jdstrand | zyga: yes, well, at least it was found by us rather than the field blowing up | 13:09 |
| zyga | jdstrand: well, the field may still blow up :) | 13:10 |
| jdstrand | zyga: but, for sure classic distro is feeling this and don't know it | 13:10 |
| jdstrand | 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 |
| zyga | jdstrand: yeah | 13:11 |
| jdstrand | zyga: so OOM won't usually be hit unless other things are going on | 13:11 |
| zyga | jdstrand: if you want to review it today I may look at a quick PR after standup | 13:11 |
| zyga | jdstrand: if you are fine waiting it will be done next week | 13:12 |
| jdstrand | but, they could crawl with swap and be a bit slow | 13:12 |
| jdstrand | zyga: yes. I am off Mon and Tue. 2.43 material is fine | 13:12 |
| zyga | jdstrand: oh | 13:12 |
| zyga | hmmm | 13:12 |
| jdstrand | zyga: I'm going to try to get through all the PR reviews today | 13:12 |
| zyga | perhaps a quick iteration today is better | 13:12 |
| zyga | well, I'll leave that to you | 13:12 |
| zyga | otherwise I will resume reviews | 13:13 |
| zyga | and try to finish a bugfix for apparmor in another area | 13:13 |
| zyga | (the one with the include file) | 13:13 |
| jdstrand | zyga: 2.43 wasn't even branched yet as of... yesterday? wed? | 13:13 |
| zyga | I don't know the status of the release | 13:13 |
| zyga | sorry | 13:13 |
| === kristijanzic[m] is now known as KristijanZic | ||
| jdstrand | 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 |
| jdstrand | zyga: ^ | 13:14 |
| zyga | ok | 13:14 |
| jdstrand | 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 |
| mup | Bug #1848516: snap connections/interfaces shows dropped interfaces as connected after refresh <snapd:New> <https://launchpad.net/bugs/1848516> | 13:15 |
| jdstrand | zyga: oh, I did. I was really surprised remove/install still showed it as connected :) | 13:15 |
| jdstrand | erf, 'time' isn't in UC | 13:17 |
| ogra | jdstrand, lies ... my UC16 has it :P | 13:18 |
| jdstrand | ogra: /usr/bin/time? | 13:18 |
| ogra | ah, no, the shell builtin | 13:19 |
| jdstrand | (it is also a shell builtin, but that isn't what I want) | 13:19 |
| * jdstrand nods | 13:19 | |
| jdstrand | using classic snap | 13:19 |
| jdstrand | ogra: I totally forgot how handy /usr/bin/time is | 13:20 |
| ogra | haha, yeah | 13:20 |
| jdstrand | the builtin is all I think I've used for years :) | 13:20 |
| ogra | oh, it actually even comes from its own deb | 13:20 |
| ogra | i thought it is part of coreutils | 13:20 |
| jdstrand | huh, me too | 13:23 |
| jdstrand | zyga: oh, /usr/bin/time -f "wall: %E, mem: %M Kb" apparmor_parser -QTK -Ono-expr-simplify -j1 -o cache profile | 13:23 |
| jdstrand | zyga: (forgot -Ono-expr-simplify) | 13:23 |
| jdstrand | zyga: armhf: wall: 0:00.24, mem: 3472 Kb | 13:24 |
| === ricab|lunch is now known as ricab | ||
| jdstrand | zyga: arm64: wall: 0:00.45, mem: 5392 Kb | 13:39 |
| * jdstrand moves along | 13:40 | |
| ogra | heh, thats the same binary you call on armhf and arm64 i guess ? | 13:40 |
| ogra | (surprising it isnt 2x the memory size but only 1.5 or so ... ) | 13:41 |
| ijohnson | hey Chipaca got a question yesterday about store connections from snapd... is there a single global timeout we set for all connections to the store? and if there is, can a user adjust that timeout? | 14:02 |
| mup | PR snapd#7574 closed: client: add KnownOptions to Know() and support remote assertions <Created by mvo5> <Merged by mvo5> <https://github.com/snapcore/snapd/pull/7574> | 14:09 |
| pedronis | ijohnson: John is off today, we have a couple of timeouts and retry strategies, they are not user configurable atm | 14:09 |
| ijohnson | ah okay, thanks pedronis | 14:09 |
| mvo | 7622 is now also ready for a review | 14:10 |
| mvo | zyga: hm, it looks like I did not look closely enough, 7613 was the smallest snap-recovery PR in flight right now, I will apply your feedback to the other to this one. sorry that I overlooked this earlier | 14:13 |
| zyga | mvo: no worries :) | 14:13 |
| pedronis | mborzecki: re-reviewed 7509 | 14:23 |
| mborzecki | pedronis: thanks | 14:24 |
| jdstrand | pedronis: hey, those snap-ids, they were for all stores, correct? | 14:31 |
| pedronis | jdstrand: yes | 14:32 |
| jdstrand | thanks | 14:32 |
| zyga | jdstrand: I think it works | 14:38 |
| zyga | jdstrand: have a look https://usercontent.irccloud-cdn.com/file/FB0h24Ik/deduped-update-ns-profile | 14:39 |
| zyga | I need to adjust tests but it's simple enough | 14:39 |
| zyga | but firstd | 14:39 |
| zyga | foood :D | 14:39 |
| * zyga had only breakfast and it's almost 5PM | 14:39 | |
| jdstrand | zyga: yikes. that happened to me yesterday :\ | 14:40 |
| zyga | as a super-cheap approach for now until tests pass | 14:41 |
| zyga | jdstrand: diff that makes it go https://www.irccloud.com/pastebin/TCWMioYP/ | 14:41 |
| zyga | it relies on spec.AddUpdateNS() de-duplicating already | 14:41 |
| zyga | ok, time for that food | 14:41 |
| joeubuntu | hey all - looking for a pointer. I've heard tales of a scenario where snaps can be built by pulling source down from github. Is there a link you can share so I can read up on it? I have not been able to find it with the googles. | 14:54 |
| joeubuntu | i.e. I'd somehow modify my snapcraft.yaml to git clone my repo? Is that a thing or do I misunderstand the feature? | 14:58 |
| ijohnson | joeubuntu: do you mean you have a snapcraft.yaml source on github you want to be built into a snap? or you just want to include some github repo in your snap? | 14:58 |
| ijohnson | joeubuntu: we have build.snapcraft.io which will automatically re-build your snap when you push to the github repository and publish the built snap into the edge channel | 14:59 |
| joeubuntu | ijohnson that is the goal! right now I just copy my files in to ~/snap/snapName/snap/files | 15:00 |
| ijohnson | hmm I guess I didn't quite follow which thing is the goal :-) | 15:01 |
| joeubuntu | I want to use build.snapcraft.io , so thanks ! | 15:02 |
| ijohnson | joeubuntu: ah okay cool, feel free to let us know if you can't get it setup | 15:05 |
| ogra | oh, so interesting (and i have never noticed that before ...) | 15:05 |
| ogra | $ du -hcs /usr/lib/chromium-browser | 15:05 |
| ogra | 230M/usr/lib/chromium-browser | 15:05 |
| ogra | $ ls -lh /var/lib/snapd/snaps/chromium_881.snap | 15:06 |
| ogra | -rw------- 1 root root 156M Okt 11 03:00 /var/lib/snapd/snaps/chromium_881.snap | 15:06 |
| ogra | oSoMoN, why the heck dont we blog left and right about this ^^^ :) | 15:06 |
| ogra | (i just had another "snaps are too big" discussion with someone ... and actually checked the sizes for the first time ... what a surprise :) | 15:07 |
| ijohnson | ogra: maybe popey should update that blog post? | 15:08 |
| ogra | well, its a bit unfair given you have 3 copies with the snap ... so you lose the advatage in the end again ... but the fact alone that the snap itself is 80M smaller is definitely not well known among the haters (and their parrots) | 15:09 |
| joeubuntu | Sorry for all the question... now that I am using build.snapcraft.io what does my snapcraft.yaml have to look like ? prior to using b.s.io I manually copied my files in to files/bin/* ? In the repo they are just in the root. I imagine I need to just update wrapper: source: to / and change the command from bin/foo to /foo ? | 15:12 |
| ijohnson | joeubuntu: so you have some files that are just on your mahcine's filesystem that need to be in the snap? if it doesn't make sense to put those files into your git repository, then you usually would either build those files inside a part in your snapcraft.yaml or upload those files somewhere and use the dump plugin in snapcraft with a source pointing to wherever you uploaded those files | 15:19 |
| ijohnson | does that make sense? | 15:19 |
| zyga | re | 15:20 |
| joeubuntu | ijohnson thanks, I'll poke around. Thanks for the tips! | 15:20 |
| popey | ijohnson: I'm on vacation. Please email if there is something that needs my attention. Ta | 15:21 |
| * ogra hugs popey ... no action needed enjoy holiday beers ! | 15:22 | |
| ijohnson | popey: enjoy the vacation! do something fun and stop checking notifications :-) | 15:23 |
| * cachio lunch | 15:23 | |
| oSoMoN | ogra, yeah, IÂ wrote about it at some point to answer one of those "snaps are big" claim | 15:46 |
| oSoMoN | to be fair, that's quite the exception, snaps do tend to be bigger than their deb counterparts | 15:46 |
| oSoMoN | which is totally normal, given that they embed their dependencies | 15:47 |
| mvo | zyga: I addressed some of the feedback in 7629 in 7613, probably best to focus on 7613 for now and once that landed I will work on the remaining bits in the other one | 15:50 |
| zyga | mvo: cool, I'll finish the de-dupper for jdstrand and look | 15:50 |
| mvo | zyga: I still need to look at how not-to-install snpa-recover on rpms :/ but I'm running out of time for today | 15:52 |
| zyga | mvo: rm -f works :) | 15:52 |
| zyga | mvo: porting it to use snapd.mk also works | 15:52 |
| zyga | mvo: in snapd.mk there's a section for things that are removed if we are packaging for a classic distribution | 15:53 |
| zyga | mvo: thank you for looking into this :) | 15:53 |
| ogra | oSoMoN, indeed | 15:53 |
| mvo | zyga: ok, feel free to just do it and push while I'm away but I will try to get to it later tonight | 15:53 |
| * mvo waves | 15:53 | |
| zyga | mvo: I won't have time, I will finish this bug and work on a test | 15:53 |
| mup | PR snapd#7631 opened: interfaces/pulseaudio: adjust to manually connect by default <Created by jdstrand> <https://github.com/snapcore/snapd/pull/7631> | 15:54 |
| pedronis | jdstrand: what's the status of #7228 ? | 15:55 |
| mup | PR #7228: interfaces: add audio-playback/record and pulseaudio spread tests <Created by jdstrand> <https://github.com/snapcore/snapd/pull/7228> | 15:55 |
| jdstrand | pedronis: that is next up after I do PR reviews and store reviews, so, for next week | 16:01 |
| jdstrand | pedronis: but for 2.43 for sure. disco has the changes needed to actually write those tests now, so I can move forward | 16:02 |
| pedronis | jdstrand: ok, thanks for the update | 16:02 |
| jdstrand | pedronis: fyi, I talked with kenvandine and he said that the desktop team will hopefully have the sru done for bionic and xenial (see https://bugs.launchpad.net/ubuntu/+source/pulseaudio/+bug/1781428) | 16:04 |
| mup | Bug #1781428: please enable snap mediation support <patch> <pulseaudio (Ubuntu):Fix Released by jamesh> <pulseaudio (Ubuntu Xenial):Triaged by jamesh> <pulseaudio (Ubuntu Bionic):Triaged by jamesh> <https://launchpad.net/bugs/1781428> | 16:04 |
| jdstrand | pedronis: all the necessary dependencies are SRU'd, so just need the pulseaudio patch (though there may be a snapd-glib bug that needs fixing on xenial) | 16:04 |
| kenvandine | jdstrand: that reminds me to check on that :) | 16:05 |
| jdstrand | pedronis: I mention that because if it is SRUd before 2.43, I can adjust the aforementioned spread test to also include xenial and bionic rather than just disco (and now that I think of it, eoan) | 16:05 |
| jdstrand | kenvandine: thanks :) | 16:05 |
| jdstrand | popey, Wimpress: is there anything besides snapcraft-desktop-helpers and electron-builder that might templatize snapcraft.yaml/snap.yaml to include pulseaudio? | 16:09 |
| === pstolowski is now known as pstolowski|afk | ||
| zyga | jdstrand: ok, I have a fix | 16:28 |
| zyga | jdstrand: I'll work on a perf test now | 16:29 |
| mup | PR snapd#7632 opened: interfaces/apparmor: avoid excessive repetition of snap-update-ns mount rules <Created by zyga> <https://github.com/snapcore/snapd/pull/7632> | 16:34 |
| popey | jdstrand: we are both on holiday, please email or forum thread | 16:34 |
| zyga | jdstrand: https://github.com/snapcore/snapd/pull/7632 | 16:34 |
| mup | PR #7632: interfaces/apparmor: avoid excessive repetition of snap-update-ns mount rules <Created by zyga> <https://github.com/snapcore/snapd/pull/7632> | 16:34 |
| zyga | jdstrand: if you don't mind I'll EOD now but watch for your feedback | 16:36 |
| zyga | jdstrand: I can implement the spread test that checks for memory usage next week | 16:36 |
| zyga | jdstrand: I wanted to ack the main part with you | 16:36 |
| zyga | jdstrand: namely changes to apparmor.Specification.AddUpdateNS | 16:37 |
| * zyga EODs and waits for jdstrand's input passively | 16:38 | |
| jdstrand | zyga: thanks! I've got a bunch of PRs enqueued ahead of that so may not get to it today | 16:49 |
| zyga | jdstrand: boo ;-) | 16:50 |
| zyga | jdstrand: can you just ack 3 line diff then | 16:50 |
| zyga | jdstrand: (or not ack) | 16:50 |
| zyga | that's the essence | 16:50 |
| zyga | https://github.com/snapcore/snapd/pull/7632/files#diff-8d0225478e724e1ef6689c6f12feac06L184 | 16:50 |
| zyga | this one | 16:50 |
| mup | PR #7632: interfaces/apparmor: avoid excessive repetition of snap-update-ns mount rules <Bug> <Created by zyga> <https://github.com/snapcore/snapd/pull/7632> | 16:50 |
| zyga | I just realized this is just the layout part, not the content part | 16:51 |
| zyga | shoo | 16:51 |
| zyga | anyway, that's that | 16:51 |
| jdstrand | zyga: I'm going to want to really examine it and not rush it to make sure we end up with identical cache files | 16:51 |
| zyga | jdstrand: note, you don't have to approve it | 16:52 |
| zyga | jdstrand: just disapprove if the approach is too simple | 16:52 |
| zyga | jdstrand: but it's your call | 16:52 |
| zyga | jdstrand: I'll add the perf test and handle content interface next | 16:52 |
| jdstrand | zyga: it's apparmor policy generation. I requested myself for review | 16:53 |
| jdstrand | popey: sorry, thanks, done | 16:56 |
| mup | PR snapd#7616 closed: interfaces/many: allow k8s/systemd-run to mount volume subPaths plus cleanups <Created by jdstrand> <Merged by jdstrand> <https://github.com/snapcore/snapd/pull/7616> | 17:31 |
| jdstrand | kjackal: fyi ^. that should be in monday's core in edge | 17:32 |
| kjackal | woohoo awesome jdstrand | 18:07 |
| zyga | jdstrand: FYI https://github.com/snapcore/snapd/pull/7632 is updated to handle all the interfaces that used update-ns snippets | 18:11 |
| mup | PR #7632: interfaces/apparmor: avoid excessive repetition of snap-update-ns mount rules <Bug> <Created by zyga> <https://github.com/snapcore/snapd/pull/7632> | 18:11 |
| zyga | jdstrand: I _really_ EOD now :) | 18:11 |
| ijohnson | jdstrand: feel free to push #7579 to the back of your queue FWIW, the other PR's open in snapd are more important and we don't have anybody asking for that right now, just something I noticed | 18:19 |
| mup | PR #7579: interfaces/network-setup-observe: add Info D-Bus method accesses <Created by anonymouse64> <https://github.com/snapcore/snapd/pull/7579> | 18:19 |
| * zyga waves and wishes everyone a good weekend | 18:22 | |
| ijohnson | have a good weekend zyga | 18:29 |
| jdstrand | ijohnson: it is on the list, thanks | 18:30 |
| jdstrand | zyga: thanks! | 18:30 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!