[05:48] <mborzecki> morning
[06:02] <mardy> mborzecki: hi!
[06:02] <mborzecki> mardy: hey
[06:50] <mborzecki> mvo: hey
[06:51] <mvo> good morning mborzecki 
[06:53] <zyga-mbp> good morning
[06:58] <mvo> good morning zyga-mbp 
[06:59] <mvo> fwiw, 10287 needs a second review
[07:03] <pstolowski> morning
[07:07] <mvo> good morning pstolowski !
[07:07] <zyga-mbp> pstolowski nice work!
[07:08] <zyga-mbp> oh wait, that was actually ian!
[07:08] <zyga-mbp> sorry ian and pawel
[07:08] <zyga-mbp> I'm a bit sleepy still
[07:08] <pstolowski> hey zyga-mbp  :)
[07:08] <mborzecki> mvo: reviewed, there seems to be some race in the test, google:centos-8-64:tests/main/snap-quota-groups failed and there's more than 1 task in the cgroup
[07:08] <mborzecki> pstolowski: zyga-mbp: hey
[07:10] <mvo> mborzecki: oh, interessting, thanks, looking
[07:21] <mvo> mborzecki: I think I do a followup, this is strange
[07:22] <mborzecki> mvo: that's fine, let's land it and we can tweak the test later
[07:22] <mborzecki> at least the whole feature will progress
[07:32] <mvo> 10329 should now also be unblocked
[07:51] <mborzecki> another one merged, we're down to 65 PRs
[08:12] <mborzecki> mvo: i've added some comments to https://github.com/snapcore/snapd/pull/10329 i can address those once i push out an rfc with serial & remodel, unless you want to have a go at it first
[08:13] <mvo> mborzecki: go wild, I'm looking at spread things right now
[08:24] <pedronis> mborzecki: does it mean the task count is always 0 on centos ?
[08:29] <mborzecki> pedronis: it's not, https://paste.ubuntu.com/p/R487f3RGRy/, disabling TasksAccounting makes system not create a group for the slice under the pids controller 
[08:29] <mborzecki> pedronis: at least that what i determined through experimentation
[08:30] <mborzecki> pedronis: oh and when there's no cgroup, that's the current tasks count i get: TasksCurrent=18446744073709551615 so fun again
[08:31] <mborzecki> otoh, the value of MemoryCurrent is 0, so not sure what's really happening there
[08:32] <pedronis> mborzecki: I suspect we shoild just not support this there for now, it seems need a lot of attention and we don't really know what other bugs might hide there
[08:32] <mborzecki> it's only high when there's no memory controller at all, i.e. no memory limit set for given slice
[08:32] <mborzecki> where high == exabytes ian saw
[08:37] <pstolowski> mvo, pedronis there's an interesting old comment from zyga-mbp on a download gettng stuck bug here: https://bugs.launchpad.net/snapd/+bug/1891618/comments/2
[08:38] <pedronis> mborzecki: mvo: it sounds like we would need a lot spread tests to be sure things actually work
[08:39] <pedronis> mborzecki: mvo: anyway I don't think it makes sense to block the chain of PRs on this, it's probably best to not support these systems and reconsider later
[08:39] <mvo> pstolowski: that is interessting indeed
[08:39] <mvo> pedronis: +1 for not supporting these old systemds
[08:39] <zyga-mbp> mvo is it possible to move snapd to latest go?
[08:39] <mborzecki> pedronis: mvo: added one more comment with more explanation and a pastebin
[08:42] <mborzecki> pedronis: postponing support for older systems sounds like a good call, we can always revisit this once we've done more experiments
[08:43] <mborzecki> wondering if systemd somehow reports that memory controller is available at all
[08:43] <pedronis> zyga-mbp: we plan to move to 1.13 but not latest
[08:43] <mborzecki> zyga-mbp: do you recall anything like that in systemd? ^^
[08:43] <zyga-mbp> mborzecki like what exactly?
[08:44] <mborzecki> zyga-mbp: whether it's possible to find out if memory controller is even available
[08:44] <zyga-mbp> hmmm
[08:44] <zyga-mbp> isn't that visible in mountinfo in v1?
[08:45] <mborzecki> iirc it doesn't need to be mounted if memory accounting is disabled globally, and then you can always disable it in the kernel
[08:45] <zyga-mbp> (in v2 you have to consult what's enabled in the part you belong to
[08:45] <mborzecki> zyga-mbp: it is ;) but i'm more into poking systemd to know if that's doable
[08:45] <zyga-mbp> I see
[08:45] <zyga-mbp> I don't recall _but_ systemd has extensive dbus apis
[08:45] <zyga-mbp> perhaps it's somewhere there
[08:45] <zyga-mbp> I'd have to look
[08:46] <mborzecki> zyga-mbp: hm yeah, i should take a quick look
[08:47] <zyga-mbp> there is a property 
[08:47] <zyga-mbp> MemoryAccounting
[08:47] <zyga-mbp> bool 
[08:47] <zyga-mbp> on org.freedesktop.systemd1.Service 
[08:47] <zyga-mbp> but I'd double check how it is implemented to know it is what you want
[08:50] <zyga-mbp> mborzecki https://github.com/systemd/systemd/blob/7dbc38db509f153256d3a3bfe6cbb26e2731c741/man/systemd.resource-control.xml#L269
[08:52] <mborzecki> zyga-mbp: mhm, thanks, looking at it
[10:21] <mardy> pstolowski: does this line look totally correct to you? https://github.com/snapcore/snapd/blob/master/overlord/servicestate/service_control_test.go#L295
[10:21] <mardy> shouldn't the action name be "test-snap.foo"?
[10:22] <pstolowski> looking
[10:31] <pstolowski> mardy: correct. it works because makeControlChange test helper creates apps out of it and current implementation of servicestate.Control() doesn't care about inst.Names anymore and uses apps. So it's confusing but test-only issue afaict
[10:32] <mardy> pstolowski: good! Then I guess it's fine if I make a small change to makeControlChange and to the tests, to use the fill names, right?
[10:32] <mardy> s/fill/full/
[10:33] <pstolowski> mardy: yes, sounds fine
[10:46] <mborzecki> pedronis: opened https://github.com/snapcore/snapd/pull/10330 with the check for serial assertion in remodel, need to sort out the spread tests still
[10:47] <pedronis> mborzecki: thx, I will look in a bit
[11:00] <mborzecki> thanks
[11:29] <mborzecki> hm i think i got the tests runnig now
[12:01] <pstolowski> mborzecki: do you have some time for a 2nd review of https://github.com/snapcore/snapd/pull/10261 ?
[12:02] <mborzecki> pstolowski: yes, i can take a look in a bit
[12:03] <pstolowski> thx
[12:36] <mborzecki> meh the remodel spread tests
[13:38] <mborzecki> pedronis: quick questions, even if i don't put my account & account key assertions in the seed, will they be automatically pulled from the store?
[13:39] <pedronis> mborzecki: they are needed for first seeding, prepare-image puts them there for you, but  I don't know what the test does
[13:39] <pedronis> mborzecki: without first seeding can't validate the model
[13:39] <mborzecki> pedronis: ok, i need to drop them there then
[13:40] <mborzecki> pedronis: the remodel tests, clean all of state from /var/lib/snapd and kick off first boot again
[13:45] <GunnarHj> pedronis: Hi Samuele! Apparmor fixes are needed to make fcitx5 (bug #1928360) and to some extent ibus (bug #1890905) work as expected in the snaps. (The fcitx5 task is higher priority.) Any chance you can take it on?
[13:46] <GunnarHj> https://launchpad.net/bugs/1928360
[13:46] <GunnarHj> https://launchpad.net/bugs/1890905
[13:53] <mardy> pedronis: I forgot to ask you to put https://github.com/snapcore/snapd/pull/10282 back into your review queue :-)
[13:55] <ijohnson> GunnarHj: you might want to also try pinging jamesh about such issues those bugs smell very desktopy to me
[13:56] <pedronis> GunnarHj: apparmor fixes in which sense? fixes to apparmor or to our apparmor rules?
[13:59] <GunnarHj> pedronis: The fcitx5 issue is about our apparmor rules, but the ibus one is about improving apparmor itself to start with.
[14:00] <GunnarHj> ijohnson: Thanks for the tip. Yeah, the bugs are indeed desktop'ish.
[14:08] <mborzecki> dumped the assertions into a single file, wondering if that will work
[14:15] <mborzecki> wooo, remodel-base workd
[15:03] <mborzecki> and pushed, remodel spread tests should work again
[15:10] <mvo> mborzecki: \o/
[15:50] <ijohnson> mvo: pedronis: can you land https://github.com/snapcore/snapd/pull/10099 ? the failure on debian-10 are unrelated
[15:55] <ijohnson> mvo: also I noticed you cherry picked https://github.com/snapcore/snapd/pull/10324 to release/2.51, so I added the cherry-picked label to that PR for you
[16:01] <pedronis> ijohnson: I think mvo beat me to it
[16:01] <ijohnson> yeah, no worries thanks for checking on it though
[16:09] <mvo> ijohnson: thanks! I keep forgetting this
[16:09] <ijohnson> it will become habit and save us much time in the future :-)
[16:16] <mvo> yeah, it's good!
[18:12] <ijohnson> pedronis: can you merge 10331 then ? I will open up our two alternatives after that so you can decide in the AM what direction we want to go 
[18:12] <ogra> did anyone tak to iemeyer yet about mup ? 
[18:13] <ogra> (i see it re-connecting in a 30sec loop in the old channel ... FWIW)
[18:14] <ijohnson> ogra: I thought mvo did already, I assume Gustavo's been a bit busy lately to move it over
[18:14] <pedronis> ijohnson: how can you open the alternatives if we don't have spread tests about current memory usage?
[18:14] <ogra> ijohnson, ah
[18:14] <pedronis> ijohnson: we probably want to fix the CLI first
[18:15] <ijohnson> pedronis: well now that the daemon is reporting memory usage, regardless of whether the CLI displays it, if there are problems the CLI dies
[18:15] <pedronis> ijohnson: yes, but not dying is not a criteria enough to decide on the workarounds
[18:17] <ijohnson> I mean yeah I guess if you want to do the CLI changes first then we can, but we can't really measure the memory usage specifically in the spread test
[18:17] <ijohnson> the spread test update I had in the CLI change PR just sees that there are numbers in the output, it doesn't actually confirm that the numbers make sense
[18:18] <ijohnson> it would be hard to check that the numbers make sense given that memory usage is not predictable in exact terms, I guess I could compare to make sure the number is under 100 megabytes or something obvious like that
[18:18] <pedronis> ijohnson: there's probably a min and max that can be applied
[18:19] <ijohnson> I guess in other words, I'm just not sure what the threshold is to decide on the workaround
[18:19] <ijohnson> alright I guess I can write a min/max detection snippet for the functional spread test
[18:22] <pedronis> ijohnson: do we have a test where we see processes hit the OOM ?
[18:22] <ijohnson> yes that's `tests/main/snap-quota-groups`
[18:22] <pedronis> I see one where they don't start, it that the OOM or something else?
[18:22] <ijohnson> yes that's checking the OOM
[18:22] <ijohnson> well indirectly checking that the service fails due to oom
[18:22] <ijohnson> we check that the service starts properly with a large limit, then we check that it doesn't start with a small limit
[18:26] <pedronis> it seems interfaces-packagekit-control has started failing consistently on arch
[18:27] <pedronis> ijohnson: I merged that PR, please consider my comment in the next, anyway I still think we want to fix the CLI first thing, before considering the corner case distros
[18:28] <ijohnson> pedronis: ok yes I have that change implemented in the CLI PR, which I will open shortly after adding this min / max logic to the snap-quota-groups spread test for that PR
[18:29] <ijohnson> pedronis: also yeah I saw that test has started to fail, I will try to have a look this afternoon, if I don't get to it, I left a comment about it with logs in the SU doc for someone to look at
[18:29] <pedronis> ijohnson: I suppose it might be something for Maciej to look at
[18:30] <ijohnson> yeah nothing obvious came up when I looked at it
[18:30] <ijohnson> to me at least
[19:04]  * cachio afk