/srv/irclogs.ubuntu.com/2020/05/14/#snappy.txt

=== Eickmeyer is now known as Eickmeyer__
=== Eickmeyer__ is now known as Eickmeyer_E
mborzeckimorninb05:10
mborzeckimorning05:10
mborzeckimvo: good morning06:29
mvomborzecki: good morning06:30
mborzeckimvo: do we need https://github.com/CanonicalLtd/subiquity/pull/770 and https://github.com/snapcore/snapd/pull/8661 for beta?06:32
mupPR CanonicalLtd/subiquity#770: console-conf: always run when triggered for the chooser <Created by bboozzoo> <https://github.com/CanonicalLtd/subiquity/pull/770>06:32
mupPR #8661: configcore: add "service.console-conf.disable" config option <β›” Blocked> <Created by mvo5> <https://github.com/snapcore/snapd/pull/8661>06:32
mvomborzecki: it's not beta critical06:32
mvomborzecki: I will not even backport it :)06:32
mborzeckimvo: ah ok06:33
zygaGood morning06:45
mborzeckizyga: hey06:47
mvohey zyga06:50
pstolowskimorning06:59
ackkgood morning, I'm seeing a weird behavior with a core20-based snap. If I call the snapraft-runner script in a configure hook, it segfaults07:01
ackkit worked fine with core1807:01
mborzeckipstolowski: hey07:09
ackkoh, actually I might know what's the issue07:11
ackkhave snap hook always been duplicated in snap/hooks and meta/hooks ?07:16
mborzeckihmm google:ubuntu-18.04-64:tests/main/mount-ns:reboot failing?07:20
zygamborzecki: how?07:21
mborzeckizyga: https://paste.ubuntu.com/p/NSHS5rHk34/07:22
mborzeckilook slike / is moutned with different options and efivars is present07:22
zygaAre you up to date in relation to master?07:22
zygaThis changed yesterday to match the new image07:23
mborzeckiah ok, i need to update tht branch then07:23
zygaackk: hooks must be in meta/hooks07:24
zygaPerhaps snapcraft is copying them?07:24
ackkzyga, yeah I mean you put them in snap/hooks for snpcraft to find them right?07:25
zygaI don’t really know, 90% of my snaps are hand made07:25
ackkheh07:26
ackkah yeah, it seems they've always been in both places.07:28
ackkmaybe snapcraft could not populate snap/hooks?07:29
zygaThat does seem weird07:29
mupPR snapd#8667 opened: interfaces/fwupd: allow bind mount to /boot related on core <Created by woodrow-shen> <https://github.com/snapcore/snapd/pull/8667>07:38
ackkzyga, IIUC snapcraft copies snap/ as-is, with the exception of snap/local07:43
mupPR snapd#8663 closed: interfaces/desktop: silence more /var/lib/snapd/desktop/icons denials <Simple πŸ˜ƒ> <Created by jdstrand> <Merged by mvo5> <https://github.com/snapcore/snapd/pull/8663>07:57
mupPR snapd#8664 closed: interfaces/desktop: silence more /var/lib/snapd/desktop/icons denials - 2.45 <Simple πŸ˜ƒ> <Created by jdstrand> <Merged by mvo5> <https://github.com/snapcore/snapd/pull/8664>08:05
mupPR snapd#8665 closed: interfaces/desktop: silence more /var/lib/snapd/desktop/icons denials - 2.44 <Simple πŸ˜ƒ> <Created by jdstrand> <Merged by mvo5> <https://github.com/snapcore/snapd/pull/8665>08:05
pstolowskixnox: hey09:04
ackkmvo, hi, to confirm, there's currently no way via sudo/su to drop from root the snap_daemon user, right?09:09
mvoackk: hey, not sure I understand the question, can you please give me a little bit more context? what do you want to archive exactly. you are root and drop to the snap daemon-user inside the snap?09:11
ackkmvo, yes, I want to run a process as daemon user, ideally from a bash script. afaict sudo/su don't work in the snap09:13
ackkmvo, I thought we did that in maas, but actually we have a python script that does that09:15
mvoackk: I'm out of my depth here unfortunately, I think we need jdstrand to help if you can use su/sudo inside a snap to drop from root to the snap-daemon user09:15
ackkmvo, ok, np, thanks09:15
pstolowskizyga: what was the PR you wanted reviewed to unblock you (mentioned yesterday in the standup)?09:43
zygapstolowski: let me find it09:43
zygahttps://github.com/snapcore/snapd/pull/856609:43
mupPR #8566: c/snaplock/runinhibit: add run inhibition operations <Created by zyga> <https://github.com/snapcore/snapd/pull/8566>09:43
pstolowskik09:44
mborzeckiwow, https://github.com/snapcore/snapd/pull/8650 is green09:47
mupPR #8650: daemon, tests: indicate system mode, test switching to recovery and back to run <UC20> <Created by bboozzoo> <https://github.com/snapcore/snapd/pull/8650>09:47
ijohnsonackk: if you want to drop privs to snap_daemon from a shell script you need to use one of the tricks mentioned in "Usage considerations" on https://snapcraft.io/docs/system-usernames09:51
ackkijohnson, ah, thanks09:53
zygare09:53
zygasorry, helped wife with lucy for a moment09:53
zygamborzecki: makes all the test fixing worth it :)09:54
zygapstolowski: I removed the hacks around /home/ubuntu from https://github.com/snapcore/snapd/pull/8633 it now simply ignores it10:00
mupPR #8633: tests: detect and report root-owned files in /home <Test Robustness> <Created by zyga> <https://github.com/snapcore/snapd/pull/8633>10:00
zygapstolowski: also extended tests to show this10:00
pstolowskithanks10:00
zygapstolowski: and extended help to show the invariants10:00
zygapstolowski: I think it's good now10:00
zygaand rebased on master to get all the fixes10:00
zygaI didn't do the extra test suite as that's somewhat more work10:01
pstolowskizyga: naive question to the lock PR10:12
zygalooking10:12
zygapstolowski: as in when the lock file exists but is empty?10:13
zygapstolowski: if you look at unlock() that's exactly an unlocked lock :)10:13
zygapstolowski: we truncate to unlock10:13
pstolowskizyga: exists & not-empty10:13
zygaif it exists and is non-empty then it is locked10:13
pstolowskizyga: yes and if reboot happens it remains non-empty, is it going to ihibit snap run?10:14
zygaperhaps the confusing aspect is that there are two locks in one: the contents of the file and the flock protecting reading/writing the contents10:14
zygayes10:14
zygapstolowski: that's the fature10:14
zyga*feature10:14
zygait can very much happen that you reboot to uninhibit10:14
pstolowskizyga: is there a guarantee it gets released?10:14
pstolowski(i'm missing bigger picute)10:14
pstolowski*picture10:14
zygapstolowski: it's not a part of this branch10:14
zygapstolowski: it's the same question as link/unlink snap10:15
zygapstolowski: those are just the primitives10:15
zygapstolowski: and the primitives do allow having a lock for indefinite amount of time10:15
pstolowskiok. my only concern is that this interrupts a snapd op in flight and we leave snap run unusable for given snap10:15
pstolowskibut yes, i can see it belongs elsewhere to handle this10:15
zygapstolowski: if you look at the usage of this in another PR10:15
zygaI currently inihibit before unlinking10:15
zygaand uninhibit after linking10:16
zygathough my preference is to expand that period so we inhibit when we decide to refresh10:16
zygaand uninhibit when refresh is either done or aborted10:16
ijohnsonmborzecki: have you seen https://bugs.launchpad.net/snapd/+bug/1878505 ?10:20
mupBug #1878505: recovery chooser doesn't trigger on rpi with core20 <snapd:New> <https://launchpad.net/bugs/1878505>10:20
mborzeckiijohnson: hm not surprised really, it's probably a usb keyboard, so it may come up whenever during the boot10:21
ijohnsonmborzecki: yes he mentioned it was a usb keyboard, in fact he has multiple attached10:22
mborzeckiijohnson: probably after loadign the modules, and udev has to settle and create input devices by the time the trigger detection runs10:22
ijohnsonmborzecki: so perhaps the chooser should gain a systemd depends on systemd-modules-load and systemd-udev-settle (or whatever it's called) ?10:22
mborzeckiijohnson: the post-pivot one runs after snapd seeding is done iirc10:23
ijohnsonah I see10:23
mborzeckiijohnson: what i would expect to work pretty well is using gpio-keys in dtb, setting up one gpio to emit KEY_110:23
pstolowskiquick errand, bbiab10:25
mupPR snapd#8650 closed: daemon, tests: indicate system mode, test switching to recovery and back to run <UC20> <Created by bboozzoo> <Merged by mvo5> <https://github.com/snapcore/snapd/pull/8650>10:26
mborzeckiijohnson: added a note, not sure udev settle would fix that reliably tho10:33
mborzeckiit's a usb stack after all :/10:33
zygabrb10:38
ijohnsonmborzecki: ack thanks for looking at it10:44
mupPR snapd#8668 opened: cmd/snap-bootstrap/partition: add Disk, methods to cross-check mountpounts <UC20> <Created by anonymouse64> <https://github.com/snapcore/snapd/pull/8668>10:45
ijohnsonpedronis: mvo: I opened https://github.com/snapcore/snapd/pull/8668 which is the first step to cross-check, complete with some tests, not sure if y'all will want more tests or not, it's already at ~1k lines10:45
mupPR #8668: cmd/snap-bootstrap/partition: add Disk, methods to cross-check mountpounts <UC20> <Created by anonymouse64> <https://github.com/snapcore/snapd/pull/8668>10:45
pedronisijohnson: thanks I will look, we would also like to move partition somewhere else before 1.0, so need also to think whether this bits belong here or not10:46
ijohnsonok10:47
pedronis*these bits10:47
mvoijohnson: thanks10:50
zygare10:51
zygaTIL, well, not T but recently, gparted cannot resize FAT1610:52
zygathose small boot partitions, you need some hand holding and remaking from scratch to grow them10:52
ijohnsonmvo: also could you do a sudo git merge on https://github.com/snapcore/snapd/pull/8659 ?10:52
ijohnsonthe test failures are unrelated10:52
mupPR #8659: cmd/snap-bootstrap/initramfs-mounts: use booted kernel partition uuid if available <UC20> <Created by anonymouse64> <https://github.com/snapcore/snapd/pull/8659>10:52
abeatoo/ is it possible to do 'snap run --gdb' on UC? It complains about gdb command not found10:59
zygaabeato: IIRC not at present10:59
zygaabeato: we discussed some work required for that but it's not scheduled11:00
abeatook, thanks11:00
mborzeckian errand, back in 2-3h11:03
mupPR snapd#8659 closed: cmd/snap-bootstrap/initramfs-mounts: use booted kernel partition uuid if available <UC20> <Created by anonymouse64> <Merged by mvo5> <https://github.com/snapcore/snapd/pull/8659>11:03
=== Eighth_Doctor is now known as Conan_Kudo
=== Conan_Kudo is now known as Eighth_Doctor
pstolowskire11:26
mvopstolowski: thanks your view in 8531! great comments11:33
zygahmm11:33
zygathat binfmt misc change is annoying11:33
zygalet me take it out of the equation11:33
pstolowskimvo: yw, glad you found it useful11:37
mvopedronis: fwiw, I updated the vitality score pr, need to address the comemnts from pawel still though, feel free to have a look still11:43
mupPR snapd#8669 opened: tests/mount-ns: stop binfmt_misc mount unit <Test Robustness> <Created by zyga> <https://github.com/snapcore/snapd/pull/8669>11:47
zygahttps://github.com/snapcore/snapd/pull/8669 fixes random occurance of mounted of binfmt_misc brekaing mount-ns test11:47
mupPR #8669: tests/mount-ns: stop binfmt_misc mount unit <Test Robustness> <Created by zyga> <https://github.com/snapcore/snapd/pull/8669>11:47
zygamvo: ^11:55
pstolowskicachio: hi, i commented on your nested.sh PR12:04
cachiopstolowski, thanks12:04
xnoxpstolowski:  hi12:13
pstolowskixnox: hey! i've sent you an email12:14
zygamborzecki: I updated https://github.com/snapcore/snapd/pull/8656/12:17
mupPR #8656: snap-mgmt: perform cleanup of user services <Created by zyga> <https://github.com/snapcore/snapd/pull/8656>12:17
zygaI think it's okay now12:17
zygamvo: I have one tiny fix for you https://github.com/snapcore/snapd/pull/8670 -- debian packaging12:17
mupPR #8670: packaging: update sid packaging to match 16.04+ <Simple πŸ˜ƒ> <Created by zyga> <https://github.com/snapcore/snapd/pull/8670>12:17
zygaijohnson: do you want to review https://github.com/snapcore/snapd/pull/8566 -- it has +2 and I'd like to merge it to make progress12:18
mupPR #8566: c/snaplock/runinhibit: add run inhibition operations <Created by zyga> <https://github.com/snapcore/snapd/pull/8566>12:18
zygayou marked yourself as a reviewer12:18
mupPR snapd#8670 opened: packaging: update sid packaging to match 16.04+ <Simple πŸ˜ƒ> <Created by zyga> <https://github.com/snapcore/snapd/pull/8670>12:18
zygabut that was two weeks ago12:18
ijohnsonzyga: go ahead without me, I started a review but didn't finish12:19
zygaok12:19
zygathanks :)12:19
pedronisijohnson: I reviewed #8668, it probably could help if it go split at least in two, I comment as much. I need to go take of an errand, but happy to chat about it later if you have questions. I also think it should live under osutil/disks or something like that.12:24
mupPR #8668: cmd/snap-bootstrap/partition: add Disk, methods to cross-check mountpounts <UC20> <Created by anonymouse64> <https://github.com/snapcore/snapd/pull/8668>12:24
pedronis*take care of an errand12:24
pedronismvo: notice that I commented on one of pstolowski considerations and your answer12:26
* pedronis errands12:26
* zyga found a curious failure in session agent test, looking12:29
zygaa daily reminder that spread -seed is your friend12:29
mvozyga: thank you12:56
* zyga breaks for a moment, for the next meeting13:49
cachiozyga, please tell be what you mentioned about the spread user13:51
ijohnsonpedronis: yes let me finish going through your comments, splitting out is probably a good idea and fine to do, I had forgotten I still had the exported mocking methods in the file, those are used in a followup13:51
cachiozyga, couldn't hear13:51
mborzeckire14:03
zygacachio: in a moment, in another call14:04
mvopedronis: what was the second comment you had about 8351? I will work on the seeding spread test now but forgot what comment 2 was14:07
pedronismvo: we should block people from putting snapd itself in the list14:07
pedronisbecause we are not going to support making it less important than other things14:08
mvopedronis: nice, will do14:09
pedronisijohnson: I asked a question, and answered/made a suggestion to something else in that PR14:17
pedronispstolowski: mvo's PR has landed (and maybe we got new enough kernels), #8533 needs  master merge14:40
mupPR #8533: image, tests: core18 early config <Created by stolowski> <https://github.com/snapcore/snapd/pull/8533>14:40
pstolowskipedronis: ok, thanks14:40
zyga20 minutes until next call14:40
zygamvo: I feel your pain :D14:40
zygajdstrand: before the next call, do you have time to look at https://github.com/snapcore/snapd/pull/857814:43
mupPR #8578: interfaces: add system-packages-doc interface <Created by zyga> <https://github.com/snapcore/snapd/pull/8578>14:43
zygajdstrand: it's doing the same thing, just some naming changes14:43
ijohnsonthanks pedronis sounds good14:43
pedronisijohnson: made a suggestion based on your answer14:52
mborzeckipedronis: do you have a minute to talk abou the tweaks to keeping track of the current system? i implemented the changes we discussed and have some concerns that we may be overcomplicating it14:53
ijohnsonthanks yes a Dev() method is fine too14:53
ijohnsonalso a quick review on https://github.com/snapcore/snapd/pull/8652 by anybody would be great  :-)14:53
mupPR #8652: cmd/snap-bootstrap/initramfs-mounts: copy auth.json and macaroon-key in recover <Simple πŸ˜ƒ> <UC20> <Created by anonymouse64> <https://github.com/snapcore/snapd/pull/8652>14:53
pedronismvo: should I come to the next meeting? mborzecki wants to chat about something with me14:54
jdstrandzyga: before the next call? no. as mentioned yesterday when talking about the coordination email I sent, I will be getting to PR reviews today/tomorrow. I have a bunch of meetings so likely tomorrow14:54
zygajdstrand: sure, no worries14:56
jdstrandackk, mvo: you may not use sudo/su in a snap to drop to the snap_daemon user. those go through the pam stack. I outlined in https://forum.snapcraft.io/t/system-usernames/13386 with a link to code on how to do it. I believe there is something in the snapcraft realm that (cc cjp256) to make it even easier14:57
jdstrandone of the reasons why is that sudo and su use the pam stack. but you don't need pam for this14:58
mupPR snapd#8669 closed: tests/mount-ns: stop binfmt_misc mount unit <Test Robustness> <Created by zyga> <Merged by zyga> <https://github.com/snapcore/snapd/pull/8669>14:58
ijohnsonthanks zyga for the review!14:59
zyga:)14:59
zygaI need to look at the snap routine portal info test14:59
zygabut14:59
zygamore meetings14:59
mupPR snapd#8652 closed: cmd/snap-bootstrap/initramfs-mounts: copy auth.json and macaroon-key in recover <Simple πŸ˜ƒ> <UC20> <Created by anonymouse64> <Merged by anonymouse64> <https://github.com/snapcore/snapd/pull/8652>14:59
mborzeckipedronis: no worries, we can talk after your meeting15:04
cachiopstolowski, hey15:04
cachionested is working again15:05
cachiocmatsuoka, told me the problem15:05
=== erich is now known as E_Eickmeyer
pstolowskicachio: yay, that's great15:05
cachiopstolowski, I'll push in a bit15:05
pstolowskithx15:07
zygawoot15:11
pedronismborzecki: I'm available now, standup?15:12
mborzeckipedronis: ok15:12
pedroniszyga: yes, I looked at your PR, we indeed should talk, also because if I understand that's not the final place you want to put those in, right?15:12
pedronisI mean the inhibit locks15:12
zygapedronis: that's right, I'd like to grab the lock earlier, when we do the soft check and it succeeds15:13
zygapedronis: I will look at it in detail tomorrow15:13
zygamy main question was where should this live15:13
zygaas there will be more code for a non-demo version (undo, all exit paths covered, tests)15:13
cachiopstolowski, I pushed the change15:15
cachioI already tested that15:16
zygacachio: my question was about the external user15:16
zygacachio: what's the reason for using a non-root user to log into the test device in spread, when using the ad-hoc backend15:16
zygacachio: and a follow-up, if we could use regular root instead15:16
cachiozyga, I suppose that in core device we need a user15:17
cachiodifferent than root15:17
zygacachio: do you know why?15:17
pstolowskicachio: could you push this fix as a separate PR? i think this could land quickly if independent of your nested lib PR15:17
cachioI mean, if you can setup the password for root you can use root15:18
cachiozyga, but you need to specify the passwotd in the spread.yaml15:18
zygacachio: and how is that different from the external user? we specify a password for it15:18
zygacachio: I want to avoid the difference between ad-hoc and all the other backends15:19
cachiozyga, I mean, it is the same15:19
zygacachio: because we get more and more session level things15:19
zygaand now we will get a new session that only occurs in a rarely-exercised test interaction15:19
cachiozyga, we could use root15:19
cachiozyga, I dindt'15:19
cachiodidn't try with root in a core device but I dont see any restriction15:20
cachiozyga, let me make a try15:20
zygacachio: cool, if we can do it that is preferable15:20
zygacachio: if we cannot do it I will try to accommodate the external user instead15:20
cachiopstolowski, yes I can, but I'll need a bitr of time because there are few other thinks needed as well15:20
cachiozyga, ok15:21
pstolowskicachio: ah, if it is more than this 2 line change to apply it on master than maybe it's not worth it15:21
cachiopstolowski, I'll make a try after lunch15:22
cachioI need to review what is needed15:22
zygaijohnson: hey, if you have a moment, I'd love to get a 2nd review for https://github.com/snapcore/snapd/pull/8633 -- I have some extra checks piled after that, which might help with flaky tests15:23
mupPR #8633: tests: detect and report root-owned files in /home <Test Robustness> <Created by zyga> <https://github.com/snapcore/snapd/pull/8633>15:23
zygaI merged master to get some more fixes but it should otherwise pass (unless network issues show up)15:24
zygamvo: I guess I could look at proposing prompting in to master15:26
zygamvo: it needs considerable setup so it would be a distinct suite / backend perhaps15:26
cachiomvo, is it possible to add a password to root in ubuntu core?15:27
cachiozyga, ~15:27
zygahome :)15:27
ijohnsonzyga: sure I will take a look at it15:30
zygaahhh15:32
zygaI just understood why we still see some failures15:33
zygathe failover fixes15:33
zygathey don't reload root's session systemd --user15:33
zygaI'll send a follow up shortly15:33
zygathe things we learn :)15:33
pedroniszyga: btw not urgent, but I also spent some time thinking about how to name the *-tool things. we should think a good time to talk about that15:34
zygapedronis: I'm off tomorrow but we can try early next week15:34
pedronisok15:34
zygapedronis: or by email if it's low bandwidth15:34
zygapedronis: I see the problem but I also love the -tool suffix is recognizable as our internal thing and it's short15:35
pedronisyes, I agree, we need recognizable or at last standing out names15:36
pedronisbut the problem with -tool is that is not easy to know what to actually expect from them15:36
pedroniszyga: anyway this is a bit an area were it's easy to bikeshed, I might have to simply make a call about where to got at some point15:37
zygapedronis: SURE15:38
zygasure15:38
zyga:)15:38
zygasorry, tabbed out of vm with the wrong key15:38
pedronisalso we have snap-tool that being consistent should be called snapd-tool-tool15:39
pedronis:)15:39
pedronisanyway15:40
zygapedronis: snapd-metatool15:41
zygahttps://github.com/snapcore/snapd/pull/8671 should fix the cases that fail with errors like15:42
mupPR #8671: tests: reload root's systemd --user after snapd tests <Test Robustness> <Created by zyga> <https://github.com/snapcore/snapd/pull/8671>15:42
zyga- Make snap "test-snapd-user-service-sockets" (unset) available to the system (Post http://0/v1/service-control: read unix @->/run/user/0/snapd-session-agent.socket: read: connection reset by peer)15:42
mupPR snapd#8671 opened: tests: reload root's systemd --user after snapd tests <Test Robustness> <Created by zyga> <https://github.com/snapcore/snapd/pull/8671>15:42
zygaand with that I'd like to do some paperwork and EOD15:42
zygacachio: ^15:42
zygaijohnson: I should probably move the defer.sh + tac trick into a helper15:44
zygaijohnson: cleanup-tool (accepting any rename after we agree on new names)15:44
mupPR snapd#8654 closed: tests: test registration with serial-authority: [generic] <Created by pedronis> <Merged by pedronis> <https://github.com/snapcore/snapd/pull/8654>15:45
zygathis is so nice15:46
zygaI wanted to do my own model a few times15:46
zygaand now with the generic authority I can just really do that15:46
zygaand not feel like it's half baked15:46
* cachio lunch15:48
* zyga EOWs15:50
zygasee you on Monday everyone15:50
pedronispstolowski: there seem to be some selinux failures showing up in #853316:03
mupPR #8533: image, tests: core18 early config <Created by stolowski> <https://github.com/snapcore/snapd/pull/8533>16:03
pstolowskilooking16:03
mupPR snapd#8672 opened: o/devicestate: change how current system is reported for different modes <UC20> <Created by bboozzoo> <https://github.com/snapcore/snapd/pull/8672>16:09
pstolowskihmm that's weird, seems unrelated16:09
mvocachio: adding a password to the root user is pretty hard, it's /etc/passwd,shadow are readonly so we would need to play bind-mount tricks16:10
ackkis it possible to share unix sockets via a content interface between snaps?16:20
zygaackk: yes16:23
zygaackk: it's also tested16:23
zygaackk: and really works :)16:23
ackkzyga, lol. yeah I think I have misconfiguration on my side16:26
ackkthanks16:27
zyga:)16:27
zygaI'm EOD but can respond to questions with small alg16:27
zyga*lag16:27
pstolowskipedronis: ok, it's not obvious with selinux and the test alone against master didn't fail, i'll investigate tomorrow16:27
zygapstolowski: looking16:28
pstolowskizyga: it's about #853316:29
mupPR #8533: image, tests: core18 early config <Created by stolowski> <https://github.com/snapcore/snapd/pull/8533>16:29
pstolowskiafk16:29
zygaok, next week16:29
pedronisijohnson: questions: https://github.com/snapcore/snapd/pull/8668#discussion_r42530912717:27
mupPR #8668: cmd/snap-bootstrap/partition: add Disk, methods to cross-check mountpounts <UC20> <Created by anonymouse64> <https://github.com/snapcore/snapd/pull/8668>17:27
mupPR snapd#8671 closed: tests: reload root's systemd --user after snapd tests <Test Robustness> <Created by zyga> <Merged by zyga> <https://github.com/snapcore/snapd/pull/8671>17:38
mupPR snapd#8633 closed: tests: detect and report root-owned files in /home <Test Robustness> <Created by zyga> <Merged by zyga> <https://github.com/snapcore/snapd/pull/8633>17:39
ijohnsonpedronis: well iiuc from xnox is that what we are trying to do re: cross checking may be racy in that we are walking through entries in sysfs and udev, and "the-tool" currently doesn't wait for udev, it just waits for systemd-modules-load17:47
ijohnsonI think maybe making the-tool run after udev would eliminate some of the raciness, in which case maybe it's okay to say it's deterministic, but I just don't honestly know17:48
ijohnsonI have not seen it fail at any point mid way through the function with incomplete output fwiw, but that also isn't a good representation17:49
pedronisijohnson: sorry, so we write a lot of code to avoid the raciness of lsblk but actually we have raciness anyway?17:54
ijohnsonpedronis: well I don't know how much / what is racy17:55
ijohnsonpedronis: so to combat not knowing I am just assuming it's racy and doing a best effort in the function17:56
ijohnsonpedronis: if you'd rather I can just turn all of the "continue"s into return nil, err` but then there will be very many checks there and it feels all a bit fragile to me17:56
pedronisijohnson: we should step back a bit I think17:57
ijohnsonalso fun fact the "unsafe encoding" actually comes from blkid, and the format is already deprecated yet udev continues to use it:-/17:57
ijohnsonor rather the "safe encoding" format comes from blkid17:57
ijohnsonpedronis: ok, sure how should we proceed17:58
ograsnap changes18:04
ograerror: no changes found18:04
ograis there any particular reason why we consider this an "error" ?18:04
xnoxijohnson:  "currently doesn't wait for udev" => we did discuss this before, to order it after udev trigger is done....18:07
ijohnsonxnox: true but we didn't do this yet18:07
ijohnsonxnox: so if we ordered the-tool to run after udev, would you expect things in /sys/dev/block/<maj:min>/uevent to change ?18:07
xnoxijohnson:  what do you mean to change?18:08
xnoxuevent is a kernel api file there.18:08
xnoxit's dynamic write/read response api18:08
ograis "udevadm settle" not existing anymore ?18:08
ogra(that used to block until udev is done)18:09
xnoxogra:  why would you run that under systemd?18:09
xnoxwhen there is a  unit to order one after?18:09
ijohnsoni.e. in order to implement mounting of a consistent set of partitions, we need to look at all of the devices with uevent files like that that match some filter, in this case the same major number18:09
ograxnox, dunno ... probably because i'm from last century ;)18:09
ijohnsonand I'm wondering if in the course of finding the set of devices that match the same major device number, would anything read from say the first uevent we look at change by the time we are done and look at the last uevent?18:10
ijohnsonxnox: from the sounds of it you are saying it wouldn't change18:10
xnoxijohnson:  i'm not sure, why you key onto the uevent file. but the /sys/dev/block/<maj:min> numbers are stable from device inception as seen in userspace from kernel.18:10
zygahttps://github.com/snapcore/snapd/pull/8669 fixes random occurance of mounted of binfmt_misc brekaing mount-ns test18:10
mupPR #8669: tests/mount-ns: stop binfmt_misc mount unit <Test Robustness> <Created by zyga> <Merged by zyga> <https://github.com/snapcore/snapd/pull/8669>18:10
ijohnsonsorry we're not keying on the uevent file, we just need to read that file18:10
zygasorry, paste frommdauaf\t\18:10
zygakeyBa18:10
zygafghhh18:10
zygalol18:11
zygashe loves backlit keys18:11
ijohnsonreally we glob /sys/dev/block/NUM:*/uevent and read all those files basically18:11
zyga(that was Lucy smashing keys)18:11
xnoxijohnson:  yeah, reading it is fine.18:11
ijohnsonxnox: ok, next question for each device that matches the filter above, we also run `udevadm --name <devname> --query property`, would you expect the output from that to change at all from the first device we query to the last one?18:12
xnoxijohnson:  note, that will only have kerenl uevent details, not the udev uevent details. Normally one subscribes to the kernel netlink uevents and just listens the stream of them.18:12
ijohnsonxnox: again, if we ordered after udev18:12
xnoxor like subscribe to udevd's events, which have more information.18:12
ijohnsonxnox: perhaps this would be easier if you just reviewed my PR and tell me if what I'm doing looks racy/non-racy18:13
xnoxijohnson:  you are asking very low level questions, without the overall goal known.18:13
ijohnsonxnox: see disks.go in https://github.com/snapcore/snapd/pull/8668/files#diff-6c8f8423221c269ce0465372c62a975118:13
xnoxijohnson:  yeah. Normally, one uses udev apis and talks against it's database which is cached / consistent.18:13
mupPR #8668: cmd/snap-bootstrap/partition: add Disk, methods to cross-check mountpounts <UC20> <Created by anonymouse64> <https://github.com/snapcore/snapd/pull/8668>18:13
ijohnsonxnox: the overall algorithm is, we have a mountpoint, how do we get 1) the mapper partition from that mountpoint, 2) the underlying encrypted partition from that mapper partition, 3) the disk that the encrypted partition comes from18:14
xnoxsounds fun18:16
xnoxijohnson:  and you don't want to use libudev bindings to query all of those things? cause it has like apis to query all the things, including "who is parent of this device", and again, and again, etc.18:17
xnoxijohnson:  https://pkg.go.dev/github.com/farjump/go-libudev?tab=doc ? you rather have your own code, instead of using libudev1, right?18:17
* xnox is kind of surprised that snapd doesn't already use libudev bindings => it's the fastest thing to query drives18:18
xnoxand is the ultimate source of truth.18:18
ijohnsonxnox: last I heard is that we don't want cgo18:18
ijohnsonif there are pure go bindings to libudev then probably that would help, but would it help by much?18:19
xnoxthere are pures ones too, but executing udevadm imho is safer than any custom native bindings.18:22
xnoxijohnson:  you seem to do a lot of work walking all the things in /dev/ when you have all of that information in the udev database already.18:22
ijohnsonxnox: is there a way that we can get udev params from device major minor directly ?18:22
xnoxyes18:22
ijohnsonhow?18:22
ijohnsonusing udevadm directly I mean18:23
ijohnsonnot the libudev library18:23
xnoxi normally, just do `udevadm info --export-db` which exports a consistent snapshot of _everything_18:23
xnoxand in there you have all the major:minor, all the DM_UUID, all the parents already pre-declared.18:23
ijohnsonxnox is there a way you can get that in the --query output format though?18:23
xnoxxnox:  and you can ask udevadm to tell you which major:minor a given device is on18:23
ijohnsonthe format that udevadm outputs by default is slightly more complicated to parse than I would like18:24
xnoxijohnson:  there is also this:18:25
xnox$ udevadm info --device-id-of-file /boot/18:25
xnox259:518:25
xnoxwhich prints major:minor of the device a given file is on18:25
xnoxsuch that if something is mounted, you can just query give me the device of the things.18:25
ogrageezs ... allthehiddenknobs !18:25
=== KindTwo is now known as KindOne
=== KindTwo is now known as KindOne
jdstrandackk: fyi, I updated "Dropping privileges" in https://forum.snapcraft.io/t/system-usernames/13386 to discuss the 'setpriv' command with an example snap: https://git.launchpad.net/~jdstrand/+git/test-setpriv/tree/snapcraft.yaml19:01
jdstrandackk: with snapd 2.45 you can just use setpriv without LD_PRELOAD19:02
jdstrandackk: hope that helps19:02
ackkjdstrand, oh, great, thanks! that will make things easier19:03
jdstrandcool :)19:04
jdstrandfyi emitorino ^19:04
ijohnsonjdstrand: why does setpriv in snapd 2.45+ work without LD_PRELOAD?19:06
jdstrandijohnson: https://github.com/snapcore/snapd/pull/822019:07
mupPR #8220: interfaces/seccomp: allow passing an address to setgroups <Created by alfonsosanchezbeato> <Merged by mvo5> <https://github.com/snapcore/snapd/pull/8220>19:07
ijohnsonjdstrand: so does that mean that setpriv is doing the same thing as dnsmasq and is arguably a coding error?19:09
jdstrandijohnson: yes19:09
emitorinojdstrand, ack19:10
ijohnsonhmm jdstrand but couldn't upstream setpriv change to not do this and effectively break snaps that are relying on this behavior ?19:11
mupPR snapd#8673 opened: tests: enable degraded test on uc20 <Simple πŸ˜ƒ> <Created by mvo5> <https://github.com/snapcore/snapd/pull/8673>19:13
jdstrandijohnson: sure, but that is true of anything that you stage when you change bases19:15
jdstrandijohnson: fyi, this is the code: https://git.kernel.org/pub/scm/utils/util-linux/util-linux.git/tree/sys-utils/setpriv.c#n103519:19
jdstrandijohnson: but actually, the only way they could break it would be to call setgroups(<something not 0>,)19:19
jdstrandijohnson: ie, it doesn't matter what they do with the second argument (use the document NULL or something else), cause our filter no longer cares19:20
jdstranddocumented*19:20
jdstrand(and it is safe that we don't care as per the PR discussion)19:21
pedronisijohnson: so I'm looking at #8668, and even ignoring the raciness I think there's too much going on, given in the end we are going to call udevadm info, we don't need to use /sys/ that much, we can pass /sys paths to udevadm19:21
mupPR #8668: cmd/snap-bootstrap/partition: add Disk, methods to cross-check mountpounts <UC20> <Created by anonymouse64> <https://github.com/snapcore/snapd/pull/8668>19:21
pedronisdirectly19:21
ackkjdstrand, does that mean that apps using initgroups/setgroups will also work without preload in 2.45?19:34
jdstrandackk: no. this is about using setgroups(0, <something>) only19:37
jdstrandackk: before we required setgroups(0, 0). dnsmasq didn't do that so we investigated and got comfortable with setgroups(0, <something>)19:38
jdstrandackk: as it happens, that makes setpriv with --clear-groups also work19:38
jdstrandackk: setgroups with a size of > 0 is not mediatable by the sandbox, since we can look at what is in the second argument19:39
jdstrandackk: so, we can't differentiate between setgroups(1, 584788) and setgroups(1, 0)19:40
jdstrandmeh19:40
jdstrandsetgroups(1, [584788]) and setgroups(1, [0,1,1000,...])19:40
jdstrand(we can't dereference a user space point at the time of mediation as a limitation of seccomp's design and implementation)19:41
jdstrandit should be possible to mediate setgroups when apparmor grows its syscall filtering, but that is not on a near term roadmap19:42
mupPR snapcraft#3122 closed: pluginhandler: make the build environment available to all steps <bug> <Created by sergiusens> <Merged by sergiusens> <https://github.com/snapcore/snapcraft/pull/3122>19:43
* cachio afk19:43
jdstrandackk: (and initgroups() is a glibc wrapper around the setgroups system call, so it has all the same issues)19:44
jdstrandackk: erf19:45
jdstrands/since we can look at.../since we can*not* look at.../19:45
jdstrandackk: hopefully I answered your question (sorry for the typos)19:46
mupPR snapcraft#3123 opened: cmake v2 plugin: configure with $SNAPCRAFT_PART_SRC <Created by sergiusens> <https://github.com/snapcore/snapcraft/pull/3123>19:55
zygaheh20:16
zygaare we again waiting on travis?20:16
zygahttps://github.com/snapcore/snapd/pull/8566 is stuck on travis20:16
mupPR #8566: c/snaplock/runinhibit: add run inhibition operations <Created by zyga> <https://github.com/snapcore/snapd/pull/8566>20:16
zygaclose/reopen to fix it, as usual20:17
ijohnsonpedronis: part of the issue with the code tho is that udevadm just doesn't give us enough info for various bits20:30
ijohnsonWe still need to go to sysfs for various things unless there's more magical flags to udevadm that tell us what we need20:30
pedronisijohnson: what doesn't it tell us?20:31
ijohnson(sorry internet cutting out here)20:32
ijohnsonpedronis it doesn't have heirarchical info and doesn't know much about the dm devices20:32
pedronisijohnson: well the hierarchical issues are kind of the same20:33
pedronisijohnson: afaict it has the same info about dm devices that /sys has20:33
ijohnsonpedronis how do you mean20:33
ijohnsonI run udevadm on the dev mapper and it gives me very little info20:33
ijohnsonBut regardless I see your suggestion now, if you would rather me iterate with udevadm calling on the minor incremented I can do that, I don't see the advantage of that vs inspecting sysfs20:34
pedronisijohnson: there is a lot of back and forth between reading /sys and udevadm, each of that needs an error check20:35
pedronisit's all very roundabout20:35
pedronisijohnson: afaict dm name and dm uuid here are present as DM_UUID and DM_NAME20:36
ijohnsonWell I mean yes all of this is extremely roundabout since there's not a complete set of info from udev or sysfs20:36
pedronisijohnson: i still don't understand the complete info bit, they kind of have the same information20:36
pedronisin slightly different ways20:36
ijohnsonpedronis those were defined for udev env vars for me before we started mounting as /dev/mapper, but now that we use that I don't get that info20:36
ijohnsonPedronis can you share what you see running udevadm where you get that info20:37
ijohnsonPerhaps my uc20 install is buggy somehow20:37
ijohnsonpedronis: this is what I see now with udevadm20:38
ijohnsonhttps://www.irccloud.com/pastebin/xy1iR8UT/20:38
ijohnson(my internet seems to be back so back at my computer now)20:38
ijohnsonI get the same output if I try to access it by /dev/disk/by-label/ubuntu-data or by the major device number as well20:39
pedronisijohnson: I was playing with a focal system, not uc20, I get a ton more info for dm devices20:41
pedronisstrange20:41
pedronisI wonder what is different20:41
ijohnsonpedronis: yes on my focal classic system I have more info too, but not in uc2020:41
ijohnsoncould be in the options we use in creating the luks device, etc.20:42
pedronisI doubt that20:42
pedronisthis is about the mapper info20:42
ijohnsonwell I mean I really don't know, what I know is that the code I have works with the info we currently have on uc20 and that there is probably about 1000 other ways we could do the same thing20:43
pedronisijohnson: that code is full of continue, that is the part that worries me, I would really like to have as little things returning errors as possible20:44
pedronisto have to deal with20:44
pedronisand also even with less info is still seems a bit too roundabout20:45
ijohnsonpedronis: I can try to refactor it to use udevadm calls with incremented minor numbers perhaps that will make it easier/have less `continue`/`return nil, err` but I think at the end of the day unless we have some kind of magical udevadm flag or config option or something that I don't know about we're going to have to get our hands dirty with this code20:45
pedronisijohnson: like I don't think we need to do all the find DEVNAME then use the DEVNAME20:46
pedronisudevadm will happily take things under /sys/20:46
ijohnsonsure let me try to refactor that and see how much goes away20:47
pedronisijohnson: do your partion all have ID_PART_ENTRY_DISK at least ?20:49
ijohnsonpedronis: you mean physical partitions or dm partitions20:49
ijohnsondm partition does not have20:49
pedronisI mean the physical partitions20:49
ijohnsondm partition does not have ID_PART_ENTRY_DISK20:49
ijohnsonthe physical partitions all seem to have that prop though yes20:50
pedroniswell, it's not a partition20:50
pedronisit has DEVTYPE=disk20:50
pedronisbut it here it has a little bit more info20:51
pedronisbut we can deal with that20:51
ijohnsonok, but yes all of the vda* have that defined20:51
pedronisanyway as I also remarked it doesn't make sense to find all the partition, especially when we just want to find if the mount points come from the same disk20:51
pedronisit seems we need two primitives  diskFromMountPoint(path) -> major:minor20:52
pedronisdiskIter(major:minor, func(udev props) (done bool)) or something like that20:53
ijohnsonby diskIter you just mean a function which finds all the partitions for the specific disk?20:54
pedronisyes, and calls a function passing the result udevadm for each partition until the function is happy20:54
pedronisthis are the internal primitives20:54
pedronisnot what we expose as api to be clear20:54
ijohnsonseems overly complicated to me, why do that over a for loop ?20:55
mupPR snapcraft#3124 opened: storeapi: update api and error messages from push to upload <bug> <Created by sergiusens> <https://github.com/snapcore/snapcraft/pull/3124>20:55
ijohnsondo we need different kinds of searches ?20:55
pedronisijohnson: we need to mock both of those in this world20:55
ijohnsonso I think what the impl will look like is diskfrommountpoint returns a wrapper object around the major/minor and doesn't do any searching yet and then the MountPointIsFromDisk and FindMatchingPartitionUUID functions actually do the searching, in the case of mount point from disk we may not actually need to do any searching20:57
pedronisijohnson: yes, something like that20:58
ijohnsonbecause MountPointIsFromDisk really just gets the dev maj:min for the mount point and compares20:58
ijohnsonso we only have to do one search, which is for FindMatchingPartitionUUID20:58
ijohnsonsee originally I had different function names where you called one function and not two like this and it seems the refactor carried a bit too much implementation much the before the factor20:59
pedronisijohnson: yes, I still think that the helper would make clearer what's constraints and responssibility, and also make mocking a bit more limited21:00
ijohnsonright21:00
ijohnsonalright well I will re-work this a little bit later today, need to go run an errand, perhaps we can sync on this shortly tomorrow after/before SU to see if I'm in a better direction then21:00
pedronisanyway looking at a focal system it seems the extra DM_ info comes from /lib/rules.d/55-dm.rules21:01
pedronisI wonder if they are not in core 20 or something they need is not there21:01
pedronisanyway this is the initramfs21:02
pedronisso maybe it has less rules anyway21:02
ijohnsonpedronis: I have that file in /usr/lib/udev/rules.d/55-dm.rules from the core20 snap21:02
ijohnsonlet me quickly check the initramfs21:02
ijohnsonhmm actually that same file is also in the initramfs21:03
pedronisinteresting21:04
pedronisanyway as you said we are not waiting for udev, whatever that means21:04
ijohnsonyes there's a systemd udev service that runs in the initramfws21:05
ijohnsonwe should make the-tool depend on it21:05
ijohnsonbut also I've been looking at userspace where it should also have run21:05
pedronisreading that rule files it sounds that we probably have only what we got from the initrd21:10
pedronisgiven that we carry the mount device through from it21:10
pedronisso userspace doesn't matter21:10
pedronisthe question is just why that doesn't work in the initrd21:11
pedronisnot our biggest concern anyway atm, but interesting/confusing21:11
pedronisijohnson: actualy does _FLAG you have sounds like they have been turned off (maybe)21:13
pedronis*those _FLAG21:13
ijohnsonpedronis what _FLAG ?21:16
pedronisijohnson: you have things like: DM_UDEV_DISABLE_SUBSYSTEM_RULES_FLAG=121:16
pedronisin your udevadm output21:16
pedronisit sounds like things are turned off21:16
mupPR snapd#8566 closed: c/snaplock/runinhibit: add run inhibition operations <Created by zyga> <Merged by zyga> <https://github.com/snapcore/snapd/pull/8566>21:54
mupPR snapcraft#3123 closed: cmake v2 plugin: configure with $SNAPCRAFT_PART_SRC <bug> <Created by sergiusens> <Merged by sergiusens> <https://github.com/snapcore/snapcraft/pull/3123>22:13
mupPR snapcraft#3124 closed: storeapi: update api and error messages from push to upload <bug> <Created by sergiusens> <Merged by sergiusens> <https://github.com/snapcore/snapcraft/pull/3124>23:22

Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!