[01:34] PR core20#86 opened: .travis.yml: use stable snapcraft now [05:22] morning [05:22] school run and then a quick errand, back around 9 [07:01] morning [07:03] zyga-kaveri: hi, we are getting google:ubuntu-16.04-64:tests/main/lxd:snapd_cgroup_just_outside failures sometimes, also cgroup-tracking on arch sometimes fails [07:03] zyga-kaveri: for example, https://github.com/snapcore/snapd/pull/9325/checks?check_run_id=1100227725 [07:03] PR #9325: strutil: add SortedListsUniqueMerge [07:06] mborzecki: hi, should we sync? [07:07] PR snapd#9325 closed: strutil: add SortedListsUniqueMerge [07:16] mvo: mborzecki: hello, should we sync? [07:17] pedronis: I'm trying to get an overview about the open PR over the night right now [07:18] pedronis: but yeah, a quick sync would be good I think [07:20] re [07:20] pedronis: sorry starting only now, i can join in 5 [07:20] mborzecki: mvo: ok, I can be in SU in 5 [07:21] pedronis, mborzecki call we meet at :30 ? nice round time, only slightly more than 5min :)? [07:22] that's fine too [07:23] mvo: works for me [07:52] PR snapd#9324 closed: secboot: adjust parameters to buildPCRProtectionProfile [08:02] mborzecki: I will push some tweak to the reasel PR, it seems some code was copied before applying feedback about errors [08:12] pedronis: ok [08:17] PR snapd#9320 closed: boot: group SealKeyModelParams by model, improve testing [08:18] thx [08:22] good morning [08:34] zyga: hey [08:35] long night [08:35] * zyga EODd at about 1AM [08:41] mborzecki: mvo: I merged master and fixed/adjusted things into #9322, it's ready for 2nd/3rd reviews [08:41] PR #9322: boot: add call to reseal an existing key [08:41] pedronis: cool, will refresh and go over it again [08:58] pedronis: approved 9322 as well, funny that we have very similar comments there [09:00] pedronis: InstallHostFDEDataDir is avaialble in run mode? [09:01] mborzecki: for that one we really need a different var for run mode [09:01] I think [09:02] Int [09:02] InstallHost doesn't make sense during run mode [09:03] mborzecki: we might need a helper that gives a dir based on mode, depends on details [09:11] pstolowski: I noticed the spread tests in 9221 tend to fail, are you on this or shall I have a look ? I'm reviewing this right now and was wondering [09:13] mvo: i'm working on it right now, apart from shellcheck errors it needs to be disabled on some systems because it's too intrusive e.g. for selinux [09:13] and also for core [09:13] (as it's messing up with /var/lib/snapd) [09:20] pstolowski: cool, I will just wait for the update then [09:33] hey mvo, good morning, did you restart tests in #9208 at all? the most recent run I see on that PR does not have the kernel-failover test failing, just the fundamentals test and preparing generally failing [09:33] PR #9208: tests/nested/core20/kernel-failover: add test for failed refresh of uc20 kernel [09:35] ijohnson: I didn't restart, I though master was merged [09:36] mvo: ok, sorry just trying to understand your comment there cause you said the failover test was failing in that pr [09:38] no worries, just restart if needed [09:39] ok, just that I haven't seen the actual kernel-failover test fail and if you saw it fail I would be very interested in logs :-) [09:40] good morning ijohnson :) [09:40] morning zyga [09:48] gosh [09:48] gnome-shell is stuck [09:48] I have a full-screen prompt [09:48] like polkit [09:49] that prompts me about ssh key of a host [09:49] I have no idea why [09:49] there are two buttons that don't do anything [09:49] the machine is up, I can run stuff but I cannot get that modal popup to go away [09:49] I guess it's save and reboot for the new kernel anyway :/ [09:49] but one argument against modal shell popups [09:49] and lack of QA [09:50] (this is on focal) [09:55] it's 2020 but I can ssh -X from a mac to ubuntu and run git gui [09:55] I'm happy [10:01] pedronis: do i remember correctly we shoudl always reseal if the boot chain contains an unasserted kernel? [10:01] mborzecki: yes, there's a todo in bootchain how to achieve that [10:02] mborzecki: see predictableBootChainsEqualForReseal [10:03] PR snapd#9327 opened: overlord: assorted typos and miscellaneous changes [10:08] man after this is all done I feel you guys should do a presentation about resaling [10:09] it seems to have grown into this large complex system all by itself [10:09] not that it's any different from the reality it models [10:11] I'm actually very curious if there's another linux distro out there that does full disk encryption with automatic resealing like this, I'm inclined to think not [10:15] ijohnson windows ;D [10:15] but jokes aside, it does work for windows [10:15] haha yeah [10:15] which is impressive as it shipped about ~ well boatload years ago [10:15] when was bitlocker introduced? [10:16] ijohnson also macos but there it's a bit different in implementation [10:16] yeah that's not using a generic tpm 2.0 afaik [10:16] (for mac) [10:16] I suspect apple's T2 is more like android encryption [10:16] yeah [10:16] yeah [10:16] yeah, T2 is totally different [10:16] I remember setting up bitlocker on windows 7 in like 2010 [10:17] but it does handshake each hardware in the system [10:17] right, I was wondering if I remembered win7 had bitlocker in the pro edition [10:17] still [10:17] it's one hell of a thing to bring it to linux this way [10:17] I would love to see this eventually arrive to classic desktop for 22.04 [10:17] even if you have some more rigidity in result [10:18] yeah the automatic nature of it makes it super useful for installing I think, if your system has tpm 2.0 + uefi, bam fde for free w/ no passphrase, etc. [10:19] I think this will be very popular for corporate users [10:27] oh my [10:27] * zyga realized unexport-content should be after unlink-snap [10:31] mvo: pushed to 9221 [10:31] pstolowski: thanks, looking [10:32] I need more spread tests [10:32] or a check in all tests [10:33] PR snapd#9327 closed: overlord: assorted typos and miscellaneous changes [10:33] thanks mvo [10:38] PR snapd#9328 opened: boot: store boot chains during install, helper for checking whether reseal is needed [10:39] pedronis: ^^ [10:44] neat, no failures [10:44] let's add some spread tests now [10:46] pstolowski: let me poke at this a bit, I have some ideas [10:48] mvo: ok, thank you. i'm playing with it as well [10:53] pstolowski: how does http://paste.ubuntu.com/p/ZDk8fR5Pz9/ look (untested, but should give hte idea)? [10:55] mvo are we measuring space in snapshots or in the parent dir? [10:55] mvo: hmm i wonder if we will get correct size in snapd if it's not a single filesystem [10:55] yeah exactly, what zyga says [10:56] brb, need to stretch a little [10:58] pstolowski I wonder if we could stop snapd, mv all of var/lib/snapd over, mount a tmpfs, copy things and resume the test [10:58] bonus points for cleanup that just unmounts [11:00] yeah i had that initially but hit some issues (don't remember what that was anymore, probably solvable) [11:04] pstolowski: it looks like we test for the actual path so that should work, I can tweak and run locally to double check [11:05] mvo: i can try that, i like how it simplifies it and avoid space calc [11:06] pstolowski: yeah, that's the appeal [11:19] pedronis: hmm boot.Device needs to have Model() now too right? [11:19] mborzecki: yes [11:19] I knew about that but forgot to mention [11:20] mborzecki: I reviewed #9328, mostly a lot of nitpicks [11:20] PR #9328: boot: store boot chains during install, helper for checking whether reseal is needed [11:20] pedronis: cool, thanks (and thanks to mvo too!) [11:20] mborzecki: thx for it [11:24] mvo: so this gives this - Download snap "hello-world" (29) from channel "stable" (link /var/lib/snapd/snaps/hello-world_29.snap /var/lib/snapd/cache/b07bdb78e762c2e6020c75fafc92055b323a6f8da3ab42a3963da5ade386aba11f77e3c8f919b8aa23f3aa5c06c844f9: invalid cross-device link) :( [11:24] mvo: which reminded me that i tried this approach before [11:24] mvo: that's because of our hardlink cache logic [11:24] mborzecki: one more comment [11:25] i need a short break and lunch [11:25] * pstolowski lunch [11:27] pstolowski: yeah, just noticed this too, slightly annoying that our code is not more robust here, I think that's a bug in itself. oh well [11:27] * mvo also lunches [11:28] * zyga is bad at taking breaks [11:28] taking one now for real [11:33] mborzecki: should I do a small PR to complete predictableBootChainsEqualForReseal ? [11:34] pedronis: go ahead [11:34] thanks! [11:34] mborzecki: made another remark [11:58] PR snapd#9329 opened: boot: consider boot chains with unrevisioned kernels incomparable [11:58] mborzecki: done ^ [12:00] so in #9307 i added "tee" to tests/lib/snaps/test-snapd-policy-app-consumer/meta/snap.yaml , yet the test still fails and does not seem to find my added lines ... [12:00] PR #9307: interfaces/tee: add TEE/OPTEE interface [12:00] https://github.com/snapcore/snapd/pull/9307/checks?check_run_id=1092075286 is just telling me to add it to that file [12:01] am i missing anything (and do we perhaps have some doc where to typically turn the knobs to add the expected tests) [12:06] (bah, there seems to be a missing space in the plug decl. ignore the question .. i'll ask again if it fails again) [12:08] ijohnson: answered your review question [12:10] hmm [12:12] pedronis: ack makes sense, but why skip spread on that PR? [12:12] sigh [12:12] ijohnson: nothing is using that function yet [12:12] ok [12:14] ijohnson: #9328 uses it, but in a helper that itself is not used yet, but ideally we should land this first then that [12:14] PR #9328: boot: store boot chains during install, helper for checking whether reseal is needed [12:14] makes sense, I +1d it [12:14] I'm working through a review of 9328 now [12:14] well actually gonna make more coffee then will continue working through that [12:14] wow, seal tests are pita to set up [12:15] mborzecki: yes, we might need some helper [12:22] mborzecki: hmm is it a typo in TestMakeBootable20RunMode that we use Revision: snap.Revision{N: 0}, for pc-kernel_1.snap ? [12:22] should that actually be snap.Revision{N:1}, ? [12:22] it's just unexpected to me that in the BootChain you read from the file at the end of that test we get ` KernelRevision: "",` [12:23] that seems weird [12:23] revisions starts at 1, 0 is unset [12:23] if the goal is to make it look local/unasserted then its N: -1 [12:24] yeah I think it's a typo from the previous PR [12:24] (which coincidentally I approved /o\ ) [12:26] mvo: fyi my get_disk_usage helper has an issue in that spread test, i might have a fix [12:29] pedronis: I finished lookingat 9328, is there another pr I should look at before SU today? [12:29] s/today/right now/ [12:30] ijohnson: #9322 could use a 3rd review (given that I worked a bit on it myself) [12:30] PR #9322: boot: add call to reseal an existing key [12:30] sure [12:36] #9239 needs a 2nd review, it's small [12:36] PR #9239: many: misc doc-comment changes and typo fixes [12:36] oh? [12:37] heh [12:37] I meant #9329 [12:37] PR #9329: boot: consider boot chains with unrevisioned kernels incomparable [12:39] mborzecki: are you still running arch? are you able to install the vokoscreen-ng snap via snap install ? [12:53] PR snapd#9329 closed: boot: consider boot chains with unrevisioned kernels incomparable [12:58] PR snapd#9322 closed: boot: add call to reseal an existing key [13:28] PR snapd#9330 opened: boot: reseal when updating boot assets [13:47] mborzecki: I re-reviewed #9328 [13:47] PR #9328: boot: store boot chains during install, helper for checking whether reseal is needed [13:50] mvo: only unrelated failures in #9221 now [13:50] PR #9221: tests: disk space awareness spread test [13:50] * zyga errand [13:53] pstolowski: cool, looking [13:55] zyga, when you have a momento could you take a look again to #9316 [13:55] PR #9316: tests: use full path to test-snapd-refresh.version binary [13:56] cachio: I commented on your other similar PR that's the wrong way to fix this [13:56] we should figure out why /snap/bin is not on $PATH [13:59] ijohnson, you mean for the tests.tool right? [13:59] because it is on $PATH for snap run o if I just execute the snap [14:00] cachio: I mean this comment https://github.com/snapcore/snapd/pull/9315#issuecomment-690375166 [14:00] PR #9315: tests: fix snap-routime-portal-info test [14:04] ijohnson, I am not sure yet but think the issue is related to the tests.session setup, but really couldn't find where [14:05] becuause it fails just when we run using tests.session [14:06] ijohnson, I mean, this works: su -c 'snap run test-snapd-refresh.version' test | MATCH v1 [14:24] pedronis: cmatsuoka: hmm, current code assumes we always have a trusted assets bootloader :/ [14:24] and fails otherwise [14:24] (i mean the reseal code) [14:25] mborzecki: we should check that outside, if it's not trusted there is nothing to do, really [14:25] we need to cleanup bootloader interface zoo [14:25] but basically is like that atm [14:26] mborzecki: do you need more input on this? [14:27] mborzecki: well, outside, maybe inside, I don't know how you structured your helpers [14:34] * cachio afk [14:38] cachio sure [14:39] cachio replied [15:14] PR snapd#9331 opened: boot: reseal when changing kernel [15:16] pedronis: mvo: cmatsuoka: more like an RFC really ^^ [15:17] mborzecki: thank you! [15:19] mborzecki: I'll check after lunch, thanks! [15:19] mborzecki: why rfc? it looks pretty reasonable from a quick look [15:19] mborzecki: anything to watch out for? [15:20] mvo: hope i got all the places that need to be tweaked, the tests could use more work [15:20] and bootloadertest.MockTrustedBootAssetsBootloader is kind of getting in the way [15:21] * mvo nods [15:21] btw. the tests are spotty for calls to seal/reseal when the bootlaoder is not a trusted assets one [15:21] and probably the same if there's no trusted assets (even if bl implements the interface) [15:24] landing #9328 will make the other PRs considerably smaller [15:24] PR #9328: boot: store boot chains during install, helper for checking whether reseal is needed [15:29] wth `src/github.com/snapcore/snapd/overlord/snapstate/check_snap.go:485:26: ambiguous selector deviceCtx.Model`? [15:29] but it builds just fine locally, maybe a go version change? [15:37] mborzecki: IsResealNeed is not used yet, right? [15:38] pedronis: it's not [15:42] pedronis: cmatsuoka: mvo: do we need to sync? i'll need to wrap it up ~6 to take the kids to a swimming class [15:45] mborzecki: I think the code is understable, anyway we need to land one piece at a time [15:47] i see recurring failures of google:ubuntu-16.04-64:tests/main/lxd:snapd_cgroup_just_inside [15:47] yes, I mentioned that in the morning [15:51] right, not sure zyga saw your message [15:52] there are some missing tests in 9328 [15:53] mmh [16:02] damn, i'm late, i'll check back in later [16:03] mborzecki: sorry, my answer was a no [16:03] maybe it wasn't clear [16:13] mborzecki: sorry, I was away, back now [16:15] cmatsuoka: can we sync quickly? [16:16] pedronis: sure [16:16] cmatsuoka: going to the standup channel [16:32] cmatsuoka: https://github.com/snapcore/snapd/pull/9328#pullrequestreview-486951806 [16:32] PR #9328: boot: store boot chains during install, helper for checking whether reseal is needed [16:32] pedronis: ack [16:34] * pedronis dinner break [16:55] ijohnson, updated the PR following the zyga's recomendation [16:55] cachio: ok, which pr ? [16:56] ijohnson, #9315 [16:56] PR #9315: tests: fix snap-routime-portal-info test [16:57] ijohnson, I think it is still open the fact that the env is not correct and needs to be fixed [16:57] ijohnson, there is another one opened with that discussion #9316 [16:57] PR #9316: tests: use full path to test-snapd-refresh.version binary [16:57] ok, as long as we know that's the issue and that should be fixed eventually I think it's fine to fix the way you did [16:58] cachio: can you add a TODO in tests.session somewhere to debug why that doesn't work? [16:58] doesn't have to be in these PR's, can be a different PR [16:58] ijohnson, sure, thanks!! [17:20] ijohnson: nested tests failed here: https://github.com/snapcore/snapd/pull/9328 we need to understand why [17:20] PR #9328: boot: store boot chains during install, helper for checking whether reseal is needed [17:21] pedronis: mmm looks like the generic problem we keep seeing where sometimes uc20 users don't get created or get created too slowly [17:21] should I just try again? [17:22] just a second let me finish perusing the logs [17:22] I mean to be clear, yes there is a real problem with the nested uc20 tests somewhere and I'm doing my best to track it down [17:23] but just because they failed on this pr doesn't mean that this pr caused that [17:25] pedronis: yeah go ahead and restart that pr, there's nothing indicative in the logs [17:26] pedronis: note that the minimal-smoke test just timed out there, the other one we just never got a user login [17:26] * ijohnson lunches [17:26] ijohnson: let's see how it goes, the PR adds writing one more file during install fwiw [17:26] so it's not completely neutral [17:34] pedronis, hey, I am trying to fix some tests which require a model signed by canonical [17:34] pedronis, is it ok if I add a env var to set when the model is not signed by canonical so then we can skip some parts of the tests [17:35] because surrently the smapd-model test fails on beta validation because I am signing the models used for those images [17:35] cachio: it depends, we have other option nowadays, you can also put serial-authority: generic in your model [17:36] pedronis, ahh, right [17:36] the test needs changes but not skipping full bits then [17:36] My idea is just to skip small parts of the test [17:37] I'll try adding serial-authority: generic first [17:37] I understand my point is that you could set a var that says if we expect serial signed by canonical or generic etc but depends on details [17:37] cachio: is it a lot of tests ? a few [17:37] ? [17:37] pedronis, just a few [17:38] refresh-* and snap-model [17:38] cachio: I don't have a lot of time today, if you can make a list I could look next week? [17:38] pedronis, sure [17:38] thanks [17:55] re [17:59] pedronis, I think the problem is solved now, I moved all the model to canonical, I used to use my models because I needed a user assertion but now I am using cloud init in all the cases [18:00] so I'll just leave the model signed by me for nested tests where we test user assertions [18:00] pedronis, does it make sense? [18:00] mborzecki: cmatsuoka: I just noticed, is not that we don't check if we have the right boot loader, we also don't check if we have a key or encrypted disk at all, in reseal? is that right? [18:01] hmm, this check will be needed somewhere [18:01] * cmatsuoka takes note [18:01] pedronis: yes, we don't [18:02] ok, just double checking [18:03] heh, https://github.com/snapcore/snapd/pull/9330 panics (the unit tests are passing though) [18:03] PR #9330: boot: reseal when updating boot assets