| mborzecki | morning | 05:13 |
|---|---|---|
| mardy | 'morning! | 05:57 |
| mborzecki | mardy: hey | 06:06 |
| pstolowski | morning | 06:15 |
| mborzecki | pstolowski: hey | 06:35 |
| === zyga_ is now known as zyga | ||
| zyga | good morning :) | 07:11 |
| * zyga braces for 34C today | 07:11 | |
| zyga | ufff | 07:11 |
| pstolowski | hey zyga | 07:12 |
| * pstolowski physio | 07:49 | |
| mardy | pedronis: on the mount interface: I guess that when removing a snap, we should unmount all the mounts created by the snap, right? Or is it the snap's responsibility to do so? | 08:24 |
| pedronis | mardy: no, we should do that | 08:24 |
| === zyga_ is now known as zyga | ||
| zyga | mardy, perdronis: would a snap disable also unmount those? | 08:26 |
| zyga | it's a tricky question, because it carries into what happens when the snap refreshes, would those mounts go away ? | 08:26 |
| mardy | pedronis: OK. Any hints on where in the code we should do it? | 08:28 |
| mardy | zyga: I would expect that disable would simply disable the mount unit, without doing the unmount | 08:28 |
| pedronis | given the use case I suspect that we would do something heavy ended only on remove | 08:29 |
| pedronis | for the rest it would be up the snap to do things from the hooks | 08:29 |
| pedronis | *handed | 08:30 |
| zyga | mardy: that's interesting but also not what snapd does historically | 08:36 |
| zyga | pedronis: yeah, I think this is more practical | 08:36 |
| pstolowski | re | 08:52 |
| mardy | I still don't know where I should hook into the removal process to do the unmount... I see that Repository.DisconnectSnap() calls disconnect(), which just calls delete(). | 08:54 |
| zyga | mardy: so about that | 08:55 |
| zyga | mardy: is mount-control going to use a new interface backend? | 08:56 |
| zyga | mardy: or is it going to define something entirely new? | 08:56 |
| zyga | mardy: historically all of the interface woo woo was going on in interfaces/*.Backend type | 08:56 |
| zyga | mardy: like the mount backend writing our special .fstab files and calling snap-update-ns | 08:57 |
| zyga | mardy: what is your plan for the mount control interface? | 08:57 |
| mardy | zyga: nope, it's just a new interface, using the existing backends | 09:02 |
| mardy | let me push what I have so far | 09:02 |
| zyga | mardy: hmm | 09:03 |
| zyga | mardy: but none of the existing backends can create system-level mount units, unless you count the systemd interface which is only used by gpio | 09:03 |
| mardy | zyga: https://github.com/snapcore/snapd/pull/10473 | 09:04 |
| mardy | zyga: the mounts are not created by the interface itself, but by a new snapctl subcommand | 09:04 |
| zyga | mardy, looking | 09:05 |
| zyga | mardy, what's the mount::%s thing? | 09:06 |
| mardy | zyga: in parseStringList(), the %s can be either "options" or "type" (if I understood your question :-)) | 09:08 |
| pedronis | pstolowski: I re-reviewed https://github.com/snapcore/snapd/pull/10515 thanks for the work there | 09:10 |
| zyga | mardy, I meant the double colon | 09:10 |
| mardy | zyga: ah, I guess it's a sign I've been writing C++ for too long :-) | 09:11 |
| zyga | ah, haha | 09:11 |
| zyga | no worries, I was just curious | 09:11 |
| pstolowski | pedronis: ty! | 09:11 |
| mardy | zyga: so, the interface itself does very little: it verifies the attributes (which show what are the allowed mount options) and sets up the apparmor snippet, so that the snap could call mount itself as long as it matches the declared attributes | 09:14 |
| zyga | yeah, I see | 09:14 |
| zyga | hold on, let me read the rest | 09:14 |
| mardy | zyga: then we have a snapctl mount command, which can do the mount, and if --persistent is given, sets up a mount unit | 09:15 |
| mborzecki | pedronis: i've updated https://github.com/snapcore/snapd/pull/10510 the tests inflate the diffstat a bit | 09:18 |
| mborzecki | i should probably come up with more tests for the error path | 09:18 |
| zyga | mardy, yeah, so it's not like a typical interface for sure | 09:20 |
| zyga | I left two random comments towards the bottom | 09:20 |
| zyga | mardy, and your question on lifecycle stands | 09:20 |
| mardy | zyga: thanks! | 09:23 |
| mardy | so, interfaces don't have a method which gets called when plugs and sockets get disconnected? | 09:24 |
| pedronis | mborzecki: thx, it's in my queue, I'll see when I can look at it, lots of meetings today | 09:30 |
| mborzecki | pedronis: that's ok, thanks! | 09:30 |
| zyga | mardy, they do | 09:48 |
| zyga | mardy, it's just not that direct | 09:48 |
| zyga | mardy, what gets called is backend methods | 09:48 |
| zyga | mardy, what you connect impacts a backend specification | 09:48 |
| zyga | mardy, and in the end backend spec is used to tell the backend what to do | 09:48 |
| zyga | mardy, the final call is backend.Setup and Remove calls | 09:49 |
| zyga | mardy, everything in the middle just feeds the system | 09:49 |
| zyga | mardy, since snapctl doesn't really change the interface system, it doesn't seem that you can rely on this mechanism here for mount-control | 09:54 |
| zyga | mardy, but I may have missed something | 09:54 |
| mardy | zyga: what about the "auto-disconnect" event? Could I add a handler for it, which does the unmount? (assuming that you can have more than one handler) | 09:58 |
| zyga | mardy, auto-disconnect is when the interface is disconnected due to snap removal _I think_ | 09:58 |
| zyga | I mean | 09:58 |
| zyga | don't get me wrong | 09:58 |
| zyga | you _can_ hook to something | 09:58 |
| zyga | in the snap remove flow for sure | 09:58 |
| zyga | my point was that it's not for free by changing the mount backend | 09:59 |
| pedronis | mardy: you are going through wrappers to create the mount units? we can probably add somethign there to deal with this. Anyway because how mount units are named, we probably need to add markers anyway. We have to do that for some other kinds of files like for dbus | 10:15 |
| zyga | pedronis, oh that's a good point | 10:16 |
| zyga | there's no way to auto-detect snap mount units | 10:16 |
| zyga | because their name is decided and cannot contain a specific marker | 10:16 |
| pedronis | yes, we have a similar problem with dbus activation files | 10:17 |
| pedronis | mardy: you might want to look at wrappers/dbus.go , especially RemoveSnapDBusActivationFiles for some inspiration | 10:19 |
| pedronis | that one is called much more often, but we can have a similar helper called more precisely/rarely | 10:19 |
| ogra | should "snap logout" not force me to use sudo for snap install/remove ? https://forum.snapcraft.io/t/sudo-less-snap-commands/25441 | 10:26 |
| zyga | ogra, I _guess_ so | 10:36 |
| pstolowski | pedronis: can you land https://github.com/snapcore/snapd/pull/10511 ? unrelated failures, including deltas | 11:26 |
| pedronis | pstolowski: done | 11:34 |
| pstolowski | thanks | 11:37 |
| _moep_ | Hello! how is it possible to install via snap a package and deploy the content from .config/ to /snap/bla/current/.config/? When I don't start the snap, /snap/…/.config doesn't exist. Do you have any suggestions? | 12:19 |
| mardy | pedronis: the method to create mount units is in systemd/systemd.go; do you think I should create a wrapper for it under wrappers/ instead (maybe wrappers/services.go)? Or is it fine to use it from where it is? | 12:30 |
| pedronis | mardy: as I said we probably need to extra markers, I think it probably needs to be in wrappers calling something in systemd for symmetry | 12:33 |
| zyga | _moep_, no, because all the content in /snap/bla/$number is not a bunch of files you can modify but an entire file system that was mounted | 12:40 |
| pedronis | _moep_: if you need something from .config that is specific to your app , you can ask a personal-file access for it and add code to one of the installation hooks to copy it when the snap gets installed | 12:49 |
| _moep_ | pedronis: thx and how can I do this? | 13:28 |
| ogra | _moep_, you add a personal-files interface entry to your snapcraft.yaml, upload to the store (this goes into manual review) and file a forum post in the "store-requests" category at forum.snapcraft.io ... take a look at exsiting requests in there | 13:40 |
| _moep_ | ok thx | 13:43 |
| mardy | pedronis: it looks like there is a marker already: https://github.com/snapcore/snapd/blob/master/systemd/systemd.go#L1052 | 13:45 |
| mborzecki | damn it's hot | 13:55 |
| pedronis | mardy: well we probably need to change those description for this new units, that description is meant to indicate the mount of the snap itself | 14:02 |
| pedronis | *these | 14:02 |
| pedronis | mborzecki: I made a pass on https://github.com/snapcore/snapd/pull/10510 | 14:32 |
| mborzecki | pedronis: thanks, yeah had a bit of hard time picking the right name for post finish, nothing fit reasonably well | 14:33 |
| mborzecki | pedronis: tried to answer the question about recovery system and errors there, perhaps we need to make sure that GoundDeviceContext() is always the one with the old model, which I think will not be the case now once set device runs | 14:55 |
| mborzecki | although the state won't be updated until unlock, so it should be ok if we reboot before that point, and the contexts will be correct | 14:56 |
| pedronis | mborzecki: sorry, I don't understand your comment | 14:57 |
| mborzecki | pedronis: quick chat? | 14:58 |
| pedronis | mborzecki: I don't think my question is about recover in that sense. I mean about the recovery system recover | 14:58 |
| pedronis | mborzecki: I have meetings | 14:58 |
| pedronis | mborzecki: I mean, what happens if reboot and and do a system action recover | 14:58 |
| mborzecki | pedronis: as in by running `snap recover`? or picking a recovery system from boot list? | 14:59 |
| pedronis | picking from the boot list | 14:59 |
| pedronis | but either really | 15:00 |
| pedronis | the rules are the same afair | 15:00 |
| pedronis | I'm not too picky if we have an error, which system we can recover with, as long as there is one. My worry is that neither works | 15:00 |
| pedronis | perhaps | 15:01 |
| pedronis | does the question make sense? | 15:01 |
| pedronis | I think the test should check something about this | 15:01 |
| pedronis | *tests | 15:01 |
| mborzecki | pedronis: afaiu one of them works, depending on the state the reboot happens | 15:01 |
| pedronis | mborzecki: that's fine, would be good to check | 15:01 |
| mborzecki | pedronis: if the reboot happens after we swap the models, just the new one works, if it happens before but after try model is set, then it's either of the systems, if even before that, then just the old system | 15:02 |
| pedronis | mborzecki: can we add checks in tests about this? | 15:03 |
| mborzecki | pedronis: you mean a spread test? | 15:03 |
| pedronis | mborzecki: whatever makes sense | 15:03 |
| pedronis | I'm fine to check in the integrationy unit tests if it's trustable enough | 15:04 |
| mborzecki | the unit tests already verify which model is present in modeenv and which model is on the disk | 15:04 |
| mborzecki | pedronis: a spread test, i.e. trying to boot the system, is the only way to make sure it works | 15:04 |
| pedronis | mborzecki: my question is because of how currentSeededSystem is defined, I think there is something off | 15:07 |
| mborzecki | pedronis: we've only had one until now, so it's likely picking the first one is not quite what we want there | 15:08 |
| pedronis | mborzecki: we should probably discuss tomorrow, I'm a bit confused about this | 15:08 |
| mborzecki | pedronis: ok, let me schedule something in the morning | 15:09 |
| mborzecki | pedronis: added soemthig before desktop sync, let me know if we should try another time slot | 15:12 |
| cachio | mborzecki, ijohnson[m] pr #10409 is fixed, when you have a time could you please take a look? | 15:13 |
| pedronis | mborzecki: sounds fine | 15:23 |
| ijohnson[m] | cachio: sure, I'll take a look, also you mentioned some test was still failing somewhere and that I should take another look ? | 16:04 |
| ijohnson[m] | what test was that? | 16:04 |
| cachio | ijohnson[m], not on this pr | 16:05 |
| cachio | there are some random errors related to timeouts trying to connect to uc20 instances | 16:05 |
| cachio | I couldn't reproduce it yet | 16:05 |
| cachio | but I saw that in some github action logs | 16:06 |
| ijohnson[m] | cachio: is that what was discussed about using the wrong machine type with GCE that might be fixed with one of your pr's to spread ? | 16:06 |
| cachio | ijohnson[m], there are 2 errors | 16:11 |
| cachio | that one | 16:12 |
| cachio | and something new | 16:12 |
| cachio | I saw some connections timeout | 16:12 |
| cachio | on uc20 | 16:12 |
| cachio | and the logs I reviewed didnt show errors | 16:12 |
| cachio | ijohnson[m], I also see this error https://github.com/snapcore/snapd/pull/10409/checks?check_run_id=3068339849#step:5:184 | 16:15 |
| ijohnson[m] | hmm that last one you sent is new, I haven't seen that before, it seems like the machine just got stuck and froze | 16:17 |
| mvo | ijohnson[m]: is a squash merge of 10522 okay? for easier cherry-picking? | 16:19 |
| ijohnson[m] | let me look | 16:19 |
| ijohnson[m] | mvo: yeah that's fine | 16:19 |
| ijohnson[m] | mvo: did we decide if we need security review on this ? | 16:19 |
| cachio | ijohnson[m], https://github.com/snapcore/snapd/pull/10409/checks?check_run_id=3068339849#step:5:1756 | 16:19 |
| cachio | this test also failed | 16:20 |
| mvo | ijohnson[m]: let me quickly look at it | 16:20 |
| ijohnson[m] | huh that's interesting | 16:20 |
| cachio | it is where we are installing 2.45 | 16:20 |
| cachio | perhaps it is rebooting | 16:21 |
| cachio | for refresh | 16:21 |
| cachio | ijohnson[m], I'll try to reproduce it locally | 16:22 |
| mvo | ijohnson[m]: looks good, added one question | 16:28 |
| ijohnson[m] | mvo: do you want me to fix that typo in the PR before squash merging ? | 16:29 |
| mvo | ijohnson[m]: would be nice, but not super important. if you do it, make sure to add skip-spread to avoid a full run | 16:30 |
| ijohnson[m] | mvo: ah well too late I already pushed it | 16:30 |
| ijohnson[m] | also does skip-spread work if we already opened the PR without skip-spread ? | 16:30 |
| ijohnson[m] | I thought skip-spread is like run-nested in that it has to be closed and re-opened | 16:30 |
| mvo | ijohnson[m]: I think so but only before you push | 16:30 |
| mvo | ijohnson[m]: but maybe I'm wrong, I'm a bit out of touch | 16:31 |
| ijohnson[m] | hmm interesting, I'll have to give it a try sometime | 16:31 |
| ijohnson[m] | mvo: do you want me to cancel the run and add the label and re-try ? | 16:31 |
| mvo | ijohnson[m]: it's fine | 16:32 |
| mvo | ijohnson[m]: we are not in a rush I think(?) | 16:32 |
| mvo | ijohnson[m]: do we have a deadline for the interface? | 16:32 |
| mvo | ijohnson[m]: (again, sorry for not being on top of this :/) | 16:32 |
| ijohnson[m] | mvo yeah I think TPE needs it rather urgently unfortunately | 16:34 |
| ijohnson[m] | So sooner it's in beta the better | 16:34 |
| ijohnson[m] | Not sure if you were planning to start a release today or not | 16:34 |
| ijohnson[m] | mvo see MM | 16:35 |
| mvo | ta | 16:36 |
| cachio | ijohnson[m], https://paste.ubuntu.com/p/YmMbJ2t4fZ/ | 16:43 |
| cachio | there is an auto-refresh on the vm | 16:43 |
| ijohnson[m] | cachio: ah interesting | 16:50 |
| cachio | Is it ok to force refresh.hold in this scenario? | 16:50 |
| ijohnson[m] | cachio: yeah probably | 16:50 |
| cachio | ok, I0'll try that | 16:50 |
| cachio | ijohnson[m], so | 17:38 |
| cachio | still fails | 17:38 |
| cachio | auto-refresh is happening | 17:38 |
| cachio | perhaps the way is to update default config for pc gadget | 17:39 |
| ijohnson[m] | cachio: ah yeah that makes sense, do you remember how to do that? | 18:04 |
| ijohnson[m] | there are other nested tests which modify gadgets too | 18:04 |
| cachio | yes, I'll try to reuse that | 18:07 |
| cachio | ijohnson[m], I am not sure if it is gonna work | 18:09 |
| cachio | but I am gonna try | 18:09 |
| cachio | because the core is 2.45 and the pc gadget is the current | 18:10 |
| cachio | but it the images builds ok now it means it should work | 18:10 |
| ijohnson[m] | hmm | 18:10 |
| ijohnson[m] | cachio: could you instead unpack and repack the core snap before building the image such that it doesn't have the same hash and thus won't be refreshable since it will be installed dangerously? | 18:11 |
| cachio | sure | 18:12 |
| cachio | that is easied | 18:12 |
| cachio | easier | 18:12 |
| cachio | ijohnson[m], pushed the fix | 18:33 |
| cachio | tested and tests are passing | 18:33 |
| _moep_ | what is a good default value for refreshing snaps? (at a stable branch and no bleading edge) | 18:40 |
| ijohnson[m] | cachio: ack I'll take a look in a little bit, prepping 2.51.3 right now | 20:26 |
| ijohnson[m] | _moep_: what do you mean? default value of what ? | 20:26 |
| cachio | ijohnson[m], which changes are included in .3? | 20:35 |
| cachio | is it a bug fix? | 20:35 |
| ijohnson[m] | cachio: just a change for http response header timeouts from pawel and the sd-control interface which TPE needs very urgently | 20:36 |
| ijohnson[m] | it's a very small release | 20:36 |
| cachio | ijohnson[m], ok | 20:36 |
| cachio | I'll make sure the validation is done overnight | 20:37 |
| ijohnson[m] | cachio: it's probably fine for you to start tomorrow, no need to rush to get it started tonight | 20:37 |
| ijohnson[m] | cachio: I can only assume that the core snap is going to get stuck in review because it always does | 20:37 |
| ijohnson[m] | but maybe the snapd snap should be ready earlier | 20:38 |
| cachio | ijohnson[m], sure | 20:38 |
| ijohnson[m] | thanks cachio I'll ping you when the snaps are in the beta channel | 20:38 |
| cachio | ijohnson[m], nice, thanks | 20:39 |
| ijohnson[m] | cachio: approved #10409, some followup material but otherwise lgtm | 21:33 |
| cachio | nice, thanks | 21:33 |
| ijohnson[m] | cachio: snapd snap 2.51.3 is now in beta | 22:31 |
| cachio | ijohnson[m], nice, tests are starting now | 23:25 |
| ijohnson[m] | Great thanks | 23:27 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!