[01:17] <jamesh> amurray: in case the other message didn't make it, reviewing https://github.com/snapcore/snapd/pull/8943 would be more useful than 6258: it was split out from 6258, and there may not be anything useful in the older PR once it is merged.
[01:17] <mup> PR #8943: wrappers: generate D-Bus service activation files <Needs security review> <Created by jhenstridge> <https://github.com/snapcore/snapd/pull/8943>
[02:16] <amurray> thanks jamesh - ok I'll go with 8943 then - let me know if you do need 6258 as well and I can come back to that after
[06:45] <mborzecki> morning
[07:47] <mvo> good morning zyga
[07:47] <zyga> good morning
[07:53] <mborzecki> zyga: hey
[07:54] <mborzecki> mvo: hey
[07:54] <mvo> mborzecki: hey, good morning
[07:55] <mborzecki> mvo: zyga: wondering, shouldn't this point to libexecdir? https://github.com/snapcore/snapd/blob/master/interfaces/udev/spec.go#L94
[08:00] <mborzecki> mvo: #9600 grew a bit :/
[08:00] <mup> PR #9600: cmd/s-b/initramfs-mounts: refactor recover mode to implement degraded mode <Complex> <UC20> <Created by anonymouse64> <https://github.com/snapcore/snapd/pull/9600>
[08:00] <zyga> re
[08:01] <zyga> hey mborzecki, mvo
[08:01] <zyga> looking
[08:01] <zyga> mborzecki yes, it should
[08:01] <zyga> mborzecki note that this is affected by https://github.com/snapcore/snapd/pull/7614
[08:01] <mup> PR #7614: cmd/snap-confine: implement snap-device-helper internally <⛔ Blocked> <Created by zyga> <https://github.com/snapcore/snapd/pull/7614>
[08:02] <zyga> I really hope UC20 can be released and we can just merge this branch
[08:02] <zyga> then there's only one consumer of snap-device-helper - the host system
[08:02] <mborzecki> i reckon since this is started by udevd it's running in pid 1 ns, so $(libexecdir) rathre than hardcoded /usr/lib/snapd
[08:02] <zyga> and all paths can be easily understood
[08:02] <zyga> yes, correct
[08:03] <pstolowski> morning
[08:06] <mborzecki> pstolowski: hey
[08:07] <zyga> btw, is core20 released?
[08:07] <zyga> or branched at least?
[08:34] <mvo> zyga: uh, it's just "one more thing" :/ or :) depending on POV, we need to review the degraded recovery mode PR and then we are good
[08:36] <zyga> mvo better to be careful than sorry, even when late
[08:36] <mvo> zyga: yeah, but it's really really the last thing
[08:36] <zyga> mvo: maybe bump version to 3.0 :)
[08:36] <mvo> mborzecki: if you have cycles, a review for 9540 would be great, should be easy I hope
[08:37] <mvo> zyga: yeah, not a bad idea actually
[08:37] <mvo> pstolowski: good morning!
[08:37] <mvo> (btw :)
[08:37] <pstolowski> o/
[08:39] <mborzecki> mvo: sure, will do, looking at 9600 atm
[08:40] <mvo> ta
[08:51] <mup> PR snapd#9514 closed: interfaces/fwupd: enforce the confined fwupd to align Ubuntu Core ESP layout <Created by woodrow-shen> <Merged by mvo5> <https://github.com/snapcore/snapd/pull/9514>
[09:17] <mborzecki> duh, the more i look at 9600 the more i think secboot.UnlockVolumeUsingSealedKeyIfEncrypted is doing too much
[09:22] <mvo> mborzecki: if secboot.UnlockVolumeUsingSealedKeyIfEncrypted is doing too much, maybe we need to break it apart :/
[09:23] <mborzecki> mvo: i think it shouldn't fall back to doing anything about the unencrypted device, perhaps it'd be better if the caller did the device lookup and knew whether there is an encrypted device or not, and if there is one, just ask to open it
[09:24] <mborzecki> still, if we change that, better do that after 9600 lands and snapd is branched
[09:30] <mvo> mborzecki: ok
[09:31] <pedronis> mborzecki: hi, looks like 9605 can be merged ?
[09:36] <mup> PR snapd#9605 closed: gadget, overlord/devicestate: validate that system supports encrypted data before install <Run nested> <UC20> <Created by bboozzoo> <Merged by bboozzoo> <https://github.com/snapcore/snapd/pull/9605>
[09:38] <mborzecki> pedronis: yup, just merged it, there were some failed tests in the morning, but looked like the store downtime that happend on friday
[09:46] <mborzecki> pedronis: wdyt about https://github.com/snapcore/snapd/pull/9600#discussion_r519560708 or maybe we should use dirs.SnapRunDir (i.e. /run/snapd/)?
[09:46] <mup> PR #9600: cmd/s-b/initramfs-mounts: refactor recover mode to implement degraded mode <Complex> <UC20> <Created by anonymouse64> <https://github.com/snapcore/snapd/pull/9600>
[09:47] <pedronis> mborzecki: I will re-review it in a little bit and try to answer
[10:15] <mborzecki> hm weird, with the latest snapd from edge: taskrunner.go:271: [change 2 "Setup system for run mode" task] failed: internal error: system encryption keys are unset
[10:17] <pedronis> mborzecki: wrong kernel?  something wrong with directories moves?
[10:24] <mborzecki> pedronis: idk yet, investigating
[10:27] <mborzecki> pedronis: heh, missing ubuntu-save in gadget ofc
[10:28] <mborzecki> pedronis: but te snapd snap i had did not have 9605 yet, so instead snapd hit the sanity check that encryption keys for data & save exist
[10:46] <mborzecki> mvo: trivial PR https://github.com/snapcore/snapd/pull/9608 it'd be nth for 2.48
[10:46] <mup> PR #9608: gadget/install: add progress logging <Run nested> <Simple 😃> <UC20> <Created by bboozzoo> <https://github.com/snapcore/snapd/pull/9608>
[10:46] <mvo> mborzecki: looking
[10:46] <mup> PR snapd#9608 opened: gadget/install: add progress logging <Run nested> <Simple 😃> <UC20> <Created by bboozzoo> <https://github.com/snapcore/snapd/pull/9608>
[10:46] <mvo> mborzecki: heh, I did something similar for my fde hooks hackish branch :)
[10:48] <mborzecki> haha
[10:56] <pedronis> mborzecki: I did a nother pass on #9600, some naming comments, biggest suggestion (unclear if we can do it) is https://github.com/snapcore/snapd/pull/9600#discussion_r519714758
[10:56] <mup> PR #9600: cmd/s-b/initramfs-mounts: refactor recover mode to implement degraded mode <Complex> <UC20> <Created by anonymouse64> <https://github.com/snapcore/snapd/pull/9600>
[10:56] <mup> PR snapd#9609 opened: tests: add unit test for auto-refresh with validate-snap failure <Created by stolowski> <https://github.com/snapcore/snapd/pull/9609>
[11:42] <pstolowski> mvo: so as suspected last week, there is an issue with snapd-generator & the units for snapfuse (the change relaeted to preseeding from a few months ago)
[11:42] <mvo> pstolowski: oh, thanks for finding this. is it hard to fix? something for 2.48?
[11:43] <pstolowski> mvo: not hard at all, but annoying since maybe they need to revert preseeding in groovy until new deb is available
[11:43] <pstolowski> mvo: i'm not super clear about what is affected yet, will talk to Dmitri today
[11:47] <mvo> pstolowski: ta
[11:48] <mvo> pstolowski: let's try to get a fix in asap so that it's part of 2.48
[11:48] <mvo> pstolowski: I still want to branch that today
[11:48] <pstolowski> mvo: hmm yes, but would be good to combine both fixes (task-chaining)?
[11:48] <mborzecki> pedronis: hm https://github.com/snapcore/snapd/pull/9600#discussion_r519701222 double checkign you meant to rename MountPointIsFromDisk or verifyMountPoint to mountIsFromDisk?
[11:48] <mup> PR #9600: cmd/s-b/initramfs-mounts: refactor recover mode to implement degraded mode <Complex> <UC20> <Created by anonymouse64> <https://github.com/snapcore/snapd/pull/9600>
[11:49] <mvo> pstolowski: yes, I reviewed the task-chaining
[11:49] <mvo> pstolowski: that is also tagged 2.48 :)
[11:49] <pedronis> mborzecki: I meant MountPointIsFromDisk but is not the most important thing
[11:50] <pstolowski> mvo: yep i saw the review (thanks! need to think about your question still),  but missed the tag
[12:15] <mborzecki> pedronis: pushed everything but this one https://github.com/snapcore/snapd/pull/9600#discussion_r519714758 which could probably be done in a followup
[12:15] <mup> PR #9600: cmd/s-b/initramfs-mounts: refactor recover mode to implement degraded mode <Complex> <UC20> <Created by anonymouse64> <https://github.com/snapcore/snapd/pull/9600>
[12:16] <mborzecki> quick errand, back in 30
[12:20] <mup> PR snapcraft#3358 closed: Fix colcon v1 workspace sourcing <Created by artivis> <Merged by sergiusens> <https://github.com/snapcore/snapcraft/pull/3358>
[12:29] <mborzecki> re, heh quicker than i anticipated
[12:30] <mup> PR snapcraft#3352 closed: tests: import sort with isort <Created by cjp256> <Merged by sergiusens> <https://github.com/snapcore/snapcraft/pull/3352>
[12:30] <mup> PR snapcraft#3357 closed: project schema tests: ensure json format checkers are loaded <Created by cjp256> <Merged by sergiusens> <https://github.com/snapcore/snapcraft/pull/3357>
[12:38] <pedronis> mborzecki: should we chat about https://github.com/snapcore/snapd/pull/9600#discussion_r519714758 ?
[12:38] <mup> PR #9600: cmd/s-b/initramfs-mounts: refactor recover mode to implement degraded mode <Complex> <UC20> <Created by anonymouse64> <https://github.com/snapcore/snapd/pull/9600>
[13:06] <mborzecki> mvo:  something to consider for 2.48
[13:06] <mborzecki> mvo:  https://github.com/snapcore/snapd/pull/9610
[13:06] <mup> PR #9610: interfaces/udev: use distro specific path to snap-device-helper <Created by bboozzoo> <https://github.com/snapcore/snapd/pull/9610>
[13:07] <mup> PR snapd#9610 opened: interfaces/udev: use distro specific path to snap-device-helper <Created by bboozzoo> <https://github.com/snapcore/snapd/pull/9610>
[13:12] <mborzecki> heh, all squares instead of fonts again https://forum.snapcraft.io/t/draw-io-snap-system-dialogs-not-rendering-correct/21026
[13:13] <mborzecki> pedronis: sorry missed that, we can chat, i have some changes stashed for that already
[13:14] <pedronis> mborzecki: HO ?
[13:14] <mborzecki> yup
[13:32] <mup> PR snapd#9611 opened: tests: migrate test from boot.sh helper to boot-state tool <Created by sergiocazzolato> <https://github.com/snapcore/snapd/pull/9611>
[13:34] <ijohnson> morning folks
[13:34] <cjp256> \o
[13:35] <ijohnson> hey cjp256
[13:35] <ijohnson> cjp256: I definitely didn't get a chance to look at that bug we talked about, but I have every intention of looking at it today, I triple promise :-)
[13:35] <cjp256> a lovely morning over here
[13:37] <ijohnson> it's a bit rainy and gloomy here, and we are about to get a bunch of snow tomorrow so there's that
[13:38] <cjp256> ijohnson: no worries, appreciate it whenever you get to it.  I was debating resorting to an infamous sleep() :D
[13:40] <cjp256> well snow is good when it accumulates into something to play in!
[13:54] <mvo> mborzecki: indeed, approved, thanks for finding this one!
[13:56] <mup> PR snapcraft#3360 opened: project loader: advanced grammar support for lists <Created by cjp256> <https://github.com/snapcore/snapcraft/pull/3360>
[14:01] <mup> PR snapcraft#3361 opened: extensions: use SNAP_COMMON instead of SNAP_DATA for fonts <Created by sergiusens> <https://github.com/snapcore/snapcraft/pull/3361>
[14:30] <mborzecki> pedronis: pushed set(Find|Mount)State to #9600
[14:30] <mup> PR #9600: cmd/s-b/initramfs-mounts: refactor recover mode to implement degraded mode <Complex> <UC20> <Created by anonymouse64> <https://github.com/snapcore/snapd/pull/9600>
[14:30] <pedronis> mborzecki: thx, looking
[14:44] <mborzecki> mvo: hmm src/github.com/snapcore/snapd/daemon/api_system_recovery_keys.go:45:24: rkey.String undefined (type *secboot.RecoveryKey has no field or method String)
[14:44] <mborzecki> but it's somehow passing nosecboot tests
[14:46] <mvo> mborzecki: is that a test failure?
[14:46] <mvo> mborzecki: looking
[14:47] <om26er> Can I build armhf snap on aarch64 server ?
[14:47] <mborzecki> mvo: no, a build on sid: https://github.com/snapcore/snapd/pull/9610/checks?check_run_id=1374318028
[14:47] <mup> PR #9610: interfaces/udev: use distro specific path to snap-device-helper <Created by bboozzoo> <https://github.com/snapcore/snapd/pull/9610>
[14:47] <mvo> mborzecki: that is confusing indeed, looking
[14:50] <ijohnson> hmm seems I have lost the code suggestion feature from github comments :-/
[14:50] <mvo> mborzecki: I suspect "go install" in in debina/rules is not passing "-tags nosecboot" :/ but it's strange that this became an issue only now
[14:50] <mborzecki> mvo: it used to pass nosecboot
[14:51] <mborzecki> mvo: cd _build && go install -trimpath -v -p 1 -pkgdir=/home/gopath/src/github.com/snapcore/snapd/_build/std -buildmode=pie -tags "nosecboot withtestkeys"
[14:51] <mvo> mborzecki: found it
[14:52] <mvo> mborzecki: debian has issues with tags https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=956783
[14:52] <mvo> mborzecki: I push a fix shortly
[14:56] <mvo> mborzecki: there is another suggestion in the debian bug that I want to check too
[14:58] <mup> PR snapd#9612 opened: packaging: keep secboot/encrypt_dummy.go in debian <Simple 😃> <Created by mvo5> <https://github.com/snapcore/snapd/pull/9612>
[15:18] <mup> PR snapd#9613 opened: snapd-generator: set standard snapfuse options when generating mount units containers <Bug> <Created by stolowski> <https://github.com/snapcore/snapd/pull/9613>
[15:19] <pstolowski> mvo: ^
[15:21] <mvo> pstolowski: \o/
[15:37] <mborzecki> got to drive the kids a bit, i should be back around 8 to take a look at 9600
[15:43] <mup> PR snapd#9614 opened: tests: enable main lxd test on 20.10 <Simple 😃> <Created by stolowski> <https://github.com/snapcore/snapd/pull/9614>
[15:45] <ijohnson> pedronis: seems that since we use different secboot functions for run key of data and save, we can only really refactor the run key case for data, there's not much overlap between setting the unlock state for data and save, mostly because UnlockEncryptedVolumeUsingKey doesn't (yet) return an UnlockResult :-/
[15:46] <ijohnson> pedronis: I'm leaning towards leaving unlockSaveRunKey as-is (which is actually fairly readable IMHO), and just refactoring unlockDataRunKey, unlockDataFallbackKey and unlockSaveFallbackKey
[15:46] <ijohnson> thoughts ?
[15:52] <pedronis> ijohnson: you could construct a UnlockRes to pass along I think
[15:52] <ijohnson> pedronis: hmm interesting proposal, let me try that
[16:01] <pedronis> ijohnson: to be clear, the fallback cases seems the most interesting because they have quite a bit of code attached to them
[16:01] <ijohnson> yes
[16:02] <ijohnson> I actually skipped the unlockSaveRunKey thing you mentioned and will come back to that after sorting out the fallback stuff
[16:14] <ijohnson> pedronis: at the risk of imposing unnecessary spread runs, I ended up removing {save,data}Device from the state machine struct and since that is a no-test change, I am going to push that now to make it clear that it works independently, then I will push up the helper change since with everything together it can be slightly confusing to review I think
[16:24] <zyga> re
[16:49] <ijohnson> pedronis: ok, got the helper implemented, pushing it shortly, and moving on to the degraded() impl, I notice that we only save if the disk was encrypted on the state machine struct, not on the recoverDegradedState, do you think we should store whether the disk is encrypted or not on recoverDegradedState too ?
[16:50] <ijohnson> I can certainly write the function without adding that to recoverDegradedState, but I'm thinking maybe it would be nice to have there too
[16:52] <pedronis> ijohnson: I think the function being on the state machine is fine
[16:53] <pedronis> ijohnson: but you are asking whether degraded should tell if the disk are suppose to be encrypted?
[16:54] <ijohnson> pedronis: yes, I am wondering if we should put whether the disk is encrypted should be in degraded.json
[16:54] <ijohnson> pedronis: re func on state machine, that's fine that's what I'm doing now
[17:10] <pedronis> ijohnson: mmh, there's an issue, if we jump straight from failing to mount boot to unlockDataFallbackKey it's a bit unclear that the state on m.isEncryptedDev will be right
[17:10] <ijohnson> pedronis: looking
[17:10] <pedronis> there's an assumption in that code that we went through unlockDataRunKey but is not always the case
[17:11] <ijohnson> pedronis: do you mean the code in ensureConsistentUnlockResult (or whatever I named that function) ?
[17:12] <ijohnson> ensureUnlockResConsistency apparently
[17:12] <pedronis> ah
[17:12] <ijohnson> I think it's ok
[17:13] <ijohnson> because we only fail if it m.isEncryptedDev was previously _true_ and we now get false
[17:13] <ijohnson> but then again I could be missing somethign
[17:14] <pedronis> no, it's fine but is confusing
[17:14] <ijohnson> pedronis: I think it is slightly less confusing with the helpers
[17:15] <ijohnson> pedronis: I was holding off on pushing the unlock helpers, but they are done right now and I could push them now for you to look at, I was going to wait until either I EOD or I finish the degraded() impl first
[17:24] <pedronis> ijohnson: do they look straighforward? or there are unclear bits? if the latter pushing ealier might be easier
[17:25] <ijohnson> tbh I'm probably not the best judge of straightforwardness of this code given that I've been staring at just this code for the last week :-/ but I do think this is the least complex the code has looked since I started, so probably more straightforward ?
[17:26] <pedronis> sounds encouraging at least
[17:28] <pedronis> ijohnson: anyway because of my confusion I'm wondering if isEncryptedDev is needed/helping or not, I'm not 100% sure atm
[17:29] <ijohnson> pedronis: well I think it is helping at least in some of the states, for example in mountData, we need to decide which is the next state to try in the successful path, and we use isEncryptedDev to decide that
[17:30] <ijohnson> otherwise we would need to carry around unlockRes from unlockData* to mountData somehow
[17:30] <pedronis> we have UnlockState
[17:30] <pedronis> but as I said, not clear cut either way
[17:30] <ijohnson> ah good point we do
[17:31] <ijohnson> pedronis: well in any case I should have the degraded() impl done in the next few minutes, just writing some tests for degraded() directly and then will push that up
[17:46] <zyga> niemeyer, hi, do you think you could help with my forum account
[17:46] <zyga> niemeyer I didn't realize it was associated with my @canonical.com address
[17:47] <zyga> I'd love to restore it and change the email address to one of my launchpad.net addresses
[17:49] <ijohnson> pedronis: mvo: well I'm all done with the changes we agreed to this morning, and so with that I'll be EOD
[17:50] <ijohnson> I might be able to squeeze in a little bit of time to respond to comments in my late PM, but hopefully things are "good enough" that y'all can just merge it in 1-2 hours time after spread
[17:50]  * ijohnson EODs
[17:51] <ijohnson> mvo: oh also I didn't get around to talking to dave about the pi bootloader stuff, but happy to sort that out tomorrow if y'all don't get a chance to in your AM
[17:51] <mup> PR snapcraft#3290 closed: build providers: unified provider refactoring for provider setup <Created by cjp256> <Closed by cjp256> <https://github.com/snapcore/snapcraft/pull/3290>
[17:53] <pedronis> ijohnson: thx
[17:57] <ijohnson> mvo: oh also I approved 9607, maybe you can pull that in too today :-)
[17:57]  * ijohnson really EODs now
[17:57]  * cachio lunch
[17:58] <mup> PR snapd#9615 opened: tests: new commands for snaps-state tool <Created by sergiocazzolato> <https://github.com/snapcore/snapd/pull/9615>
[18:02] <zyga> cachio https://github.com/snapcore/snapd/pull/9615#pullrequestreview-526513720
[18:02] <mup> PR #9615: tests: new commands for snaps-state tool <Created by sergiocazzolato> <https://github.com/snapcore/snapd/pull/9615>
[18:34] <mvo> ijohnson: thank you!
[18:37] <mborzecki> re
[18:38] <mborzecki> wow, 9600 is +2,340 −118 now
[18:39] <mup> PR snapd#9607 closed: o/devistate: fix chaining of tasks related to regular snaps when preseeding <Bug> <Run nested> <Created by stolowski> <Merged by mvo5> <https://github.com/snapcore/snapd/pull/9607>
[18:44] <mup> PR snapd#9610 closed: interfaces/udev: use distro specific path to snap-device-helper <Created by bboozzoo> <Merged by mvo5> <https://github.com/snapcore/snapd/pull/9610>
[18:44] <mup> PR snapd#9612 closed: packaging: keep secboot/encrypt_dummy.go in debian <Simple 😃> <Created by mvo5> <Merged by mvo5> <https://github.com/snapcore/snapd/pull/9612>
[18:44] <mborzecki> yay
[18:56] <mup> PR snapcraft#3349 closed: cli: allow snap-id for validate <Created by sergiusens> <Merged by sergiusens> <https://github.com/snapcore/snapcraft/pull/3349>
[19:19] <mup> PR snapd#9608 closed: gadget/install: add progress logging <Run nested> <Simple 😃> <UC20> <Created by bboozzoo> <Merged by mvo5> <https://github.com/snapcore/snapd/pull/9608>
[19:39] <cachio> zyga, thanks
[19:39] <zyga> :-)
[23:37] <mup> PR snapcraft#3362 opened: project loader grammar: do not transform build-packages <Created by cjp256> <https://github.com/snapcore/snapcraft/pull/3362>
[23:59] <mwhudson> what's failing https://launchpadlibrarian.net/506124953/buildlog_snap_ubuntu_bionic_ppc64el_subiquity-hack_BUILDING.txt.gz
[23:59] <mwhudson> here?