[05:51] <mborzecki> morning
[06:12] <mardy> hi!
[07:04] <mborzecki> mardy: any new PRs I should look at?
[07:05] <pstolowski> morning
[07:16] <mborzecki> mardy: do you think you could take a look at https://github.com/snapcore/snapd/pull/10162 ?
[07:17] <mborzecki> pstolowski: hey, any PRs I should look at?
[07:21] <pstolowski> mborzecki: you can take a look at #10261
[07:22] <pstolowski> https://github.com/snapcore/snapd/pull/10261
[07:22] <mborzecki> sure will do
[08:11] <pedronis> mborzecki: it's a bit annoying that the tag on 2.51 is wrong now for Fedora?
[08:11] <mborzecki> pedronis: that's fine, i'm tweakign the changelog manually when i prepare a downstream package
[08:12] <pedronis> ah ok
[08:12] <mborzecki> pedronis: and i've landed https://github.com/snapcore/snapd/pull/10320 which fixes it in the branch
[08:32] <mborzecki> pedronis_: fwiw it's the same for opensuse's *.changes file, at least now i'll have some useful data for it 
[08:40] <mardy> mborzecki: I'll have a look at that MR; as for me, maybe you can also write your opinion on https://github.com/snapcore/snapd/pull/10292 vs https://github.com/snapcore/snapd/pull/10282
=== pedronis_ is now known as pedronis
[08:54] <pedronis> mardy: pstolowski: I commented here: https://github.com/snapcore/snapd/pull/10282/files#r641385183
[08:56] <pedronis> mardy: #10292 seems should be closed
[08:57] <pedronis> but other one changed
[09:02] <pedronis> mborzecki: is the new discussion on #10307
[09:02] <pedronis> s/is/see/
[09:03] <pstolowski> pedronis: thanks, that's a valid observation
[09:04] <mborzecki> pedronis: yeah saw that, i'll probably be slightly different with proc, i have a pi with 20.04, i can try to measure it there
[09:36] <mardy> the unit tests are always failing in my machine (cmd_info_test): [10]: "refresh-date: 2006-01-03" != "refresh-date: 2006-01-02"
[09:36] <mardy> I'm running them with LC_ALL=C, but that does not seem to help
[09:40] <rZr> hi. today I'll present online a project about core check teaser demo https://mastodon.social/@rzr/106312151682794643#pinball-ubuntu-core-20-rzr 
[09:59] <mborzecki> pedronis: added a comment with some results from pi4: https://github.com/snapcore/snapd/pull/10307#discussion_r641427673
[09:59] <mborzecki> mardy ^^
[10:00] <pedronis> mborzecki: are those 4ms 5ms  mostly i/o wait? or CPU use?
[10:03] <mborzecki> pedronis: this is 4-5 iterations of the loop, so waiting on sleep mostly, the fractional part was netgligible
[10:04] <mborzecki> also suggesting that reading proc & processing the string is rather insignificant
[10:04] <mborzecki> pedronis: iow. that's time elapsed between first check and after the loop is done
[10:05] <pedronis> mborzecki: ok, then it sounds like the code is ok as is I suppose
[10:06] <mborzecki> it's even better under heavy load, snap just takes longer to do the first check, and by the time it does it the process has been moved
[11:09] <pstolowski> pedronis: i proposed bumping of maxp
[11:09] <pstolowski> max postponement separately in https://github.com/snapcore/snapd/pull/10321 to reduce the other diff a bit
[11:11] <pedronis> pstolowski: thx
[11:48] <mborzecki> mardy: hm i understand there's going to be more changes based on what pedronis suggested in https://github.com/snapcore/snapd/pull/10282 ?
[11:52] <mardy> mborzecki: yes, I made the changes, now I'm strugglying to write unit tests
[11:53] <mardy> mborzecki: the serviceControlTs() function does not seem to be tested at all in overlord/servicestate/servicestate_test.go, so I guess I need to create a new test from scratch
[11:55] <mborzecki> mardy: there may be some indirect tests that exercise Control()?
[11:56] <mborzecki> mardy: oh, I use a tool go-caret to view the coverage (rather than the built-in tools), https://github.com/msoap/go-carpet once the tests are passing i run it in the pacakge directory, `go-carpet | less -R` 
[12:01] <pstolowski> mardy: it's tested in service_control_test.go, but not called directly, it's called via Control
[12:02] <pstolowski> cachio: hi, i made another pass on https://github.com/snapcore/snapd/pull/10286
[12:03] <mardy> mborzecki: nice, now I need to understand why the cmd_info_test fails for me, though :-)
[12:04] <mardy> pstolowski: thanks, they look a bit complicated, do you think it would be a good idea if I wrote a unit test just for Control() in overlord/servicestate/servicestate_test.go?
[12:14] <pedronis> pstolowski: I'm looking at #10261,  how much different is autoRefreshPhase2 from doUpdate now?
[12:20] <pedronis> pstolowski: I made some high-level comments there
[12:26] <pedronis> mborzecki: are you working on https://github.com/snapcore/snapd/pull/10291 ?
[12:27] <mborzecki> pedronis: yes, editing it right now
[12:27] <pedronis> ok, thx
[12:32] <pstolowski> pedronis: thanks, yes they are very similiar, i'll try to merge them again, might be easier now than was before
[12:32] <pedronis> pstolowski: to be clear my reason for wanting to merge them is not to have less code, is not two have to places to change if we need do changing something about their details, by definition they shouldn't need to diverge a lot of over time
[12:36] <pstolowski> pedronis: yes i share the concern. let me see
[12:43] <pstolowski> mardy: yes that's fine i think. but you will need some tests for integration with wrappers as well such as TestRestartServices in service_control_test.go, those should probably be added there
[12:46] <rbasak> Anyone else seeing "snap try" fail in a lxd container? I'm using Focal, and it doesn't seem to work at all. I can provide steps to reproduce if needed.
[12:47] <rbasak> It does seem to work on KVM.
[12:54] <pedronis> rbasak: what's the version of snapd deb inside your container? we had some bugs related to this and they need a recent deb to be fixed, newer snapd snap is not enough
[12:55] <pedronis> pstolowski might remember more about this and relevant versions
[13:00] <pstolowski> i'll check it in a moment
[13:01] <rbasak> ^ I just posted https://forum.snapcraft.io/t/snap-try-broken-in-lxd-containers/24687 before seeing your question
[13:02] <rbasak> snapd   2.48.3+20.04
[13:08] <ogra> rbasak, do you have squashfuse installed on the host ? (perhaps the upgrade removed it)
[13:09] <ogra> (just a wild guess)
[13:09] <rbasak> ogra: surely you mean the guest?
[13:09] <rbasak> I'll check, but I'm puzzled as to why it's needed when squashfs shouldn't be involved for "snap try"?
[13:10] <rbasak> ogra: it was not installed in my failing example in the guest. I installed it and tried "snap try" again, and it still fails.
[13:11] <ogra> yeah, i just checked on ym focal and it isnt instealled either anywhere (neother guest nor host) ... 
[13:11] <ogra> red herring then, sorry 
[13:38] <pstolowski> rbasak: right. the fix is in snapd 2.49 deb
[13:45] <pstolowski> mardy: test
[13:47] <pstolowski> mardy: do you have a moment to review https://github.com/snapcore/snapd/pull/10321 ? it's trivial
[13:50] <mardy> pstolowski: looking now (the test did not work :-( )
[14:12] <ijohnson> pedronis: I wonder if we should have github actions workflows that don't run on self-hosted runners (so they are not spread tests and still run when we do "skip spread") which just try to build the package for the distros to catch my mistake yesterday in the date format
[14:13] <ijohnson> I'm not sure how our github actions hosted runner queue is these days
[14:18] <pedronis> ijohnson: no idea, I suppose it would be something for sergio to work on, we need mvo input on this
[14:18] <ijohnson> pedronis: ack I will put it in the SU doc for us to discuss when mvo is back (he is back on monday right?)
[14:19] <pedronis> yes, he should be back on Monday
[14:46] <pedronis> mborzecki: any reason not to land https://github.com/snapcore/snapd/pull/10295 ?
[14:59] <pedronis> ijohnson: it's fine to land https://github.com/snapcore/snapd/pull/10139 but the tests are in a weird state
[14:59] <ijohnson> pedronis: weird how so ?
[15:00] <pedronis> it has pending things that should be run early
[15:00] <ijohnson> I just restarted the ones that were failed to see if they go green and we can land w/o needing sudo merge 
[15:00] <pedronis> ijohnson: shouldn't also been run with nested?
[15:00] <ijohnson> pedronis: ah yeah you're right it should be run with nested too
[15:01] <pedronis> so maybe not landable quite yet
[15:01] <ijohnson> I always forget that uc20 label semantically should imply nested label many times
[15:01] <pedronis> but doesn't
[15:01] <ijohnson> hence why I said "should"
[15:01] <ijohnson> but anyways let me close and re-open with the nested label, if the nested tests are happy then hopefully we can land it
[15:08] <pedronis> ijohnson: pstolowski: I answered to the question in https://github.com/snapcore/snapd/pull/10321 also added to the description the motivation behind this change.
[15:08] <ijohnson> pedronis: ack thanks for mentioning that
[15:09] <pstolowski> thanks
[15:22] <mborzecki> pedronis: not really, i've restated the spread jobs today as some runs failed, but it seems to be good, only 21.04 restore failed, but looks unrelated
[15:31] <pstolowski> pedronis: doUpdate can be nicely reused, i'm pushing it 
[15:41] <pedronis> pstolowski: thx, I will look at it first thing on monday
[15:45] <pstolowski> pedronis: i could also revert the change in doUpdate about waitPrereq & updatePrerequsitesTasksets if you thing it's unneeded?
[15:45] <pstolowski> *think
[15:45] <pedronis> pstolowski: maybe it's a good idea, as it's not used anywhere else now?
[15:47] <pstolowski> pedronis: yes, i did this to reuse it in both implementations, now it's used only in doUpdate
[15:47] <pedronis> pstolowski: unless you think the new code is much more readable?
[15:51] <pedronis> pstolowski: I added some answers or new considerations in bold to the spec.
[15:51] <pstolowski> pedronis: thanks; no, i don't think the new code is much more readable, removing it to reduce the diff
[16:01] <cachio> pstolowski, hi, if you have some time, could you please take a look to #10286 again
[16:04] <pstolowski> cachio: i already did and added one more comment
[16:07] <pedronis> ijohnson: I reviewed https://github.com/snapcore/snapd/pull/10316, nothing to change immediately but some wonderings
[16:07] <pstolowski> cachio: https://github.com/snapcore/snapd/pull/10286#discussion_r641612412
[16:08] <cachio> pstolowski, nice, thanks
[16:09] <pedronis> ijohnson: should I go ahead and merge https://github.com/snapcore/snapd/pull/10322 ?
[16:10] <ijohnson> pedronis: should we maybe consider changing the structure of the response for quota infos to be something similar to the structure we want for the CLI i.e. to have a `"current": { "memory": 100, "cpu": ...}, "constraints": { "memory": 400, "cpu": ... }` ?
[16:11] <pedronis> ijohnson: we could I suppose
[16:12] <ijohnson> pedronis: you can merge 10322 yeah, I +1d it
[16:14] <ijohnson> pedronis: not for that PR obviously, but before making it non-experimental I suppose
[16:15] <pedronis> ijohnson: yea
[16:16] <ijohnson> ok
[16:16] <cachio> mardy, hi, could you please give last check to #10162
[16:20]  * cachio lunch
[16:20] <ijohnson> cachio: can you take a look at https://github.com/snapcore/snapd/pull/10099 today? 
[16:21] <pedronis> ijohnson: I reviewed https://github.com/snapcore/snapd/pull/9897 +1 but some nitpicking
[16:21] <ijohnson> I did merge master in there just now so the tests will re-run
[16:21] <ijohnson> thanks pedronis 
[20:23] <mardy> cachio: sorry, was already off. I've approved it now