| mborzecki | morning | 05:58 |
|---|---|---|
| mardy | 'morning! | 06:28 |
| zyga-mbp | morning mardy, mvo :) | 07:07 |
| pstolowski | morning | 07:08 |
| mborzecki | zyga-mbp: pstolowski mardy: heya | 07:12 |
| mborzecki | hot, isn't it? 🙂 | 07:12 |
| mvo | good morning zyga-mbp pstolowski mborzecki and mardy | 07:15 |
| pstolowski | mborzecki: mhm.. | 07:15 |
| mvo | mborzecki: it's getting better here, we are in the low 20s right now | 07:15 |
| mborzecki | mvo: still low 30s around here | 07:16 |
| zyga-mbp | mborzecki uff yeah | 07:17 |
| zyga-mbp | mborzecki I cannot wait for the rain to come | 07:17 |
| zyga-mbp | my office has no A/C so I'm outside in the shade, where it's colder | 07:17 |
| mvo | mborzecki: woah, ok - fingers crossed that you get the colder air we currently get soon too :) | 07:19 |
| zyga-mbp | mvo: low 20s? do you have some rain already? | 07:19 |
| mvo | zyga-mbp: we had some thunderstorms over the weekend | 07:19 |
| mvo | zyga-mbp: so yes :) | 07:19 |
| ogra | zyga-mbp, he's understating ... that was more than just hunderstorms 🙂 | 07:20 |
| ogra | +t | 07:20 |
| zyga-mbp | I would love some rain but I think it's only tomorrow | 07:24 |
| zyga-mbp | I didn't do much over weekend, it's so hot that being outside is painful | 07:25 |
| mardy | pstolowski: hi! I'm looking into fixing the third point of https://forum.snapcraft.io/t/command-line-interface-to-manipulate-services/262/47 I just wanted to check that it's fine with you, since the git history shows that it was you who introduced the handling of disabled services | 08:08 |
| pstolowski | mardy: yes, of course it's fine | 08:26 |
| pstolowski | mardy: i suppose we will want to determine disabled services early before we call wrappers.StartServices, and pass them | 08:28 |
| pstolowski | mardy: so StartServices would be untouched by your changes\ | 08:28 |
| pedronis | mvo: hi, are you are aware of this: https://bugs.launchpad.net/ubuntu/+source/snapd/+bug/1925787 ? | 08:35 |
| mvo | pedronis: I was but I think the comment is not fully up-to-date, iirc julian changed something in apt so that our stuff keeps working, I need to look into my irc logs | 08:36 |
| pedronis | mvo: I assigned it to you | 08:37 |
| mvo | pedronis: but yeah, it will come back at some point, I put it on my todo and will investigate today | 08:37 |
| mvo | pedronis: thank you | 08:37 |
| mardy | pedronis, pstolowski: I guess I should also apply the same logic to sockets and timers (which are currently always unconditionally enabled when StartServices() is called)? https://github.com/snapcore/snapd/blob/master/wrappers/services.go#L270 | 08:56 |
| pedronis | mardy: I'm not suree if the irc is the best to discuss this, maybe you should write down a list of the expected changes somewhere | 08:57 |
| pedronis | expected/proposed | 08:58 |
| pstolowski | there are a couple of bug reports related to sockets/timers | 09:05 |
| mardy | pedronis: ok, I'll prepare one in Google Docs, then once we are close to an agreement we can post it to the forums | 09:11 |
| pedronis | mardy: thx (was in a meeting) | 09:59 |
| mborzecki | hmm hmm, so something works, the check for mount ns being occupied in s-c goes through cgroups now | 10:04 |
| mardy | pedronis: here's a summary: https://docs.google.com/document/d/1l0x_Qmu1BBI39ZlDAtudRzJUJcy548KUua2b6Gou0J8/edit | 10:31 |
| zyga-mbp | mborzecki something unexpected? | 10:32 |
| mborzecki | zyga-mbp: not really, just annoying | 11:08 |
| pedronis | mardy: queued to look at, thx | 11:39 |
| pedronis | pstolowski: I commented here https://github.com/snapcore/snapd/pull/10408/commits/7a56d913276e079178c76811e61f6561f084d9b5, let me know if you have questions, the related current code is fairly dispersed in the code base | 11:57 |
| pstolowski | pedronis: thanks | 12:00 |
| === sil2100_ is now known as sil2100 | ||
| pedronis | mborzecki: questions in https://github.com/snapcore/snapd/pull/10432 | 12:49 |
| ijohnson[m] | morning folks | 12:49 |
| ijohnson[m] | pedronis: I rebased https://github.com/snapcore/snapd/pull/10410, not sure if you need/want to review that one, it is simple | 12:49 |
| pedronis | ijohnson[m]: not particularly but it has no reviews atm | 12:51 |
| ijohnson[m] | yeah I know, but it got conflicted when mvo landed the other quota PR's this morning | 12:51 |
| ijohnson[m] | and it will cause further conflicts with other quota things like https://github.com/snapcore/snapd/pull/10420 for example | 12:52 |
| ijohnson[m] | if it isn't landed early | 12:52 |
| zyga-mbp | ijohnson[m] good morning :) | 13:08 |
| ijohnson[m] | Hey zyga-mbp | 13:15 |
| zyga-mbp | small update on spread -> lava exporter | 13:15 |
| zyga-mbp | before the full lava exporter is ready there will be an intermediate shellcheck exporter | 13:15 |
| zyga-mbp | that creates a tree of scripts with variable assignments | 13:15 |
| zyga-mbp | that is somewhat shellcheck friendly (somewhat because we don't retain the original location in the yaml perfectly, so errors need to be mapped by a human) | 13:16 |
| zyga-mbp | I will share that as soon as it's ready for review | 13:16 |
| ijohnson[m] | zyga-mbp: so shellcheck will be run as part of spread? or part of the lava exported? | 13:39 |
| ijohnson[m] | *exporter | 13:39 |
| zyga-mbp | ijohnson[m], I've added spread -export shellcheck | 13:39 |
| zyga-mbp | this creates a tree of files you can then shellcheck | 13:39 |
| zyga-mbp | so you don't need lava | 13:39 |
| zyga-mbp | but the logic of how to create them is internal to spread so you don't need to play catch-up with features | 13:40 |
| zyga-mbp | including debug and stuff like that, that's less frequently looked at | 13:40 |
| ijohnson[m] | interesting, so that is pretty much like the spread-shellcheck script we have in snapd then ? | 13:40 |
| zyga-mbp | yes but better | 13:40 |
| zyga-mbp | it handles variants for example | 13:40 |
| zyga-mbp | in a precise way | 13:40 |
| ijohnson[m] | nice | 13:40 |
| zyga-mbp | also no need to reinvent the wheel of how spread loads a project | 13:40 |
| zyga-mbp | that's rather non-trivial | 13:40 |
| ijohnson[m] | 👍️ | 13:43 |
| mborzecki | mvo: just opened another bit: https://github.com/snapcore/snapd/pull/10436 | 14:34 |
| mvo | mborzecki: nice, thank you | 14:34 |
| mborzecki | heh, hope it's not too bad, and there's tests included too! | 14:35 |
| mborzecki | got some errands to run | 14:36 |
| mborzecki | ijohnson: enjoy your vacation! | 14:36 |
| ijohnson[m] | thanks bboozzoo ! | 14:36 |
| ijohnson[m] | pedronis: indeed the REST API refactor conflicts with #10420, so I will open another PR on top of 10420 for the REST API refactor as a draft | 14:43 |
| pstolowski | ijohnson[m]: +1 on https://github.com/snapcore/snapd/pull/10410 with a small question; i see these extra meter (and timings) params are like a stone in the shoe ;) | 15:36 |
| ijohnson[m] | pstolowski: haha thanks for the review, yes these meters and timings are a bit annoying :-) | 15:37 |
| pstolowski | https://github.com/snapcore/snapd/pull/10433 needs 2nd review | 15:42 |
| ijohnson[m] | pedronis: also I was thinking about the spread test failures I mentioned last week in SU at some point where it seems like snapd is returning before things are fully done, whereas before we waited until they were fully done, well it's still happening and I think I understand the problem | 15:43 |
| ijohnson[m] | pedronis: the issue is that we mark the task Done (and thus also the change which consists of just that one quota-control task) _before_ we do `systemctl restart ...` - previously we didn't return until those systemctl ops were fully done | 15:43 |
| ijohnson[m] | pedronis: so in effect we are "returning" early in that the client which is checking the Done status of quota-control returns "Done" before we are done doing everything | 15:44 |
| ijohnson[m] | not sure if this is a problem or not, it is a bit unfortunate | 15:44 |
| pedronis | ijohnson[m]: you are right, this is very annoying | 15:45 |
| ijohnson[m] | pedronis: I was thinking we could fix it by making 2 tasks - one which runs the state stuff and wrappers.EnsureSnapServices(), and the second task which runs `systemctl restart ...` | 15:46 |
| pedronis | I need to think, this is really another corner case in our state machine | 15:46 |
| ijohnson[m] | then if we get rebooted after the first task is done but before the second one, the second task will be idempotent | 15:46 |
| ijohnson[m] | but yeah a corner case indeed | 15:46 |
| pedronis | ijohnson[m]: we could, it's weird though :/ | 15:46 |
| ijohnson[m] | I will mark the PR blocked and explain this so we don't forget at least | 15:47 |
| ijohnson[m] | pedronis: I left a comment in the PR and marked it blocked | 15:58 |
| pedronis | ijohnson[m]: I checked, there is nowhere were we do something like this, we sometimes set DoneStatus early but then we don't unlock immediately afaict | 15:59 |
| ijohnson[m] | :-/ | 16:00 |
| pedronis | ijohnson[m]: so it's a genunine new case, but also unique so far, I'm might just do something ugly but simple like introduce a task flag, we never do because is repeating the work of the state machine, but here it would not | 16:01 |
| ijohnson[m] | yeah that's kinda like what I described as option 3 in my comment | 16:01 |
| pedronis | it might need to be Set("state-updated", true") and a Get at the start | 16:02 |
| pedronis | Ensure is idempotent anyway | 16:03 |
| ijohnson[m] | oh no that's not like what I described, but I see what you are saying now | 16:03 |
| ijohnson[m] | yeah that's fair, should I work on trying to implement that in the PR? | 16:03 |
| pedronis | it shouldn't be too hard, it's just weird, but otherwise I can pick it up over the next days | 16:04 |
| pedronis | s/weird/unusual/ | 16:04 |
| ijohnson[m] | ack, I'll give it a shot, it does sound fairly simple I think | 16:08 |
| pedronis | ijohnson[m]: now that I look it probably needs some refactoring, like moving to a single ensureCall in the parent | 16:10 |
| pedronis | *ensure call | 16:10 |
| ijohnson[m] | pedronis: you mean in this PR or eventually? I think we know we eventually want to be able to handle multiple quota-actions in a single task, but I was hoping to not handle those sorts of things until we needed to with i.e. gadget seeding quota group setup | 16:11 |
| pedronis | ijohnson[m]: I think it's bit less repetive with that refactoring, the issue is basically that you need call ensureSnapServicesForGroup in any case, but if state-updated is true, you need to reconstruct it's arguments, the flow I think would be a bit more readable if this call was after the switch | 16:13 |
| pedronis | *its arguments | 16:14 |
| ijohnson[m] | hmm, it seemed a bit complex of a refactoring which is why I was hoping that it could be pushed off since the PR is already large and complex, but I suppose maybe it will make it easier, I will have a look | 16:14 |
| pedronis | ijohnson[m]: looking closer it seems we might have to stick the update(d) group on the task, instead of just a bool | 16:42 |
| ijohnson[m] | pedronis: hmm | 16:45 |
| ijohnson[m] | why so? | 16:45 |
| pedronis | ijohnson[m]: when we remove the group we don't have grp anymore for example | 17:53 |
| ijohnson[m] | pedronis: ah now I remember this problem yeah, hmm I'll have to look at it a bit, the group removal problem is tricky since we may need to traverse all the parents of the group that was removed | 18:24 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!