| mup | PR snapd#9586 opened: update-pot: include file locations in translation template, and extract strings from desktop files <Created by jhenstridge> <https://github.com/snapcore/snapd/pull/9586> | 06:13 |
|---|---|---|
| mup | PR snapd#9587 opened: snap: fix failing unittest for quantity.FormatDuration() <⚠ Critical> <Created by mvo5> <https://github.com/snapcore/snapd/pull/9587> | 07:09 |
| mborzecki | morning | 07:14 |
| jamesh | Do we have a unit test that assumes all months have 30 days? | 07:14 |
| zyga | good morning | 07:15 |
| zyga | 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 |
| jamesh | It looks like the TestSnapshotImportHappy test was written to start failing on November 1st | 07:16 |
| zyga | I suspect the changes are acceptable by him now, he just ran out of review time on Friday | 07:16 |
| zyga | hey jamesh :) | 07:17 |
| jamesh | hi zyga | 07:17 |
| mborzecki | zyga: jamesh: hey | 07:19 |
| mvo | good morning mborzecki and zyga | 07:22 |
| mborzecki | mvo: hey | 07:23 |
| mvo | and good afternoon jamesh :) | 07:23 |
| mvo | zyga: I will try to have a look | 07:23 |
| mvo | jamesh: I pushed a PR to fix the failing unittest | 07:23 |
| mvo | jamesh, mborzecki PR#9587 fwiw | 07:23 |
| zyga | mvo, thanks, no rush but I think making that landable will allow the rest to be reviewed in parallel | 07:24 |
| zyga | mvo, https://github.com/snapcore/snapd/pull/9204 is green and has +2 but should be acked by someone the last time | 07:26 |
| mup | PR #9204: sandbox: track applications unconditionally <Created by zyga> <https://github.com/snapcore/snapd/pull/9204> | 07:26 |
| zyga | jamesh, I think you will like that ^ | 07:26 |
| zyga | it makes snap identification work on Fedora and elsewhere | 07:26 |
| zyga | where we have cgroupv2 | 07:26 |
| jamesh | mvo: might it be better to remove the check entirely? Your fix looks like it will only help up until March | 07:28 |
| mvo | jamesh: not opposed to remove it, it's the wrong layer for this test anyway | 07:29 |
| mvo | mborzecki: how do you feel about merging 9582? | 07:34 |
| mborzecki | mvo: sounds good to me, i can open a followup with test name tweak | 07:51 |
| pstolowski | morning | 08:02 |
| mvo | good morning pstolowski | 08:02 |
| zyga | hey pstolowski :) | 08:03 |
| pstolowski | o/ | 08:05 |
| mborzecki | mvo: if you don't mind i'll push a patch dropping the check competely to #9587 | 08:05 |
| mup | PR #9587: snap: fix failing unittest for quantity.FormatDuration() <Skip spread> <⚠ Critical> <Created by mvo5> <https://github.com/snapcore/snapd/pull/9587> | 08:05 |
| mvo | mborzecki: please do | 08:05 |
| mup | PR snapd#9588 opened: dirs: add "gentoo" to altDirDistros <Created by zmedico> <https://github.com/snapcore/snapd/pull/9588> | 08:14 |
| zyga | mborzecki, https://github.com/snapcore/snapd/pull/9588#pullrequestreview-521409043 | 08:19 |
| mup | PR #9588: dirs: add "gentoo" to altDirDistros <Created by zmedico> <https://github.com/snapcore/snapd/pull/9588> | 08:19 |
| zyga | FYI | 08:19 |
| mup | PR snapd#9582 closed: gadget/quantity: introduce a new package that captures quantities <Simple 😃> <Created by bboozzoo> <Merged by bboozzoo> <https://github.com/snapcore/snapd/pull/9582> | 08:24 |
| mup | PR snapd#9589 opened: gadget/quantity: tweak test name <Simple 😃> <Skip spread> <Created by bboozzoo> <https://github.com/snapcore/snapd/pull/9589> | 08:34 |
| mup | PR snapd#9587 closed: snap: fix failing unittest for quantity.FormatDuration() <Skip spread> <⚠ Critical> <Created by mvo5> <Merged by bboozzoo> <https://github.com/snapcore/snapd/pull/9587> | 08:54 |
| * dot-tobias says hi | 09:24 | |
| mvo | can someone check 9560 please? needs a second review, maybe mborzecki ? | 09:41 |
| mborzecki | mvo: i have it open, just checkign some things that ondra raised | 09:42 |
| mvo | mborzecki: sure, thank you! no rush | 09:43 |
| mvo | zyga: nice, 9204 is green now \o/ | 09:45 |
| mup | PR snapd#9589 closed: gadget/quantity: tweak test name <Simple 😃> <Skip spread> <Created by bboozzoo> <Merged by mvo5> <https://github.com/snapcore/snapd/pull/9589> | 09:54 |
| niemeyer | Heya | 10:08 |
| niemeyer | Good morning all | 10:08 |
| niemeyer | pstolowski: Around? | 10:08 |
| pstolowski | niemeyer: hey, yes | 10:09 |
| niemeyer | pstolowski: Do you have a moment to explain the task-id/change-id presence in the timing module? | 10:09 |
| niemeyer | 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:11 |
| pstolowski | 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:18 |
| niemeyer | 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:19 |
| pstolowski | niemeyer: i don't think this is the case - see func (t *Timings) Save(s GetSaver) line 140 & 141, and flatten() line 85 | 10:22 |
| niemeyer | if len(t.timings) == 0 && !isEnsureWithChange { | 10:23 |
| niemeyer | return nil | 10:23 |
| niemeyer | } | 10:23 |
| niemeyer | pstolowski: The only use cases I can identify are clearly being used only to prevent wiping out empty timings | 10:25 |
| === mup_ is now known as mup | ||
| pstolowski | niemeyer: right, i see what you mean, i need to think what was the driver for this change | 10:29 |
| niemeyer | 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:29 |
| niemeyer | 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:31 |
| pstolowski | 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 |
| mup | PR #6904: timings: always store ensure timings as long as they have an associated change <Created by stolowski> <Merged by mvo5> <https://github.com/snapcore/snapd/pull/6904> | 10:35 |
| mup | PR #6921: timings: tweak the conditional for ensure timings <Simple 😃> <Created by stolowski> <Merged by stolowski> <https://github.com/snapcore/snapd/pull/6921> | 10:35 |
| niemeyer | 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 |
| niemeyer | pstolowski: That is, | 10:39 |
| niemeyer | 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 |
| niemeyer | pstolowski: If top-level timings are relevant even if they are empty, then we might just keep them around. | 10:40 |
| niemeyer | pstolowski: If fast timings are still relevant because people might search for them, we might just keep them around as well. | 10:41 |
| niemeyer | pstolowski: Or reduce the threshold until it's more honest | 10:41 |
| niemeyer | 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:42 |
| pstolowski | 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:43 |
| niemeyer | 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:45 |
| niemeyer | 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:47 |
| pstolowski | 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 |
| mup | PR #8283: overlord,timings,daemon: separate timings from overlord/state <Created by pedronis> <Merged by pedronis> <https://github.com/snapcore/snapd/pull/8283> | 10:52 |
| niemeyer | pstolowski: What I see there is Samuele working exactly on the direction being suggested here, which is clean up the encapsulation | 10:56 |
| pstolowski | unbelievable, it's 2 years since timings were introduced, time flies... | 10:56 |
| niemeyer | pstolowski: This is a top-level package.. | 10:56 |
| niemeyer | Yeah, timing flies | 10:56 |
| pstolowski | niemeyer: yes.. although the driver had a different angle as mentioned on mattermost... we wanted to not import state package elsewhere | 10:57 |
| niemeyer | pstolowski: Not importing the state package only has a meaning if you're not marrying with its internals :) | 10:57 |
| pstolowski | yes sure | 10:58 |
| mvo | mborzecki: 9577 looks good to me now, wdyt? | 11:31 |
| mvo | mborzecki: I fixed one missing test update in 9560 and then I think this can land :) thanks for the review! | 11:37 |
| mborzecki | mvo: i've done the followup changes, but having some trouble with mocking sfdisk properly | 11:38 |
| pstolowski | mvo: i've a spread test for download timeout, slightly tricky stuff | 12:01 |
| pstolowski | see #9590 | 12:01 |
| mup | PR #9590: tests: download timeout spread test <Created by stolowski> <https://github.com/snapcore/snapd/pull/9590> | 12:01 |
| mvo | pstolowski: nice | 12:02 |
| mvo | pstolowski: just looked, really cool | 12:04 |
| pstolowski | mvo: let's see if it's not flaky or something.. i only run it on 20.04 | 12:05 |
| mup | PR snapd#9590 opened: tests: download timeout spread test <Created by stolowski> <https://github.com/snapcore/snapd/pull/9590> | 12:05 |
| mup | PR snapd#9560 closed: gadget/many: drop usage of gpt attr 59 for indicating creation of partitions <Run nested> <UC20> <Created by anonymouse64> <Merged by bboozzoo> <https://github.com/snapcore/snapd/pull/9560> | 12:45 |
| mup | PR snapd#9591 opened: gadget, gadget/install: move helpers to install package, refactor unit tests <Run nested> <Simple 😃> <UC20> <Created by bboozzoo> <https://github.com/snapcore/snapd/pull/9591> | 12:45 |
| mborzecki | mvo: ^^ | 12:45 |
| mborzecki | mvo: pushed some changes to #9577 | 13:34 |
| mup | PR #9577: many: seal a fallback object to the recovery boot chain <Run nested> <UC20> <Created by cmatsuoka> <https://github.com/snapcore/snapd/pull/9577> | 13:34 |
| mvo | mborzecki: \o/ thank you | 13:57 |
| ijohnson | 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 |
| mup | PR #9541: osutil/disks: re-implement partition searching for disk w/ non-adjacent parts <Bug> <Run nested> <UC20> <Created by anonymouse64> <https://github.com/snapcore/snapd/pull/9541> | 14:38 |
| mvo | ijohnson: let's raise it again today | 14:39 |
| ijohnson | sounds good | 14:39 |
| * mvo puts it on the meeting agenda | 14:39 | |
| * cachio lunch | 15:09 | |
| mup | PR snapcraft#3340 closed: include SNAPCRAFT_STAGE in XDG_DATA_DIRS, needed to find Gir files built by other parts <Created by kenvandine> <Merged by sergiusens> <https://github.com/snapcore/snapcraft/pull/3340> | 15:14 |
| mup | Issue core20#94 opened: fix pi4 names for wlan0 and eth0 <Created by xnox> <https://github.com/snapcore/core20/issues/94> | 15:15 |
| mborzecki | need to drive the kids a bit | 15:47 |
| ijohnson | mvo: submitted review for 9577 | 15:58 |
| mvo | ijohnson: thanks! | 15:59 |
| ijohnson | np | 15:59 |
| ijohnson | I will work on my followup to use that logic in snap-bootstrap today as we discussed | 15:59 |
| mvo | ta | 16:02 |
| pstolowski | quick errand, bbiab | 16:15 |
| mvo | 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 |
| mup | PR #9591: gadget, gadget/install: move helpers to install package, refactor unit tests <Run nested> <Simple 😃> <UC20> <Created by bboozzoo> <https://github.com/snapcore/snapd/pull/9591> | 16:55 |
| ijohnson | 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 |
| cachio | mvo, let me cehck, I think I need to clean up | 16:55 |
| mvo | ijohnson: oh, right :( | 16:56 |
| ijohnson | 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:56 |
| cachio | ijohnson, well, they have pagination to support taht | 16:57 |
| cachio | but spread does not support pagination | 16:57 |
| mvo | 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 |
| mup | PR #9577: many: seal a fallback object to the recovery boot chain <Run nested> <UC20> <Created by cmatsuoka> <https://github.com/snapcore/snapd/pull/9577> | 16:58 |
| ijohnson | ah I see | 16:58 |
| ijohnson | mvo: looking now | 16:58 |
| mvo | ijohnson: \o/ | 16:58 |
| mvo | ijohnson: spread> there are two draft PRs for spread to support pagination but both not quite get it right :/ | 16:58 |
| ijohnson | ah I see | 16:59 |
| ijohnson | I thought this was gce's fault | 16:59 |
| cachio | mvo, do you want to re-trigger | 17:00 |
| cachio | mvo, already cleaned up | 17:00 |
| ijohnson | mvo: re 9577, do you mean in `TestMakeBootable20RunModeSealKeyErr` that ResealKeys (or SealKeys? I forget which one) is never actually called? | 17:01 |
| pstolowski | re | 17:01 |
| mvo | 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 |
| mup | PR #9577: many: seal a fallback object to the recovery boot chain <Run nested> <UC20> <Created by cmatsuoka> <https://github.com/snapcore/snapd/pull/9577> | 17:06 |
| ijohnson | mvo: ah yes you are right that is true | 17:06 |
| ijohnson | that is less of an issue than I thought you were trying to say :-) | 17:06 |
| mvo | ijohnson: it also already fails after the first call with an error so it will never be true in the switch | 17:06 |
| mvo | ijohnson: it also means we don't actually test the error transmission from the second call :) | 17:07 |
| mvo | ijohnson: it's probably not much of an issue, I agreee | 17:07 |
| * mvo gets dinner | 17:12 | |
| timothy | hi zyga, is confinement broken on Fedora 33? | 17:39 |
| timothy | https://paste.centos.org/view/e5b46b5b | 17:40 |
| === ijohnson is now known as ijohnson|lunch | ||
| === ijohnson|lunch is now known as ijohnson | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!