| mup | PR core20#86 opened: .travis.yml: use stable snapcraft now <Created by anonymouse64> <https://github.com/snapcore/core20/pull/86> | 01:34 |
|---|---|---|
| mborzecki | morning | 05:22 |
| mborzecki | school run and then a quick errand, back around 9 | 05:22 |
| pstolowski | morning | 07:01 |
| pedronis | 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 |
| pedronis | zyga-kaveri: for example, https://github.com/snapcore/snapd/pull/9325/checks?check_run_id=1100227725 | 07:03 |
| mup | PR #9325: strutil: add SortedListsUniqueMerge <Created by pedronis> <https://github.com/snapcore/snapd/pull/9325> | 07:03 |
| pedronis | mborzecki: hi, should we sync? | 07:06 |
| mup | PR snapd#9325 closed: strutil: add SortedListsUniqueMerge <Created by pedronis> <Merged by pedronis> <https://github.com/snapcore/snapd/pull/9325> | 07:07 |
| pedronis | mvo: mborzecki: hello, should we sync? | 07:16 |
| mvo | pedronis: I'm trying to get an overview about the open PR over the night right now | 07:17 |
| mvo | pedronis: but yeah, a quick sync would be good I think | 07:18 |
| mborzecki | re | 07:20 |
| mborzecki | pedronis: sorry starting only now, i can join in 5 | 07:20 |
| pedronis | mborzecki: mvo: ok, I can be in SU in 5 | 07:20 |
| mvo | pedronis, mborzecki call we meet at :30 ? nice round time, only slightly more than 5min :)? | 07:21 |
| pedronis | that's fine too | 07:22 |
| mborzecki | mvo: works for me | 07:23 |
| mup | PR snapd#9324 closed: secboot: adjust parameters to buildPCRProtectionProfile <Simple 😃> <UC20> <Created by cmatsuoka> <Merged by bboozzoo> <https://github.com/snapcore/snapd/pull/9324> | 07:52 |
| pedronis | mborzecki: I will push some tweak to the reasel PR, it seems some code was copied before applying feedback about errors | 08:02 |
| mborzecki | pedronis: ok | 08:12 |
| mup | PR snapd#9320 closed: boot: group SealKeyModelParams by model, improve testing <Run nested> <UC20> <Created by cmatsuoka> <Merged by bboozzoo> <https://github.com/snapcore/snapd/pull/9320> | 08:17 |
| pedronis | thx | 08:18 |
| zyga | good morning | 08:22 |
| mborzecki | zyga: hey | 08:34 |
| zyga | long night | 08:35 |
| * zyga EODd at about 1AM | 08:35 | |
| pedronis | mborzecki: mvo: I merged master and fixed/adjusted things into #9322, it's ready for 2nd/3rd reviews | 08:41 |
| mup | PR #9322: boot: add call to reseal an existing key <Run nested> <UC20> <Created by cmatsuoka> <https://github.com/snapcore/snapd/pull/9322> | 08:41 |
| mvo | pedronis: cool, will refresh and go over it again | 08:41 |
| mvo | pedronis: approved 9322 as well, funny that we have very similar comments there | 08:58 |
| mborzecki | pedronis: InstallHostFDEDataDir is avaialble in run mode? | 09:00 |
| pedronis | mborzecki: for that one we really need a different var for run mode | 09:01 |
| pedronis | I think | 09:01 |
| pedronis | Int | 09:02 |
| pedronis | InstallHost doesn't make sense during run mode | 09:02 |
| pedronis | mborzecki: we might need a helper that gives a dir based on mode, depends on details | 09:03 |
| mvo | 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:11 |
| pstolowski | 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 |
| pstolowski | and also for core | 09:13 |
| pstolowski | (as it's messing up with /var/lib/snapd) | 09:13 |
| mvo | pstolowski: cool, I will just wait for the update then | 09:20 |
| ijohnson | 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 |
| mup | PR #9208: tests/nested/core20/kernel-failover: add test for failed refresh of uc20 kernel <Run nested> <Test Robustness> <UC20> <Created by anonymouse64> <https://github.com/snapcore/snapd/pull/9208> | 09:33 |
| mvo | ijohnson: I didn't restart, I though master was merged | 09:35 |
| ijohnson | mvo: ok, sorry just trying to understand your comment there cause you said the failover test was failing in that pr | 09:36 |
| mvo | no worries, just restart if needed | 09:38 |
| ijohnson | 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:39 |
| zyga | good morning ijohnson :) | 09:40 |
| ijohnson | morning zyga | 09:40 |
| zyga | gosh | 09:48 |
| zyga | gnome-shell is stuck | 09:48 |
| zyga | I have a full-screen prompt | 09:48 |
| zyga | like polkit | 09:48 |
| zyga | that prompts me about ssh key of a host | 09:49 |
| zyga | I have no idea why | 09:49 |
| zyga | there are two buttons that don't do anything | 09:49 |
| zyga | the machine is up, I can run stuff but I cannot get that modal popup to go away | 09:49 |
| zyga | I guess it's save and reboot for the new kernel anyway :/ | 09:49 |
| zyga | but one argument against modal shell popups | 09:49 |
| zyga | and lack of QA | 09:49 |
| zyga | (this is on focal) | 09:50 |
| zyga | it's 2020 but I can ssh -X from a mac to ubuntu and run git gui | 09:55 |
| zyga | I'm happy | 09:55 |
| mborzecki | pedronis: do i remember correctly we shoudl always reseal if the boot chain contains an unasserted kernel? | 10:01 |
| pedronis | mborzecki: yes, there's a todo in bootchain how to achieve that | 10:01 |
| pedronis | mborzecki: see predictableBootChainsEqualForReseal | 10:02 |
| mup | PR snapd#9327 opened: overlord: assorted typos and miscellaneous changes <Simple 😃> <Skip spread> <Created by zyga> <https://github.com/snapcore/snapd/pull/9327> | 10:03 |
| zyga | man after this is all done I feel you guys should do a presentation about resaling | 10:08 |
| zyga | it seems to have grown into this large complex system all by itself | 10:09 |
| zyga | not that it's any different from the reality it models | 10:09 |
| ijohnson | 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:11 |
| zyga | ijohnson windows ;D | 10:15 |
| zyga | but jokes aside, it does work for windows | 10:15 |
| ijohnson | haha yeah | 10:15 |
| zyga | which is impressive as it shipped about ~ well boatload years ago | 10:15 |
| zyga | when was bitlocker introduced? | 10:15 |
| zyga | ijohnson also macos but there it's a bit different in implementation | 10:16 |
| ijohnson | yeah that's not using a generic tpm 2.0 afaik | 10:16 |
| ijohnson | (for mac) | 10:16 |
| zyga | I suspect apple's T2 is more like android encryption | 10:16 |
| ijohnson | yeah | 10:16 |
| zyga | yeah | 10:16 |
| zyga | yeah, T2 is totally different | 10:16 |
| ijohnson | I remember setting up bitlocker on windows 7 in like 2010 | 10:16 |
| zyga | but it does handshake each hardware in the system | 10:17 |
| zyga | right, I was wondering if I remembered win7 had bitlocker in the pro edition | 10:17 |
| zyga | still | 10:17 |
| zyga | it's one hell of a thing to bring it to linux this way | 10:17 |
| zyga | I would love to see this eventually arrive to classic desktop for 22.04 | 10:17 |
| zyga | even if you have some more rigidity in result | 10:17 |
| ijohnson | 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:18 |
| zyga | I think this will be very popular for corporate users | 10:19 |
| zyga | oh my | 10:27 |
| * zyga realized unexport-content should be after unlink-snap | 10:27 | |
| pstolowski | mvo: pushed to 9221 | 10:31 |
| mvo | pstolowski: thanks, looking | 10:31 |
| zyga | I need more spread tests | 10:32 |
| zyga | or a check in all tests | 10:32 |
| mup | PR snapd#9327 closed: overlord: assorted typos and miscellaneous changes <Simple 😃> <Skip spread> <Created by zyga> <Merged by mvo5> <https://github.com/snapcore/snapd/pull/9327> | 10:33 |
| zyga | thanks mvo | 10:33 |
| mup | PR snapd#9328 opened: boot: store boot chains during install, helper for checking whether reseal is needed <Run nested> <UC20> <Created by bboozzoo> <https://github.com/snapcore/snapd/pull/9328> | 10:38 |
| mborzecki | pedronis: ^^ | 10:39 |
| zyga | neat, no failures | 10:44 |
| zyga | let's add some spread tests now | 10:44 |
| mvo | pstolowski: let me poke at this a bit, I have some ideas | 10:46 |
| pstolowski | mvo: ok, thank you. i'm playing with it as well | 10:48 |
| mvo | pstolowski: how does http://paste.ubuntu.com/p/ZDk8fR5Pz9/ look (untested, but should give hte idea)? | 10:53 |
| zyga | mvo are we measuring space in snapshots or in the parent dir? | 10:55 |
| pstolowski | mvo: hmm i wonder if we will get correct size in snapd if it's not a single filesystem | 10:55 |
| pstolowski | yeah exactly, what zyga says | 10:55 |
| zyga | brb, need to stretch a little | 10:56 |
| zyga | 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 |
| zyga | bonus points for cleanup that just unmounts | 10:58 |
| pstolowski | yeah i had that initially but hit some issues (don't remember what that was anymore, probably solvable) | 11:00 |
| mvo | pstolowski: it looks like we test for the actual path so that should work, I can tweak and run locally to double check | 11:04 |
| pstolowski | mvo: i can try that, i like how it simplifies it and avoid space calc | 11:05 |
| mvo | pstolowski: yeah, that's the appeal | 11:06 |
| mborzecki | pedronis: hmm boot.Device needs to have Model() now too right? | 11:19 |
| pedronis | mborzecki: yes | 11:19 |
| pedronis | I knew about that but forgot to mention | 11:19 |
| pedronis | mborzecki: I reviewed #9328, mostly a lot of nitpicks | 11:20 |
| mup | PR #9328: boot: store boot chains during install, helper for checking whether reseal is needed <Run nested> <UC20> <Created by bboozzoo> <https://github.com/snapcore/snapd/pull/9328> | 11:20 |
| mborzecki | pedronis: cool, thanks (and thanks to mvo too!) | 11:20 |
| pedronis | mborzecki: thx for it | 11:20 |
| pstolowski | 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 |
| pstolowski | mvo: which reminded me that i tried this approach before | 11:24 |
| pstolowski | mvo: that's because of our hardlink cache logic | 11:24 |
| pedronis | mborzecki: one more comment | 11:24 |
| pstolowski | i need a short break and lunch | 11:25 |
| * pstolowski lunch | 11:25 | |
| mvo | 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:27 | |
| * zyga is bad at taking breaks | 11:28 | |
| zyga | taking one now for real | 11:28 |
| pedronis | mborzecki: should I do a small PR to complete predictableBootChainsEqualForReseal ? | 11:33 |
| mborzecki | pedronis: go ahead | 11:34 |
| mborzecki | thanks! | 11:34 |
| pedronis | mborzecki: made another remark | 11:34 |
| mup | PR snapd#9329 opened: boot: consider boot chains with unrevisioned kernels incomparable <Skip spread> <UC20> <Created by pedronis> <https://github.com/snapcore/snapd/pull/9329> | 11:58 |
| pedronis | mborzecki: done ^ | 11:58 |
| ogra_ | 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 |
| mup | PR #9307: interfaces/tee: add TEE/OPTEE interface <Needs security review> <Created by ogra1> <https://github.com/snapcore/snapd/pull/9307> | 12:00 |
| ogra_ | https://github.com/snapcore/snapd/pull/9307/checks?check_run_id=1092075286 is just telling me to add it to that file | 12:00 |
| ogra_ | am i missing anything (and do we perhaps have some doc where to typically turn the knobs to add the expected tests) | 12:01 |
| ogra_ | (bah, there seems to be a missing space in the plug decl. ignore the question .. i'll ask again if it fails again) | 12:06 |
| pedronis | ijohnson: answered your review question | 12:08 |
| zyga | hmm | 12:10 |
| ijohnson | pedronis: ack makes sense, but why skip spread on that PR? | 12:12 |
| zyga | sigh | 12:12 |
| pedronis | ijohnson: nothing is using that function yet | 12:12 |
| ijohnson | ok | 12:12 |
| pedronis | 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 |
| mup | PR #9328: boot: store boot chains during install, helper for checking whether reseal is needed <Run nested> <UC20> <Created by bboozzoo> <https://github.com/snapcore/snapd/pull/9328> | 12:14 |
| ijohnson | makes sense, I +1d it | 12:14 |
| ijohnson | I'm working through a review of 9328 now | 12:14 |
| ijohnson | well actually gonna make more coffee then will continue working through that | 12:14 |
| mborzecki | wow, seal tests are pita to set up | 12:14 |
| pedronis | mborzecki: yes, we might need some helper | 12:15 |
| ijohnson | mborzecki: hmm is it a typo in TestMakeBootable20RunMode that we use Revision: snap.Revision{N: 0}, for pc-kernel_1.snap ? | 12:22 |
| ijohnson | should that actually be snap.Revision{N:1}, ? | 12:22 |
| ijohnson | 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:22 |
| pedronis | that seems weird | 12:23 |
| pedronis | revisions starts at 1, 0 is unset | 12:23 |
| pedronis | if the goal is to make it look local/unasserted then its N: -1 | 12:23 |
| ijohnson | yeah I think it's a typo from the previous PR | 12:24 |
| ijohnson | (which coincidentally I approved /o\ ) | 12:24 |
| pstolowski | mvo: fyi my get_disk_usage helper has an issue in that spread test, i might have a fix | 12:26 |
| ijohnson | pedronis: I finished lookingat 9328, is there another pr I should look at before SU today? | 12:29 |
| ijohnson | s/today/right now/ | 12:29 |
| pedronis | ijohnson: #9322 could use a 3rd review (given that I worked a bit on it myself) | 12:30 |
| mup | PR #9322: boot: add call to reseal an existing key <Run nested> <UC20> <Created by cmatsuoka> <https://github.com/snapcore/snapd/pull/9322> | 12:30 |
| ijohnson | sure | 12:30 |
| pedronis | #9239 needs a 2nd review, it's small | 12:36 |
| mup | PR #9239: many: misc doc-comment changes and typo fixes <Simple 😃> <Skip spread> <Created by anonymouse64> <Merged by bboozzoo> <https://github.com/snapcore/snapd/pull/9239> | 12:36 |
| ijohnson | oh? | 12:36 |
| pedronis | heh | 12:37 |
| pedronis | I meant #9329 | 12:37 |
| mup | PR #9329: boot: consider boot chains with unrevisioned kernels incomparable <Skip spread> <UC20> <Created by pedronis> <https://github.com/snapcore/snapd/pull/9329> | 12:37 |
| ijohnson | mborzecki: are you still running arch? are you able to install the vokoscreen-ng snap via snap install ? | 12:39 |
| mup | PR snapd#9329 closed: boot: consider boot chains with unrevisioned kernels incomparable <Skip spread> <UC20> <Created by pedronis> <Merged by mvo5> <https://github.com/snapcore/snapd/pull/9329> | 12:53 |
| mup | PR snapd#9322 closed: boot: add call to reseal an existing key <Run nested> <UC20> <Created by cmatsuoka> <Merged by mvo5> <https://github.com/snapcore/snapd/pull/9322> | 12:58 |
| mup | PR snapd#9330 opened: boot: reseal when updating boot assets <UC20> <Created by bboozzoo> <https://github.com/snapcore/snapd/pull/9330> | 13:28 |
| pedronis | mborzecki: I re-reviewed #9328 | 13:47 |
| mup | PR #9328: boot: store boot chains during install, helper for checking whether reseal is needed <Run nested> <UC20> <Created by bboozzoo> <https://github.com/snapcore/snapd/pull/9328> | 13:47 |
| pstolowski | mvo: only unrelated failures in #9221 now | 13:50 |
| mup | PR #9221: tests: disk space awareness spread test <Disk space awareness> <Created by stolowski> <https://github.com/snapcore/snapd/pull/9221> | 13:50 |
| * zyga errand | 13:50 | |
| mvo | pstolowski: cool, looking | 13:53 |
| cachio | zyga, when you have a momento could you take a look again to #9316 | 13:55 |
| mup | PR #9316: tests: use full path to test-snapd-refresh.version binary <Simple 😃> <Created by sergiocazzolato> <https://github.com/snapcore/snapd/pull/9316> | 13:55 |
| ijohnson | cachio: I commented on your other similar PR that's the wrong way to fix this | 13:56 |
| ijohnson | we should figure out why /snap/bin is not on $PATH | 13:56 |
| cachio | ijohnson, you mean for the tests.tool right? | 13:59 |
| cachio | because it is on $PATH for snap run o if I just execute the snap | 13:59 |
| ijohnson | cachio: I mean this comment https://github.com/snapcore/snapd/pull/9315#issuecomment-690375166 | 14:00 |
| mup | PR #9315: tests: fix snap-routime-portal-info test <Simple 😃> <Created by sergiocazzolato> <https://github.com/snapcore/snapd/pull/9315> | 14:00 |
| cachio | ijohnson, I am not sure yet but think the issue is related to the tests.session setup, but really couldn't find where | 14:04 |
| cachio | becuause it fails just when we run using tests.session | 14:05 |
| cachio | ijohnson, I mean, this works: su -c 'snap run test-snapd-refresh.version' test | MATCH v1 | 14:06 |
| mborzecki | pedronis: cmatsuoka: hmm, current code assumes we always have a trusted assets bootloader :/ | 14:24 |
| mborzecki | and fails otherwise | 14:24 |
| mborzecki | (i mean the reseal code) | 14:24 |
| pedronis | mborzecki: we should check that outside, if it's not trusted there is nothing to do, really | 14:25 |
| pedronis | we need to cleanup bootloader interface zoo | 14:25 |
| pedronis | but basically is like that atm | 14:25 |
| pedronis | mborzecki: do you need more input on this? | 14:26 |
| pedronis | mborzecki: well, outside, maybe inside, I don't know how you structured your helpers | 14:27 |
| * cachio afk | 14:34 | |
| zyga | cachio sure | 14:38 |
| zyga | cachio replied | 14:39 |
| mup | PR snapd#9331 opened: boot: reseal when changing kernel <UC20> <Created by bboozzoo> <https://github.com/snapcore/snapd/pull/9331> | 15:14 |
| mborzecki | pedronis: mvo: cmatsuoka: more like an RFC really ^^ | 15:16 |
| mvo | mborzecki: thank you! | 15:17 |
| cmatsuoka | mborzecki: I'll check after lunch, thanks! | 15:19 |
| mvo | mborzecki: why rfc? it looks pretty reasonable from a quick look | 15:19 |
| mvo | mborzecki: anything to watch out for? | 15:19 |
| mborzecki | mvo: hope i got all the places that need to be tweaked, the tests could use more work | 15:20 |
| mborzecki | and bootloadertest.MockTrustedBootAssetsBootloader is kind of getting in the way | 15:20 |
| * mvo nods | 15:21 | |
| mborzecki | btw. the tests are spotty for calls to seal/reseal when the bootlaoder is not a trusted assets one | 15:21 |
| mborzecki | and probably the same if there's no trusted assets (even if bl implements the interface) | 15:21 |
| mborzecki | landing #9328 will make the other PRs considerably smaller | 15:24 |
| mup | PR #9328: boot: store boot chains during install, helper for checking whether reseal is needed <Run nested> <UC20> <Created by bboozzoo> <https://github.com/snapcore/snapd/pull/9328> | 15:24 |
| mborzecki | wth `src/github.com/snapcore/snapd/overlord/snapstate/check_snap.go:485:26: ambiguous selector deviceCtx.Model`? | 15:29 |
| mborzecki | but it builds just fine locally, maybe a go version change? | 15:29 |
| pedronis | mborzecki: IsResealNeed is not used yet, right? | 15:37 |
| mborzecki | pedronis: it's not | 15:38 |
| mborzecki | 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:42 |
| pedronis | mborzecki: I think the code is understable, anyway we need to land one piece at a time | 15:45 |
| pstolowski | i see recurring failures of google:ubuntu-16.04-64:tests/main/lxd:snapd_cgroup_just_inside | 15:47 |
| pedronis | yes, I mentioned that in the morning | 15:47 |
| pstolowski | right, not sure zyga saw your message | 15:51 |
| pedronis | there are some missing tests in 9328 | 15:52 |
| pedronis | mmh | 15:53 |
| mborzecki | damn, i'm late, i'll check back in later | 16:02 |
| pedronis | mborzecki: sorry, my answer was a no | 16:03 |
| pedronis | maybe it wasn't clear | 16:03 |
| cmatsuoka | mborzecki: sorry, I was away, back now | 16:13 |
| pedronis | cmatsuoka: can we sync quickly? | 16:15 |
| cmatsuoka | pedronis: sure | 16:16 |
| pedronis | cmatsuoka: going to the standup channel | 16:16 |
| pedronis | cmatsuoka: https://github.com/snapcore/snapd/pull/9328#pullrequestreview-486951806 | 16:32 |
| mup | PR #9328: boot: store boot chains during install, helper for checking whether reseal is needed <Run nested> <UC20> <Created by bboozzoo> <https://github.com/snapcore/snapd/pull/9328> | 16:32 |
| cmatsuoka | pedronis: ack | 16:32 |
| * pedronis dinner break | 16:34 | |
| cachio | ijohnson, updated the PR following the zyga's recomendation | 16:55 |
| ijohnson | cachio: ok, which pr ? | 16:55 |
| cachio | ijohnson, #9315 | 16:56 |
| mup | PR #9315: tests: fix snap-routime-portal-info test <Simple 😃> <Created by sergiocazzolato> <https://github.com/snapcore/snapd/pull/9315> | 16:56 |
| cachio | ijohnson, I think it is still open the fact that the env is not correct and needs to be fixed | 16:57 |
| cachio | ijohnson, there is another one opened with that discussion #9316 | 16:57 |
| mup | PR #9316: tests: use full path to test-snapd-refresh.version binary <Simple 😃> <Created by sergiocazzolato> <https://github.com/snapcore/snapd/pull/9316> | 16:57 |
| ijohnson | 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:57 |
| ijohnson | cachio: can you add a TODO in tests.session somewhere to debug why that doesn't work? | 16:58 |
| ijohnson | doesn't have to be in these PR's, can be a different PR | 16:58 |
| cachio | ijohnson, sure, thanks!! | 16:58 |
| pedronis | ijohnson: nested tests failed here: https://github.com/snapcore/snapd/pull/9328 we need to understand why | 17:20 |
| mup | PR #9328: boot: store boot chains during install, helper for checking whether reseal is needed <Run nested> <UC20> <Created by bboozzoo> <https://github.com/snapcore/snapd/pull/9328> | 17:20 |
| ijohnson | 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 |
| pedronis | should I just try again? | 17:21 |
| ijohnson | just a second let me finish perusing the logs | 17:22 |
| ijohnson | 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:22 |
| ijohnson | but just because they failed on this pr doesn't mean that this pr caused that | 17:23 |
| ijohnson | pedronis: yeah go ahead and restart that pr, there's nothing indicative in the logs | 17:25 |
| ijohnson | 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 | |
| pedronis | ijohnson: let's see how it goes, the PR adds writing one more file during install fwiw | 17:26 |
| pedronis | so it's not completely neutral | 17:26 |
| cachio | pedronis, hey, I am trying to fix some tests which require a model signed by canonical | 17:34 |
| cachio | 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:34 |
| cachio | because surrently the smapd-model test fails on beta validation because I am signing the models used for those images | 17:35 |
| pedronis | cachio: it depends, we have other option nowadays, you can also put serial-authority: generic in your model | 17:35 |
| cachio | pedronis, ahh, right | 17:36 |
| pedronis | the test needs changes but not skipping full bits then | 17:36 |
| cachio | My idea is just to skip small parts of the test | 17:36 |
| cachio | I'll try adding serial-authority: generic first | 17:37 |
| pedronis | 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 |
| pedronis | cachio: is it a lot of tests ? a few | 17:37 |
| pedronis | ? | 17:37 |
| cachio | pedronis, just a few | 17:37 |
| cachio | refresh-* and snap-model | 17:38 |
| pedronis | cachio: I don't have a lot of time today, if you can make a list I could look next week? | 17:38 |
| cachio | pedronis, sure | 17:38 |
| cachio | thanks | 17:38 |
| mborzecki | re | 17:55 |
| cachio | 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 | 17:59 |
| cachio | so I'll just leave the model signed by me for nested tests where we test user assertions | 18:00 |
| cachio | pedronis, does it make sense? | 18:00 |
| pedronis | 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:00 |
| cmatsuoka | hmm, this check will be needed somewhere | 18:01 |
| * cmatsuoka takes note | 18:01 | |
| mborzecki | pedronis: yes, we don't | 18:01 |
| pedronis | ok, just double checking | 18:02 |
| mborzecki | heh, https://github.com/snapcore/snapd/pull/9330 panics (the unit tests are passing though) | 18:03 |
| mup | PR #9330: boot: reseal when updating boot assets <UC20> <Created by bboozzoo> <https://github.com/snapcore/snapd/pull/9330> | 18:03 |
| pedronis | yes, I expect one I said is part of the problem | 18:04 |
| pedronis | *what I said | 18:04 |
| mborzecki | not sure why modeenv would be nil though there | 18:04 |
| pedronis | because nothing has change? | 18:05 |
| pedronis | maybe we have nop | 18:05 |
| pedronis | anyway I'm trying to look a bit around | 18:05 |
| pedronis | in the code | 18:05 |
| pedronis | to re-understand some things | 18:05 |
| mup | PR snapd#9332 opened: spread.yaml, tests/nested: misc changes <Run nested> <Simple 😃> <Created by anonymouse64> <https://github.com/snapcore/snapd/pull/9332> | 18:10 |
| mup | PR snapd#9333 opened: tests/nested, fakestore: changes necessary to run nested uc20 signed/secured tests <Run nested> <Test Robustness> <UC20> <Created by anonymouse64> <https://github.com/snapcore/snapd/pull/9333> | 18:15 |
| mup | PR snapd#9334 opened: tests/nested/manual: add test for grades above signed booting with testkeys <Run nested> <Test Robustness> <UC20> <Created by anonymouse64> <https://github.com/snapcore/snapd/pull/9334> | 18:15 |
| pedronis | mborzecki: we don't track assets at install if we are not encrpyting, but we try at update | 18:15 |
| mborzecki | pedronis: yes, we create the observer, probably shouldn't be | 18:15 |
| pedronis | yes, trying to find the cleanest way to achieve that | 18:16 |
| mborzecki | pedronis: there's a checkEncryption helper in devicestate | 18:17 |
| pedronis | I know | 18:18 |
| pedronis | we don't want to use that | 18:18 |
| pedronis | it's more whether we should encrypt first time | 18:18 |
| mup | PR snapd#9065 closed: debug: forward systemd journald output to serial console when booting for uc20 <Precious Logs> <Run nested> <Created by anonymouse64> <Closed by anonymouse64> <https://github.com/snapcore/snapd/pull/9065> | 18:20 |
| pedronis | mborzecki: we can call BeforeWrite and then Cancel together right? | 18:28 |
| * ijohnson soft-eows | 18:29 | |
| mborzecki | pedronis: cancel is called after before write, but it may also be called without before write (if we never got past the backup pass) | 18:31 |
| mborzecki | pedronis: added TODO that it makes no sense to reseal in canceled if we never resealed in before write | 18:32 |
| pedronis | mborzecki: this is not the end of the story, but does this change in itself make sense: https://paste.ubuntu.com/p/XrgxJSyrM9/ | 18:40 |
| mborzecki | pedronis: got a bit more changes, i'll push them to 9330 in a bit | 18:41 |
| pedronis | mborzecki: changes about what? | 18:42 |
| mborzecki | pedronis: tests mostly | 18:42 |
| mborzecki | pedronis: but the change in BeforeWrite is almost the same | 18:42 |
| pedronis | mborzecki: sorry, I thought you were done coding | 18:42 |
| pedronis | for the day | 18:42 |
| mborzecki | pedronis: came back for a bit, high priority thing after all | 18:43 |
| pedronis | mborzecki: yes, but maybe we need to sync, I don't think we want to do the same things twice :) | 18:44 |
| cmatsuoka | I'll merge #9328 | 18:44 |
| mup | PR #9328: boot: store boot chains during install, helper for checking whether reseal is needed <Run nested> <UC20> <Created by bboozzoo> <https://github.com/snapcore/snapd/pull/9328> | 18:44 |
| zyga | re | 18:45 |
| mup | PR snapd#9328 closed: boot: store boot chains during install, helper for checking whether reseal is needed <Run nested> <UC20> <Created by bboozzoo> <Merged by cmatsuoka> <https://github.com/snapcore/snapd/pull/9328> | 18:45 |
| mborzecki | pedronis: pushed a patch to this branch: https://github.com/snapcore/snapd/compare/master...bboozzoo:bboozzoo/uc20-reseal-with-boot-assets-wip (the last one) | 18:46 |
| mborzecki | pedronis: this one exactly: https://github.com/snapcore/snapd/commit/ed30e18ed284ce77893ea5031e3d871cbfca836e | 18:46 |
| pedronis | mborzecki: ok, better than my quick thing | 18:48 |
| mborzecki | pedronis: shall i push it? | 18:48 |
| pedronis | yes | 18:49 |
| mborzecki | ok | 18:49 |
| pedronis | code wise, that diff is all I did | 18:49 |
| pedronis | I'm thinking about something but haven't coded it yet | 18:49 |
| mborzecki | k, pushed now | 18:50 |
| mup | PR snapd#9316 closed: tests: use full path to test-snapd-refresh.version binary <Simple 😃> <Created by sergiocazzolato> <Merged by zyga> <https://github.com/snapcore/snapd/pull/9316> | 18:50 |
| mborzecki | ah, and 9328 landed, i can merge master too now | 18:50 |
| pedronis | yes | 18:51 |
| mborzecki | pushed, added 'run nested' label and closed/reopened so that we get a full run | 18:52 |
| mborzecki | eod for real this time, time to tuck the kids to bed | 18:52 |
| mup | PR snapd#9335 opened: boot: verify boot chain file in seal and reseal tests <UC20> <Created by cmatsuoka> <https://github.com/snapcore/snapd/pull/9335> | 18:55 |
| pedronis | ijohnson: could you review https://github.com/snapcore/snapd/pull/9335 ? | 19:02 |
| mup | PR #9335: boot: verify boot chain file in seal and reseal tests <Skip spread> <UC20> <Created by cmatsuoka> <https://github.com/snapcore/snapd/pull/9335> | 19:02 |
| * cmatsuoka afk for 30min, virtual school meeting for parents | 19:13 | |
| ijohnson | pedronis sure in a little bit | 19:18 |
| pedronis | thx | 19:19 |
| mvo | anything i can help withß | 19:22 |
| mvo | ? | 19:22 |
| ijohnson | +1d | 19:26 |
| pedronis | mvo: I think our reseal code has all the wrong dirs | 19:37 |
| mvo | pedronis: oh no! | 19:37 |
| mvo | ijohnson: did you see that the nested tests failed in 9334 = | 19:37 |
| mvo | ? | 19:37 |
| mvo | (sorry if I'm stating something that was already discussed) | 19:38 |
| mvo | pedronis: do you have more details somewhere? | 19:38 |
| pedronis | mvo: no, just staring at code, there's too much Install * in there | 19:38 |
| * mvo nods | 19:39 | |
| mup | PR snapcraft#3283 opened: specifications: unified provider <Created by cjp256> <https://github.com/snapcore/snapcraft/pull/3283> | 19:47 |
| pedronis | mvo: maybe they work anyway, not sure | 19:49 |
| mvo | pedronis: I suspect we have the same /run/mnt mounts in run mode | 19:49 |
| mvo | pedronis: but I need to double check | 19:49 |
| * mvo checks | 19:49 | |
| mvo | pedronis: I guess you mean InstallHostFDEDataDir = dirs.SnapFDEDirUnder(InstallHostWritableDir) which is InstallHostWritableDir = filepath.Join(InitramfsRunMntDir, "ubuntu-data", "system-data") ? we have /run/mnt/ubuntu-data also in run mode | 19:56 |
| mvo | (or am I misunderstanding?) | 19:56 |
| pedronis | yes, I mean those | 19:56 |
| pedronis | mvo: anyway #9330 can be reviewed (though I'm not sure is not missing some code to really work when we haven't encrypted) | 19:59 |
| mup | PR #9330: boot: reseal when updating boot assets <Run nested> <UC20> <Created by bboozzoo> <https://github.com/snapcore/snapd/pull/9330> | 19:59 |
| ijohnson | mvo mmm no I did not I'll have a look | 20:04 |
| cmatsuoka | pedronis: it seems to be the case, I think it will fail if the system is not encrypted | 20:04 |
| mvo | pedronis, cmatsuoka should we makr 9330 as "ready" (i.e. move it out of draft?) | 20:12 |
| mvo | it looks reasonable to me | 20:12 |
| pedronis | mvo: yes | 20:12 |
| cmatsuoka | mvo: yes, I think so | 20:15 |
| mvo | ta | 20:16 |
| * cmatsuoka is getting confused with his local branches, time to remove a few of them | 20:24 | |
| mvo | I merged master into 9331, it's a bit smaller now | 20:24 |
| pedronis | mvo: anyway I'm working on resealing only if we had sealed keys and if the boot chains are different | 20:25 |
| pedronis | that's a piece that no PR has atm | 20:25 |
| mvo | ok | 20:25 |
| pedronis | maybe we need to land 9330, maybe not | 20:25 |
| mup | PR snapd#9335 closed: boot: verify boot chain file in seal and reseal tests <Skip spread> <UC20> <Created by cmatsuoka> <Merged by cmatsuoka> <https://github.com/snapcore/snapd/pull/9335> | 20:25 |
| pedronis | sorry, I meant, maybe we need that logic already to land 9330 maybe not | 20:26 |
| mvo | pedronis: aha, right | 20:26 |
| pedronis | mvo: I looked at 9331 a bit, I think it will need a bit more work | 20:26 |
| pedronis | but haven't done a proper review | 20:26 |
| mvo | pedronis: ok, what's your hunch on what is missing? | 20:27 |
| pedronis | tests I think and the changes in bootstate20.go seems a bit too invasive (maybe they are really needd like that, not sure) | 20:28 |
| * mvo nods | 20:28 | |
| mvo | thanks | 20:28 |
| ijohnson | pedronis anything for me to review before EOW ? | 21:20 |
| pedronis | ijohnson: I'm trying to finish something but is not quite ready yet | 21:21 |
| pedronis | defeated by SnapFDEDir = SnapDeviceDirUnder(rootdir) | 21:22 |
| pedronis | spot the error | 21:22 |
| ijohnson | should it have a filepath.Join somewhere? | 21:22 |
| pedronis | no s/Device/FDE/ | 21:23 |
| pedronis | anyway apparently nothing is using it yet, except the test code I was writing | 21:23 |
| cmatsuoka | ouch | 21:23 |
| ijohnson | Ah | 21:24 |
| pedronis | now unit tests are passing, maybe I can propose this but is on top of #9330 | 21:27 |
| mup | PR #9330: boot: reseal when updating boot assets <Run nested> <UC20> <Created by bboozzoo> <https://github.com/snapcore/snapd/pull/9330> | 21:27 |
| * cmatsuoka needs to eod, but I'll check the PR at some point during the weekend | 21:36 | |
| cmatsuoka | or he will, to be consistent | 21:36 |
| pedronis | ok, I proposed #9336 on top of #9330 | 21:39 |
| mup | PR #9336: boot,many: do not reseal unless is meaningful and necessary <UC20> <Created by pedronis> <https://github.com/snapcore/snapd/pull/9336> | 21:39 |
| mup | PR #9330: boot: reseal when updating boot assets <Run nested> <UC20> <Created by bboozzoo> <https://github.com/snapcore/snapd/pull/9330> | 21:39 |
| ijohnson | I'll try to take a look later tonight | 21:39 |
| ijohnson | Have a good weekend pedronis and cmatsuoka | 21:39 |
| cmatsuoka | thanks! you too | 21:39 |
| mup | PR snapd#9336 opened: boot,many: do not reseal unless is meaningful and necessary <UC20> <Created by pedronis> <https://github.com/snapcore/snapd/pull/9336> | 21:40 |
| pedronis | seems I skipped one commit :/ | 21:42 |
| pedronis | ijohnson: I'll have to close it and open it again, it's a bit confusing atm | 21:44 |
| mup | PR snapd#9336 closed: boot,many: do not reseal unless is meaningful and necessary <UC20> <Created by pedronis> <Closed by pedronis> <https://github.com/snapcore/snapd/pull/9336> | 21:46 |
| pedronis | reproposed as #9337 | 22:05 |
| mup | PR #9337: boot,many: reseal only when meaningful and necessary <Run nested> <UC20> <Created by pedronis> <https://github.com/snapcore/snapd/pull/9337> | 22:05 |
| mup | PR snapd#9337 opened: boot,many: reseal only when meaningful and necessary <Run nested> <UC20> <Created by pedronis> <https://github.com/snapcore/snapd/pull/9337> | 22:06 |
| ijohnson | ack I'll take a look at that one then | 22:14 |
| mup | PR snapd#9315 closed: tests: fix snap-routime-portal-info test <Simple 😃> <Created by sergiocazzolato> <Merged by sergiocazzolato> <https://github.com/snapcore/snapd/pull/9315> | 22:21 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!