| mborzecki | morning | 05:58 |
|---|---|---|
| mardy | 'morning! | 06:03 |
| mborzecki | mardy: heya | 06:08 |
| mborzecki | hmm a bunch of google:ubuntu-20.04-64:tests/main/lxd:* failed | 06:10 |
| mborzecki | found a couple of PRs with the same problem, maybe something changed in lxd? | 06:21 |
| mardy | the "retry" command that we are using in the spread tests, is it a standard UNIX command, or where is it coming from? | 06:34 |
| mardy | ah, found it: ./tests/lib/tools/retry | 06:37 |
| mborzecki | mardy: no it's a custom thing zyga-mbp wrote a while back | 06:52 |
| mborzecki | mardy: guess it's more like `until <...>; do ... ; sleep ; done` | 06:53 |
| pstolowski | morning | 07:02 |
| mborzecki | pstolowski: hey | 07:07 |
| mborzecki | errands, need to take some paperwork to my accountant and then i'll meet up with koza and work from a cafe for a bit | 07:13 |
| zyga-mbp | good morning | 07:18 |
| zyga-mbp | mborzecki say hi to koza :) | 07:18 |
| mborzecki | will do | 07:22 |
| pedronis | mvo: hi, https://github.com/snapcore/snapd/pull/10379 is asking your review | 08:11 |
| mvo | pedronis: sure, looking. | 08:11 |
| mvo | I just pushed 10400 which I think we should also include in 2.51.1. I think we are ready then and I will do 2.51.1 then | 08:13 |
| pedronis | mvo: +1 | 08:18 |
| mvo | ta | 08:18 |
| mborzecki | zyga-mbp: koza says hi | 08:28 |
| zyga-mbp | mborzecki coffee or beer time? :-) | 08:28 |
| mborzecki | coffee ofc | 08:28 |
| zyga-mbp | *hic* | 08:28 |
| pstolowski | re | 09:21 |
| pstolowski | pedronis: hi, have you seen my reply https://github.com/snapcore/snapd/pull/10384#discussion_r650955557 ? | 09:34 |
| pedronis | pstolowski: yes, I'm still thinking about it | 09:35 |
| pstolowski | pedronis: ack, thanks | 09:36 |
| mborzecki | pedronis: i've updated https://github.com/snapcore/snapd/pull/10386 | 09:53 |
| mborzecki | mvo: pstolowski: can you take a look at https://github.com/snapcore/snapd/pull/10361 ? | 09:55 |
| pedronis | mborzecki: thx, I'll look after lunch/meeting | 09:56 |
| mborzecki | pedronis: thank you! | 09:57 |
| pstolowski | looking | 09:59 |
| mborzecki | thanks! | 10:00 |
| pstolowski | mborzecki: https://github.com/snapcore/snapd/pull/10362 needs 2nd review if you have some time | 10:28 |
| mborzecki | pstolowski: will do | 10:34 |
| pstolowski | ty | 10:37 |
| mborzecki | pstolowski: +1 | 10:58 |
| pstolowski | mborzecki: thanks! | 10:58 |
| pedronis | pstolowski: I discussed a bit more there, let me know wheter what I wrote makes sense (I might be confused) or if you want to chat a bit on this | 11:27 |
| pstolowski | pedronis: ok, ty | 11:27 |
| mborzecki | pstolowski: mardy: need help with reviews? | 12:00 |
| pedronis | mborzecki: https://github.com/snapcore/snapd/pull/10386 question/comments there, I didn't look close enough when I reviewed the first time | 12:00 |
| pstolowski | mborzecki: i'm fine, thanks | 12:02 |
| mborzecki | pedronis: hm i think i've done something about this in my wip branch, let me double check | 12:05 |
| pedronis | mborzecki: sorry, when I said to use the interface I thought we could just use the one from secboot, but now I notice that we need uniqueID but really when we have a bootchain anway | 12:20 |
| mborzecki | pedronis: i think i should rename it now to something in the lines of `modelForSealing()` | 12:20 |
| mborzecki | and still have modeenv generate one for bootchain and then bootchain do the same for secboot's model params | 12:20 |
| mborzecki | pedronis: i can drop the internal interface and just use the concrete type | 12:21 |
| pedronis | mborzecki: you mean having methods on both modeenv and bootchain to generate one? | 12:21 |
| mborzecki | yes, we need to have something that matches secboot.SnapModel when we generate secboot.ModelParams | 12:23 |
| mborzecki | we use modeenv to generate boot chains, then use the boot chains to generate model params | 12:23 |
| mborzecki | guess i could pass modeenv to a place which genrates model params, but we already have that information in boot chains because we store it on disk | 12:24 |
| pedronis | mborzecki: no, it's easier to have the modeenv and the bootchain be able to do the same thing | 12:30 |
| pedronis | I wouldn't try not to repeat five or six lines | 12:31 |
| pedronis | mborzecki: what's I'm saying is that both Modeenv.modelForSealing and bootChain.modelForSealing can exist | 12:32 |
| mborzecki | pedronis: ack, do you think the internal interface as it is now still makes sense? | 12:33 |
| pedronis | mborzecki: no, given that you don't to stick model on bootChain at all anymore I think | 12:33 |
| pedronis | you just need to rename the struct not to say modeeenv | 12:34 |
| mborzecki | ok | 12:34 |
| mborzecki | yup, something like `modelForSealing` should work i guess | 12:34 |
| pedronis | mborzecki: the fact that we had to stick asserts.Model on bootChain was always a bit unfortunate/a hack | 12:35 |
| pedronis | so it's good if we can simply stop needing such a field | 12:35 |
| mborzecki | yeah, it'd good that we're addressing some of that debt | 12:35 |
| mborzecki | pedronis: i've updated https://github.com/snapcore/snapd/pull/10386 | 13:38 |
| pedronis | mborzecki: thx, taking a break and then I'll look at it again | 13:38 |
| mborzecki | sure | 13:38 |
| mborzecki | thanks for reviews! | 13:38 |
| cachio_ | mborzecki, hey | 13:39 |
| cachio_ | about your comment https://github.com/snapcore/snapd/pull/10396#discussion_r651483335 | 13:39 |
| cachio_ | so, I don't see any tags remaining after removing the snap | 13:40 |
| cachio_ | mborzecki, what other thing I can check? | 13:41 |
| pedronis | https://github.com/snapcore/snapd/pull/10401 needs reviews (gci fun) | 13:43 |
| mborzecki | cachio_: so if i understand correctly, we don't remove a snap via snap remove, as a result the udev rulesa re not updated and udev isn't retriggered, then snap-mgmt --purge runs and removes just the snap and the files | 13:43 |
| mborzecki | cachio_: but if we run snap remove and trigger the rules, things work? | 13:43 |
| mborzecki | cachio_: hmm seems to be udev trigger && udev settle shouldn't be necessary? | 13:44 |
| mardy | mborzecki: thanks, this is hopefully near the landing stage: https://github.com/snapcore/snapd/pull/10363 | 13:48 |
| mardy | and I wouldn't mind some early feedback on https://github.com/snapcore/snapd/pull/10375 | 13:48 |
| mborzecki | cachio_: can you try this diff instead of the restore section? https://paste.ubuntu.com/p/c3cnfjkMFD/ | 13:48 |
| mborzecki | i think this should do the trick and will work for all tests | 13:50 |
| cachio_ | mborzecki, in reset.sh we do snap remove --purge "$snap" | 13:52 |
| cachio_ | mborzecki, is it ok to force that clean up? | 13:53 |
| cachio_ | because perhaps at some point we can hide a real issue | 13:53 |
| mborzecki | cachio_: snap remove --purge is executed on a core ssytem afaict | 13:53 |
| cachio_ | mborzecki, but the errors were in ubuntu-core-18 | 13:54 |
| mborzecki | cachio_: ah w8, but you've seen it fail on a core system | 13:54 |
| mborzecki | hmm | 13:54 |
| mborzecki | cachio_: ok, can you add udevadm trigger && udevadm settle in reset_all_snap() after we remove the base? | 13:55 |
| cachio_ | mborzecki, yes | 13:55 |
| cachio_ | mborzecki, also I can do it for non core systems | 13:56 |
| cachio_ | mborzecki, change done | 13:59 |
| ijohnson[m] | pedronis: mvo: can you merge https://github.com/snapcore/snapd/pull/10391? only failures are lxd and are unrelated | 14:08 |
| mborzecki | cachio_: cool, let's see if that works | 14:08 |
| mborzecki | maybe we're missing something universal for all snaps in the cleanup | 14:08 |
| mvo | ijohnson[m]: sure | 14:09 |
| cachio_ | mborzecki, yes, it could be | 14:09 |
| ijohnson[m] | thanks | 14:09 |
| cachio_ | in parallel I am testing the test in a loop | 14:09 |
| pedronis | mborzecki: I re-reviewed, some refinement suggestions, is a bit odd except in special cases to have exported methods return internal types | 14:11 |
| ijohnson[m] | pedronis: oh regarding #10346 I don't know what to do with the fact that it's 4K per sub-group | 14:13 |
| pedronis | ijohnson[m]: as you can't know how many there are before hand? | 14:14 |
| ijohnson[m] | I still think we should have at least a 4K minimum, but as Maciej pointed out that minimum doesn't really make sense because the nested one can not really doing anything with that sort of memory limit either and we have no way of knowing how many sub-groups will be put eventually into a given group when we create that group | 14:15 |
| ijohnson[m] | yeah | 14:16 |
| ijohnson[m] | you create the parent first without any sub-groups in it, then you add the sub-groups later, specifying the parent when the sub-group is created | 14:16 |
| ijohnson[m] | I suppose we could do something to the effect of specifying the number of expected sub-groups for a parent group when we create that parent group, but that would be a bit weird IMHO | 14:17 |
| pedronis | ijohnson[m]: I suppose either we keep the minimum 4k or we make it something like 1m | 14:17 |
| ijohnson[m] | I can say that making the minimum 1MB would be unfortunate as our spread test which checks that a service is killed by oom-killer I think might sometimes not pass if we can only lower the memory limit to 1M | 14:19 |
| pedronis | ijohnson[m]: then let's keep it 4k and see if somebody has feedback, otherwise it becomes a bit a small number of k number | 14:22 |
| pedronis | exercise | 14:22 |
| pedronis | *a pick a small number... | 14:23 |
| ijohnson[m] | pedronis: sure sounds good | 14:23 |
| ijohnson[m] | then that PR is unblocked, not sure if you still want to review it or if it can go in | 14:23 |
| pstolowski | mvo: sould https://bugs.launchpad.net/snapd/+bug/1919077 be still 'in progress'? | 14:23 |
| ijohnson[m] | oh except for conflicts fun | 14:23 |
| mvo | pstolowski: this is hopefully fixed | 14:24 |
| pstolowski | ah, nice | 14:24 |
| mvo | pstolowski: in a meeting, I set it to fix released but maybe worth to quickly verify | 14:24 |
| pedronis | mborzecki: hope my comments there are not too annoying, sorry for the back and forth | 14:24 |
| pstolowski | mvo: it was https://github.com/snapcore/snapd/pull/10042 right? | 14:25 |
| ijohnson[m] | eh I guess I would feel better if someone else +1d the 4K quota group PR before merging it | 14:28 |
| mborzecki | pedronis: that's fine, they make sense, pushing in a bit | 14:30 |
| mvo | pstolowski: yeah | 14:35 |
| pedronis | ijohnson[m]: I made a comment there about the error | 14:35 |
| pedronis | sorry | 14:35 |
| * cachio_ afk and lunch | 14:38 | |
| mvo | cachio_: I pushed 2.51.1 snapd to beta | 14:44 |
| ijohnson[m] | mvo: should we add updating https://launchpad.net/snapd/+milestones to the release checklist ? seems we have not done anything here since June last year /o\ but again not that it matters much I think | 15:09 |
| mvo | ijohnson[m]: looking | 15:10 |
| ijohnson[m] | I took the liberty of creating a 2.51 and 2.52 milestone for snapd on LP so we can tag bugs to it | 15:10 |
| mvo | ijohnson[m]: ok | 15:11 |
| ijohnson[m] | degville: perhaps you were already made aware of this, but if you find yourself with copious amounts of free time, maybe a quick look / review at https://github.com/snapcore/core-initrd/pull/28 would be nice | 16:42 |
| degville | ijohnson[m]: thanks for letting me know - I hadn't seen that. I'll definitely take a look! | 16:44 |
| ijohnson[m] | ack | 16:46 |
| ijohnson[m] | thanks | 16:46 |
| pstolowski | pedronis_: i think refresh control (what landed already) has a slight problem in that it allows the hook to call --proceed (which resets holdState of the snap), followed by --hold, effectively letting it bypass own duration limit. I think to prevent it, --proceed should just be cached on the context's action and performed in the hook handler. --hold should be executed immediately as it is right now. | 17:00 |
| pstolowski | anyway, i'll get to it tomorrow, should be a small PR | 17:04 |
| pstolowski | cu | 17:04 |
| === pedronis_ is now known as pedronis | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!