| mborzecki | morning | 05:47 |
|---|---|---|
| mborzecki | mvo: hey | 06:16 |
| mvo | hey mborzecki | 06:17 |
| mborzecki | mvo: idk if you've seen https://github.com/snapcore/snapd/pull/8325#issuecomment-614223448 | 06:17 |
| mup | PR #8325: snap-bootstrap: copy auth data from real ubuntu-data in recovery mode <UC20> <Created by mvo5> <https://github.com/snapcore/snapd/pull/8325> | 06:17 |
| mvo | mborzecki: how are the tests today? | 06:17 |
| mvo | mborzecki: not seen yet, looking | 06:17 |
| mvo | mborzecki: oh no :( | 06:17 |
| mvo | mborzecki: I wonder if the initrd really "reads" what we ask it to mount or if it has it's own list of things to mount | 06:18 |
| mborzecki | on a side note, i tried some tricks with snap-store and zoom-client snaps, sice one only renders boxes and theother just segfaults on arch (backtrace points to something fonts related) | 06:20 |
| mborzecki | mounting a clean tmpfs over /etc/fonts so that it dones't pick up anything from the host does not change anyting so still no luck :( | 06:20 |
| mvo | mborzecki: hrm, hrm, this is strange, does mounting tmpfs over the fontcache dirs also yield no results? | 06:22 |
| mborzecki | mvo: first thing i tried ;) | 06:23 |
| mvo | mborzecki: heh - I suspected that. sad :( | 06:27 |
| mvo | mborzecki: also strange what is bleeding into the namespace | 06:27 |
| mup | PR snapd#8506 closed: Add libnvidia-opticalflow as Nvidia library <Created by joedborg> <Merged by mvo5> <https://github.com/snapcore/snapd/pull/8506> | 06:32 |
| mup | PR snapd#8497 closed: boot/bootstate20: re-factor kernel methods to use new interface for state <UC20> <Created by anonymouse64> <Merged by mvo5> <https://github.com/snapcore/snapd/pull/8497> | 06:46 |
| zyga | hey | 06:57 |
| zyga | what's up guys? | 06:57 |
| * zyga is feeling terrible | 06:58 | |
| mborzecki | zyga: hey | 06:58 |
| zyga | since 5AM I'm awake and blowing my nose every 20-40 seconds | 06:58 |
| zyga | something I'm allergic to is blooming | 06:58 |
| zyga | mborzecki: jamie asked us to review the base permissions PR | 06:59 |
| mborzecki | mhm, added myself there | 06:59 |
| pstolowski | hi | 07:01 |
| mborzecki | pstolowski: hey | 07:01 |
| zyga | hey pawel | 07:12 |
| zyga | I will start late | 07:12 |
| zyga | sorry | 07:12 |
| mborzecki | jamesh: any idea whether the cache namespace is still per job when a matrix setup is used? | 07:16 |
| jamesh | mborzecki: I'd assume it is shared. Or is the real question whether matrixed jobs have their own github.job value? | 07:18 |
| mborzecki | jamesh: that or whether a different combination of matrix elements is a new job, thus a new namespace(?) or still the same job, so a shared namespace | 07:20 |
| jamesh | mborzecki: a job is a unit of work deployed to a single runner, so the matrix expands to multiple jobs | 07:21 |
| mborzecki | jamesh: can check that by pushing code but wanted to ask you first, since i seems you've been on that road already :P | 07:21 |
| mup | PR core18#150 closed: static: make /etc/dbus-1/session.d writable <Created by jhenstridge> <Merged by mvo5> <https://github.com/snapcore/core18/pull/150> | 08:05 |
| * zyga is unable to work for some time | 08:09 | |
| * zyga loves summer but sometimes hates the spring | 08:15 | |
| mborzecki | quitk errand, back in 30 | 08:27 |
| mup | PR snapd#8507 opened: packaging: fix build on Centos8 to support BUILDTAGS <Created by mvo5> <https://github.com/snapcore/snapd/pull/8507> | 09:07 |
| mborzecki | re | 09:17 |
| pstolowski | mvo: hey, i've found out some more about my yesterday's problem (and also recreated the image from the spread test locally). i think that for some reason the extra user defined by cloud init data is not copied to extra-users (but sshd actually works) - as if messing with /etc wrt early config somehow affected cloud init (?) | 09:21 |
| mvo | pstolowski: meh, sorry, I did not really managed to look at this yet, can ypu please paste the link again? so sorry | 09:22 |
| pstolowski | mvo: system-data/var/lib/cloud/seed/nocloud-net/ on the image looks ok to me, afaict | 09:23 |
| pstolowski | mvo: no worries, i know it was late yesterday | 09:23 |
| pstolowski | mvo: https://github.com/stolowski/snapd/tree/core18-early-config | 09:23 |
| mvo | pstolowski: aha, that is interessting, can you share the image via gdrive or something? if you boot it with "systemd.debug-shell=1" do you see anything interessting in the cloud-init logs that may indicate what went wrong? | 09:30 |
| pstolowski | mvo: let me try; sure i can upload it | 09:32 |
| * zyga found some anti-allegric meds | 09:33 | |
| mvo | pstolowski: maybe first poke a bit with the debug shell but if you don't find anything I can poke a bit too | 09:34 |
| pstolowski | mvo: yes, looking, thanks | 09:34 |
| mvo | pstolowski: the general rule is that if there is a "clash" of files/dirs in /snap/core18/current/... with the dir in the image and writable-path then there is aproblem | 09:34 |
| mvo | pstolowski: i.e. if there is /var/lib/cloud in writable-path and there is such a dir in core18 with relevant data and the image also creates it | 09:35 |
| mvo | pstolowski: does that explaination make sense? if not, we can have a quick HO | 09:35 |
| pstolowski | mvo: yes, makes sense | 09:42 |
| * zyga gets to work | 09:42 | |
| pstolowski | mvo: passing systemd.debug-shell=1 to kernel commandline has no effect (no extra tty created for it), is it expected to work with core? | 10:03 |
| zyga | try _ | 10:03 |
| zyga | pstolowski: it changed across versions | 10:03 |
| zyga | pedronis: systemd.debug_shell=1 | 10:03 |
| zyga | er | 10:03 |
| zyga | pstolowski: ^ | 10:03 |
| zyga | sorry pedronis, bad tab complete | 10:03 |
| mvo | pstolowski: it should work, this is core or core18 ? but in either case it should work :( | 10:04 |
| pstolowski | mvo: core18 | 10:04 |
| * mvo quickly double checks | 10:04 | |
| zyga | mvo: ^^^^ | 10:04 |
| zyga | mvo: systemd.debug_shell=1 | 10:04 |
| pstolowski | ah underscore? | 10:05 |
| zyga | cat /snap/core18/current/lib/systemd/system/debug-shell.service and man systemd-debug-generator | 10:06 |
| zyga | yes | 10:06 |
| zyga | it used to be - | 10:06 |
| zyga | now it is _ | 10:06 |
| mvo | pstolowski: fwiw on uc18 https://photos.app.goo.gl/UfrQFWkfRhAaEqNC6 works for me | 10:06 |
| zyga | mvo: https://github.com/systemd/systemd/blob/master/src/debug-generator/debug-generator.c#L62 | 10:07 |
| mvo | zyga: sure, just saying this works on uc18 | 10:08 |
| zyga | sure :) | 10:08 |
| zyga | shame old value doesn't work | 10:08 |
| mvo | zyga: yeah, it seems strange to break backwards compat | 10:08 |
| mvo | zyga: oh well | 10:08 |
| mvo | zyga: otoh, it's just a debug value | 10:08 |
| pstolowski | mvo: and you're getting a tty9 with a debug shell? | 10:15 |
| mvo | pstolowski: yes | 10:16 |
| mvo | pstolowski: not sure which one, I just use alt-arrow to cycle through them | 10:17 |
| pstolowski | mhmm | 10:17 |
| pstolowski | mvo: yes, it works, thank you! i had to run qemu with its ui; for some reason with -nographic it wouldn't work (i was using sendkey .. from the monitor) | 10:34 |
| mvo | pstolowski: ok | 10:36 |
| mup | PR snapcraft#3045 opened: grammar: pick from properties if attributes not in the plugin <Created by sergiusens> <https://github.com/snapcore/snapcraft/pull/3045> | 10:55 |
| pstolowski | mvo: so it appears that cloud-init is not executed at all (no logs in /var/log/). only when i start cloud-init service manually it creates extra-users. i could see that /var/lib/cloud was mounted at boot time | 10:58 |
| pedronis | pstolowski: anything interesting in /etc/cloud ? | 11:01 |
| mup | PR snapd#8508 opened: github: run all spread systems in a single go with cached results <Skip spread> <⛔ Blocked> <Created by mvo5> <https://github.com/snapcore/snapd/pull/8508> | 11:06 |
| mvo | pstolowski: what does /etc/cloud look like? any "nocloud" files anywhere? | 11:08 |
| pedronis | mvo: I am a bit confused by the caching branches, shouldn't the test be re-run if there were more commits to the PR? | 11:18 |
| pstolowski | pedronis, mvo : cloud.cfg, ds-identity.cfg, templates and cloud.cfg.d | 11:19 |
| mvo | pedronis: they will be | 11:21 |
| mvo | pedronis: it's all a bit mood right now anyway because there is a bug | 11:21 |
| pedronis | pstolowski: is there anything in /run/cloud-init ? | 11:21 |
| pstolowski | pedronis: yes, cloud.cfg, enabled, and two .log files. fwtw i run cloud-init service manually, should i check this directory right after boot of my pristine image? | 11:24 |
| === alan_g is now known as alan_g_ | ||
| pedronis | pstolowski: yes | 11:25 |
| pstolowski | pedronis: it looks the same | 11:43 |
| pedronis | pstolowski: look at the log files then, something cloud-init run | 11:45 |
| pstolowski | pedronis: there are no logs from cloud-init until i manually systemctl restart cloud-init (as if it wasn't run during the boot) | 11:47 |
| pedronis | pstolowski: sorry, then didn't look the same | 11:48 |
| pedronis | pstolowski: there's only an empty directory for /run/cloud-init? no directory? only ds-indentify stuff? | 11:48 |
| pstolowski | pedronis: /run/cloud-init & /etc/cloud look the same (and run/cloud-init has two .log files). but there are no logs under /var/log unless i start cloud-init manually | 11:50 |
| pedronis | pstolowski: sorry, I mean to look at the logs in run/cloud-init | 11:50 |
| pedronis | I know you said there are no /var/logs | 11:51 |
| pstolowski | "Found single datasource: NoCloud" in ds-identify.log; relevant? | 11:54 |
| pedronis | yes, but that's just expected | 11:55 |
| pedronis | it means it will look at user-data etc in the usual places | 11:55 |
| pedronis | pstolowski: mvo: also our early does debug-shell run? | 12:07 |
| pedronis | it might just be normally too early? | 12:08 |
| pedronis | from the description it sounds like it's meant ot run very | 12:09 |
| pedronis | early | 12:09 |
| pstolowski | pedronis: fwtw i see 'Cloud-init target reached' as one of the last messages on tty1; the other tty has console-conf waiting | 12:10 |
| mup | PR snapd#8502 closed: github: try caching test results <Skip spread> <Created by bboozzoo> <Closed by mvo5> <https://github.com/snapcore/snapd/pull/8502> | 12:10 |
| mborzecki | ehh bummer | 12:10 |
| mvo | mborzecki: well, it's not all lost, once the github cache thing is fixed we will land this! | 12:11 |
| mborzecki | mvo: thanks for trying! subscribed to https://github.com/actions/cache/issues/208 | 12:14 |
| emitorino | good morning all | 12:18 |
| pedronis | pstolowski: can you remind me? we are trying to debug why ssh doesn't work right? | 12:28 |
| pstolowski | pedronis: yes. but sshd works, just user doesn't get created | 12:28 |
| pedronis | pstolowski: ok, maybe we can do a HO sharing a screen and I can help more? | 12:30 |
| pstolowski | pedronis: and it seems the be happening when i add defaults: to gadget. although i probably need to re-try without defaults again | 12:30 |
| zyga | I made good progress today | 12:32 |
| zyga | I'm writing actually useful dbus tests now | 12:32 |
| zyga | I'll split this off this effort and propose separately after the standup | 12:32 |
| pstolowski | pedronis: sure, appreciate it | 12:33 |
| pstolowski | pedronis: is after standup fine? | 12:36 |
| pedronis | pstolowski: might after a short break after the standup | 12:37 |
| pstolowski | ok | 12:37 |
| cmatsuoka | ijohnson: does this look like the same snap upgrade problem you investigated? https://bugs.launchpad.net/snapd/+bug/1873260 | 12:46 |
| mup | Bug #1873260: All installed snaps are broken after upgrading to Focal <snapd:New> <https://launchpad.net/bugs/1873260> | 12:46 |
| ijohnson | cmatsuoka: no that looks slightly different on the surface | 12:53 |
| ijohnson | cmatsuoka: I'll respond though | 12:53 |
| cmatsuoka | ijohnson: thanks | 12:54 |
| mup | PR snapcraft#3045 closed: grammar: pick from properties if attributes not in the plugin <Created by sergiusens> <Merged by sergiusens> <https://github.com/snapcore/snapcraft/pull/3045> | 13:05 |
| zyga | ijohnson: there's a bug for this issue but I cannot find it now | 13:13 |
| ijohnson | zyga: you mean the stale snapd tools in the mount ns ? | 13:13 |
| zyga | yes | 13:13 |
| ijohnson | k, let me know if you find it but if not I'll just file a new one sometime today | 13:13 |
| zyga | ok | 13:13 |
| zyga | it's just a pretty old bug as we went to core18 with it | 13:13 |
| mup | PR snapcraft#3044 closed: build providers: use ubuntu-ports mirrors for non-x86 platforms <Created by cjp256> <Merged by sergiusens> <https://github.com/snapcore/snapcraft/pull/3044> | 13:14 |
| zyga | ijohnson: let's open a new one | 13:20 |
| zyga | ijohnson: perhaps it is reported on the forum, not in the tracker | 13:20 |
| zyga | or may have been reported in github while we still had issues | 13:20 |
| zyga | I think it must be the forum | 13:20 |
| zyga | because we discussed ideas there | 13:20 |
| zyga | https://forum.snapcraft.io/t/injecting-snapd-tools-into-base-snaps-and-keeping-them-up-to-date/12139 | 13:20 |
| zyga | ijohnson: ^^^^ | 13:20 |
| zyga | and you commented on it as the last person :) | 13:21 |
| ijohnson | right I remember your forum topic | 13:21 |
| ijohnson | :-) | 13:21 |
| zyga | please link it to the bug | 13:21 |
| zyga | and we should really make some progress and fix it | 13:21 |
| zyga | it will affect all core18 and core20 systems | 13:21 |
| ackk | hi, is this somehow intentional or should I file a bug about it: https://paste.ubuntu.com/p/5pxZwTYgx3/ ? | 13:34 |
| ackk | (inside a snap run --shell for a strictly confined snap) | 13:35 |
| ijohnson | ackk: yes because `test -r` will use the `stat`syscall which we always allow | 13:37 |
| ackk | ijohnson, won't that break stuff as if you test that you can read something and then try to read it, it fails? | 13:39 |
| ijohnson | ackk: I'll defer to jdstrand and/or zyga on why stat is always allowed, I don't recall the why, I just recall that it is | 13:39 |
| zyga | ackk: stat is always allowed by apparmor | 13:39 |
| zyga | ackk: it's not mediated IIRC | 13:40 |
| zyga | ackk: (by apparmor) | 13:40 |
| ackk | zyga, so it ends up lying because the policy is actually only enforced on open() ? | 13:40 |
| zyga | ackk: and you are correct, there is even a comment somewhere in the LSM stack about this | 13:40 |
| zyga | yes | 13:40 |
| ackk | I see | 13:40 |
| ijohnson | zyga: you mean stat is always allowed by seccomp | 13:40 |
| ackk | zyga, side, note, why is /etc/issue not readable? :) | 13:41 |
| zyga | ackk: as in, not allowed by the policy? | 13:41 |
| zyga | because anything that was allowed by the policy would be something we have to keep | 13:41 |
| zyga | so it was used as a way to limit what we need to keep across core revisions | 13:41 |
| zyga | so if you cannot read it, it's not "ABI" | 13:42 |
| zyga | as to issue specifically | 13:42 |
| zyga | just nobody asked? | 13:42 |
| zyga | ijohnson: seccomp separately | 13:42 |
| zyga | ijohnson: but apparmor just says "yes" to stat | 13:42 |
| ijohnson | right that's what I meant sorry we are in agreement | 13:42 |
| zyga | ijohnson: right | 13:42 |
| ackk | zyga, out of curiosity is that something that apparmor plans to fix (returning the truth in stat) or is that not possible? | 13:45 |
| zyga | I don't know | 13:46 |
| zyga | perhaps jjohansen or jdstrand can respond | 13:46 |
| zyga | it's possible, it's just code | 13:46 |
| ackk | SMOP :) | 13:46 |
| zyga | smop? | 13:46 |
| ackk | small matter of programming | 13:46 |
| zyga | haha | 13:46 |
| zyga | and upstreaming | 13:46 |
| zyga | Small matter of sending it to the LKML and getting it merged ;) | 13:47 |
| ackk | zyga, I meant more, not doable as filtering stat() might heavily affect performance or something | 13:47 |
| zyga | IIRC selinux does this | 13:47 |
| ackk | I guess it'd have to do a bunch of extra checks | 13:47 |
| ackk | oh, I see | 13:47 |
| jdstrand | ackk: we used to mediate it but found that basically everything needed it and required large-scale policy changes | 13:49 |
| jdstrand | there is a bug on it. let me see if we can find it | 13:50 |
| jdstrand | https://bugs.launchpad.net/apparmor/+bug/1655435 | 13:50 |
| mup | Bug #1655435: stat() unconditionally allowed via apparmor_inode_getattr() <AppArmor:Triaged> <https://launchpad.net/bugs/1655435> | 13:50 |
| ackk | jdstrand, thanks | 13:51 |
| jdstrand | it's possible to fix with a number of changes | 13:51 |
| zyga | ackk: are you using stat or access? | 13:52 |
| ackk | zyga, I saw the issue in a bash script that was doing pretty much the same thing as my paste | 13:56 |
| ackk | it seems test uses stat() | 13:56 |
| mup | Bug #1873276 opened: Deliberate use of 'partial confinement' in order to mean 'unconfined' <selinux> <Snappy:New> <https://launchpad.net/bugs/1873276> | 13:57 |
| zyga | I'm off for lunch | 13:57 |
| mup | Bug #1873276 changed: Deliberate use of 'partial confinement' in order to mean 'unconfined' <selinux> <Snappy:Invalid> <https://launchpad.net/bugs/1873276> | 14:09 |
| pstolowski | pedronis: do you have a moment now? | 14:51 |
| pedronis | pstolowski: yes, sorry the after standup was long and needed a break | 14:52 |
| pedronis | pstolowski: I'm in the standup HO | 14:54 |
| pstolowski | 1 sec | 14:54 |
| ijohnson | mvo: lxd latest/candidate snap is unbroken so I think we can merge #8505, the only unfailed test there is from an unstable system | 14:58 |
| mup | PR #8505: spread.yaml: switch back to latest/candidate for lxd snap <Test Robustness> <⛔ Blocked> <Created by anonymouse64> <https://github.com/snapcore/snapd/pull/8505> | 14:59 |
| mvo | ijohnson: cool, looking | 15:00 |
| mup | PR snapd#8505 closed: spread.yaml: switch back to latest/candidate for lxd snap <Test Robustness> <Created by anonymouse64> <Merged by mvo5> <https://github.com/snapcore/snapd/pull/8505> | 15:01 |
| ijohnson | thanks mvo | 15:01 |
| mup | PR snapd#8356 closed: cmd/snap: Implement a "snap routine file-access" command <Created by jhenstridge> <Merged by mvo5> <https://github.com/snapcore/snapd/pull/8356> | 15:35 |
| mup | PR snapcraft#3043 closed: package-repositories: initial schema and meta read/write support <Created by cjp256> <Merged by sergiusens> <https://github.com/snapcore/snapcraft/pull/3043> | 15:50 |
| jjohansen | zyga, ackk: we would love to fix the whole stat issue, doing so however requires us to get a patch upstream that some people are fundamentally opposed to | 15:55 |
| jjohansen | we haven't tried recently, it is something we should circle around to and try again | 15:56 |
| zyga | I solved the dbus signature issue | 16:00 |
| zyga | making more progress now | 16:00 |
| zyga | ijohnson: not for me, but perhaps ackk has some priority there | 16:00 |
| ijohnson | zyga: sorry missed the context, what's up? | 16:01 |
| zyga | er, jjohansen ^ | 16:01 |
| zyga | sorry I'm bad at tab completion apparently:) | 16:01 |
| zyga | or perhaps my body tells me to make coffee | 16:01 |
| zyga | or both | 16:01 |
| jjohansen | zyga: ? | 16:06 |
| jjohansen | I was responding to "(06:45:48 AM) ackk: zyga, out of curiosity is that something that apparmor plans to fix (returning the truth in stat) or is that not possible? | 16:07 |
| jjohansen | (06:46:03 AM) zyga: I don't know | 16:07 |
| jjohansen | (06:46:15 AM) zyga: perhaps jjohansen or jdstrand can respond" | 16:07 |
| * cachio lunch | 16:07 | |
| jjohansen | yes it is just code, yes we would love to fix it, no it isn't happen soon because there are people upstream who are opposed to it | 16:07 |
| jjohansen | it is one of the abilities we lost when upstreaming | 16:08 |
| jjohansen | so 12 years ago apparmor actually did mediate stat and access | 16:08 |
| pstolowski | pedronis: i've created a clean core18 with cloudinit data and diff'ed system-data before and after first boot; and the problematic image is missing those https://pastebin.ubuntu.com/p/CgdTHzTXkz/ | 16:31 |
| pstolowski | pedronis: now need to find: why :/ | 16:31 |
| * pstolowski afk | 16:33 | |
| jdstrand | thanks jjohansen :) | 16:43 |
| pedronis | mvo: ^ what Pawel mentioned seems writable path related | 17:20 |
| mvo | pedronis: totally, let me have a HO with him in the morning | 17:24 |
| mvo | pedronis: setup a meeting with him for this | 17:25 |
| pedronis | mvo: we created something in /etc/systemd/system in the image and /etc/systemd is one of the things handled by writable-paths | 17:30 |
| pedronis | we do the same with /etc/cloud but that has a strange comment in in the writable-path config | 17:30 |
| mvo | pedronis: yes, that is exactly my suspicion | 17:32 |
| mvo | pedronis: if mode is not "synced" dirs that exist will be ignored | 17:32 |
| pedronis | mvo: but if I understand the issue is kind of the reverse, the problem is that /etc/systemd/system is not empty in core18 | 17:37 |
| pedronis | (not that we can fix that easily) | 17:37 |
| mvo | pedronis: so we create a /dev/null link in /etc/systemd/system/rsyslog.service in the image already, right? that's what pawel is doing in this test? | 17:43 |
| pedronis | mvo: yes | 17:43 |
| pedronis | is not the test code to be clear, it's actual configcore code run early | 17:44 |
| mvo | pedronis: then it all makes sense, sorry, I should have had this conversation with him earlier :( https://paste.ubuntu.com/p/CwkSnnGgqB/ | 17:44 |
| mvo | 17:44 | |
| mvo | pedronis: this would fix it but the price is high | 17:44 |
| mvo | pedronis: I think we need to ponder what to do | 17:45 |
| pedronis | mvo: I think, yes, that's what we need | 17:45 |
| pedronis | anyway the commetn about /etc/cloud is also obsolete | 17:45 |
| pedronis | I think your code | 17:45 |
| pedronis | also need /etc/cloud synced | 17:45 |
| pedronis | that one is already mark as synced though | 17:45 |
| mvo | pedronis: let's talk tomorrow, I need to get dinner but I really want to explore what we can do, setting syned has it's own issues | 17:47 |
| pedronis | mvo: yes, just making clear that we have the same kind of issues both for /etc/systemd and /etc/cloud | 17:47 |
| pedronis | it's not just pawel new code | 17:48 |
| pedronis | mvo: I mean this code https://github.com/snapcore/snapd/blob/master/overlord/devicestate/handlers_install.go#L105 and the same kind of code in bootstrap | 17:49 |
| pedronis | mvo: both the base and potentially writable have files to put there | 17:50 |
| mup | PR snapcraft#3041 closed: V2 python plugin <Created by sergiusens> <Merged by sergiusens> <https://github.com/snapcore/snapcraft/pull/3041> | 18:05 |
| * zyga -> dinner | 18:27 | |
| mup | PR snapcraft#3046 opened: plugins: introduce v2.GoPlugin <Created by sergiusens> <https://github.com/snapcore/snapcraft/pull/3046> | 18:48 |
| sergiusens | zyga: stgraber lately I daily experience "Error: websocket: close 1006 (abnormal closure): unexpected EOF" which kicks me out of my "lxc exec"... is this new behavior expected? | 19:07 |
| stgraber | sergiusens: this isn't a new behavior, it happens when LXD reloads, usually because of a refresh | 19:08 |
| stgraber | if this didn't happen because of an auto-refresh, then there may be an associated LXD crash we'd want to look at | 19:09 |
| sergiusens | stgraber: container itself is not killed, right? If I were ssh'ed in I wouldn't see it | 19:09 |
| stgraber | sergiusens: correct | 19:09 |
| stgraber | we never restart containers on refresh even during upgrades, but anything connected to the daemon over the REST API does get disconnected which includes exec sessions | 19:09 |
| stgraber | we've considered some options around that but they all kinda suck | 19:10 |
| sergiusens | stgraber: refresh happened 10 minutes ago, coincides with this event, so no worries and carry on | 19:10 |
| stgraber | we could have a grace period and have the daemon tell you you're about to disconnect due to refresh | 19:10 |
| stgraber | but we have no idea how to tell you that without injecting stuff in your terminal | 19:10 |
| stgraber | which is a very bad idea on its own :) | 19:10 |
| sergiusens | that would be nice, if only to save my bash history | 19:10 |
| stgraber | as people redirect "lxc exec" to various things, including netcat, files, disk, ... so having it ever write something other than what you expect or an error would be a big issue | 19:11 |
| sergiusens | yeah, this would need some snapd/desktop intergration that you could hook into (for the local scenario) | 19:11 |
| stgraber | yeah, if the user who's logged in on a desktop happens to be in the lxd group, then we could in theory hold on the restart, notify them that they have 10min to disconnect all pending exec sessions, then continue | 19:13 |
| stgraber | we do have the API in place to list all ongoing exec sessions (lxc operation list) so it's pretty much just packaging at that stage | 19:13 |
| ijohnson | stgraber: well also if there was refresh app awareness, then you wouldn't get refreshed automatically if there's an `lxc exec` process running | 19:13 |
| stgraber | ijohnson: oh no, we definitely do want to kill those eventually | 19:14 |
| stgraber | ijohnson: a security fix in a service running as root and listening to the network, definitely trumps the inconvenience of having to re-connect to an exec session :) | 19:14 |
| ijohnson | sure, but you could at least defer a refresh for some amount of time while there are things running | 19:14 |
| sergiusens | with refresh app awareness, snapd would not trigger the refresh at all, so not sure you have a say in that if someone enables it | 19:15 |
| ijohnson | sergiusens: there will also be an api for a snap to be notified or query if there is a pending refresh | 19:15 |
| zyga | lxc is capable of escaping all tracking | 19:15 |
| * zyga is afk | 19:15 | |
| ijohnson | well true, lxc is it's own special snowflake when it comes to snapd features I guess | 19:16 |
| stgraber | the `lxc` command doesn't do any bypass that I'm aware of (other than apparmor) so yeah, that'd work, unless your LXD is clustered in which case as soon as any of the cluster nodes detects a new revision, they all self-refresh whether you like it or not | 19:16 |
| stgraber | but yeah, some kind of grace period on LXD shutdown when we have ongoing operations is probably fair | 19:17 |
| * stgraber adds to ideas list | 19:17 | |
| pedronis | yes, there will be ways to be aware, though I think we need to redefine our thinking there, because there is some assumptions about a user being present | 19:17 |
| pedronis | the design of that feature which is still wip, was driven more by desktop apps | 19:18 |
| mup | PR snapd#8509 opened: boot/bootstate20: small changes to markSuccessful <Simple 😃> <UC20> <Created by anonymouse64> <https://github.com/snapcore/snapd/pull/8509> | 19:19 |
| ijohnson | pedronis: I opened ^ because it's a simple thing, but could be considered a behavior change so I wanted to make it easy to review | 19:19 |
| pedronis | ijohnson: it doesn't read as simple at all, also how can it be a behavior change if no tests changed | 19:24 |
| ijohnson | pedronis: it's not a behavior | 19:25 |
| ijohnson | change | 19:25 |
| ijohnson | sorry maybe I should not move the comment in this PR | 19:25 |
| pedronis | ijohnson: also remember that Marksucceful is called on all kind of boths | 19:26 |
| pedronis | even non successful ones | 19:26 |
| pedronis | it has kind of a bad name | 19:26 |
| pedronis | s/boths/boots/ | 19:27 |
| ijohnson | pedronis: but is there any situation where MarkSuccessful shouldn't result in having DefaultStatus at the end of it? | 19:27 |
| ijohnson | pedronis: because that's the current behavior, I'm just making that decision to always set DefaultStatus more obvious by not making that decision in commit() and instead making that decision in markSuccessful() proper | 19:28 |
| pedronis | ijohnson: I'm just saying that you are adding a large commit that sounds like mark successful | 19:28 |
| pedronis | is there only for successful boots | 19:28 |
| pedronis | s/commit/comment | 19:28 |
| pedronis | that is not true | 19:29 |
| ijohnson | mmm, I guess in my mind, a "successful" boot is one where we get to userspace with some boot snap combo | 19:29 |
| ijohnson | and thus we get to call MarkSuccessful | 19:29 |
| ijohnson | even if the overall operation of upgrading a boot snap failed, we still eventually successfully booted something | 19:30 |
| pedronis | yes, but as I said the terminogy is confusing here | 19:31 |
| pedronis | it's very old | 19:31 |
| pedronis | we need to be careful not to give the wrong impression | 19:31 |
| ijohnson | okay, so instead of renaming all markSuccessful ish things here how about I just adjust the comment that says "always set the commit status on the bks on the bootStateUpdate we return to be empty" | 19:31 |
| ijohnson | to say something like: | 19:32 |
| pedronis | basically I'm not sure this PR is an improvement | 19:32 |
| pedronis | it adds even more action at a distance | 19:32 |
| ijohnson | "set the kernel status to be default again because we booted some kernel snap" | 19:32 |
| pedronis | but I'm probably missign the simplification that comes with it | 19:32 |
| ijohnson | pedronis: the simplification is that right now, the decision to always set kernel_status to default after calling MarkSuccessful is done in commit() | 19:33 |
| ijohnson | pedronis: I thought that was confusing because if you just read markSuccessful you won't get that impression | 19:33 |
| ijohnson | pedronis: so instead I thought let's make that more obvious and explicit by doing it in the markSuccessful function instead of commit() | 19:33 |
| ijohnson | as I said there isn't an actual behavior change, in my mind this is just making it more obvious what we are doing | 19:34 |
| ijohnson | if we are doing the wrong thing right now, then perhaps that's justification enough that we should never have made that decision to do that in commit() in the first place | 19:35 |
| ijohnson | (because it wasn't obvious) | 19:35 |
| pedronis | ijohnson: I think the issue is more that markSuccefulKernel is strange | 19:35 |
| pedronis | I probably didn't notice it yesterday | 19:36 |
| ijohnson | pedronis: well right now that function is just markSuccessful | 19:36 |
| pedronis | but remember that my plan was not to attach commitKernelStatus to that level | 19:36 |
| pedronis | so it's a bit different from my original idea | 19:36 |
| ijohnson | mm I guess it is | 19:37 |
| pedronis | it's not a problem, but reading it again in the new context, it's clear that some bits are confusing | 19:37 |
| ijohnson | after looking at it more I conclude that it's clear that it's unclear | 19:37 |
| ijohnson | well anyways I don't really need this PR, it was just going to make things a bit nicer | 19:38 |
| ijohnson | I think if this PR is going to cause us to rathole on what it means to mark something successful it's probably not worth the time right now | 19:38 |
| pedronis | basically this if is odd: bks.commitKernelStatus != bks.currentKernelStatus | 19:39 |
| pedronis | given that then we pick a constant inside it | 19:39 |
| pedronis | I don't think setting commitKernelStatus helps though | 19:39 |
| ijohnson | pedronis: well in the PR I just opened that if looks much more sane | 19:40 |
| pedronis | yes in some way, no in others | 19:40 |
| ijohnson | if bks.commitKernelStatus != bks.currentKernelStatus { ... use bks.commitKernelStatus | 19:40 |
| ijohnson | we're not using a constant there anymore | 19:40 |
| pedronis | I know | 19:41 |
| ijohnson | okay how about this, should I just get rid of commitKernelStatus on extractedRunKernelImageBootloaderKernelState and instead carry that on bootState20Kernel instead ? | 19:41 |
| ijohnson | then markSuccessfulKernel() takes in the status to set as an arg like you originally watned? | 19:41 |
| ijohnson | *wanted | 19:41 |
| pedronis | ijohnson: well, it's always going to be DefaultStatus, as far as we understand, right? | 19:42 |
| pedronis | so we don't need to pass it to it I suppose | 19:42 |
| pedronis | but we do need to pass it in to the other method | 19:42 |
| ijohnson | pedronis: what I don't want is to have the constant inside markSuccessfulKernel | 19:42 |
| ijohnson | because that function is only called from commit() | 19:42 |
| ijohnson | it feels weird to me that commit() is essentially making that decision to always use DefaultStatus | 19:42 |
| pedronis | it's a commit of a given operation, is not a general commit | 19:43 |
| pedronis | one can see it both ways | 19:43 |
| ijohnson | mmm again it seems we have something that is clearly unclear | 19:44 |
| pedronis | ijohnson: the issue really, is this, the idea of commit comes from the bootstate16 work | 19:44 |
| pedronis | there commit is fairly generic | 19:44 |
| ijohnson | yes | 19:44 |
| pedronis | we lost that property in bootstate20 | 19:45 |
| pedronis | so in obvious (constants) and non obvious ways | 19:45 |
| pedronis | there are some assumptions and semantics encoded in the commits | 19:45 |
| pedronis | that are operation related | 19:45 |
| pedronis | so as long at ther is this hybridity I'm just not seeing a big win moving things before or later than the commit | 19:46 |
| pedronis | as long as the behavior is correct and explained | 19:47 |
| pedronis | but that might just be me | 19:47 |
| ijohnson | yes this hybridness does make things a bit weird | 19:47 |
| ijohnson | I'm not sure, but let me frame the question this way, do you want me to refactor the setNextKernel to take in the status ? | 19:48 |
| pedronis | yes | 19:48 |
| ijohnson | okay, so then let me make a symmetry argument and let's just make markSuccessfulKernel take in a status too? | 19:49 |
| pedronis | I'm ok with that, is not strictly necessary but that's ok | 19:49 |
| ijohnson | ok | 19:50 |
| pedronis | ijohnson: notice that we don't this for the base either, the state is hard coded in commit | 19:53 |
| ijohnson | yes I'd say that's just as wrong as kernel tbh | 19:54 |
| pedronis | as I said, given how we organized is not wrong or not wrong | 19:54 |
| pedronis | commit is not generic | 19:54 |
| pedronis | we have 3 of them | 19:54 |
| ijohnson | I mean looking at the doc-comment for commit() it says "bootStateUpdate carries the state for an on-going boot state update. At the end it can be used to commit it" | 19:55 |
| ijohnson | it doesn't feel like the commit() method on bootStateUpdate should really be making decisions, it should just take the state that's already in bootStateUpdate and then commit it | 19:55 |
| pedronis | ijohnson: if we followed the spirit of that, we would have one commit implementation | 19:56 |
| pedronis | we don't | 19:56 |
| pedronis | as I said, is not wrong or not wrong | 19:56 |
| ijohnson | well I'd say the reason we don't have one commit implementation is for robustness reasons | 19:56 |
| ijohnson | we can't commit everything in the same order and always be robust | 19:57 |
| pedronis | I say, it's more for readability | 19:57 |
| pedronis | we could have one commit | 19:57 |
| pedronis | it would be not very readable | 19:57 |
| ijohnson | fair | 19:57 |
| ijohnson | anyways 8509 now passes in status and I unmarked "Simple" :-) | 19:58 |
| pedronis | ijohnson: I commented, I still don't think that the comment on the interface method trumps the realities of the actual implementations' code, also less state manipulation is almost always a win in my book | 20:42 |
| mup | PR snapd#8509 closed: boot/bootstate20: small changes to markSuccessful <UC20> <Created by anonymouse64> <Closed by anonymouse64> <https://github.com/snapcore/snapd/pull/8509> | 20:51 |
| ijohnson | pedronis: I don't think it's productive for either of us to pursue this more right now, I will have another PR which just makes the comment move change I wanted | 20:52 |
| pedronis | ijohnson: I think the changes to the interface where fine and improved things (less places carrying state) | 20:53 |
| ijohnson | sure I will just do that change then | 20:53 |
| mup | PR snapcraft#3047 opened: repos: fix returned strings for install_stage_packages() <Created by cjp256> <https://github.com/snapcore/snapcraft/pull/3047> | 21:03 |
| mup | Bug #1873363 opened: openvswitch interface support for ovs-appctl <Snappy:New> <https://launchpad.net/bugs/1873363> | 22:12 |
| mwhudson | how can i see the base for an installed snap? | 22:19 |
| mwhudson | if i say snap info <snap> it's talking to the store | 22:19 |
| mwhudson | ah meta/snap.yaml | 22:20 |
| mup | PR snapd#8510 opened: boot/bootstate20: small changes to bootloaderKernelState20 <UC20> <Created by anonymouse64> <https://github.com/snapcore/snapd/pull/8510> | 22:37 |
| pedronis | ijohnson: thanks ^ | 22:41 |
| ijohnson | pedronis: np, I am close to opening up the next PR which is a big refactor of the tests to enable easy writing of the new tests for the uc16 style bootloader | 22:42 |
| pedronis | ijohnson: great, I should really go to rest here though | 22:45 |
| ijohnson | of course, have a good night, talk to you tomorrow | 22:46 |
| === msalvatore_ is now known as msalvatore | ||
| mwhudson | is there a way to see the base of a snap from a non-default channel? | 23:02 |
| mwhudson | i.e. something like snap info --channel, but that doesn't exist | 23:02 |
| ijohnson | mwhudson: do you mean for a snap that's not installed? | 23:19 |
| mup | PR snapd#8511 opened: tests/boot: refactor to make it easier for new bootloaderKernelState20 impl <Test Robustness> <UC20> <Created by anonymouse64> <https://github.com/snapcore/snapd/pull/8511> | 23:21 |
| mup | PR snapd#8512 opened: boot/bootstate20: add pure bootenv bootloader implementation <Test Robustness> <UC20> <Created by anonymouse64> <https://github.com/snapcore/snapd/pull/8512> | 23:24 |
| mwhudson | ijohnson: yes | 23:31 |
| mwhudson | ijohnson: found a workaround though, download the snap and point info at that | 23:31 |
| ijohnson | mwhudson: I don't think you can do that through any snap cmd, but you can get it through the store's json | 23:32 |
| ijohnson | I just EOD'd so I don't have an example in front of me unfortunately | 23:32 |
| mwhudson | ijohnson: it's ok we thing :) | 23:32 |
| mwhudson | *think | 23:32 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!