[00:16] PR snapcraft#3014 closed: meta: migrate get_build_base to Snap [05:52] morning [05:53] hey mborzecki [05:53] mvo: hey hey [06:01] brb, new kernel [06:04] and back [06:07] PR snapd#8413 closed: interfaces: don't use the owner modifier for files shared via document portal [06:07] mborzecki: welcome back! [06:15] Hey [06:15] Sleepy a bit [06:15] zyga: hey [06:22] mvo: the spread job on 19.10 didn't run here https://github.com/snapcore/snapd/pull/8443 but all the other jobs are green [06:22] PR #8443: interfaces/many: miscellaneous policy updates xliv [06:22] mvo: 2.44 port of that PR is green, so we could probably merge 8443 [06:25] I also had a CI run pass on everything but 19.10 today: https://github.com/snapcore/snapd/pull/8356 [06:25] PR #8356: cmd/snap: Implement a "snap routine file-access" command [06:27] mvo: hi, uc20-snap-recovery has a failure, is searching for a 3G partition size but now we have 1G again [06:30] so that 19.10 failure pops up in a number of PRs [06:30] mvo: also should we move those main/uc20-* tests to 20.04 ? (they are running on 19.10 atm) [06:31] pedronis: do you have a log from that failure in uc20-snap-recovery? [06:32] mborzecki: yes, but I would expect anything to fail with it at this point [06:33] mborzecki: https://github.com/snapcore/snapd/pull/8435/checks?check_run_id=565884165 [06:33] PR #8435: configcore,tests: fix setting watchdog options on UC18/20 [06:33] pedronis: I remember that there was a PR to shrink the size of the ubuntu-data partition down to 1G from claudio iirc, probably this is what is causing this failure :( [06:33] pedronis: thanks, looking [06:33] PR snapd#8438 closed: tests/session-tool: stop anacron.service in prepare [06:33] pedronis: totally +1 for moving those from 19.10->20.04 [06:34] mborzecki: are you on this? that's great \o/ [06:34] mvo: it landed [06:35] mborzecki: nothing deep, but I need to go have breakfast: [06:35] + MATCH '/dev/loop2p4 .* 3G\s* Linux filesystem' [06:35] + sfdisk -l /dev/loop2 [06:35] that's what's failing afaict [06:37] * mvo onds [06:37] * mvo nods [06:39] mvo: pedronis: isn't that because of https://github.com/snapcore/pc-amd64-gadget/commit/a0eb11c6f6945d9395a858aaf396f3f3c0c609cf#diff-3da8b5c9ab650c29257cf39fccfa8be7 ? [06:39] mborzecki: I think so [06:40] mborzecki: while you are in those tests, please also move them from 19.10 to 20.04 (sorry if this is redundant information, we mentioned it above already but I wanted to be explicit :) [06:40] mvo: ack [06:41] ta [06:43] hmm that test should probably also check that the system-data was automatically expanded before it proceeeds to add that 110MB chunk in the next scenario [06:49] mborzecki: maybe two PRs ? one to fix it and one to change/improve/move things? [06:50] they should also be called uc20-create-partions-* at this point [06:52] * pedronis will not ask for a pony as well [06:55] * mvo lols and wants a pony too [06:58] :P [07:01] morning [07:01] good morning pstolowski [07:06] mvo: did a new pass on #8439 [07:06] PR #8439: secboot: import secboot on ubuntu, provide dummy on !ubuntu [07:09] PR snapd#8445 opened: tests/main/uc20-snap-recovery: unbreak, rename to uc20-create-partitions [07:09] mvo: pedronis: this should do it ^^ [07:09] o/ [07:10] mborzecki: can you review https://github.com/snapcore/snapd/pull/8437 [07:10] PR #8437: tests/session-tool: collect information about services on startup [07:10] it's just extra debug in case that service shows up again [07:10] mborzecki: thx, there's another couple of uc20-snap-recovery-* tests that need the renaming, ->20.04 change [07:10] mborzecki: maybe in the follow up? [07:11] pedronis: yeah, i'll add the tweaks to check the auto grow there too [07:11] thank you [07:13] zyga: pstolowski: hi, #8390 needs a 2nd/re-review [07:13] sure [07:13] looking [07:13] PR #8390: state: add state.CopyState() helper [07:13] looking [07:14] it's interesting to look at test durations below [07:14] some are >> 50minutes [07:16] https://github.com/snapcore/snapd/actions/runs/72216991 shows a run that was killed by the hard 5 hour timeout [07:22] interesting [07:28] thanks mborzecki ! [07:34] mvo: https://github.com/snapcore/snapd/pull/8390#pullrequestreview-388854965 is good to merge [07:34] PR #8390: state: add state.CopyState() helper [07:34] mvo: I think it's okay to override the test that failed due to session-tool [07:34] mborzecki: I made some comments in #8433 [07:34] PR #8433: overlord/devicestate: support for recover and run modes [07:34] thanks [07:35] zyga: thank you [07:36] PR snapd#8390 closed: state: add state.CopyState() helper [07:38] #8411 needs 2nd reviews [07:38] PR #8411: boot: cleanup more things, simplify code [07:38] * zyga looks at 8411 [07:41] zyga: are you familiar with bootstate20.go ? otherwise you probably should read it first [07:41] yeah, I need to [07:41] there's loads of new things in core 20 for me [07:44] pedronis: what is modeenv? [07:53] zyga: boot related state and settings that are not needed before we open writable, so they can be kept there, main stuff is really which base to use [07:53] k [08:04] mvo: hey would snapd be ok with having go 1.14 as default in focal? [08:05] mwhudson: we test with master go [08:05] mwhudson: so I suspect so [08:07] mwhudson: pretty sure yes [08:07] mwhudson: like zyga said, we do run all our unit tests on go-master already [08:08] mwhudson: might be worthwhile to run a full spread run with 1.14 though, when do you want to change the default? [08:08] zyga, mvo: thanks [08:08] mvo: well about 3 weeks ago i guess [08:08] mvo: before release [08:08] mwhudson: *cough* sorry, I guess that was a dumb question [08:09] i've kind of dropped the ball here really :/ [08:09] heh [08:10] mvo: it's there an easy way to do that, use 1.14 to build snapd in a full run? [08:10] PR snapd#8446 opened: tests/main/uc20-create-partitions-*: tweaks, renames, switch to 20.04 === doko_ is now known as doko [08:17] pedronis: mvo: arch, sid and fedora 32 tests probably run with 1.14 already [08:20] yeah, I'm pretty sure we already test this variant for a while [08:21] zyga: have we lost the PR check in the github actions? spread fails on #8446 but not github [08:22] PR #8446: tests/main/uc20-create-partitions-*: tweaks, renames, switch to 20.04 [08:22] sorry, I mean the PR title check [08:22] pedronis: let me check [08:23] pedronis: not removed but it doens't trigger, it loads TRAVIS_PULL_REQUEST for context [08:23] pedronis: let me see if we can port that [08:23] and what else is using TRAVIS_* [08:33] PR snapd#8447 opened: github: check pull request title [08:33] pedronis: ^^ [08:35] oh boy [08:35] almost [08:35] maybe we should just host that as our own action [08:35] (it's not marked as public) [08:36] zyga: also what permission does such an action get? [08:37] zyga: Note that you should have access to the event payload via the file $GITHUB_EVENT_PATH [08:37] pedronis: it runs on the worker [08:37] what permissions has the worker? [08:37] put that through a json parser, and you can see pull request title, labels, etc [08:37] pedronis: it gets a token from github to make API requests [08:38] pedronis: but I don't know if it can make any write requests [08:38] and it can make arbitrary changes to the workspace before the rest of the job continues [08:38] I suppose there should be a way to control that, but if we don't want to dig [08:38] better not use 3rd party action on our stuff [08:38] and access anything left on the system by previous steps [08:39] yeah, we can fork and host our own copy [08:40] maybe that will actually make it work, I'm puzzled by the error [08:42] PR snapd#8447 closed: github: check pull request title [08:44] PR snapd#8445 closed: tests/main/uc20-snap-recovery: unbreak, rename to uc20-create-partitions [08:44] yay [08:45] mborzecki: does not cherry pick cleanly, we will need a 2.44 version as a PR :/ [08:45] mvo: ok, will do [08:47] PR snapd#8437 closed: tests/session-tool: collect information about services on startup [08:47] zyga: I think the release tag you picked is simply busted. The file line 2 of https://github.com/DylanVann/check-pull-request-title/blob/v0.1.14/dist/index.js doesn't exist: it does in the v0.1.13 tag though [08:47] yeah [08:47] I looked at the forks [08:47] there's a fork with better code and a status check that can be enforced [08:48] I'll park this for now and resume in the evening, I think we need to fork it to our own org and manage it there [08:48] json.load(open(os.environ["GITHUB_EVENT_PATH"], "r")) might be a better option [08:50] yeah [08:50] that's what github.context.payload in the action is [08:51] mvo: hmm don't think we need that patch for 2.44 branch tho [08:52] mborzecki: aha, cool [08:53] mvo: running the test to make sure, but it looks like the test was never updated there in the first place [08:53] (i mean updated to check gadget partitions sizes) [08:53] mborzecki: even better, if so we can just remove the milestone [08:53] mborzecki: might still be worthwhile to move from 19.10->20.04 in 2.44 so maybe cherrypick the other one [08:54] yeah [08:55] mborzecki: I will try to cherry-pick once 8446 landed [08:55] mvo: c3019489d2 (19.10 -> 20.04) applies cleanly [08:56] mborzecki: nice! and cherry-picked [09:07] PR snapd#8448 opened: tests/session-tool: add session-tool --dump === Wouter01009 is now known as Wouter0100 === Wouter01009 is now known as Wouter0100 [10:15] * pstolowski going to the grocery and other supplies, bb in 1h [10:16] pedronis: does this make sense now? https://github.com/snapcore/snapd/pull/8442#issuecomment-610298206 [10:16] PR #8442: tests/many: stop importing osutil from dirs, mock ProcSelfMountInfo more <⛔ Blocked> [10:17] pedronis: does this make sense now? https://github.com/snapcore/snapd/pull/8442#issuecomment-610298206 [10:21] ijohnson: it still seems that there should be something simpler that can be done [10:23] pedronis: do you mean a simpler way to mock proc/self/mountinfo all the places we need to? [10:24] first of all I notice that LoadMountInfo usage is a bit weird [10:25] it's always used with /proc/self/mountinfo except it takes a path afaict [10:25] for the benefit of its own tests [10:25] pedronis: LoadMountInfo ends up getting used in many places outside of it's own tests [10:26] yes, so it's optimizing for the wrong thing [10:26] pedronis: additionally, part of the issue I found with our current tests is that during the tests of other things, it will be used but on the go process's mountinfo, instead of a mocked one [10:27] ijohnson: so my suggestion is like this LoadMountInfo should take nothing or if that's annoying take "" to mean the default location, there should be no visible ProcSelfMountInfo afaict [10:28] ijohnson: MockProcSelfMountInfo(path, content string) should become just MockMountInfo(content) [10:28] and convince internall LoadMountInfo to use content instead of readin the file [10:28] pedronis: what is most convenient for other tests, including the ones I have not yet proposed, is that all of the bits of the codebase that in effect read /proc/self/mountinfo should actually really be reading /proc/self/mountinfo instead of the real one [10:29] (for the tests only obviously) [10:29] this means that LoadMountInfo needs a way to know the dirs.GlobalRootDir [10:29] ijohnson: see my suggestion, I don't want to even bother with files [10:30] unless we have a reason to [10:30] do we have a reason? [10:30] hmm ok I can do that instead I suppose [10:31] I don't think we have a reason to use files, other than it was slightly nicer to have a empty mountinfo by default when you just set RootDir to , but now we also need to actually set an empty mountinfo too [10:37] ijohnson: does it affect a lot tests? the current PR looks very churny [10:37] * pedronis needs to have lunch [10:37] pedronis: there are many tests that end up triggering reads to /proc/self/mountinfo [10:37] and there are also actually quite a few tests that need to mock a specific mountinfo [10:50] hey ijohnson :) [10:50] hey zyga [10:51] we should get self-hosted workers from canonistack today so we will see much faster iteration [10:51] that's great news [11:00] ijohnson: we are already detecting if we are in testing for snapdUnsafeIO, so maybe we can do something based on that, possibly panic if we are in tests and proc mountinfo is not mocked [11:01] hmm good point I can look at that for inspiration [11:02] ijohnson: basically var inTesting := strings.HasSuffix(os.Args[0], ".test") [11:02] very blunt but simple [11:02] we don't have a lot of places, but we do have a couple that do things like that [11:05] * ijohnson waits for many many tests to panic [11:07] * zyga is puzzled by something [11:09] $ go test ./... | grep PANICKED | grep -Po '[0-9]+ PANICKED' | awk '{print $1}' | paste -sd+ | bc [11:09] 207 [11:09] hooray [11:11] zyga: session-tool failure here: https://github.com/snapcore/snapd/pull/8435/checks?check_run_id=567093683 on centos ? now I need to re-run all the tests? [11:11] PR #8435: configcore,tests: fix setting watchdog options on UC18/20 [11:12] looking [11:12] do you recall when you merged master into this? [11:12] pedronis: technically yes but I think since nothing else failed, you can just ask mvo to merge [11:13] that doesn't scale a lot though [11:13] ah, this is before the extra stop and debug patches [11:13] merging now [11:13] but yeah, does not really scale [11:14] in travis we had to restart 50% of the tests, which was somewhat better [11:14] PR snapd#8435 closed: configcore,tests: fix setting watchdog options on UC18/20 [11:14] I will see if there'a programmatic way to do what we want [11:14] maybe the GUI is missing but we can just handle that in other ways [11:15] one thing we could do today is to just put those into separate workflows [11:15] then we can restart each one [11:15] in any way, point taken, I'll check [11:35] PR snapd#8443 closed: interfaces/many: miscellaneous policy updates xliv [11:35] zyga: thank you! [11:36] PR snapd#8444 closed: interfaces/many: miscellaneous policy updates xliv - 2.44 [11:37] PR snapd#8429 closed: github: port CLA check to Github Actions [11:42] pedronis: ok now 8442 is doing as requested [11:50] ijohnson: there's a bunch of place that SetRootDir where is a bit unclear if they needed at some stage, or also now. [11:51] pedronis: probably leftover from prior commits, let me try to clean that up [11:51] pedronis: I added squash merge since this PR is going through quite a bit of churn [11:52] ijohnson: it's also trying to do a quite a bit in one go, like the apparmor dirs move [11:53] well since the goalpost for this PR has moved it can probably be split up now, moving apparmor can at least be split out too [11:53] I wasn't able to split it up before since the apparmor dirs was causing an import loop [12:03] ijohnson: other question, is this related to the cross-checks ? or something else? [12:03] pedronis: yes I need to mock mountinfo from the cross-checks PR [12:04] ok [12:04] and I didn't want to add yet another instance of mocking it in that PR [12:04] if this is really too much work I suppose I could it just makes me feel icky inside [12:05] I'm just trying to undetstand the context [12:05] ijohnson: I also thought we would use findmnt or something for the cross check [12:06] instead of doing using our own code [12:06] pedronis: I mean we could, but I thought it would be better to use our own mountinfo parsing code rather than rely on parsing findmnt ? [12:06] ijohnson: findmnt has a json output [12:07] ijohnson: I'm ok using our own code if it's obvious [12:07] ... but still if we already have code ? [12:07] my memory is that using mountinfo is not that trivial [12:07] but maybe I'm misremembering [12:07] it's just LoadMountInfo() and then loop over looking for MountDir == the-thing-I-care-about [12:08] and then the other place I need it will be to look for the efivars fs that is mounted to incorporate the suggestion from maciej and claudio in the efi vars code in boot pkg [12:10] PR snapd#8449 opened: dirs: don't depend on osutil anymore, mv apparmor vars to apparmor pkg [12:11] I broke out the apparmor stuff into this ^ [12:16] ijohnson: there's a bug in that one, probably we don't have test that reveal it [12:17] pedronis: you mean a bu in 8449 ? [12:17] *bug [12:17] yes [12:17] hmm okay, well now it's independent so we can just leave that open and deal with later when we have more time [12:22] it might fail in spread, I don't know [12:24] ijohnson: it's still shows up in the other diff? am I missing something? [12:25] pedronis: do you have saved comments on the original big PR? I have broken out another PR now and rebased/rebased so it is simpler to understand the commit history [12:25] pedronis: I would prefer just to close the big one and re-open at this point if you don't have saved comments there [12:26] no, just global comments [12:26] afaict [12:27] PR snapd#8450 opened: selinux: export MockIsEnforcing; systemd: use in tests [12:27] pedronis: so I'm okay to close 8442? [12:31] ijohnson: yes [12:41] pedronis: what is your idea for hooking FilesystemOnlyApply into core16/18 firstboot and preseeding flows? core config hook run twice? [13:01] pstolowski: no for those we need to call it from image [13:03] PR snapd#8442 closed: tests/many: stop importing osutil from dirs, mock ProcSelfMountInfo more <⛔ Blocked> [13:03] pedronis: mind if i do this change? [13:04] pstolowski: no, we can chat quickly about it, I'm in another meeting atm [13:04] though [13:04] pedronis: ok, sure, let me know when you have a moment [13:09] PR snapd#8446 closed: tests/main/uc20-create-partitions: tweaks, renames, switch to 20.04 [13:45] zyga: was this fixed https://bugs.launchpad.net/snapd/+bug/1870343 ? [13:46] Bug #1870343: Unit test failure on launchpad build [13:46] checking [13:46] I don't know, I only reported it [13:46] let me look deeper [13:47] I doubt it was fixed [13:47] it was a random failure but it is non the less interesting, that code is not full of timers and sleeps [13:52] 8325 (recover mode) should be ready for a second review [13:53] ack [13:58] pedronis: hey, whenever you get a chance can you comment on: https://forum.snapcraft.io/t/support-for-daemon-dbus/8855/11 ? [14:01] jdstrand: yes, I'll try to get to it in the next couple of days [14:02] pedronis: I'm going to add an UPDATE comment for the move away from bus-name, so please read that [14:02] pedronis: thanks! [14:04] mvo: what about 8010? are you going to close that one? iirc that one is blocked waiting for pedronis [14:09] ijohnson: it's a subset of the other one, I don't mind either way [14:10] ok, I reviewed 8010 but was waiting to review 8325 until 8010 was merged, if you like I can review 8325 now anyways? [14:15] ijohnson: let's ask pedronis if we want to review 8010 first or juts do it all in one go in 8325 [14:17] mborzeck1: could I trouble you for a review on https://github.com/snapcore/snapd/pull/8411 ? [14:17] PR #8411: boot: cleanup more things, simplify code [14:19] degville: I know you worked on the hooks docs recently, have you seen this comment: https://forum.snapcraft.io/t/snap-common-looks-different-from-snap-shell/3056/5?u=ijohnson ? [14:22] ijohnson: no, I'd not seen that comment - thanks! I totally agree too. I'll try and make the hook docs a little more intuitive.. [14:22] thanks :-) [14:36] PR snapd#8451 opened: osutil: mock proc/self/mountinfo properly everywhere [14:37] mborzeck1: pstolowski: there's no way to make journald emit is current config right? systemd has systemctl show ? but I don't think there is something for journald [14:41] #8411 still needs 2nd review (I think I said it before) [14:41] PR #8411: boot: cleanup more things, simplify code [14:41] mvo: small review for https://github.com/snapcore/snapd/pull/8325#pullrequestreview-389162078 [14:41] PR #8325: snap-bootstrap: copy auth data from real ubuntu-data in recovery mode [14:43] mvo: I think closing 8010 is ok [14:46] pedronis: i couldn't spot anyting in the man. only this: "By default, the configuration file in /etc/systemd/ contains commented out entries showing the defaults as a guide to the administrator." [14:48] PR snapcraft#3016 opened: cli: remove experimental config.yaml support [14:50] PR snapd#8010 closed: snap-bootstrap: add support for "recover" mode