[05:48] morning [06:02] mborzecki: hi! [06:02] mardy: hey [06:50] mvo: hey [06:51] good morning mborzecki [06:53] good morning [06:58] good morning zyga-mbp [06:59] fwiw, 10287 needs a second review [07:03] morning [07:07] good morning pstolowski ! [07:07] pstolowski nice work! [07:08] oh wait, that was actually ian! [07:08] sorry ian and pawel [07:08] I'm a bit sleepy still [07:08] hey zyga-mbp :) [07:08] 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] pstolowski: zyga-mbp: hey [07:10] mborzecki: oh, interessting, thanks, looking [07:21] mborzecki: I think I do a followup, this is strange [07:22] mvo: that's fine, let's land it and we can tweak the test later [07:22] at least the whole feature will progress [07:32] 10329 should now also be unblocked [07:51] another one merged, we're down to 65 PRs [08:12] 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] mborzecki: go wild, I'm looking at spread things right now [08:24] mborzecki: does it mean the task count is always 0 on centos ? [08:29] 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] pedronis: at least that what i determined through experimentation [08:30] pedronis: oh and when there's no cgroup, that's the current tasks count i get: TasksCurrent=18446744073709551615 so fun again [08:31] otoh, the value of MemoryCurrent is 0, so not sure what's really happening there [08:32] 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] it's only high when there's no memory controller at all, i.e. no memory limit set for given slice [08:32] where high == exabytes ian saw [08:37] 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] mborzecki: mvo: it sounds like we would need a lot spread tests to be sure things actually work [08:39] 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] pstolowski: that is interessting indeed [08:39] pedronis: +1 for not supporting these old systemds [08:39] mvo is it possible to move snapd to latest go? [08:39] pedronis: mvo: added one more comment with more explanation and a pastebin [08:42] pedronis: postponing support for older systems sounds like a good call, we can always revisit this once we've done more experiments [08:43] wondering if systemd somehow reports that memory controller is available at all [08:43] zyga-mbp: we plan to move to 1.13 but not latest [08:43] zyga-mbp: do you recall anything like that in systemd? ^^ [08:43] mborzecki like what exactly? [08:44] zyga-mbp: whether it's possible to find out if memory controller is even available [08:44] hmmm [08:44] isn't that visible in mountinfo in v1? [08:45] 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] (in v2 you have to consult what's enabled in the part you belong to [08:45] zyga-mbp: it is ;) but i'm more into poking systemd to know if that's doable [08:45] I see [08:45] I don't recall _but_ systemd has extensive dbus apis [08:45] perhaps it's somewhere there [08:45] I'd have to look [08:46] zyga-mbp: hm yeah, i should take a quick look [08:47] there is a property [08:47] MemoryAccounting [08:47] bool [08:47] on org.freedesktop.systemd1.Service [08:47] but I'd double check how it is implemented to know it is what you want [08:50] mborzecki https://github.com/systemd/systemd/blob/7dbc38db509f153256d3a3bfe6cbb26e2731c741/man/systemd.resource-control.xml#L269 [08:52] zyga-mbp: mhm, thanks, looking at it === doko_ is now known as doko [10:21] 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] shouldn't the action name be "test-snap.foo"? [10:22] looking [10:31] 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] 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] s/fill/full/ [10:33] mardy: yes, sounds fine [10:46] 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] mborzecki: thx, I will look in a bit [11:00] thanks [11:29] hm i think i got the tests runnig now [12:01] mborzecki: do you have some time for a 2nd review of https://github.com/snapcore/snapd/pull/10261 ? [12:02] pstolowski: yes, i can take a look in a bit [12:03] thx [12:36] meh the remodel spread tests [13:38] 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] 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] mborzecki: without first seeding can't validate the model [13:39] pedronis: ok, i need to drop them there then [13:40] pedronis: the remodel tests, clean all of state from /var/lib/snapd and kick off first boot again [13:45] 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] https://launchpad.net/bugs/1928360 [13:46] https://launchpad.net/bugs/1890905 [13:53] pedronis: I forgot to ask you to put https://github.com/snapcore/snapd/pull/10282 back into your review queue :-) [13:55] GunnarHj: you might want to also try pinging jamesh about such issues those bugs smell very desktopy to me [13:56] GunnarHj: apparmor fixes in which sense? fixes to apparmor or to our apparmor rules? [13:59] pedronis: The fcitx5 issue is about our apparmor rules, but the ibus one is about improving apparmor itself to start with. [14:00] ijohnson: Thanks for the tip. Yeah, the bugs are indeed desktop'ish. [14:08] dumped the assertions into a single file, wondering if that will work [14:15] wooo, remodel-base workd [15:03] and pushed, remodel spread tests should work again [15:10] mborzecki: \o/ [15:50] mvo: pedronis: can you land https://github.com/snapcore/snapd/pull/10099 ? the failure on debian-10 are unrelated [15:55] 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] ijohnson: I think mvo beat me to it [16:01] yeah, no worries thanks for checking on it though [16:09] ijohnson: thanks! I keep forgetting this [16:09] it will become habit and save us much time in the future :-) [16:16] yeah, it's good! === ijohnson is now known as ijohnson|lunch === ijohnson|lunch is now known as ijohnson [18:12] 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] did anyone tak to iemeyer yet about mup ? [18:13] (i see it re-connecting in a 30sec loop in the old channel ... FWIW) [18:14] ogra: I thought mvo did already, I assume Gustavo's been a bit busy lately to move it over [18:14] ijohnson: how can you open the alternatives if we don't have spread tests about current memory usage? [18:14] ijohnson, ah [18:14] ijohnson: we probably want to fix the CLI first [18:15] 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] ijohnson: yes, but not dying is not a criteria enough to decide on the workarounds [18:17] 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] 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] 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] ijohnson: there's probably a min and max that can be applied [18:19] I guess in other words, I'm just not sure what the threshold is to decide on the workaround [18:19] alright I guess I can write a min/max detection snippet for the functional spread test [18:22] ijohnson: do we have a test where we see processes hit the OOM ? [18:22] yes that's `tests/main/snap-quota-groups` [18:22] I see one where they don't start, it that the OOM or something else? [18:22] yes that's checking the OOM [18:22] well indirectly checking that the service fails due to oom [18:22] 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] it seems interfaces-packagekit-control has started failing consistently on arch [18:27] 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] 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] 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] ijohnson: I suppose it might be something for Maciej to look at [18:30] yeah nothing obvious came up when I looked at it [18:30] to me at least === axino is now known as ux === ux is now known as axino [19:04] * cachio afk