[05:07] morning [05:08] Hey hey [05:09] Will you have a moment to look at my reasoning today? [05:09] As soon as the kids go to school we could have a quick call [05:10] zyga: hey [05:10] zyga: sure, ping me [05:10] Also https://github.com/snapcore/snapd/pull/6796 is simple and I have more later :/ [05:10] PR #6796: cmd/snap-update-ns: add no-op load/save current user profile logic [05:51] ok, both kids are handled now [05:52] mborzecki: https://meet.google.com/rug-kzid-ewz?authuser=0 [05:57] zyga: ok, let me get some coffee [05:58] sure, thank you [06:46] PR snapd#6829 closed: devicestate: use deviceCtx in checkGadgetOrKernel [06:47] https://github.com/zyga/hello-cuda [06:53] mvo: hi, #6833 is the next one in the series, is actually the refactoring about creating models in tests I started Friday (that derailed me a bit) [06:53] PR #6833: many: introduce assertstest.SigningAccounts and AddMany test helpers [06:58] mvo: I'm going to merge 6776, it will be turned into calls to remodel ctx methods soon [06:59] pedronis: ok, I will check 6833 hopefully this morning === pstolowski|afk is now known as pstolowski [06:59] morning [06:59] PR snapd#6776 closed: devicestate: set "new-model" on the remodel change [07:00] pedronis: about 6776> sounds good, will you work on the rmeodel ctx method transition or shall I? [07:00] pedronis: having it will make the kernel-remodel PR much smaller, so thats nice [07:02] mvo: we fixed it [07:02] mvo: wooooot [07:02] the bug is real, it's easy to fix and it's well understood now [07:02] zyga: \o/ [07:02] zyga: is it a kernel bug? [07:02] no, my bug :) [07:03] but very elusive, you'll see soon [07:03] * zyga is euphoric [07:04] I'll chop the fixes up and start sending it right away [07:04] kenvandine: I've fixed a massive bug relating to layouts, content and magic-it-is-broken-bugs [07:04] that explains a lot of things not working :D [07:04] zyga: is this a 2.39.1? [07:04] mvo: yes, we should [07:04] it will help everyone on the desktop [07:04] mvo: the fix is small [07:04] so should be fine, I'll start preparing branches [07:04] mvo: it's really this: [07:05] when we mount tmpfs we don't set propagation so it stays private [07:05] that's all :) [07:05] this applies to layout tmpfs and mimic tmpfs [07:05] mborzecki and me just spent last hour debugging it in a live pair programming session [07:05] mborzecki thank you so much for helping :) [07:05] mvo: hey [07:06] zyga: not sure i helped much though :) [07:06] mvo: so the nvidia state is not only great but also working reliably now :) [07:06] pstolowski: hey to you too :) [07:06] o/ [07:08] zyga: cool [07:08] hey mborzecki and pstolowski [07:08] zyga: can we spread test this somehow? [07:09] Yes [07:10] zyga: excellent [07:10] mvo: sorry, I will work on the remodel methods translation, my next task is actuall implementing brand store switch for real [07:10] pedronis: excellent, thanks [07:10] pedronis: just wanted to double check if I can help you in this specific task [07:10] mvo: though with some loopholes because of per user mount namespaces [07:10] mvo: but yes; we can test this [07:11] pstolowski: I reviewed 6793 - do you think a unit test cmd_debug_timings_test.go for a followup would be hard? just something simple? [07:11] zyga: cool! [07:12] mvo: we need a follow up anyway because of the discussion about having an artificial line to make the ensure bits more like the change bits [07:12] mvo: yes, i was thinking about that, some rudimentgary unit tests would be good to have, will do in a followup [07:13] pstolowski: but with the branch we can gather data on the pis which would be great [07:14] yes [07:19] pstolowski: great! I +1 as not having this test is not a regression and we can fix in a followup :) [07:19] ty [07:22] Saviq: hey [07:22] Saviq: I think I also fixed the bug that was affecting mir [07:22] the one you asked me about a few weeks ago [07:23] this is the bug of the month for me :) [07:23] * zyga reports bugs to begin working on regression tests to begin chopping fixes into patches [07:23] zyga: got a bug#? :) [07:23] Saviq: I'm reporting it now [07:24] Saviq: but this is when you wanted to share mir backend drivers with mir and then share mir stack + backend driver to apps [07:24] I know why this is broken now [07:24] Oh [07:24] yeah :) [07:24] it's getting fixed today [07:24] That'd be awesome :) [07:24] this single bug explains LOADS of mysteriously incorrect behavior [07:26] kenvandine: ^ this is the bug that was breaking themes and gnome platform snap and all kinds of "connected but not connected" bugs [07:26] mvo: ^ you can see why I'm so happy now [07:28] zyga: \o/ [07:35] zyga: great, is it easy to add tests about it? [07:36] pedronis: not every easy but 100% doable, the bug is extremely elusive and two of the components of the bug would make a simple spread test not catch it [07:36] pedronis: I'm describing the bug in detail [07:36] pedronis: and I know how to write a spread test for it [07:36] pedronis: none of our current tests doing content or layout would capture it [07:36] ok, just wondering because we didn't catch it so far [07:36] it's a tricky bastard :) [07:36] but good if we can have a spread test [07:36] yes, that's 2nd step after reporting the bug :) [07:36] write a test and show it fail :) [07:37] mvo: I'm going to cleanup managers_test.go a little bit as a drive-by, mostly ms -> s, and using testutil AddCleanup [07:38] pedronis: \o/ [07:40] PR snapd#6844 opened: interfaces: special-case "snapd" in sanitizeSlotReservedForOS* helpers <⚠ Critical> [07:41] mvo, pedronis ^ so this fixes hotplug on core18+snapd, tested manually on pi3 [07:41] and i'll prep proper followup (touching 80+ files) as discussed yesterday [07:42] pstolowski: nice [07:42] pstolowski: this is also 2.39.1 I suppose? [07:42] mvo: yes [07:42] ta [07:44] mborzecki, mvo, pedronis, Saviq: https://bugs.launchpad.net/snapd/+bug/1828354 [07:44] Bug #1828354: mount event propagation on snapd-mounted tmpfs is incorrect [07:44] I will start writing the spread test [07:44] in case I get hit by a bus mborzecki knows how to fix it too :) [07:44] hahah [07:45] let me know if the bug description is unclear [07:46] i'll also think about spread test for core18+snapd & hotplug, need to talk to Sergio about that [07:54] zyga: any news on https://bugs.launchpad.net/snapd/+bug/1819875 ? [07:54] Bug #1819875: base core16->core18 migration [07:56] Saviq: it's stalled, I know what to do, will attack it after Lyon [07:56] Saviq: the initial patch was ANACked, I started working on v2 and have close-but-not-quite code for that [07:56] mborzecki, mvo: I also reported https://bugs.launchpad.net/snapd/+bug/1828357 [07:56] Bug #1828357: snap-update-ns incorrectly sorts mount profile entries [07:57] Saviq: there are several more bugs in core->core18 migration [07:57] Saviq: one is that snapd tools go stale [07:58] Saviq: there are several other bugs affecting core18, I don't think they are all reported [07:58] so "not yet" :) [07:59] no, apart from Lyon-driven work it is close to the top of my queue [07:59] (no as in: "no, not yet") [08:02] mborzecki: replied to your comment in https://github.com/snapcore/snapd/pull/6793 ; can you re-review? [08:02] PR #6793: cmd/snap: support for --ensure argument for snap debug timings [08:03] pstolowski: thanks, it was more like an open question, nothing to be done in that PR anyway [08:06] pedronis: should I add switching gadgets (with the caveat that we cannot do asset upadates yet) or do you think thats premature? [08:07] mvo: that is more delicate, we really need to do the right thing with GadgetInfo [08:07] it's used much more than KernelInfo [08:09] * mvo nods [08:12] a second review of 6835 would be great - its small [08:13] mborzecki: I tried to answer your question, bit confused by the 2nd because that's the central point of the PR, though it might be sutble [08:15] 6751 has two +1 but I added the unit test - so a second review from someone who is not me would be great [08:15] its pretty small and should be simple to review [08:16] mborzecki: I changed the description of 6822, made that should makes thing clear. It also go outdated at some point. [08:17] s/made/maybe/ [08:17] mvo: I can look in a little bit [08:20] pedronis: what i meant in the 2nd question is cosmetic mostly, whether to embed *DeviceManger or have it as a field like `manager *DeviceManger` [08:21] mborzecki: ah, I didn't understand that way [08:21] pedronis: sorry, should have made it more clear in the comment [08:23] mborzecki: no, you are right, the wrapper implements all the interface methods [08:23] so it could be a field [08:23] this changed a bit over the history of the branch [08:24] or I could remove .DeviceManager [08:24] and keep it this way [08:24] but as it is is all a bit silly [08:25] pedronis: i mean it's not a problem, we could clean it up later on, or keep it like this, either way the code works [08:26] pstolowski: o/ [08:26] mborzecki: yea, for context, at some point DeviceManager had Device [08:26] but now that is hidden [08:26] hey Chipaca [08:27] pstolowski: hiya. I've got a question about hooks :-) [08:27] * pstolowski is hiding [08:27] shoot [08:28] pstolowski: I'm getting 'cannot find hook' when trying to install (of a hook i've added), from snap-exec, but in snap-exec I print info.Hooks and the hook is right there, but the snap-exec does not seem to be the one from /usr/lib/snapd … is it always from core or sth? [08:29] Chipaca: what hook? [08:29] pstolowski: I've just answered myself by doing a bind-mount, and yes it's the one from core [08:29] (i got a different panic now \o/ ) [08:30] pstolowski: check-health [08:32] Chipaca: you'll need to whitelist a new hook in supportedHooks (and possibly in one other place, but not sure from top of my head). but you probably already found that out [08:33] pstolowski: yep :-) [08:33] pstolowski: I just got the whole thing working \o/ [08:34] nice! \o/ [08:37] PR snapd#6796 closed: cmd/snap-update-ns: add no-op load/save current user profile logic [08:38] pedronis: can we talk about storage for a bit? [08:39] pedronis: actually, i'll just write out my question and you answer when you have time [08:41] pedronis: some decisions need to be done on 'edge', ie when transitioning from good to bad or vice versa, and I wonder if what we actually want is a log of all the runs (or the last N) [08:41] PR snapd#6845 opened: cmd/snap-update-ns: move apply{Profile,{User,System}Fstab} to same file [08:42] pedronis: so the question is: should I make it an in-state thing that keeps the current result and the previous one, or the current result and the previous status, or should it be on disk with a log of everything? [08:42] pstolowski: asked a question in https://github.com/snapcore/snapd/pull/6844 [08:42] PR #6844: interfaces: special-case "snapd" in sanitizeSlotReservedForOS* helpers <⚠ Critical> [08:43] pedronis: are actions going to be taken in-hook-handler, looking at current and previous result only, or are they going to be run from outside and look at all history, or...? [08:44] Chipaca: which decisions need to be made on edge ? I don't remeber that [08:44] pedronis: when it was ok but suddenly isn't [08:45] pedronis: warning the user? alerting the backend? [08:45] not sure (it's not part of this round of work) [08:45] but there was some action we wanted to do when a snap had been fine and then started being broken [08:46] we want to check more often when is broken [08:46] but that itsn't about the edge [08:46] we do need to know what the status was before a refresh and after [08:46] Chipaca: atm I would expect to take "actions" from check-rerefresh [08:47] Chipaca: I mean, we always discussed needing to run the hook both at the start and during a refresh [08:48] pedronis: and then take actions based on the difference, right? [08:48] possibly yes [08:48] ie bad->good vs good->bad [08:48] but that info could be kept on the change/task in the change [08:49] anyway if you think we should keep the last two different values [08:49] that's fine [08:49] I would put this in the state [08:49] ok [08:50] Chipaca: we need to remember for which revision they are though? though the idea is that health itself is not for a revision [08:50] brb, coffee [08:52] Chipaca: I mean, is not clear to me whether we should just stick the last value in the change , or we keep always two, but we haven't dug deep into all the things we might want to do with this [08:54] pedronis: I'll make it a map[instance]HealthState; we can always have HealthState have a Previous HealthState [08:55] pedronis: I'll add Revision to HealthState: like the timestamp, it's not that the health is 'for' a timestamp or revision, but that when the health was this, these were the timestamp and revision [08:55] will help with debugging / auditing / developing at the least [08:56] yes [08:56] pedronis: initial PR will just run the hook at the end of install [08:56] fwiw [08:56] yea, that's fine [08:57] Chipaca: you mean install/refresh, or only install? [08:57] pedronis: doInstall :-) install/refresh/try/revert [08:57] ok [08:58] zyga: replied [08:58] pstolowski: thanks, looking [08:59] pstolowski: +1 [09:00] zyga: ty [09:00] zyga: i'm preparing a followup to remove these helpers from all interfaces. that means reservedForOS in common.go will go away [09:01] pstolowski: why? [09:01] zyga: because base declaration do the same [09:01] pstolowski: but base declaration is *not* considered when doing snap install --dangerous [09:01] or am I mistaken? [09:01] zyga: oh [09:01] it will give people false impressions when developing snaps [09:02] perhaps we should run the base policy checker but just print a warning on install when --dangerous is used [09:02] zyga: fair point if this is the case [09:02] then I think this is sensible [09:02] and far less code too [09:12] pedronis: do you want to tweak 6822 further or can it go in ? it has two +1 but there is this discussion about initialRegistrationContext [09:13] PR snapd#6846 opened: cmd/snap-confine: unshare per-user mount ns once [09:13] mborzecki: https://github.com/snapcore/snapd/pull/6846 [09:13] PR #6846: cmd/snap-confine: unshare per-user mount ns once [09:14] mvo: I can change it a bit in a follow up [09:14] it can go in [09:15] PR snapd#6822 closed: overlord/devicestate: introduce registrationContext [09:15] pstolowski: why is hookstate.Context's SnapRevision "unset", when the revision is something like -4? [09:15] pedronis: it can I think - I'm inclined to merge 6833 as well or do you think this needs further reviews (all tests/test-helpers) [09:16] ooohhh hooksetup [09:16] * Chipaca looks into it [09:16] we never set the revision I think [09:16] actually [09:16] there was a lot of back and forth on that [09:17] Chipaca: this predates the time i started working on snapd.. i think Gustavo and Kyle worked on this, i don't know the background [09:18] mvo: I don't know about 6833 , it's all tests but it's big [09:19] also maybe people have opinions on names [09:19] I don't know [09:21] pstolowski: I answered your question in 6822 [09:22] ty [09:23] pedronis: I think its a thing of beauty so maybe someone more reviews are actually good :) [09:24] mvo: updated #6844 [09:24] PR #6844: interfaces: special-case "snapd" in sanitizeSlotReservedForOS* helpers <⚠ Critical> [09:24] pstolowski: \o/ [09:24] mvo: replied on that unshare bug [09:25] let me know if you want me to pursue the test, I personally think it's not worth it [09:25] Bug #1828374 opened: Unable to install qabro [09:26] zyga: thank you [09:26] mvo: we could write a test that would use trace but it would be super tricky because this code only runs when *not* root but we'd need to run snap-confine via sudo + strace and then somehow drop back to user while running under strace already [09:27] CE has reported a bug related to seeding [09:27] https://bugs.launchpad.net/snappy/+bug/1828374 [09:27] Bug #1828374: Unable to install qabro [09:27] there's one change, seeding, it is stuck for two days [09:27] the Doing task is Doing 2 days ago, at 04:16 EDT - Ensure prerequisites for "gnome-calculator" are available [09:27] is this the ordering bug [09:27] zyga: hm,hm and there is nothing else with propagation (or lakc thereof) that is easy(ish) to observe? [09:27] where bases must be before snaps? [09:27] mvo: correct [09:28] zyga: is this 2.39 material? [09:28] mvo: yes, but only because I would like to test this together with the 2nd bug fix and not without [09:28] mvo: and if it passes and nobody spots any hole in reviews it is a clear bug to get rid of [09:29] zyga: ok, we should probably have a quick call on it if it goes into 2.39 without some sort of test [09:29] mvo: what would we discuss? [09:29] zyga: a bit later though, I will wait for the second PR [09:29] mvo: ok [09:29] mvo: I can write the allocation test [09:29] zyga: not sure thats worth it [09:29] it'd be a bit fugly but if you strognly one one [09:29] zyga: maybe I'm too cautious [09:29] zyga: yes, that seems and ordering bug [09:29] or missing base [09:29] pedronis: is the ordering documented somewhere that I can point CE to? [09:30] zyga: anyway, please focus on the second PR and then we can talk a bit more [09:30] mvo: ok [09:30] doing that now [09:30] zyga: thank you! [09:30] zyga: we really shouldn't have ordering bugs actually, but missing stuff is an issue [09:31] pedronis: I agree, I would add that snapd should bail if it detects a missing base and not just stall for dayts [09:32] the more focuses we are on the big things the more is likely we can do something about the small ones [09:34] pedronis: yeah, agreed [09:34] Mornings [09:34] mvo, pedronis_: Do you have a moment for a quick call? [09:35] niemeyer: yes, can be there in 1min [09:36] Sweet [09:36] Let's use the standup link [09:45] pstolowski: question about something I'm finding hard to follow: I'm getting my hook run even for snaps that don't have the hook. Where does the decision not to run hooks that aren't present happen? the (to me) obvious place, hookmgr's runHook, knows there isn't a hook and carries on [09:46] bah [09:46] it's not the (non-existent) hook that's run, it's the handler [09:46] the Done() thing [09:47] pstolowski: so i change my question: how can the Done handler know whether the hook was run? [09:54] pstolowski: or, why doesn't runHook just return nil when it knows there isn't a hook to run? [09:55] Chipaca: it seems it's always called even if hook doesn't exist. and the only case when it's not run is when error occurs and IgnoreError is not set [09:56] Failed for "golang.org/x/crypto/cast5" (failed to clone repo): exit status 128 [09:56] Chipaca: i think we have never relied on Done() so far [09:56] hmm ubuntu-image is full of surprises [09:56] calculated = ceil(farthest_offset / 1024 + 17) * 1024 [09:56] pstolowski: grmbl [09:56] that's how image size is calculated befire it's actually built [09:56] there's more from godbus :/ [09:56] pedronis: looking at blame maybe i should've asked you the above :-) or maybe kyrofa [09:56] Chipaca: again.. this is old code, almost untouched since initial impl [09:57] Chipaca: what you're saying sounds reasonable to do imho [09:57] and a TODO note too: # TODO: Hard-codes last 34 512-byte sectors for backup GPT, empirically derived from sgdisk behavior. [10:27] * pstolowski lunch [10:27] * zyga is hungry too [10:35] zyga: great! [10:36] * kenvandine high fives zyga [10:50] kenvandine: I'm totally crazy happy about this bug finally coming out of the shadows :) [10:51] kenvandine: this is https://bugs.launchpad.net/snapd/+bug/1828354 if you want to track [10:51] Bug #1828354: mount event propagation on snapd-mounted tmpfs is incorrect [10:51] I'll fix it today [10:51] but first, I need to take the dog out, he's looking at me and asks for it :) [11:08] PR snapd#6845 closed: cmd/snap-update-ns: move apply{Profile,{User,System}Fstab} to same file [11:16] PR snapd#6847 opened: cmd/snap-update-ns: make apply{User,System}Fstab identical [11:25] PR snapd#6848 opened: First pass at healthstate (no tests yet) [11:25] pedronis: ^^ draft PR (working on adding tests) [11:25] pedronis: will probably drop the IsCtrl thing until later, as it's not essential and clutters the pr [11:25] Chipaca: thx [11:26] pedronis: but thought you might want an early look [11:30] do we have some code already that escapes runes into \x.. hex encoding like systemd-escape? [11:30] Chipaca: is SnapRevision actually set? doesn't need to be set in CheckHealth ? [11:30] mborzecki: systemd.Escape ? [11:31] mborzecki: systemd.EscapeUnitNamePath [11:31] pedronis: it is set [11:31] Chipaca: thanks! [11:31] pedronis: it's always "unset" for local revisions [11:31] pedronis: it is set for store revisions [11:31] pedronis: so that's probably good enough [11:31] Chipaca: ok, bit puzzled by that [11:31] me three [11:31] :) [11:32] I don't remember it being that way [11:32] woah, this "draft" feature is cool [11:32] pedronis: you can try it locally, becaues Done is always called so health is always set to something [11:32] * mvo hugs Chipaca for learning something new [11:32] Chipaca: the other question, not immediate, if it will always want to write directly to the health state, or sometimes we'll need to keep the health pending somewhere [11:32] * Chipaca hugs mvo for the attitude [11:32] but we should have ways to add that indirection if needed [11:33] pedronis: yep [11:41] mvo: 6751 looks good but I have a comment about lack of comments :) [11:42] pedronis: thanks, will fix [11:58] Download snap "motion-ogra" (16) from channel "edge" 0% 17.2kB/s 63.3m [11:58] hmpf ! [12:06] off to pick up the kids [12:13] back from lunch and walk [12:16] mvo: can you revise your review on https://github.com/snapcore/snapd/pull/6751 [12:16] PR #6751: overlord/snapstate: perform hard refresh check [12:23] ogra: heya! Youve got a pi 3b+ yeah? [12:26] zyga: sure, checking [12:26] mvo: I think the PR needs a comment [12:26] but perhaps one can be added via github directly [12:26] cwayne, who hasnt ? (its the default you get since 1.5y if you order one :P ) [12:28] zyga: yeah, I have a meeting now will see when I can add it [12:28]