[00:19] PR snapcraft#3265 closed: colcon v2 plugin: honour http(s) proxy for stage-runtime-dependencies [00:19] PR snapcraft#3266 closed: cli: add --enable-experimental-extensions option for expand-extensions [05:20] * zyga-x240 debugs failing core/apt test [05:24] morning [05:27] hey [05:27] something broke, new core18 has apt-get [05:27] I've adjusted a test and will push a branch soon [05:27] master is red currently [05:27] mborzecki: I have something for you [05:27] https://github.com/snapcore/snapd/pull/9212 [05:27] PR #9212: cgroup,snap: track hooks on system bus only [05:27] can you quickly look please, it's very small apart from extra tests [05:28] zyga-x240: hey [05:28] master red? ehh [05:28] yep [05:28] but fix is coming [05:55] mborzecki: https://github.com/snapcore/snapd/pull/9229 [05:55] PR #9229: tests: account for apt-get on core18 <⚠ Critical> [05:55] this should fix master [05:55] PR snapd#9229 opened: tests: account for apt-get on core18 <⚠ Critical> [06:09] mborzecki: so your question was on the right track [06:09] mborzecki: hooks were tracked in the root session [06:09] mborzecki: that is fixed with that PR [06:10] mborzecki: the next step is to rebase the selinux patches, this change made them useless [06:10] mborzecki: when mvo is around we can start making progress [06:28] mvo: hey [06:28] mvo: good morning [06:29] mvo: I have a few things for you [06:34] good morning zyga-x240 [06:34] mvo: hello [06:34] mvo: so first thing first, master is broken because core18 now ships the apt-get wrapper script [06:34] mvo: because we no longer maintain core18 I chose to adjust tests instead of chasing the snap [06:35] mvo: this is https://github.com/snapcore/snapd/pull/9229 [06:35] PR #9229: tests: account for apt-get on core18 <⚠ Critical> [06:35] mvo: second thing, the bug you poked me about last night [06:35] mvo: it's very embarrassing as I wrote that code [06:35] mvo: the fix is in https://github.com/snapcore/snapd/pull/9228 and should be merged to 2.46 if we are getting a .1 [06:35] PR #9228: interfaces/systemd: compare dereferenced Service [06:36] * zyga-x240 goes to review https://github.com/snapcore/snapd/pull/9098 [06:36] PR #9098: tests: new organization for nested tests [06:36] and then other things [06:36] zyga-x240: thank you! [06:36] * mvo hugs zyga-x240 for 9228 [06:38] mvo: it never failed before because our tests and our customers never had a case where a gpio was used by more than one plug at a time [06:38] mvo: anyway, it's fixed now [06:39] zyga-x240: great job [06:39] zyga-x240: also good timing as 2.46.1 will happen very soon [06:39] that's great [06:39] mvo: I would also like to include one more thing [06:39] https://github.com/snapcore/snapd/pull/9212 [06:39] PR #9212: cgroup,snap: track hooks on system bus only [06:39] it's only for people who enabled r-a-a [06:39] this corrects a mistake in how we track hooks [06:39] mvo: hey [06:39] it should be better even if root logs out [06:39] good morning mborzecki and welcome back [06:40] (right now the hook might get killed if root logs out after the hook starts running) [06:40] not a must but I'd love to get it in, it's only used for feature flag users anyway [06:42] zyga-x240: re core18> looking now but there are some messages that core18 is held in the review queue, maybe related [06:42] mvo: maybe it got unblocked? [06:43] in any case, master is broken now so we should probably do something to the test, other ideas are welcome [06:43] zyga-x240: yeah, probably, but in any case, it's fine that apt-get exists [06:43] yeah, I think so [06:43] zyga-x240: so your fix is probably right, looking at it now [06:44] mvo: meh [06:44] I botched the fix [06:44] sorry [06:44] zyga-x240: which one? [06:45] the one for apt [06:45] I forgot * [06:45] ubuntu-core-16-* [06:45] zyga-x240: no worries, just force push [06:45] done [06:45] PR snapd#9228 closed: interfaces/systemd: compare dereferenced Service [06:48] zyga-x240: nice 9212 is green, selinux does not complain [06:49] mborzecki: not really [06:49] mborzecki: selinux is not tested there [06:50] mborzecki: the other PR enabled tracking to get more coverage [06:50] mborzecki: this one just prepares for that, selinux still needs changes [06:50] at some point we should run all tests with selinux checks [06:50] but that would be a lot of work today, I fear [06:51] mborzecki: would you mind if I make those changes in a follow-up [06:51] the PR is green now [06:51] zyga-x240: it's fine [06:51] ok [06:51] merging then, thank you [06:52] and we have more tests for tracking now as well, thank you for reviewing :) [06:53] * zyga-x240 runs for quick breakfast [06:55] PR snapd#9212 closed: cgroup,snap: track hooks on system bus only [07:00] PR snapd#9223 closed: mkversion.sh: simple hack to include dirty in version if the tree is dirty [07:02] morning [07:02] good morning pstolowski [07:03] pstolowski: heya [07:19] mborzecki: hi, I added this comment: https://github.com/snapcore/snapd/pull/9201#discussion_r478204904, not sure it's clear [07:19] PR #9201: [RFC] boot: observe update & rollback of trusted assets [07:21] back from breakfast [07:36] I tweaked #9227 [07:36] PR #9227: snap: add size to the random access file return interface [07:40] ta [07:50] * zyga-x240 goes for the desktop call [07:50] pedronis: yes, i think it makes sense, i need to think a bit how to do it reliably for newly added assets though [07:50] pedronis: and i've updated the PR since the tweaks for install observer landed [07:55] mborzecki: we are holding the state lock during the whole gadget assets update right? [08:05] pedronis: let me double check, i don't think so [08:06] pedronis: we don't, there's an explicit unlock/lock around call to gadget.Update [08:10] ullo ullo [08:10] Chipaca: heya [08:10] zyga: snapd.socket is dead right now, want to know more? [08:10] Chipaca yes [08:10] mborzecki ^^^ [08:10] I'm in a call [08:10] please use this opportunity to learn more [08:10] Chipaca did snapd snap refresh or did the classic package update? [08:10] mvo ^ [08:11] could be pretty serious [08:11] neither afaik, but i'll check [08:11] logs are swamped by microk8s poking snapctl (and failing) all the time [08:11] Chipaca: systemctl status snapd.socket please [08:12] mborzecki: https://paste.ubuntu.com/p/K87wFBQwt3/ [08:13] Chipaca: heh interesting, Transaction for snapd.service/start is destructive (systemd-suspend.service has 'start' job queued, but 'stop' is included in transaction) [08:13] the system is suspending so systemd won't start the socket or what? [08:14] woah [08:14] interesting [08:14] Chipaca are you suspending your computer often? [08:15] 21.55.20 is a 'reached target Sleep' [08:15] zyga: when i don't use it [08:15] zyga: so, no :-) but daily for sure [08:25] zyga, mborzecki, anything else you want to get out of this, or should i go ahead and restart the socket? [08:25] Chipaca I think restarting the socket is ok [08:25] Chipaca: hm maybe snap changes and snap change if there's something relevan tin there [08:25] mborzecki: can't do that without restarting the socket :-) [08:26] mborzecki: no changes newer than yesterday at 18:54 [08:26] and that was core18 refreshing [08:27] Chipaca: ha, you actually can now, snap debug state [08:27] oooh, schmancy! [08:27] Chipaca haha, yeah [08:27] snap debug state /var/lib/snapd/state.json [--change= if there's anything relevant] [08:59] * zyga reboots for upgrade quickly [09:11] PR snapd#9226 closed: cmd/snap-bootstrap/initramfs-mounts: compute string outside of loop [09:11] PR snapd#9229 closed: tests: account for apt-get on core18 <⚠ Critical> [09:15] mvo do you need a backport of the GPIO bug fix? [09:15] zyga: I already cherry-picked it [09:15] superb, thanks! [09:15] mvo it's such a terrible bug, I'm sorry for causing this [09:16] zyga: don't worry [09:22] mborzecki: there's probably a bad idea at this point, we need to rethink that [09:23] mborzecki: that's probably a bad idea at this point [09:23] pedronis: not locking state? [09:23] mborzecki: I mean releasing the lock [09:23] yes [09:23] it's not clear that that op should so slow to matter [09:23] and now we are playing with modeenv without a lock [09:23] not a good idea [09:23] ah right [09:24] pedronis: hm thre's some unclear scenarios, can you request a reboot to recovery when assets update is in progress? [09:25] mborzecki: well, if we hold the lock you won't be able to [09:25] no? [09:26] mborzecki: RequestSystemAction asks for the lock [09:27] pedronis: yeah [09:28] pedronis: otoh, it's not like there's going to be GBs of assets being updated, so holding a lock during the update isn't too bad [09:28] yes [09:28] and the new code is reading modeenv once and then writing it multiple times [09:29] so we really need some kind of lock [09:29] I don't think it's very explicit but in general the assumption is that we hold the lock [09:29] when manipulating modeenv [09:31] pedronis: would a boot package level modeenv lock work? [09:32] mborzecki: yes, but I think it will be messy, I wouldn't do that unless we have a strong reason to [09:32] pedronis: hm state lock also has some nice checks built in already [09:33] like the code as is would have to old for the existence of the observer [09:33] you need to unlock at the right times also on error paths etc etc [10:21] * zyga quick break for something warm (temperature dropped by 15C) and back to reviews [11:00] mvo: seems 20.04 is failing degraded with secureboot-db.service loaded failed failed Secure Boot updates for DB and DBX [11:02] pedronis: fun, I saw this too [11:02] pedronis: do you plan to review 9227 or shall I do that ? the size() helper one [11:03] mvo: I touched it myself now so a review from somebody else is better [11:03] pedronis: sure thing, doing that now [11:03] pedronis: similar question about 9213 [11:06] mvo: 9213, it looks okish, I'm not sure it makes 100% sense but I'm also not sure this the last version of that code we need [11:08] mvo: it looks too much like guessing to me [11:12] pedronis: thanks [11:15] mvo: should I leave a comment there? [11:16] pedronis: I think that would be good [11:16] PR snapd#9209 closed: daemon: correctly parse Content-Type HTTP header [11:17] pedronis: I also looked at 9227 and added a possible suggestion [11:23] mvo: done https://github.com/snapcore/snapd/pull/9213/files#r478343623 [11:23] PR #9213: secboot: read kernel efi image from snap file [11:25] pedronis: ta [11:25] mvo: your suggestion is exactly what I undid , see my new comment [11:25] the code originally had the error and use stat [11:25] in Size [11:27] mvo: sorry, you wasted a bit of time, your suggesting is exactly the reverse of my last commit [11:27] pedronis: aha, then I misunderstood your comment, I thought you said too much stuff in stat is ill-defined. but this variation is only returning size not the full stat info [11:28] mvo: sorry, that was the original comment, my motivation for my last change is its commit [11:28] pedronis: I see it there now, that's fine then. thank you [11:28] mvo: snaps are meant to be immutable (notwithstanding all the fun of try and snapdir) so the size shouldn't really be variable [11:30] pedronis: yeah, it makes sense [11:31] PR snapd#9227 closed: snap: add size to the random access file return interface === msalvatore_ is now known as msalvatore [11:52] pedronis: while reviewing validation sets and refreshing my memory, I came across a TODO and wrote https://github.com/zyga/snapd/commit/76049517f35f25e68b165fe0f427555fb790bf93 -- should I propose it later? [11:54] zyga: I don't know, I expect there are changes needed also outside of asserts [11:54] ah, I didn't think about that [11:54] anyway, it's not a big thing, just something I noticed while reviewing [11:55] yes, is something to do [11:55] at least seed and seedwrite needs changes as well [11:55] don't know if there are other places [12:00] naive grepping says likely only seed and seed/seedwriter [12:08] pedronis validation sets have perfect test coverage, nice [12:10] zyga, hey [12:10] hey! [12:10] I had a quick look at your branch [12:10] I left some comments, [12:10] so, yesterday tried everything [12:11] to make work the user session for root [12:11] cachio did you get a logind session? [12:11] also tried the change in spread [12:11] no [12:11] cachio ok, I'll try to help after the standup [12:12] zyga, thnanks [12:25] morning folks [12:26] pstolowski: hi, I re-reviewed #9211 [12:26] PR #9211: o/snapstate: disk space check with InstallMany [12:27] pedronis: thank you! [12:27] will push the tweaks in a moment [12:42] pedronis https://github.com/snapcore/snapd/pull/9155#pullrequestreview-476677226 [12:42] PR #9155: asserts/snapasserts: introduce ValidationSets [13:27] PR snapd#9230 opened: overlord/devicestate: do not release the state lock when updating gadget assets [13:43] cachio I'll look at the external thing in a moment, let me grab some food [13:43] zyga, sure, thanks a lot [13:43] pedronis: actually sorry I forgot I need to step out for a few minutes, can we chat in like 15 minutes or later today about the ensure boot problem ? [13:47] PR snapd#9213 closed: secboot: read kernel efi image from snap file [13:50] ijohnson: I have meetings in 10 mins for 1h [13:50] I can chat after that [13:54] cachio back with food, let me tinker a little and I'll get back to you [13:58] zyga, sure [14:01] pedronis: ack I'll send a meeting invite just to be sure [14:30] cachio I have something that works [14:30] let me minimize it [14:30] great [14:32] mvo: hey, so I addressed all feedback in PR 8920 quite some time ago and this is one of the items that I milestoned for 2.46. I'm not sure if people are waiting on pedronis because it has his tag, but I addressed all his feedback so I think really anyone could review [14:32] PR #8920: interfaces: update cups-control and add cups for providing snaps [14:34] mvo: zyga did review and approve the now closed PR 9194 (with some non-blocking comments), but 8920 doesn't have reviews [14:34] PR #9194: interfaces: update cups-control and add cups for providing snaps - 2.46 [14:34] s/reviews/approvals/ [14:34] mmm [14:34] I should review those [14:43] cachio ok [14:43] cachio try this: sudo systemd-run --unit "spread-$RANDOM" --property=User=root --property=PAMName=runuser-l --pipe sh -c "loginctl" [14:44] works on 18.04 for sure, I can look at older systems too [14:44] you can drop the sh -c thing, just give it a path to a script [14:45] ok [14:46] systemd-run: unrecognized option '--pipe' [14:46] cachio 16.04? [14:46] zyga, core16 [14:46] can you try on newer system [14:46] I will adjust it to core16 [14:46] try core18 for now [14:47] sure, let me get the image [14:52] cachio for 16.04 [14:52] sudo systemd-run --unit "spread-$RANDOM" --property=User=root --property=PAMName=runuser-l --tty sh -c "loginctl" [14:52] it's not the best but meh [14:52] it probably is enough [14:55] zyga, works [14:55] note that --pty may be problematic [14:55] so we may need something slightly different like a hand-rolled code that does this and bridges stdin/stdout [14:55] without making a pty [14:55] ptys are problematic [14:56] so, we a new unit that runs this? [14:56] to keep the session up during the test? [14:57] cachio this runs a program with a PAM name [14:57] in isolation from the session of the calling user [14:57] that's enough [14:58] https://paste.ubuntu.com/p/WsPfXcfTH8/ [14:58] I see this [14:59] same if I run with a test user [14:59] right [15:00] on core16 with core18 snap installed: [15:02] nah, that doesn't work [15:02] try what I gave you [15:02] and I'd like to know what you tried [15:03] since it didn't work for you before [15:03] pedronis: ready ? [15:03] ijohnson: finishing previous meeting [15:04] sure let me know when you're ready [15:04] zyga, just tried manually with the command you sent [15:04] which command? [15:04] sudo systemd-run --unit "spread-$RANDOM" --property=User=root --property=PAMName=runuser-l --tty sh -c "loginctl" [15:04] that one [15:04] ijohnson: ready now, sorry [15:04] I mean before, when it failed? [15:05] yesterday tried updating spread to use runuser instead of sudo -i [15:05] to run the script [15:05] also tried manually to start the session for root using runuser [15:06] I created a systemd unit with that [15:07] something similar to what you just passed but without --property=PAMName [15:07] I manually created the unit [15:08] I tried to make the unit sleep forlong time [15:08] to it was going to be running [15:09] zyga, does it make sense? [15:09] cachio I wonder why that didn't work [15:09] but anyway [15:09] now you have something to work with [15:10] zyga, yes, I'll try with this [15:10] I think I can make it work [15:11] zyga, thanks [15:12] I'll try again after lunch [15:12] * cachio lunch [15:45] jdstrand: rebuilding a snap with go 1.15, I see a seccomp denial that isn't there when compiled with go 1.14 for copy_file_range, and that is only allowed for docker-support currently [15:46] jdstrand: is it possible / feasible / sensible to add that to the default template ? [15:46] the denial looks like this: `audit: type=1326 audit(1598542961.776:12423): auid=4294967295 uid=0 gid=0 ses=4294967295 pid=157684 comm="security-secret" exe="/snap/edgexfoundry/x1/bin/security-secrets-setup" sig=0 arch=c000003e syscall=326 compat=0 ip=0x47b5ea code=0x50000` [15:50] jdstrand: see also this update in the go 1.15 release notes: [15:51] > The os.File type now supports a ReadFrom method. This permits the use of the copy_file_range system call on some systems when using io.Copy to copy data from one os.File to another. [15:53] jdstrand: ah apparently there is an upstream fix for this where if copy_file_range returns EPERM then Go falls back to an alternate impl that should work in confinement see https://go-review.googlesource.com/c/go/+/249257/ [16:06] so I guess this problem will just go away with the next Go release, but is there a security reason not to allow copy_file_range in the default profile ? [16:07] * zyga goes to work upstairs [16:16] cachio: the regexp in ubuntu-core-20-64:tests/main/listing needs to be updated too to account for pre versions [16:16] cachio: see the failure here: https://pastebin.ubuntu.com/p/MBG5Crr8bC/ [16:19] ijohnson, ah, yes, I'll do it today [16:20] ijohnson, thanks for the heads up [16:20]