[01:09] PR snapd#10215 opened: store/store_download.go: make snapd resilient on slow network [03:24] PR snapd#10216 opened: o/servicestate: address comments from previous PR [03:29] PR snapd#10217 opened: o/servicestate: restart slices + services on modifications [03:29] PR snapd#10218 opened: o/servicestate/quota_control.go: add RemoveSnapFromQuota [05:55] morning [06:03] good morning [06:03] almost long weekend :) [06:03] yeah, happy that this week is almost over [06:06] mborzecki, I'm just looking at a build I did last evening [06:06] rpi4 stock image [06:06] trying to figure out how to wic it to an SD card [06:07] bmaptool! [06:07] never heard of this tool before [06:08] zyga: wic only builds the image, i.e. creates partitions and so on [06:08] I have raw partitions [06:08] just looking up docs, I see another tool I never used before: kas [06:09] kas feels like repo with extras [06:10] zyga: never used kas, but i submitted a bunch of upstream patches for wic [06:10] zyga: fwiw, there should be a wks (iirc the extension was) file in meta-raspberry [06:11] looking [06:11] yep [06:11] got it [06:11] I know what to do with wic [06:11] but now I'm intrigued with bmaptool [06:12] something to learn later [06:12] let's do simple wic now [06:12] i always assumed that wic mocks the name of the mic tool tha was used to create meego images (notice w is like upside down m) [06:12] copy the image and see [06:12] to me the names are a bit all over the place :) [06:12] not consistent or logical [06:12] but I get that projects have history [06:13] * zyga looks at CI to recall how to use wic [06:14] ... something is happening [06:15] haha [06:16] ./sdimage-raspberrypi-202104300815-mmcblk0.direct [06:16] woot [06:17] now to dd [06:17] I [06:17] I'll optimize later [06:17] I need to get a small screen for debugging stuff one day [06:20] copying now [06:21] I need to do some power supply tricks to have automatic power as well [06:22] copy was quick :) [06:50] * zyga goes to another system [07:05] morning [07:11] pstolowski: hey [07:15] pstolowski: any PRs to look at? [07:18] hey mvo [07:19] good orning zyga-mbp and mborzecki and pstolowski [07:20] hey [07:20] pstolowski: any PRs to look at? [07:20] mvo: hey [07:20] mborzecki: any 'quota'-tagged PRs, except for spread test [07:21] pstolowski: ok, will do [07:22] pstolowski: about https://github.com/snapcore/snapd/pull/10210#discussion_r622967611 the problem i saw there was that if there's any inconsistentcy, that access will panic if i'm reading this right [07:22] PR #10210: cmd/snap, client: snap quotas command [07:22] mborzecki: yes [07:22] mborzecki: we can be paranoid, otoh this data comes from snapd and if it is inconsistent we are in big trouble [07:23] yeah, that's true [07:23] mborzecki: we make sure it is consistent when we manipulate in snapd [07:24] mborzecki: i can add some chceks in a followup [07:33] mvo: ijohnson changed state locking per Samuele's comment, we need to be careful when landing my rest api (i'll update my api for it, but then we need to land them in order) [07:35] PR snapd#10219 opened: interfaces: add a polkit interface [07:35] mvo: https://github.com/snapcore/snapd/pull/10214 is missing some dependencies apparently [07:35] PR #10214: features,servicestate: add experimental.quota-control flag [07:44] mborzecki: oh, ok, let me check [07:45] mborzecki: aha, yes, let me fix this [08:12] pedronis: #10210 has +2, can it land or do you want to take a look as well? [08:12] Bug #10210: Second head doesn't work with Mobility Radeon 9600 M10 [08:12] PR #10210: cmd/snap, client: snap quotas command [08:13] pedronis: hi btw :) [08:28] pstolowski: it seems fine for now, unless you think there is something interesting I should look at? [08:28] pedronis: not really, thanks [08:29] mvo: i may need to ask you for manual merge again (##10210) [08:29] Bug #10210: Second head doesn't work with Mobility Radeon 9600 M10 [08:29] PR #10210: cmd/snap, client: snap quotas command [08:29] pstolowski: what's the status of https://github.com/snapcore/snapd/pull/10208 ? does it need https://github.com/snapcore/snapd/pull/10216 first? [08:29] PR #10208: daemon: implement REST API for quota groups (create / list / get) [08:29] PR #10216: o/servicestate: address comments from previous PR [08:30] pedronis: I commented in 10208, I think it needs the locking fix [08:31] pedronis: yes, because of locking change. my PR will hang atm [08:31] mvo: i fixed that already [08:32] pstolowski: yes, sorry, I was not precise in my wording. for unit tests to work we need https://github.com/snapcore/snapd/pull/10208#discussion_r623705832 the locking changes or the full PR from ian [08:32] PR #10208: daemon: implement REST API for quota groups (create / list / get) [08:32] pstolowski: thanks for updating it of course! [08:33] pedronis: please tell us what you prefer, the review of ians full PR or if we should cherry pick just the locking change or split the locking change as it's own PR. whatever is easiest for you :) [08:34] mvo: I suppose we should cherry pick Ian's first two commits (also the rename/unexporting) into Pawel's PR? [08:34] yeah cherry-picking of Ian's commit into my PR sounds good [08:35] i don't think i need renaming changes in my PR [08:36] pstolowski: cool, please cherry pick :) [08:36] thanks pedronis [08:36] pstolowski: yes, but I think it's saner to cherry-pick a prorer prefix of Ian PR [08:36] pstolowski: so please cherry pick the rename too [08:36] ok [08:36] pstolowski: when you are don I can review your PR [08:36] thanks [08:36] *done [08:48] pedronis: pushed [08:49] pstolowski: thx, will start looking at it now [08:52] mvo: i'm not sure if you saw my earlier message, but #10210 is good to land but may need manual merge [08:52] Bug #10210: Second head doesn't work with Mobility Radeon 9600 M10 [08:52] PR #10210: cmd/snap, client: snap quotas command [09:01] pstolowski: sure, happy to merge it, does 10210 need a pedronis review or is it ok without? [09:01] mvo: he is ok to land it [09:10] pstolowski: I commented on 10208 [09:12] pstolowski: on it [09:15] PR snapd#10210 closed: cmd/snap, client: snap quotas command [09:19] pedronis: thanks, replied to one of your points [09:25] pedronis: re your rootOnly comment, rootOnly: true + userOK: true combination would be ok? [09:26] pstolowski: I don't remember, you need to try [09:27] ok [09:27] pstolowski: not sure why we think GetQuota is awkward? what is awkward is it's error behavior [09:28] *its [09:28] pedronis: yes mainly that, but also it just gets allquotas anyway [09:28] but i'm fine either way [09:29] pedronis: rootOnly + userOK is not allowed, we explicitly reject such combo in canAccess() [09:31] so i guess it needs to be rootOnly for now for both post+get until James' changes land [09:44] pstolowski: snapstate.Get also get all the snaps, except as *json.RawMessage, we could tweak that way if we want [09:44] but most things have a get me one thing only helper [09:47] pstolowski: what might be true is that it has far less use cases thatn snapstate.Get [09:47] pedronis: ok i'll use GetQuota and we can optimize for *json.RawMessage as a followup [09:51] pstolowski: can you stick a TODO or change GetQuota to return an error and not nil? [09:51] ok [09:52] pedronis: state.ErrNoState will be fine? [09:53] pstolowski: no, really some kind ErrQuotaNotFound [09:53] ErrNoState is kind of horrible [10:18] pedronis: updated [10:36] pedronis: hi, snapd isn't considered a required or an essential snap in core20 model? [10:36] neither RequireWithEssentialSnaps() or EssentialSnaps() returns it [11:26] happy Friday folks === pedronis_ is now known as pedronis [11:55] mborzecki: you need to add it yourself if it's not there, there's code like in seed, we maybe need a helper for this [11:55] mborzecki: let me find the code I have in mind [11:56] pedronis: ah, i was confused to not see it, and the 10181 i'm already explicitly listing it in models (like we do in our reference snapcore/models) [11:57] mborzecki: https://github.com/snapcore/snapd/blob/master/seed/seed20.go#L487 [11:57] thx [11:58] pedronis: can you take a look at https://github.com/snapcore/snapd/pull/10181/files#r623782805 ? is this correct or am i talking nonsense here? [11:58] PR #10181: overlord/devicestate: add helper for creating recovery systems at runtime [11:58] about using RequiredWihEssentialSnaps() basically [11:59] mborzecki: why? you do SnapsWithoutEssential below [11:59] you really just need to add snapd itself if it's not there [11:59] the code looks right to me [11:59] ok [11:59] mborzecki: you do this below: https://github.com/snapcore/snapd/pull/10181/files#diff-20edc53689a51b3a9151170b37a75c880cf04d818936018f1dcc20d9540507ccR232 [11:59] PR #10181: overlord/devicestate: add helper for creating recovery systems at runtime [12:01] mborzecki: you can even do it in between the two for loops I suppose, if modelSnaps["snapd"] == nil { ... } [12:01] pedronis: hmm let me double check with the tests i'm adding, i had something missing when i had not added base and snapd snaps to the model i mocked [12:05] pedronis: double checked and the base is included in essential snap, my bad :) [12:06] pstolowski: I +1ed the PR, maybe you could look at addressing your own comments in https://github.com/snapcore/snapd/pull/10214 ? [12:06] PR #10214: features,servicestate: add experimental.quota-groups flag [12:08] pedronis: thanks. yes i can but trying to finish spread test first, so in a bit [12:09] pstolowski: ok, just not sure how busy mvo is [12:09] it would be good to actually have the flag [12:09] if we land the api [12:10] thx [12:23] i wonder if we shouldn't have feature flag checks in other places to handle cases where quotas are created and feature gets disabled... the backend will continue to reconsider quotas from state. but maybe an edge case, and possibly we have this problem with other features [12:26] PR snapd#10220 opened: seed/seedwriter: fail early when system seed directory exists [12:31] PR snapd#10221 opened: snap/snaptest: helper that mocks both the squashfs file and a snap directory [12:39] pstolowski: once you turn it on, it's a bit unclear whether disabling feature on top of creating is useful or annoying [12:40] pstolowski: in the end you have new systemd things on your system [12:40] unless you remove/dismantle them carefully before turning it off [12:41] pstolowski: unless we implemente a sideeffects of turning it off which goes and deletes all slices and rewrites all services before clearing quotas in state [12:41] pedronis: turning it OFF could remove all servicestate and then ensureServices, but that's probably a bit annoying\ [12:52] i hit an internal error in my spread test: "cannot get quota "group-two": missing group "group-two" referenced as the sub-group of group "group-top1" when removing a leaf group [12:53] afterwards 'sanp quotas' returns error as well [12:53] error: cannot get quota groups: cannot get quotas: missing group "group-two" referenced as the sub-group of group "group-top1" [12:53] but maybe ijohnson's followup fixes that [12:54] (and yes i'm fixing these weird looking errors as well) [13:01] PR snapd#10208 closed: daemon: implement REST API for quota groups (create / list / get) [14:01] mvo, pedronis i've updated #10213, should be passing now [14:01] PR #10213: tests: basic spread test for snap quota commands <⛔ Blocked> [14:01] Bug #10213: evolution is crashing if you click cancel when authenticating. [14:02] and it fixes error messages [14:04] pstolowski: thx [14:09] pstolowski: nice, thanks [14:10] mvo: also pushing tweaks to your PR [14:11] pstolowski: amazing, thank you [14:22] pstolowski: pedronis mvo actually sorry I was wrong, the bugfix for removing a sub-group is in https://github.com/snapcore/snapd/pull/10216, it's https://github.com/snapcore/snapd/pull/10216/commits/97dadc54978642ee511ac6168b8e6ce79dfc7171 [14:22] PR #10216: o/servicestate: address comments from previous PR [14:23] so if we land 10216 then we should be good to get the spread test working, though we still would have the problem around needing to restart services, and still would have the problem around removing snaps that were in a quota group at time of removal [14:23] ijohnson: thanks! it looks like 10216 needs a master merge are you on it or shall I do that real quick [14:24] ijohnson: (looks trivial, happy to do/push) [14:25] mvo: sure go for it, but I'm a bit confused what landed that created conflicts 🤔 [14:25] ijohnson: looks like the comment that got moved down [14:25] ijohnson: we landed the first two commits from it via Pawel api's PR [14:26] oh I see [14:26] tbh I think it has been very confusing to land partial commits from other PR's inside other PR's, but this week has been exceptional I suppose [14:26] pstolowski: I reviewed https://github.com/snapcore/snapd/pull/10214 , overall it looks right but the TODO that mvo put there seem a bit confusing, don't think we will unexport UpdateQuota [14:26] PR #10214: features,servicestate: add experimental.quota-groups flag [14:27] pedronis: you reviewd it with the changes I pushed a few minutes ago right? [14:28] pedronis: oh, sorry for wrong comments [14:28] pstolowski: yes [14:28] afaict [14:28] mborzecki: you still need reviews for 10181, yes? [14:30] pstolowski: were any of your comments on 10216 blockers that you want me to address before landing that PR ? [14:31] PR snapd#10222 opened: overlord: support snapctl --halt|--poweroff in gadget install-device [14:32] mvo: ijohnson: mborzecki: last significant bit of "factory mode" code ^ [14:32] ijohnson: it also need a 2nd review 10216, I can try in a little bit [14:32] pedronis: ack should I review that now? was your plan to land that today ? [14:33] ijohnson: I would like to land that today or monday morning [14:33] pedronis: ok I can review it today, just wondering what I should be working on now, it's unclear to me if 10216 needs work or not, it has one +1 [14:34] ijohnson: yea, I'm going to review #10216 [14:34] PR #10216: o/servicestate: address comments from previous PR [14:34] Bug #10216: postfix-to-mailman.py", line 83, in ? import paths ImportError: No module named paths [14:34] ok, thanks, in that case I will review your snapctl pr right now then [14:34] I still need to open two minor "factory mode" PRs as well and land one [14:34] ijohnson: i'm slightly unsure about https://github.com/snapcore/snapd/pull/10216/files#r623691016 and if it can have undesired consequences [14:34] that has +2 but a request for a small change, which I probably will do in a follow up [14:35] ijohnson: but maybe my comment doesn't make sense [14:38] pstolowski: I responded, ptal [14:39] "ptal", didn't know that one ;) [14:39] :-D [14:40] always learning [14:40] +1 [14:44] thanks [14:46] PR snapd#10223 opened: cmd/snap: small tweaks based on previous reviews [14:54] ijohnson: I finished reviewing #10216, not really blockers but it would be good to address the test one there or in a follow-up [14:54] Bug #10216: postfix-to-mailman.py", line 83, in ? import paths ImportError: No module named paths [14:54] PR #10216: o/servicestate: address comments from previous PR [14:54] let me take a look, I'm pretty close to finishing a review of your 10222 [14:54] pedronis: yeah good point I can add a test for that [14:56] PR snapd#10203 closed: image,c/snap: implement prepare-image --customize [14:58] pedronis: pushed [15:19] pedronis: approved 10222 too [15:19] enjoy your long weekend, see you back in a week :) [15:20] * ijohnson doesn't get a long weekend :-/ [15:36] ijohnson: i'm not sure about the state of mvo's snap-quota-full spread test, but it is a separate test so i think my PR can land (and it is focued on cli checks [15:36] pstolowski: yeah [15:37] pstolowski: that test I need to look at again [15:38] i'm going to drop in a moment, is there anything you need from me? [15:41] PR snapd#10184 closed: tests: moving the snaps which are not locally built to the store directory [15:44] mhm i think i broke a test in feature flag PR, fixing [15:45] ah no, actually api tests now need to enable the feature [15:50] updated [15:55] ijohnson: is #10217 needed today as well? [15:55] Bug #10217: ubuntu ignores my second disk on install [15:55] PR #10217: o/servicestate: restart slices + services on modifications [15:56] pstolowski: I'm not sure, I wanted to ask mvo and pedronis about whether we need that for the MVP or not, without that PR, adding a snap to a quota group doesn't take effect unless you manually restart the service [15:57] the followup to that one, 10218 I think is probably more important tbh but there will be lots of conflicts in the two PR's if I separate them [15:57] it needs master merge btw [15:57] yeah was hoping to land 10216 first before updating those other pr's [16:00] ijohnson: sorry, had tons of meeting, what can I help with ? anytihng I can review? [16:07] oh vitality test needs to enable the feature as well, fun [16:09] ijohnson: I commented 10218 where/when I think that call should go/be [16:10] pedronis: thanks [16:16] ijohnson: sorry I have new comment in 10216 [16:16] about the bunch of the new code [16:17] ah-ha well that was the risk about adding more code there [16:24] ijohnson: I also tried to answer your questions in my PR [16:32] 👍️ [16:35] pedronis: oh wait actually we couldn't ever hit the error condition I would be logging about in RemoveQuota as-is, since the first thing we do is get all groups via AllQuotas which does the verification, so the if the parent doesn't reference the child then the whole function dies at step 0 [16:36] * ijohnson is not sure if that is a good thing or not [16:37] it's probably not bad but not great either [16:38] yeah since if there does end up being any inconsistency then we can't do anything with any quota :-/ [16:38] I mean we essentialy run the same code before setting quotas and after, but it might not be the same snapd version doing that [16:39] so I guess we have a few options [16:40] 1) we could relax the checks in RemoveQuota specifically - this will probably need to be done in a followup, it's a bit of work since we might have to change ResolveCrossReferences to be "loose" sometimes [16:41] 2) we could just leave ResolveCrossReferences as is and remove the extra checking code I have in RemoveQuota since those conditions should be impossible to hit now [16:41] ah Set quota doesn't call ResolveCrossReferences [16:41] sorry, I mean RemoveQuota doesn't call ResolveCrossReferences before calling Set("quotas" [16:42] pedronis: in master you are correct, but in my PR RemoveQuota does call that [16:42] ijohnson: I'm in favor of 2) atm, ah ok [16:43] ok then the code becomes a bit simpler [16:55] pedronis: ok, hopefully now 10216 is ready for real, the error handling code was removed since it was impossible to hit those error conditions [16:56] PR snapd#10213 closed: tests: basic spread test for snap quota commands [17:01] PR snapd#10224 opened: snap-seccomp: update syscalls.go list [17:03] pedronis: does your comment in 10222 say that this PR should not land as is or that there will be a followup? sorry, isn't entirely clear to me [17:05] mvo: I would prefer to push a fix there [17:05] but nothing will explode given nothing is using the feature [17:05] not even sure something would explode if using the feature [17:05] it's mostly about snap waiting on things [17:06] I mean snap the command [17:08] pedronis: sure, I marked it blocked just to be sure, just unblock when you feel it's ready [17:08] pedronis: ok, either way is fine, I will review anyway [18:22] PR snapd#10216 closed: o/servicestate: address comments from previous PR [19:22] PR snapd#10225 opened: boot,image: support image.Customizations.BootFlags [19:22] ijohnson: ^ setting boot flags from prepare-image [19:30] pedronis: lgtm [19:30] pedronis: it occurred to me that while working on removing a snap from a quota group in discard-snap, that we should also save what group it was in if the snap was being disabled, so that info can be restored when the snap is enabled again [19:31] I feel like it's probably less of a bug that after disabling a snap the service leaves the quota group upon re-enabling than the current bug which is that a quota group becomes immutable when a snap in that group is disabled/removed so maybe we just figure that out later [19:32] also I rebased #10217 [19:32] PR #10217: o/servicestate: restart slices + services on modifications