[06:13] PR snapd#9586 opened: update-pot: include file locations in translation template, and extract strings from desktop files [07:09] PR snapd#9587 opened: snap: fix failing unittest for quantity.FormatDuration() <⚠ Critical> [07:14] morning [07:14] Do we have a unit test that assumes all months have 30 days? [07:15] good morning [07:16] mvo, with samuele away for the part of the week, 9546 won't make official progress but perhaps it could be reviewed by others to spot any mistakes or weirdness [07:16] It looks like the TestSnapshotImportHappy test was written to start failing on November 1st [07:16] I suspect the changes are acceptable by him now, he just ran out of review time on Friday [07:17] hey jamesh :) [07:17] hi zyga [07:19] zyga: jamesh: hey [07:22] good morning mborzecki and zyga [07:23] mvo: hey [07:23] and good afternoon jamesh :) [07:23] zyga: I will try to have a look [07:23] jamesh: I pushed a PR to fix the failing unittest [07:23] jamesh, mborzecki PR#9587 fwiw [07:24] mvo, thanks, no rush but I think making that landable will allow the rest to be reviewed in parallel [07:26] mvo, https://github.com/snapcore/snapd/pull/9204 is green and has +2 but should be acked by someone the last time [07:26] PR #9204: sandbox: track applications unconditionally [07:26] jamesh, I think you will like that ^ [07:26] it makes snap identification work on Fedora and elsewhere [07:26] where we have cgroupv2 [07:28] mvo: might it be better to remove the check entirely? Your fix looks like it will only help up until March [07:29] jamesh: not opposed to remove it, it's the wrong layer for this test anyway [07:34] mborzecki: how do you feel about merging 9582? [07:51] mvo: sounds good to me, i can open a followup with test name tweak [08:02] morning [08:02] good morning pstolowski [08:03] hey pstolowski :) [08:05] o/ [08:05] mvo: if you don't mind i'll push a patch dropping the check competely to #9587 [08:05] PR #9587: snap: fix failing unittest for quantity.FormatDuration() <⚠ Critical> [08:05] mborzecki: please do [08:14] PR snapd#9588 opened: dirs: add "gentoo" to altDirDistros [08:19] mborzecki, https://github.com/snapcore/snapd/pull/9588#pullrequestreview-521409043 [08:19] PR #9588: dirs: add "gentoo" to altDirDistros [08:19] FYI [08:24] PR snapd#9582 closed: gadget/quantity: introduce a new package that captures quantities [08:34] PR snapd#9589 opened: gadget/quantity: tweak test name [08:54] PR snapd#9587 closed: snap: fix failing unittest for quantity.FormatDuration() <⚠ Critical> [09:24] * dot-tobias says hi [09:41] can someone check 9560 please? needs a second review, maybe mborzecki ? [09:42] mvo: i have it open, just checkign some things that ondra raised [09:43] mborzecki: sure, thank you! no rush [09:45] zyga: nice, 9204 is green now \o/ [09:54] PR snapd#9589 closed: gadget/quantity: tweak test name [10:08] Heya [10:08] Good morning all [10:08] pstolowski: Around? [10:09] niemeyer: hey, yes [10:09] pstolowski: Do you have a moment to explain the task-id/change-id presence in the timing module? [10:11] pstolowski: Even ignoring the encapsulation concern (timing knows about details of tasks/changes usage of itself), it seems curious that this logic takes effect precisely when timings are empty [10:18] niemeyer: yes. 1 - we enrich the data returned to snap debug timings with doing/undoing times obtained from tasks, and we combine this data when queried; 2 - for change-id, we want to correlate a change with ensure loop that triggered it [10:19] pstolowski: Yeah, I understand the idea of tagging. Seems sound. I don't understand the idea of preserving a timing that has no timings. [10:22] niemeyer: i don't think this is the case - see func (t *Timings) Save(s GetSaver) line 140 & 141, and flatten() line 85 [10:23] if len(t.timings) == 0 && !isEnsureWithChange { [10:23] return nil [10:23] } [10:25] pstolowski: The only use cases I can identify are clearly being used only to prevent wiping out empty timings === mup_ is now known as mup [10:29] niemeyer: right, i see what you mean, i need to think what was the driver for this change [10:29] pstolowski: If it's irrelevant, that's a nice cleanup. Besides simplifying the logic and making it less obscure, it also takes away the only reason why there's breakage of encapsulation there. [10:31] pstolowski: There's also a potential change in API that would reduce the API surface of the module. But that's a more involved improvement and that isn't really fixing any clear structural issues. [10:35] niemeyer: so related PR is https://github.com/snapcore/snapd/pull/6904 and second paragraph of the description summarizes the reason for keeping empty timing in this case; there was a small followup https://github.com/snapcore/snapd/pull/6921 where I unfortunately dropped the comment that was useful [10:35] PR #6904: timings: always store ensure timings as long as they have an associated change [10:35] PR #6921: timings: tweak the conditional for ensure timings [10:39] pstolowski: Ah, it's nice to have the background, thanks. It still seems suboptimal, though.. you have two pieces of logic inside the module fighting with each other, and in a pretty convoluted manner, requiring knowledge of completely unrelated things. [10:39] pstolowski: That is, [10:40] pstolowski: There's one thing that removes fast timings, one thing that removes top-level empty timings, and one thing that prevents both of these from working. [10:40] pstolowski: If top-level timings are relevant even if they are empty, then we might just keep them around. [10:41] pstolowski: If fast timings are still relevant because people might search for them, we might just keep them around as well. [10:41] pstolowski: Or reduce the threshold until it's more honest [10:42] Either way, thanks for the background. That's indeed what I was looking for and now I know how to proceed with the skeleton here. This is not super important for snapd, so I'll let you decide whether/when to look into it. [10:43] niemeyer: yes, i see how it is convoluted. the only concern is not to exhaust our timings "buffer" in the state, we want to keep a limited number of them, and we want only those that are time-consuming, so we don't want empty timings there in most cases [10:45] pstolowski: Sounds like the code disagrees. You've made a decision based on UX to actually have non-time consuming things around to satisfy user searches. [10:47] pstolowski: Proper encapsulation would try to understand that requirement and design for it, instead of teaching the module to do guess work based on deep knowledge of things far away. [10:52] niemeyer: no disagreement. in the defence of that, this was initially all about task/changes (and imported state). a year later it was separated with https://github.com/snapcore/snapd/pull/8283 [10:52] PR #8283: overlord,timings,daemon: separate timings from overlord/state [10:56] pstolowski: What I see there is Samuele working exactly on the direction being suggested here, which is clean up the encapsulation [10:56] unbelievable, it's 2 years since timings were introduced, time flies... [10:56] pstolowski: This is a top-level package.. [10:56] Yeah, timing flies [10:57] niemeyer: yes.. although the driver had a different angle as mentioned on mattermost... we wanted to not import state package elsewhere [10:57] pstolowski: Not importing the state package only has a meaning if you're not marrying with its internals :) [10:58] yes sure [11:31] mborzecki: 9577 looks good to me now, wdyt? [11:37] mborzecki: I fixed one missing test update in 9560 and then I think this can land :) thanks for the review! [11:38] mvo: i've done the followup changes, but having some trouble with mocking sfdisk properly [12:01] mvo: i've a spread test for download timeout, slightly tricky stuff [12:01] see #9590 [12:01] PR #9590: tests: download timeout spread test [12:02] pstolowski: nice [12:04] pstolowski: just looked, really cool [12:05] mvo: let's see if it's not flaky or something.. i only run it on 20.04 [12:05] PR snapd#9590 opened: tests: download timeout spread test [12:45] PR snapd#9560 closed: gadget/many: drop usage of gpt attr 59 for indicating creation of partitions [12:45] PR snapd#9591 opened: gadget, gadget/install: move helpers to install package, refactor unit tests [12:45] mvo: ^^ [13:34] mvo: pushed some changes to #9577 [13:34] PR #9577: many: seal a fallback object to the recovery boot chain [13:57] mborzecki: \o/ thank you [14:38] mvo: I also think we need https://github.com/snapcore/snapd/pull/9541 for 2.48, but we are still blocked on moving forward with that :-/ [14:38] PR #9541: osutil/disks: re-implement partition searching for disk w/ non-adjacent parts [14:39] ijohnson: let's raise it again today [14:39] sounds good [14:39] * mvo puts it on the meeting agenda [15:09] * cachio lunch [15:14] PR snapcraft#3340 closed: include SNAPCRAFT_STAGE in XDG_DATA_DIRS, needed to find Gir files built by other parts [15:15] Issue core20#94 opened: fix pi4 names for wlan0 and eth0 [15:47] need to drive the kids a bit [15:58] mvo: submitted review for 9577 [15:59] ijohnson: thanks! [15:59] np [15:59] I will work on my followup to use that logic in snap-bootstrap today as we discussed [16:02] ta [16:15] quick errand, bbiab [16:55] cachio: I see failures like: "2020-11-02 16:00:35 Cannot allocate google-nested:ubuntu-18.04-64: cannot find any Google image matching "ubuntu-1804-64-virt-enabled" on project "computeengine" or "ubuntu-os-cloud"" in pr 9591, is there some gce issue? [16:55] PR #9591: gadget, gadget/install: move helpers to install package, refactor unit tests [16:55] mvo: cachio I think the issue is that there are too many images in our gce project, usually cachio just goes in and clears out some old images and it works again [16:55] mvo, let me cehck, I think I need to clean up [16:56] ijohnson: oh, right :( [16:56] because gce api is silly and still doesn't know how to return more than X images if there are more than X images in the project [16:57] ijohnson, well, they have pagination to support taht [16:57] but spread does not support pagination [16:58] ijohnson: I noticed two small issues around 9577 https://github.com/snapcore/snapd/pull/9577#discussion_r516113171 and https://github.com/snapcore/snapd/pull/9577#discussion_r516062913 otherwise this is good to go [16:58] PR #9577: many: seal a fallback object to the recovery boot chain [16:58] ah I see [16:58] mvo: looking now [16:58] ijohnson: \o/ [16:58] ijohnson: spread> there are two draft PRs for spread to support pagination but both not quite get it right :/ [16:59] ah I see [16:59] I thought this was gce's fault [17:00] mvo, do you want to re-trigger [17:00] mvo, already cleaned up [17:01] mvo: re 9577, do you mean in `TestMakeBootable20RunModeSealKeyErr` that ResealKeys (or SealKeys? I forget which one) is never actually called? [17:01] re [17:06] ijohnson: sorry, I mean in https://github.com/snapcore/snapd/pull/9577#discussion_r516113171 that there is no check like "c.Assert(sealKeysCalls, Equals, 2)" (or 1) [17:06] PR #9577: many: seal a fallback object to the recovery boot chain [17:06] mvo: ah yes you are right that is true [17:06] that is less of an issue than I thought you were trying to say :-) [17:06] ijohnson: it also already fails after the first call with an error so it will never be true in the switch [17:07] ijohnson: it also means we don't actually test the error transmission from the second call :) [17:07] ijohnson: it's probably not much of an issue, I agreee [17:12] * mvo gets dinner [17:39] hi zyga, is confinement broken on Fedora 33? [17:40] https://paste.centos.org/view/e5b46b5b === ijohnson is now known as ijohnson|lunch === ijohnson|lunch is now known as ijohnson