| mborzecki | morning | 06:12 |
|---|---|---|
| mup | PR snapd#7703 closed: cmd/snap: make 'snap list' shorten latest/$RISK to $RISK <Channels 🏷️> <Simple 😃> <Created by chipaca> <Merged by mvo5> <https://github.com/snapcore/snapd/pull/7703> | 06:21 |
| mvo | hey mborzecki | 06:24 |
| mborzecki | mvo: hey | 06:26 |
| mup | PR snapd#7454 closed: interfaces: extend the fwupd slot to be implicit on classic <Created by jhenstridge> <Merged by mvo5> <https://github.com/snapcore/snapd/pull/7454> | 06:36 |
| zyga | good morning | 07:06 |
| mborzecki | zyga: hey | 07:06 |
| zyga | 5 days left | 07:06 |
| zyga | let's make it count! | 07:06 |
| === Girtablulu|Away is now known as Girtablulu | ||
| mup | PR snapd#7717 closed: tests/docker-smoke: add minimal docker smoke test <Simple 😃> <Created by anonymouse64> <Merged by bboozzoo> <https://github.com/snapcore/snapd/pull/7717> | 07:18 |
| mborzecki | mvo: some conflicts in #7714 | 07:19 |
| mup | PR #7714: overlord: refactor mgrsSuite and extract kernelSuite <Created by mvo5> <https://github.com/snapcore/snapd/pull/7714> | 07:19 |
| mborzecki | mvo: i can look into that, unless you've already fixed it | 07:19 |
| mvo | mborzecki: missed the ping, either way is fine | 07:23 |
| mvo | hey zyga | 07:23 |
| mborzecki | mvo: ok, i'll push it | 07:24 |
| mvo | ta! | 07:28 |
| mborzecki | mvo: #7715 is based on 7714 isn't it? | 07:30 |
| mup | PR #7715: overlord: add base->base remodel undo tests and fixes <Created by mvo5> <https://github.com/snapcore/snapd/pull/7715> | 07:30 |
| mvo | mborzecki: correct | 07:31 |
| mborzecki | mvo: ok, i'll update that one too then | 07:32 |
| mvo | ok | 07:32 |
| mvo | mborzecki: it also needs a review and some more things (that samuele pointed out) but lets do the mechanical bits first | 07:32 |
| mvo | mborzecki: hm, looks like 7665 is in some strange state, says its waiting for CI for me, mabe we ned to push master to it | 07:33 |
| mborzecki | mvo: yeah, i'll be addressing your comments and will push there | 07:34 |
| mvo | ta | 07:34 |
| zyga | brb | 07:48 |
| mvo | mborzecki: are you working on the remodel-kernel-undo-tests PR? it has no conflicts so I could just address the feedback and push and ask you to review :) ? | 07:56 |
| mborzecki | mvo: no, i just pushed a little patch to 7714 and gave it +1 | 07:58 |
| mvo | mborzecki: cool, I will just address samuele points in 7682 then and push and then you can review that too, | 07:58 |
| mborzecki | mvo: i think we can land 7714 and then update 7715, otherwise i'll get messy i think | 07:58 |
| mvo | mborzecki: its a subset so review should be trivial | 07:58 |
| mvo | mborzecki: aha, even better | 07:59 |
| mvo | mborzecki: one small issue is that 7714 includes 7682 | 07:59 |
| mvo | mborzecki: so I need to make this one ok first ./ | 08:00 |
| mvo | mborzecki: sorry, I should have done the refactor indepedantly | 08:00 |
| mborzecki | haha | 08:00 |
| zyga | re | 08:06 |
| === pstolowski|afk is now known as pstolowski | ||
| pstolowski | morning | 08:11 |
| zyga | hey pawel | 08:12 |
| mborzecki | pstolowski: hey | 08:21 |
| pstolowski | o/ | 08:38 |
| zyga | mborzecki: trying to fix the name=snapd branch now | 08:47 |
| zyga | mborzecki: made it conditional on v1/v2 mode, to use different directories | 08:48 |
| zyga | hoping /sys/fs/cgroup/snapd survives into lxd | 08:48 |
| zyga | and permissions there are sufficient | 08:48 |
| mborzecki | zyga: you likely need to merge master, paralle instances landed on friday | 08:48 |
| zyga | mborzecki: I know, I plan to soon | 08:48 |
| mborzecki | mvo: left a comment about retry-tool in place of the while loop, feels a bit awkward bc of grep | 08:49 |
| mborzecki | mvo: so i'll skip this one, and the mockSuccessfulReboot(), actually that second one is mroe interesting, it assumes snap_mode = try, but we don't switch the gadget snap during reboot, only base/kernel | 08:49 |
| zyga | brb | 08:55 |
| mvo | mborzecki: aha, right - good point | 08:57 |
| mvo | mborzecki: sorry, I missed that | 08:57 |
| mborzecki | mvo: no worries, i think we could make mockSuccessfulReboot work with some tweaking to catch unintended test cases | 08:58 |
| mvo | mborzecki: *nod* | 08:58 |
| zyga | chrome said to restart | 09:02 |
| zyga | mvo: ^ | 09:02 |
| Chipaca | mborzecki: mo'in. Does your +1 to #7669 apply to #7670? :) | 09:55 |
| mup | PR #7669: snap, cmd/snap: support (but warn) deprecated multi-slash channel <Channels 🏷️> <Needs Samuele review> <Created by chipaca> <Closed by chipaca> <https://github.com/snapcore/snapd/pull/7669> | 09:55 |
| mup | PR #7670: cmd/snap: support (but warn) using deprecated multi-slash channel <Channels 🏷️> <Needs Samuele review> <Created by chipaca> <https://github.com/snapcore/snapd/pull/7670> | 09:55 |
| mborzecki | Chipaca: let me take a look | 09:56 |
| mborzecki | Chipaca: idk, the suffering strings are gone ;) | 09:58 |
| Chipaca | mborzecki: ikr | 09:58 |
| Chipaca | mborzecki: OTOH valid value + error is a weird flex | 09:58 |
| Chipaca | mborzecki: how did you make a comment on an error message that is no longer there? | 10:01 |
| mborzecki | haha | 10:02 |
| mborzecki | yeah | 10:02 |
| mborzecki | Chipaca: btw. do we use the british or us spelling in the messages? | 10:04 |
| Chipaca | mborzecki: us :-/ | 10:11 |
| zyga | another test pass, | 10:14 |
| zyga | test passed | 10:19 |
| zyga | that's very promising | 10:19 |
| zyga | running more challenging test | 10:19 |
| Chipaca | mvo: #7714 has two +1s but it includes #7682 which has some nits pointed out by pedronis | 10:40 |
| Chipaca | mvo: up to you whether to merge it and then followup or what | 10:40 |
| mup | PR #7714: overlord: refactor mgrsSuite and extract kernelSuite <Created by mvo5> <https://github.com/snapcore/snapd/pull/7714> | 10:40 |
| mup | PR #7682: overlord: add kernel remodel undo tests and fix undo <Priority :racehorse:> <Created by mvo5> <https://github.com/snapcore/snapd/pull/7682> | 10:40 |
| Chipaca | grr | 10:40 |
| Chipaca | #7682 | 10:41 |
| mup | PR #7682: overlord: add kernel remodel undo tests and fix undo <Priority 🏇> <Created by mvo5> <https://github.com/snapcore/snapd/pull/7682> | 10:41 |
| Chipaca | better :) | 10:41 |
| mvo | Chipaca: I'm just finishing the unit test asked for in 7682, should be ready in some minutes | 10:53 |
| zyga | making progress, next up is the lxd test | 10:54 |
| mvo | Chipaca, mborzecki 7682 should be ready now, I addressed the comments from samuele | 11:11 |
| zyga | updated cgroup dir based on v1 / v2 mode in snapd side, resuming tests | 11:14 |
| mvo | Chipaca, mborzecki actually, yeah, probably/possible easier if I merge 7714 and do my other bits as a followup, up to you if you feel this is easier to review | 11:15 |
| zyga | mborzecki: the changes are not too terrible, not great | 11:16 |
| Chipaca | mvo: sgtm | 11:16 |
| zyga | mborzecki: we should do runc-like change later on | 11:16 |
| mborzecki | mvo: landing 7714 sgtm | 11:16 |
| mup | PR snapd#7714 closed: overlord: refactor mgrsSuite and extract kernelSuite <Created by mvo5> <Merged by mvo5> <https://github.com/snapcore/snapd/pull/7714> | 11:16 |
| mvo | mborzecki: 7665 now has conflicts, let me know if I should help. and a strange spread failure | 11:19 |
| mvo | Chipaca, mborzecki 7682 should be much simpler to review now :) | 11:20 |
| Chipaca | \o/ | 11:20 |
| zyga | tests passed | 11:20 |
| zyga | good | 11:20 |
| zyga | moving on | 11:20 |
| mborzecki | mvo: that failure in 7665 looks like something raised in the forums during the weekend | 11:20 |
| zyga | running the lxd test now | 11:20 |
| mborzecki | mvo: https://forum.snapcraft.io/t/snap-remove-fails-because-of-user-data/13990 | 11:21 |
| Chipaca | mborzecki: saw that one, still puzzled by it | 11:21 |
| * Chipaca replies with as much | 11:22 | |
| mvo | mborzecki: ok, feel free to restart the test if its unrelated | 11:28 |
| pstolowski | zyga: i've responded to #7675 | 11:30 |
| mup | PR #7675: release: preseed mode flag <Prebaking 🍞> <Simple 😃> <Created by stolowski> <https://github.com/snapcore/snapd/pull/7675> | 11:30 |
| zyga | pstolowski: thank you, looking now | 11:30 |
| zyga | pstolowski: replied | 11:32 |
| zyga | mvo: lxd test has passed with the new feature enabled *outside* | 11:32 |
| pstolowski | zyga: thanks | 11:33 |
| zyga | mvo: this is promising, looking at making the inside case now | 11:33 |
| zyga | mvo: the other case is just test adjustment which will later on become a change to lxd | 11:34 |
| zyga | mvo: this is really promising, I think it will work | 11:35 |
| mvo | zyga: cool | 11:35 |
| mborzecki | Chipaca: hmm regarding snap download, there's some code in image/helpers.go that just calls os.Exit(1) on sigint ;) | 11:46 |
| Chipaca | mborzecki: does it also Finalize the progress bar? | 11:46 |
| Chipaca | (also, yes, exit 1 would probably be the right thing to do) | 11:47 |
| Chipaca | (but after finalizing the progress bar) | 11:47 |
| mborzecki | it's Finished() followed by os.Exit() | 11:47 |
| Chipaca | perfect :) | 11:47 |
| Chipaca | Finished, Finalize, Fwhatever | 11:47 |
| mborzecki | hm also, snap download --remote leaves a *.partial file behind, but one that goes via snapd does not? | 11:47 |
| Chipaca | make-the-terminal-usable-again | 11:47 |
| Chipaca | mborzecki: --direct doesn't leave a .partial when interrupted? | 11:48 |
| Chipaca | that'd be a(nother) regression | 11:48 |
| mborzecki | Chipaca: --direct does leave *.partial, the non-direct one does not | 11:49 |
| * mborzecki tries to parse what he just wrote | 11:49 | |
| Chipaca | mborzecki: ah, that's a known regression | 11:50 |
| Chipaca | mborzecki: non-direct does not know how to resume | 11:50 |
| Chipaca | and indeed it'll take a refactor of the download endpoint to do it | 11:50 |
| mvo | Chipaca: yeah, I think we need to talk what to do - I think I will give your idea of "snap download --indirect" a shoot and then we can iterate on this without regressions | 11:57 |
| mvo | mborzecki: thanks for looking at this so carefully! | 11:57 |
| Chipaca | mvo: my biggest concern is that i don't see how to get resume without a rest api refactor | 11:58 |
| Chipaca | actually thinking a bit more, it's a minor refactor, a change to which things are obligatory i guess | 12:00 |
| Chipaca | and a change to how it's used for sure | 12:00 |
| Chipaca | that's my brain coming back to me with this that i'd been mulling over the weekend | 12:00 |
| Chipaca | mvo: for snap download, what you want to do is first do an exact search by name, and take the info you get back to (1) check whether you have that file already, (2) check if you have a partial, (3) download exactly that revision, maybe with a resume in it | 12:01 |
| Chipaca | so the refactor for download is (a) always take the revision, and (b) optioanlly take a resume position | 12:02 |
| Chipaca | that's at the api level; internally it's a bit bigger of a change because snap download itself shouldn't be doing the two requests, i guess? in that maybe there's a way to not have to hit info again server-side | 12:02 |
| Chipaca | in the fast case at least | 12:02 |
| Chipaca | but that can be a later optimization | 12:03 |
| Chipaca | or | 12:03 |
| Chipaca | OR! | 12:03 |
| Chipaca | we could make snap download take the snap id, and then reconstructing the download irl is easier? | 12:04 |
| zyga | mvo: I'm working on the denial, no idea what is it yet | 12:09 |
| zyga | mvo: I've simplified the test and will iterate interactively on the next shell | 12:11 |
| mborzecki | Chipaca: for starters we could just write the data to *.partial file and rename, seeing *.snap that isn't really complete is bit confusing | 12:12 |
| mborzecki | cachio: did you have a patched version of spread that can run tests in a specific order? | 12:13 |
| cachio | mborzecki, yes | 12:13 |
| mborzecki | can you point me to it? | 12:14 |
| zyga | currently stuck on one denial: | 12:14 |
| zyga | cannot remount /sys/fs/cgroup read-write: Permission denied | 12:14 |
| zyga | Nov 04 12:13:45 nov041205-146458 audit[28564]: AVC apparmor="DENIED" operation="mount" info="failed flags match" error=-13 profile="lxd-my-ubuntu_</var/snap/lxd/common/lxd>" name="/sys/fs/cgroup/" pid=28564 comm="snap-confine" flags="rw, remount" | 12:14 |
| cachio | mborzecki, sure | 12:14 |
| zyga | I changed the apparmor profile of lxd to contain | 12:15 |
| zyga | mount options=(remount, rw) /sys/fs/cgroup/, | 12:15 |
| cachio | mborzecki, https://cachio.s3.amazonaws.com/spread/spread2 | 12:15 |
| zyga | but perhaps I misunderstand, need to check what those paths map to in practice | 12:15 |
| mborzecki | zyga: isn't that a profile that lxd created for the container? | 12:15 |
| zyga | mborzecki: yes | 12:15 |
| mborzecki | cachio: thanks! | 12:15 |
| zyga | mborzecki: the denial is for the profile from lxd | 12:15 |
| cachio | mborzecki, np | 12:15 |
| zyga | mborzecki: that is what I am changing | 12:15 |
| zyga | mborzecki: inside the test I patch and reload that profile | 12:16 |
| zyga | mborzecki: on the container host | 12:16 |
| mborzecki | ah ok | 12:16 |
| cachio | just use: -order -workers 1 | 12:16 |
| cachio | mborzecki, this will use 1 instance for all the tests | 12:16 |
| cachio | and run the tests in the order you add them in the command line | 12:17 |
| mvo | Chipaca: thank you, yeah, in some sense its an optimization | 12:18 |
| zyga | mborzecki: interesting, I can switch all of the apparmor for lxd into complain mode | 12:18 |
| mborzecki | cachio: ok, running it now | 12:18 |
| zyga | and it doesn't help | 12:18 |
| zyga | I probably don't understand what is really going on | 12:19 |
| mvo | mborzecki: 7665 has now conflicts :/ | 12:21 |
| mborzecki | mvo: mhm, first trying to catch the problem with removal of /var/snap/ | 12:21 |
| mvo | mborzecki: no worries, I can fix the conflicts | 12:24 |
| * pstolowski lunch | 12:26 | |
| mborzecki | mvo: thanks for merging master to the remodel branch! | 12:29 |
| mvo | mborzecki: no worries, I want to see it merged :) | 12:31 |
| Chipaca | mvo: is there an IRL bug that triggered #7682? | 12:36 |
| mup | PR #7682: overlord: add kernel remodel undo tests and fix undo <Priority 🏇> <Created by mvo5> <https://github.com/snapcore/snapd/pull/7682> | 12:36 |
| cachio | zyga, hey | 12:36 |
| mvo | Chipaca: no real life bug | 12:36 |
| cachio | did you see something like this: https://paste.ubuntu.com/p/6NYyBbjtMW/ | 12:37 |
| Chipaca | mvo: phew | 12:37 |
| mvo | Chipaca: it was just the result of missing undo testing in the handlers code | 12:37 |
| mvo | Chipaca: yeah | 12:37 |
| cachio | it is failing on ubunto-core-18 tests on pi3 for edge cahnnel | 12:37 |
| * mvo gets some lunch | 12:37 | |
| zyga | making some more progress | 12:38 |
| zyga | hey cachio | 12:38 |
| zyga | I realized lxd start / stop rewrites the container profile | 12:38 |
| zyga | so my changes are not effective | 12:38 |
| zyga | I started the container | 12:38 |
| zyga | and use apparmor parser to switch it to complain mode after taht | 12:39 |
| zyga | I used aa-status to check what the outcome is after taht | 12:39 |
| zyga | and see that some of the chagnes are applied, there are a number of processes with complain profiles | 12:39 |
| zyga | but I can see that the bulk of the container is confined | 12:40 |
| zyga | I'm exploring why | 12:40 |
| zyga | the output of aa-status is | 12:40 |
| zyga | https://www.irccloud.com/pastebin/3m0f3aYx/ | 12:40 |
| zyga | in case someone figures out why | 12:40 |
| cachio | zyga, all this is related to the mount namespaces error? | 12:46 |
| zyga | cachio: no, this is ongoing work on new cgroup | 12:47 |
| cachio | ahh, ok | 12:48 |
| zyga | I managed to switch snap-confine profile into complain mode | 12:54 |
| zyga | it seems there's no propagation of any kind | 12:55 |
| zyga | so changing the profile on the host doesn't impact profiles derived from it in any way | 12:55 |
| zyga | but that's fine | 12:55 |
| zyga | so | 12:55 |
| zyga | progress: | 12:55 |
| zyga | snap-confine inside is in complain mode but I still get a denial | 12:55 |
| zyga | looking at why now :) | 12:55 |
| zyga | perhaps I just put the wrong one in complain mode | 12:55 |
| zyga | it's not re-execing | 12:55 |
| zyga | checking | 12:55 |
| zyga | more profiles in complain mode https://www.irccloud.com/pastebin/cwRZuduL/ | 12:56 |
| zyga | mvo: in the meantime I'm running all tests to see if anything is impacted , 50% there, I'll know soon | 12:58 |
| mborzecki | google:ubuntu-core-18-64 .../tests/main/remodel-gadget# ls /var/snap/pc | 12:58 |
| mborzecki | 1001 1002 | 12:58 |
| mborzecki | heh, wtf?? | 12:58 |
| * diddledan sees zyga wants complaints so moans that it's not bed time yet | 12:59 | |
| zyga | _weird_ | 13:06 |
| zyga | improvement | 13:06 |
| zyga | I got ALLOWED messages | 13:06 |
| zyga | so it did switch to complain mode | 13:06 |
| zyga | and the things mentioned were really about what is new in the code | 13:06 |
| zyga | but I got the error still | 13:07 |
| zyga | but even though i still got an error from ount | 13:07 |
| zyga | checking why | 13:07 |
| zyga | the error in case anyone sees error in my logic https://www.irccloud.com/pastebin/9JmCN4RA/ | 13:08 |
| * Chipaca wanders off mumbling something about lunch | 13:08 | |
| zyga | Chipaca: good idea | 13:08 |
| zyga | I'll update the test to do what I did manually | 13:08 |
| zyga | and wrap up for lunch | 13:08 |
| zyga | test modified, restarted clean | 13:11 |
| zyga | I think it is better | 13:11 |
| zyga | I'll double check the code, maybe there is something silly in my patch | 13:11 |
| zyga | mvo: we are all doomed ;-) | 13:13 |
| zyga | https://m.tagesspiegel.de/berlin/experten-warnten-schon-2017-it-katastrophe-am-berliner-kammergericht-kam-mit-ansage/25163810.html | 13:13 |
| zyga | I want more of https://mspoweruser.com/microsoft-4-day-workweek/ though ;) | 13:14 |
| zyga | but first, let's ship it for vancouver | 13:15 |
| zyga | mvo: regular tests are move than 90% complete now | 13:15 |
| zyga | mvo: if anything exploded I'll know soon | 13:15 |
| mup | PR snapd#7718 opened: sandbox/seccomp: accept build ID generated by Go toolchain <Simple 😃> <Created by bboozzoo> <https://github.com/snapcore/snapd/pull/7718> | 13:26 |
| mborzecki | mvo: zyga: it'd be nice to get it for 2.43 | 13:26 |
| mborzecki | also pinged jdstrand for a review though the bit is trivial | 13:26 |
| cachio | mborzecki, hey, this error is hapenning when updating the arch image https://travis-ci.org/snapcore/spread-cron/builds/607000141#L2056 | 13:34 |
| cachio | any idea ? | 13:35 |
| mborzecki | cachio: yeah, looks like the package needs an update | 13:37 |
| mborzecki | let me see if there's anything i can do about it | 13:37 |
| * zyga is afk for lunch now | 13:39 | |
| zyga | only lxd and mount-ns tests failed, so good and expected | 13:39 |
| cachio | mborzecki, nice thanks | 13:40 |
| mborzecki | cachio: pinged the guy who maintains the package with a patch that should fix it, let's see if it lands today/tomorrow, if not i can fork the package, put it on github and we can pull it from that location | 13:43 |
| cachio | mborzecki, nice thanks!! | 13:45 |
| zyga | hmmm | 13:53 |
| zyga | stgraber: hello, can you please help me how lxd uses apparmor profiles | 13:53 |
| zyga | stgraber: I'm trying to understand how to switch the entire container into complain mode | 13:54 |
| zyga | stgraber: I tried running apparmor_parser -r --complain /var/snap/lxd/common/lxd/security/... | 13:54 |
| zyga | stgraber: using aa-status on the host I can see some changes but I still see a number of confined profiles | 13:54 |
| zyga | er, enforcing profiles | 13:54 |
| zyga | e.g. the output of aa-status from the container host is | 13:55 |
| zyga | https://www.irccloud.com/pastebin/08Gl52Z9/ | 13:55 |
| zyga | I would appreciate if you can shed some light on this | 13:55 |
| stgraber | LXD regenerates and reload the container profile on container startup, files are mostly there to inspect but changes won't persist | 13:55 |
| zyga | stgraber: right, but is there more than one file to change? | 14:07 |
| zyga | stgraber: I'm getting things I don't quite understand, after a lot of hand-called apparmor_parser --complian --reload I get some ALLOW but still some EPERM and nothing logged with DENIED | 14:07 |
| zyga | stgraber: is it safe to change any profiles after the container is started? | 14:08 |
| zyga | stgraber: note that I suspect that "deny" rules silence logging | 14:09 |
| zyga | stgraber: so perhaps what I'm hitting is a deny somewhere | 14:09 |
| zyga | stgraber: alternatively | 14:10 |
| zyga | stgraber: is there a knob on lxd that would allow me to run a container with complain more apparmor? | 14:10 |
| zyga | 15 minute break | 14:23 |
| zyga | stgraber: I'm all ears if you have some ideas | 14:23 |
| zyga | stgraber: but I'll be sending patches to lxd to help with this case, it's a one-liner *I believe* for now | 14:23 |
| stgraber | zyga: what' | 14:25 |
| stgraber | zyga: what's the one liner? | 14:25 |
| zyga | stgraber: to remount /sys/fs/cgroup rw | 14:25 |
| zyga | all I need is | 14:25 |
| zyga | mount -o remount,rw /sys/fs/cgroup | 14:25 |
| zyga | mkdir /sys/fs/cgroup/snapd | 14:25 |
| zyga | mount -o remount,ro /sys/fs/cgroup | 14:25 |
| zyga | mount -t cgroup cgroup /sys/fs/cgroup/snapd -o none,name=snapd | 14:26 |
| zyga | that's all | 14:26 |
| zyga | I believe all but the rw remount are permitted | 14:26 |
| stgraber | zyga: I'm reasonably sure that we cannot allow that without causing a big security issue | 14:26 |
| zyga | hmmm | 14:26 |
| zyga | because of remount or because of something else | 14:26 |
| zyga | please tell me what you think, it's important in our design | 14:27 |
| stgraber | because of the apparmor rule generator being broken | 14:27 |
| zyga | stgraber: is there anything we can do? | 14:28 |
| stgraber | if rw,remount hits the parser bug, no | 14:28 |
| zyga | aha | 14:28 |
| stgraber | adding such a rule would allow every single mounts to succeed | 14:28 |
| stgraber | zyga: why are you trying to mount your own v1 hierarchy anyway? | 14:29 |
| zyga | stgraber: can we do something else to avoid the need to mkdir, can we mkdir early in systemd somewhere? | 14:29 |
| zyga | stgraber: for tracking processes correctly, it's the only way we found that works well in v1 and v2 mode | 14:30 |
| stgraber | zyga: how do you do it in v2 where /sys/fs/cgroup is a cgroup2 mount? | 14:30 |
| zyga | stgraber: in /run/snapd/cgroup | 14:30 |
| stgraber | ah so you're assuming that a cgroup2 system will still have cgroup1 enabled in the kernel? | 14:30 |
| zyga | stgraber: I wanted it to be in /run/snapd/cgroup now but this is more complex to do so that it works in lxd correctly | 14:31 |
| zyga | stgraber: yes, for now yes | 14:31 |
| zyga | stgraber: we need to work on extending cgroup2 usage in systemd | 14:31 |
| stgraber | zyga: why would /run/snapd/cgroup be harder in LXD? | 14:31 |
| stgraber | zyga: that should actually be easier. Obviously you'll have the problem that this will fail on any system which doesn't have LXD on the host | 14:31 |
| zyga | stgraber: because /run is changed by lxd and we need to change it so that it is not necessary to have that mount inside the per-snap mount namespace | 14:32 |
| mup | PR snapd#7670 closed: cmd/snap: support (but warn) using deprecated multi-slash channel <Channels 🏷️> <Needs Samuele review> <Created by chipaca> <Merged by mvo5> <https://github.com/snapcore/snapd/pull/7670> | 14:32 |
| stgraber | zyga: so you're going to break a LOT of systems, but that's going to happen regardless so long as you want your own named controller | 14:32 |
| zyga | stgraber: can you explain how we will break things? | 14:32 |
| stgraber | zyga: unprivileged users are only allowed to mount existing controllers | 14:32 |
| stgraber | zyga: if your host doesn't have a name=snapd controller, no container will be allowed to mount it | 14:32 |
| zyga | aha | 14:32 |
| zyga | the host will have it | 14:32 |
| stgraber | no it won't | 14:33 |
| zyga | perhaps I misunderstand | 14:33 |
| stgraber | not for anyone who's not using the snap | 14:33 |
| zyga | and are there people who are not using lxd as a snap that will run into this? | 14:33 |
| stgraber | so you'll break all chromebooks, all gentoo users, all alpine users, most arch users, ... | 14:33 |
| zyga | I see | 14:33 |
| zyga | that's bad news indeed | 14:33 |
| zyga | I wonder if we have some ideas as a way out then | 14:33 |
| zyga | stgraber: can you tell me more about the rule | 14:34 |
| stgraber | we actually ran into that very same problem when systemd introduced name=systemd in the first place | 14:34 |
| zyga | stgraber: about what makes the cgroup mount fail if it doesn't exist on the host | 14:34 |
| stgraber | any distro which wasn't using systemd was unable to run systemd containers because of it | 14:34 |
| zyga | stgraber: note that the feature is still not on by default, or merged for that matter | 14:34 |
| mborzecki | mvo: got a fix for the test failure that occurred in #7665, i'll propse it separately unless the travis job that's currently running fails | 14:34 |
| zyga | stgraber: we just need _a_ way to track processes reliably | 14:34 |
| mup | PR #7665: devicestate: add support for gadget->gadget remodel <Priority 🏇> <Remodel 🚋> <Created by bboozzoo> <https://github.com/snapcore/snapd/pull/7665> | 14:34 |
| zyga | stgraber: and we have found none better than this | 14:34 |
| stgraber | zyga: the kernel effectively looks at all existing controllers (same list you'd see in /proc/self/cgroups) and anything that's not in that list, you can't mount unless you're real root outside of a cgroup namespace | 14:35 |
| zyga | I see | 14:35 |
| zyga | and it treats the name=... thing as a controller in this sense? | 14:35 |
| zyga | note that our hierarchy has no controllers at all | 14:36 |
| zyga | but I'm sure you know that, just checking if I understand right | 14:36 |
| stgraber | zyga: given that your current plan will not work on any distro which has fully transitioned to cgroup2 (no more cgroup1 in kernel) and will break for a lot of container users, it doesn't seem to me like a particularly good plan :) | 14:36 |
| stgraber | zyga: yes | 14:36 |
| zyga | stgraber: there are no better plans yet | 14:36 |
| stgraber | zyga: you wouldn't want an unprivileged container being able to mount thousands of those, allocating global kernel structs | 14:36 |
| zyga | stgraber: pure v2 doesn't let us track processes because we'll interfere with systemd | 14:36 |
| stgraber | zyga: can't you just keep snap-confine as the parent of the processes in the snap and make it a subreaper? | 14:37 |
| mborzecki | zyga: stgraber: tbh we don't seem to have a reliable, race free way of tracking processes in v2 world without complicating things | 14:38 |
| zyga | stgraber: if we do that, we know if a process dies | 14:38 |
| zyga | stgraber: but not if there are any left | 14:38 |
| stgraber | zyga: but you know that when you die all subprocesses will too | 14:38 |
| zyga | stgraber: we'd need to match fork and exit reliably but that's not what subreaper can do | 14:38 |
| zyga | stgraber: I'm not sure I follow, are you saying that we can then kill snap-confine tracking process to kill all the children? | 14:39 |
| stgraber | zyga: yes | 14:39 |
| zyga | stgraber: that's not what we are trying to do | 14:39 |
| zyga | stgraber: the goal is to know when it's safe to refresh | 14:40 |
| zyga | stgraber: when no apps are running | 14:40 |
| zyga | stgraber: in a zygote-like mode when snap-confine sticks around and watches processes die | 14:40 |
| zyga | stgraber: we could perhaps know something useful but I don't believe we can do what we want to with just that | 14:41 |
| zyga | mborzecki, mvo: I think we need to abort and come up with another idea | 14:43 |
| zyga | based on what stgraber said above I don't believe this is something we can salvage unfortunately | 14:43 |
| zyga | this still leaves us with no tracking, let alone notification past v1 | 14:44 |
| zyga | stgraber: how did you fix the issue affecting systemd needing to mount name=systemd | 14:45 |
| stgraber | zyga: most distros switched to systemd, the rest are mounting the systemd controller on the host anyway | 14:46 |
| zyga | stgraber: I see | 14:46 |
| mvo | mborzecki: thank you | 14:47 |
| zyga | mvo, mborzecki: can we meet later today | 14:48 |
| stgraber | zyga: so sadly there's no nice way that I know for you to get notified when a namespace goes away, otherwise you could use something like a uts namespace | 14:48 |
| zyga | I have an idea | 14:48 |
| zyga | stgraber: I planned to use release agent on the name=snapd hierarchy | 14:49 |
| zyga | but thinking about it now | 14:49 |
| zyga | how to work without any cgroups | 14:49 |
| stgraber | zyga: have one namespace per snap, dump all new processes in there, then when you want to know if the snap is safe to refresh, check if any process uses it | 14:49 |
| zyga | stgraber: one uts namespace? | 14:49 |
| zyga | stgraber: and in v2 mode? | 14:49 |
| zyga | stgraber: in v1 we can use the freezer for that | 14:49 |
| zyga | stgraber: (note that we need to have per-app understanding in our current model but perhaps we could drop that) | 14:50 |
| zyga | stgraber: but what the name=snapd idea was about is something that works both in v1 and v2 mode | 14:50 |
| stgraber | zyga: doesn't matter v1/v2, this wouldn't use cgroups at all | 14:50 |
| zyga | ah | 14:50 |
| zyga | sorry | 14:50 |
| zyga | I didn't understand | 14:50 |
| zyga | uts namespace | 14:50 |
| zyga | so iterate over process | 14:50 |
| zyga | and check which uts namespace is used by each? | 14:51 |
| zyga | and check if any of those match that of our snap processes? | 14:51 |
| stgraber | yeah, that's the part that's not amazing, having to iterate but otherwise should work fine | 14:51 |
| zyga | yes | 14:51 |
| zyga | that might w | 14:51 |
| zyga | that might work | 14:51 |
| zyga | I was thinking to use the existing mount namespace | 14:51 |
| zyga | given that we already create one for strict snaps | 14:51 |
| zyga | and are about to for classic snaps | 14:51 |
| stgraber | yeah but your mntns are per snap + per user or something so that wouldn't work so well | 14:52 |
| mup | PR snapd#7682 closed: overlord: add kernel remodel undo tests and fix undo <Priority 🏇> <Created by mvo5> <Merged by mvo5> <https://github.com/snapcore/snapd/pull/7682> | 14:52 |
| zyga | stgraber: yes | 14:52 |
| zyga | stgraber: so you are saying we could create and save one for each pkg.app pair | 14:52 |
| stgraber | the tracking with a uts namespace would still be bypassable by any snap allowed to do fancy clone/unshare, but that's unlikely to really matter, you're not building security on this, if a snap wants to make itself break on refresh, whatever :) | 14:53 |
| zyga | stgraber: correct but the kind of snaps that would be hurt by this (think things like docker) really would as they would get unexpected refresh semantics | 14:54 |
| zyga | stgraber: but thank you for the idea, I think this is workable | 14:54 |
| stgraber | zyga: yeah, on initial startup, you'd unshare the uts namespace, bind-mount its inode somewhere in /run/snap as you do for mntns, then any new process gets added using setns and to see if it's safe, you iterate through /proc for anything that has a matching inode on ns/uts | 14:54 |
| zyga | stgraber: correct | 14:55 |
| zyga | stgraber: I agree this would work | 14:55 |
| stgraber | you could even make actual use of the uts namespace and give each snap a unique hostname | 14:55 |
| zyga | stgraber: I think that will break snaps | 14:55 |
| zyga | stgraber: especially it would be unexpected for snap with classic confinement | 14:55 |
| stgraber | indeed :) | 14:56 |
| zyga | stgraber: or maybe the time namespace | 14:56 |
| zyga | ;-) | 14:56 |
| stgraber | one thing to keep in mind is that if the hostname changes on the host, it won't in the snap | 14:56 |
| zyga | stgraber: yeah, I think it's a good route but we need to plan which namespace to use | 14:56 |
| zyga | uts doesn't feel like it | 14:56 |
| stgraber | you certainly don't want to use ipc, mnt, net, pid or user | 14:56 |
| zyga | I would ideally do it via mount namespace and just save those with finer granularity | 14:56 |
| stgraber | so you have two options really | 14:56 |
| mborzecki | feels like it's the same borderline use as the pids controller right now | 14:57 |
| zyga | I think mount is indeed unworkable on second thought | 14:57 |
| stgraber | mount is the most used namespace, a lot of software mess with it and snapd already has more than one mntns per snap, so seems pretty hard to track | 14:57 |
| stgraber | cgroup and uts are the two easy ones, cgroup may actually be a better option | 14:58 |
| stgraber | hmm, though it may seem weird | 14:58 |
| stgraber | yeah, no, don't use cgroup, you'll break lxd :) | 14:58 |
| stgraber | uts won't break us though, the only issue with it is the hostname changes not getting propagated until a snap restart, but I don't think anyone will really notice/care | 14:59 |
| stgraber | zyga: we could probably do something with pidfds though, but that's likely to be an issue due to lack of kernel support right now and it still being in very active development (by us) | 15:00 |
| zyga | stgraber: how about netlink | 15:00 |
| zyga | and observing all process activity | 15:00 |
| * cachio lunch | 15:00 | |
| zyga | so match fork/exec/exit | 15:00 |
| stgraber | zyga: I didn't think we had a reliable API for that in netlink, short of having to ptrace stuff | 15:01 |
| zyga | stgraber: I think there's something, not ptrace, there's a program that shows that (fork exec across all system) using netlink | 15:01 |
| stgraber | zyga: also, that'll break when snapd restarts as you'll miss all those events | 15:01 |
| zyga | but yeah, I don't know how that works yet | 15:01 |
| zyga | stgraber: yes, it'd have to be a special process that's never quitting | 15:01 |
| stgraber | and if it's relying on tracing, then it won't work in containers | 15:01 |
| zyga | zygote like per snap | 15:01 |
| zyga | AFAIK it is pure netlink | 15:02 |
| mborzecki | stgraber: alternative we considered before was putting the process in a well-known cgroup name eg. snap.app under the cgroup created for it by systemd (just one level deeper), but that'd involve walking /proc/*/cgroup | 15:02 |
| mborzecki | and walking /proc/*/cgroup feels like a bad idea | 15:02 |
| zyga | in v2 everything is somewhere in a hierarchy somewhere | 15:03 |
| stgraber | zyga: forkstat/netlink connector requires real root | 15:04 |
| zyga | stgraber: I see | 15:04 |
| zyga | stgraber: thank you for checking | 15:04 |
| stgraber | zyga: it's also apparently lossy, under load there's no delivery guarantee | 15:05 |
| zyga | stgraber: thank you, this is even more relevant | 15:06 |
| zyga | stgraber: anything we could use to tag a process with | 15:07 |
| zyga | stgraber: I don't mind enumeration | 15:07 |
| zyga | it won't be done all the time | 15:07 |
| zyga | just in special moments | 15:07 |
| zyga | I just want to see if we can identify a process as belonging to a snap | 15:07 |
| zyga | even weird things like containers | 15:08 |
| zyga | stgraber: thank you for all the input, it was invaluable, | 15:08 |
| zyga | I think we need to rethink the idea | 15:08 |
| stgraber | zyga: there have been discussions about ptags in the past but nothing ever got merged | 15:09 |
| mup | PR snapd#7719 opened: tests/main/gadget-update-pc: cleanup per-revision directories <Simple 😃> <Created by bboozzoo> <https://github.com/snapcore/snapd/pull/7719> | 15:09 |
| mborzecki | mvo: this addresses the failures due to /var/snap/pc/<rev> directories being left behind in the tests | 15:10 |
| mborzecki | mvo: ^^ | 15:10 |
| stgraber | zyga: effectively arbitrary process tags which would be inherited and could even be marked immutable in some cases, it was one of the ideas put forth to solve the audit id issue for containers but after years of bikeshedding, nothing really happened | 15:10 |
| mvo | mborzecki: nice, *thank you* | 15:11 |
| zyga | stgraber: utterly crazy idea, set timerclask_ns to a unique value, use that | 15:13 |
| zyga | timerslack_ns | 15:13 |
| diddledan | I prefer timertelegram_ns :-p | 15:14 |
| diddledan | slack is second place :-D | 15:14 |
| mvo | mborzecki: but in a meeting right now so will look in a bit | 15:16 |
| mup | PR snapd#7719 closed: tests/main/gadget-update-pc: cleanup per-revision directories <Simple 😃> <Created by bboozzoo> <Closed by bboozzoo> <https://github.com/snapcore/snapd/pull/7719> | 15:36 |
| zyga | mvo: do you have time today? | 15:48 |
| zyga | mvo: for a 10 minute call? | 15:48 |
| zyga | mborzecki: do you have any technical ideas? | 15:49 |
| zyga | mborzecki: or can we exchange ideas before your EOD | 15:49 |
| zyga | mborzecki: it's fine to say no, perhaps at this stage having some rest would help | 15:49 |
| zyga | mborzecki: I don't see a way to solve this issue now | 15:49 |
| mborzecki | zyga: tomorrow morning, i'm taking kids to some extra classes in 10 | 15:50 |
| zyga | ok | 15:50 |
| zyga | thank you | 15:50 |
| xnox | stgraber: can snapd run inside lxd, on travis, on arm64? https://travis-ci.org/snapcore/core20/jobs/600808794?utm_medium=notification&utm_source=github_status | 15:59 |
| xnox | stoopkid: i see that core snap fails to setup security profiles? | 15:59 |
| xnox | stoopkid: unping | 15:59 |
| xnox | stoopkid: i see that core snap fails to setup security profiles, and i'm trying to get arm64 ci going with travis. | 15:59 |
| zyga | it seems udevadm failed | 16:00 |
| zyga | exit code 2 | 16:00 |
| xnox | i'll restart the build to see if things got any better | 16:01 |
| mup | PR snapd#7680 closed: daemon: parse and reject invalid channels in snap ops <Channels 🏷️> <Simple 😃> <Created by chipaca> <Merged by chipaca> <https://github.com/snapcore/snapd/pull/7680> | 16:06 |
| stgraber | xnox: just try to run the snap install twice | 16:07 |
| stgraber | xnox: that's usually enough for those | 16:07 |
| Chipaca | #7671 needs two (2!) reviews, and it's the last chunk of the channels preparatory work | 16:07 |
| mup | PR #7671: overlord/patch: normalize tracking channel in state <Channels 🏷️> <Needs Samuele review> <Created by chipaca> <https://github.com/snapcore/snapd/pull/7671> | 16:07 |
| zyga | Chipaca: is it okay if I call it quits now, sorry, if it was one review I'd jump in | 16:08 |
| zyga | but I need to break, think and move a little | 16:08 |
| Chipaca | zyga: you can call it quits any time you need to | 16:08 |
| Chipaca | zyga: also: stepping away from the keyboard to think is not quitting | 16:08 |
| zyga | and all my work is essentially down the drain today | 16:08 |
| zyga | so need to think if anything can save it | 16:08 |
| * Chipaca hugs zyga | 16:08 | |
| * diddledan hugs too | 16:08 | |
| * zyga hugs Chipaca | 16:08 | |
| zyga | thank you guys! | 16:08 |
| * zyga hugs diddledan too :) | 16:09 | |
| zyga | anything, as crazy as possible, that technically works, is good | 16:09 |
| zyga | I'll grab my bike and go for a ride | 16:09 |
| mvo | zyga: was in meetings, sorry | 16:14 |
| oSoMoN | jdstrand, when do you plan on making a release of review-tools that includes https://git.launchpad.net/review-tools/commit/?id=31c894178a8fd110b9f65f0ee532b7e11384f8b8 ? | 16:33 |
| jdstrand | oSoMoN: let me prepare a release now | 16:41 |
| oSoMoN | perfect, thanks! | 16:41 |
| jdstrand | roadmr: hi! would you mind pulling 20191104-1644UTC? | 16:46 |
| roadmr | jdstrand: sure | 16:47 |
| jdstrand | roadmr: thanks :) | 16:49 |
| jdstrand | oSoMoN: fyi ^ (will be a bit before in production) | 16:49 |
| oSoMoN | cheers | 16:50 |
| zyga | jdstrand: no name=snapd for now | 16:58 |
| zyga | jdstrand: if you want to know more I can summarize | 16:59 |
| zyga | jdstrand: if not we're going to discuss more tomorrow to see if we have other options | 16:59 |
| jdstrand | zyga: I read backscroll | 16:59 |
| jdstrand | zyga: it's too bad cause it seemed really elegant. guess it was too elegant, but good thing we got stgraber in the loop | 16:59 |
| zyga | yes, the lesson I learned is to ask more experts early | 17:00 |
| zyga | I wasn't aware of the extra limitations on rootless containers | 17:00 |
| zyga | actually | 17:00 |
| zyga | one idea | 17:01 |
| xnox | stgraber: har har har | 17:01 |
| zyga | stgraber: can you answer one more question please | 17:01 |
| zyga | stgraber: if we set a subreaper per snaps | 17:01 |
| zyga | *per snap* | 17:01 |
| zyga | stgraber: will we be able to follow that chain reliably | 17:01 |
| zyga | from random process, to their parent pid, to the subreaper | 17:01 |
| zyga | so that effectively, as long as the "watcher" subreapers are alive | 17:01 |
| zyga | we can reliably trace lineage? | 17:01 |
| zyga | so the idea is that instead of killing the watchers to kill the "container" | 17:02 |
| zyga | we can just scan all processes and try to match them to the reaper | 17:03 |
| zyga | hmmm, reading the man page it seems not useful | 17:04 |
| zyga | because it is not preserved across fork/clone | 17:04 |
| zyga | but is across exec | 17:04 |
| zyga | so we could watch our immediately forked process die | 17:04 |
| zyga | and if they had any children processes, we could watch them die if their parent (that we started) did quit | 17:05 |
| zyga | but I don't see anything in the man page linking the descendants to the reaper in a way that can be read | 17:05 |
| * zyga goes to experiment | 17:06 | |
| zyga | maybe it's not too bad actually | 17:06 |
| === pstolowski is now known as pstolowski|afk | ||
| mup | PR snapcraft#2783 closed: cli: run review tools before pushing to the store if available <Created by sergiusens> <Merged by sergiusens> <https://github.com/snapcore/snapcraft/pull/2783> | 17:43 |
| zyga | stgraber ^ this works | 17:45 |
| zyga | jdstrand: ^ this works | 17:45 |
| zyga | we can track all children existence | 17:45 |
| zyga | it's even better because the reaper can quit when they all all gone | 17:45 |
| zyga | so all we need to do is to unlink a "this snap is busy" file then | 17:46 |
| zyga | stgraber: ^ | 17:46 |
| mup | PR snapcraft#2785 closed: remote-build: add initial command unit tests <Created by cjp256> <Merged by sergiusens> <https://github.com/snapcore/snapcraft/pull/2785> | 17:46 |
| zyga | a super-simple interactive reaper | 17:49 |
| zyga | https://paste.ubuntu.com/p/kYqhmWGtn3/ | 17:49 |
| zyga | I don't know if this can be made to work with proper pid tracking (e.g. how would this ever work for services) | 17:49 |
| zyga | but it's *something* | 17:49 |
| zyga | need to ponder on it more | 17:49 |
| ondra | sergiusens did we change way SNAPCRAFT_PROJECT_DIR behaves? I'm getting empty string there | 18:08 |
| ondra | sergiusens SNAPCRAFT_PROJECT_NAME and SNAPCRAFT_PROJECT_GRADE seems to work fine | 18:08 |
| sergiusens | ondra: no we did not change it | 18:14 |
| ondra | sergiusens then there is regression, as suddenly I'm getting empty string | 18:15 |
| sergiusens | ondra: on stable? | 18:22 |
| ondra | sergiusens stable and candidate | 18:22 |
| ondra | sergiusens at least what I tested | 18:22 |
| sergiusens | ondra: well stable has not changed for the pass 2 months | 18:23 |
| ondra | sergiusens hmm, then it must be this particular snap triggering it | 18:23 |
| ondra | sergiusens defo happening also with stable | 18:31 |
| litheum | it seems like snap is doing something terribly wrong with apparmor on lubuntu 18.04. i've installed both snap-store and vlc and both of them just throw tons of apparmor errors when i try to start them. | 21:33 |
| Chipaca | litheum: could you open a topic about this in the forum? | 21:49 |
| Chipaca | litheum: it should just work :-) if it doesn't we want to fix it | 21:49 |
| Chipaca | but I, personally, don't want to even think about fixing it at this time | 21:49 |
| Chipaca | :-) 'm off to bed | 21:50 |
| mup | PR snapcraft#2791 opened: introduce bind-ssh support <Created by cjp256> <https://github.com/snapcore/snapcraft/pull/2791> | 23:23 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!