zygagood morning06:27
mborzeckizyga: hey06:36
mborzeckizyga: miserable weather, grey, dull, raining a bit :/06:38
zygamborzecki finally! :D06:51
zygamborzecki it was sunny for too long06:51
pedronispstolowski: hi07:00
zygagood morning pawel07:01
zygapstolowski could you look at the fruit of your recent reviews please?07:01
zygapstolowski https://github.com/snapcore/snapd/pull/9446 ! :)07:02
pstolowskizyga: hey, nice! i saw your tweet about it :)07:02
mvohey pstolowski and zyga07:02
pstolowskiwill look at it for sure07:02
zygahey mvo :)07:02
zygamvo I will focus on the feedback I got from samuele today07:02
zygamvo I would love if you could look at https://github.com/snapcore/snapd/pull/944607:02
zygamvo it can still be in edge before the sprint :)07:03
mvozyga: ok, let me look07:03
zygamvo it's mostly questions and some code07:06
zygabut it could be in edge by Friday ;-)07:06
zygajust saying07:06
pedronismvo: docker-smoke is failing on 20.10, failure seems real07:07
pedronismvo: https://github.com/snapcore/snapd/pull/9155/checks?check_run_id=118967704707:08
mvopedronis: uh, thanks, I have not seen this, will also look07:08
pedronismight be aa3 related07:08
zygastrconv.Atoi: parsing "file": invalid syntax.07:09
zygawe try to parse "file" as int07:09
zygawhatever that is07:09
pedronisdegville: mvo: different note, could you double check https://github.com/snapcore/snapd/pull/9424 looks07:09
pedroniszyga: not sure it's us, it seems docker is trying to do that07:10
zygaah, I see07:10
pedronis... 9424 looks good07:10
pedronispstolowski: could you look at my last commit in #9155 , I changed something ?07:11
pstolowskipedronis: sure07:13
mvopedronis: sure07:15
degvillepedronis: looks good! good catch on the blank line.07:16
mvopedronis: looks good (9424) - do you mind if I squash merge? then I will cherry pick into 2.4707:17
pedronismvo: yea, it's fine, thanks07:18
mvopedronis: thanks, merged and cherry picked07:21
mvopedronis: docker failure looks real indeed, running a spread -debug now to see what is going on07:21
mvozyga: 9446 is just 200 LOC ? woah07:22
zygamvo: it's an early draft, but yeah07:22
mborzeckimvo: can you take a look at https://github.com/snapcore/snapd/pull/9439 ? it's super simple07:26
mvozyga: first read - very very nice07:28
zygamvo \o/07:28
mvomborzecki: sure07:28
zygaman those reviews make all the difference in world07:28
zygalately all my code was "after core20" :-)07:28
mborzeckimvo: thanks!07:29
mvozyga: but we are past this now - you are IMPORTANT again ;)07:31
mvomborzecki: hm, I saw two failures of "snap sign" on arch, wonder if something changed there07:32
* zyga installs macos update, afk for coffee 07:32
mvomborzecki: merged it anyway, I don't think your code broke this ;)07:32
mborzeckimvo: thanks, let me check the log07:32
mborzeckimvo: hmm, looks like another gpg problem07:33
pedroniswe need to spend some time understanding those problems, they are more frequent, and we never spent time understanding them07:41
pedronisnothing immediate though07:41
pedronismborzecki: I reviewed #9440, it looks good, but I'm confused by how it keeps things working alone07:42
pedronis(probably my confusion)07:42
pedronismborzecki: interesting that no unit test broke?07:45
mborzeckipedronis: you're right about that snippet, need to fix that07:45
pedronispstolowski: thanks for the review07:45
mborzeckipedronis: also, the observer does not respond with ChangeIgnore atm yet, so we're still keeping the behavior where managed assets are asked upfront when creating the updater07:46
pedronismborzecki: yea07:47
mborzeckipedronis: new behavior is in #942707:48
mborzeckihm mup didn't pick that up07:48
mborzeckimup: poke poke07:48
mvopedronis: re docker> I suspect this happens because 20.10 has the new apparmor3 but the core snap has apparmor_parser2. i.e. the apparmor_parser(v2) from the core snap tries to read new apparmor.conf and does not understand it :/07:49
mborzeckilooks like it needs a poke from niemeyer07:49
zygamvo oooh07:56
zygamvo very problematic actually08:02
mvozyga: yeah, makes my head hurt :(08:02
mvozyga: I think we need to talk to security jamie about this08:02
pedronismvo: all sounded too easy08:02
zygait may seem we need apparmor 3 in core then?08:02
* zyga reboots08:02
pedroniscore18 would have the same issue08:02
pstolowskipedronis: updated #944208:09
mvopstolowski: \o/08:40
zygajamesh reviewed08:46
zygajamesh my main question there is the way we ask the store08:46
zygais this agreed with pedronis?08:46
jameshzyga: the code in the PR is a translation of some stuff I was doing client side.  There was some discussion on the forum about how to convert theme names to package names with some community feedback, but I don't think there was anything from the snapd side.08:48
jameshzyga: part of the idea was to keep the API relatively high level so it could be adapted to newer store query APIs if they become available08:49
jamesh(e.g. a search query based on slot definitions)08:50
zygaI understand that API may be still far away, I'm mostly concerned about suitability of the implementation in the meanwhile, will the store load be acceptable?08:50
zygaif that's a yes, then I have no objections08:50
jameshzyga: I expect that for the majority of users it will result in zero load on the store08:51
mborzeckipedronis: updated https://github.com/snapcore/snapd/pull/9440 now08:51
jameshzyga: most users' themes are provided by gtk-common-themes, so a store lookup would be avoided08:51
zygajamesh what will trigger the search? I understand this is a new API that's not used yet08:52
zygajamesh should we add provisions for avoiding searching on failures, e.g. to cap them at once a day or something08:52
jameshzyga: a daemon running in the user session.  It will do one check on login, and any time the theme settings are altered08:53
zygajamesh ok08:53
zygaI think we can introduce any kind of caching and rate-limiting there08:53
zygaanyway, have a look at the rest of the review, I think this was my main concern08:54
zygathe rest is trivial08:54
jameshthank you for going over it08:54
zygamy pleasure, all I need is a ping when my manager is looking at me ;-)09:01
pedronisjamesh: is it using info or find logic ?09:25
zygapedronis IIRC info09:28
jameshpedronis: it does full name based queries rather than pattern matching09:28
pedronisit should be ok (at least for a first pass), but yes we could add caching/rate limit, or discuss with the store09:37
pedronisotoh we should look into using the functionality that let us ask only for fields we really need09:37
mvomwhudson: hey, is stripping go binaries safe these days? I saw some old discussions about this in 2016 and was wondering if there is anything new happening since? would be nice for snapd :)09:41
mwhudsonmvo: should be fine yes09:41
zygamwhudson: can we strip the line table?09:41
pedronispstolowski: in a follow we can teach Iter to take an optional SetID and use that it list, so we stop reading bits of snapshots to find the files of one09:41
zygaor anything else that is somewhat optional and inflates the binary size09:42
mvomwhudson: nice, thank you09:42
mwhudsonzyga: not sure, do you care about go tracebacks?09:42
zygamwhudson not so much in that context09:42
zygamwhudson: to be clear, size above all nice extras09:43
mwhudsonzyga: can't actually remember what's in .gopclntab, it's been a while :)09:43
pstolowskipedronis: yes, indeed09:43
mwhudsonsome of the mapping tables are important09:43
pedroniszyga: heh, I would say more measure and decide, we do care about fixing bugs and traceback might be useful09:46
zygapedronis: we did measure, we are larger than the rest of the OS, we should figure out what we _can_ strip if possible09:47
zygaI agree in priciple but it's just "can we make go smaller" kind of thing09:47
pedronis#9234 btw needs 2nd reviews09:59
pedronispstolowski: what's the status of #8395 ?10:01
pstolowskipedronis: hmm a bit forgotten tbh, let me merge master and see10:02
pedronispstolowski: mvo said some tests were failing, anyway not urgent but it would be good to land it before 2.4910:03
pstolowskipedronis: yeah i'm going to check what's failing10:03
pstolowskiah just gomt i think10:04
pedronispstolowski: I merged my validation sets PR btw10:09
pstolowskiack, ty10:11
* zyga would love a front-end for strace logs that can offer either time-correlated or independent view of various processes 10:36
zygaone day, I will learn GUI programming and write that10:36
pedronispstolowski: #9442 can land I think? pstolowski, mvo: can we update #9036 with master then to use the changes pstolowski did?10:36
pstolowskiok merging, was waiting for tests10:38
mvopedronis: sure thing!10:40
mvopstolowski: should I take care of 9036 (merge master, deconflict?)10:46
pstolowskimvo: i can do it10:48
mvopstolowski: either way is fine with me10:49
mvopstolowski: so please go ahead, I will review once it's there. I added some review points last evening too that I failed to get to so far :/10:49
pstolowskimvo: ok10:49
mborzeckianyone familiar with aliases in snap declarations?11:04
mborzeckiis this correct? https://paste.ubuntu.com/p/9ZrpDYKGf9/ shouldn't the target be dotnet-sdk.dotnet?11:05
mborzeckipedronis: maybe you know ^^ this is from https://bugs.launchpad.net/snapd/+bug/189798411:05
mborzeckihm looking at the code the auto alias should be should be dotnet:dotnet i think11:07
zyga-mbpmborzecki perhaps the snap name is implicit11:08
pstolowskimvo: i've pushed just deconflict + trivial fix, will simplify now11:08
mborzeckizyga-mbp: hm the code looks for info.Apps[<alias-target>], maybe they had dotnet-sdk at some point, but there's no dotnet app11:09
mvopstolowski: nice11:09
mborzeckizyga-mbp: i mean there's no dotnet-sdk app at this point, maybe it was there before11:09
pedronismborzecki: there shouldn't be a dot there11:32
mborzeckipedronis: so name: dotnet, target: dotnet (as the app name is `dotnet`)11:33
mborzeckiack, left a note in the bug11:34
pedronismaybe we should add some validation code11:34
pedronisfor this11:34
mborzeckipedronis: or a log at least, that the the auto alias does not reference a known app11:35
pedronisno, I mean in the assertion parsing11:35
pedronismborzecki: sorry, there not dot problem, but they are pointing dotnet -> donet-sdk11:39
pedronisinstead of dotnet -> dotnet11:40
mborzeckipedronis: so this basically right? https://bugs.launchpad.net/snapd/+bug/1897984/comments/111:40
mborzeckicool, thanks for double checking11:41
pedronisthis is a bit messy though, if they really had a dotnet-sdk before and not now11:42
pedroniswe can't make it work both ways11:42
mborzeckicmatsuoka: hi, can you take a look at https://github.com/snapcore/snapd/pull/9440 ?11:45
cmatsuokamborzecki: sure, checking it11:45
mborzeckicmatsuoka: thanks!11:45
jdstrandmvo: re aa3> can you discuss with amurray? we have on our roadmap to vendor aa3 next cycle. also note, I've been running aa3 with snaps for weeks and weeks (but I don't use the docker snap). if it is an apparmor2 vs apparmor3 config file issue, the easiest thing would be to ship apparmor.conf from aa2 and use --config-file11:59
jdstrandmvo: jjohansen would also be able to help triage11:59
jdstrandmvo: that said, I don't see where snapd is shipping apparmor_parser so it should just use whatever is on the system and the system should have everything it needs?12:01
jdstrandoh, or is it the docker snap that is shipping it...12:01
pedroniswell, or using it from the base snap12:01
pedronisthe base snap does have it (for core)12:01
pedronisbut it would need interface support for that12:02
pedronisjdstrand: so we have to meditate on this:12:02
pedronisdocker_support.go:/sbin/apparmor_parser ixr,12:02
pedronismultipass_support.go:/sbin/apparmor_parser ixr,12:03
jdstrandmultipass-support and docker-support... yes12:03
pedronisit's also probably related to etc comes straight from host12:04
* jdstrand notices that core20 has apparmor_parser in /usr/sbin and those interfaces are broken12:05
jdstrandthis appears non-fatal12:07
jdstrandwell, it depends on the core12:08
pedronisjdstrand: would the user land v2 parser even work if not choking on the config with the rest of the system using aa3 ?12:09
jdstrand'yes', if it can understand the policy12:10
jdstrandgimme a sec12:10
pedronismborzecki: I fixed the dotnet alias, can you try and comment in the places?  afaict all the recent release revisons have dotnet as app, not dotnet-sdk12:10
mborzeckipedronis: sure12:11
mborzeckipedronis: thanks, it work snow, the alias was created12:12
pedronismborzecki: if they had current versions around with app dotnet-sdk and some with dotnet, atm we could have made only one of the two work12:13
pedronisbut if people are updated to what is on the channels it should work12:13
pedronis /on the/in the/12:14
zyga_while this is not a solution, I really really think we need to plan a way out of shared /etc12:14
zyga_it was a huge mistake, we should only share specific files, perhaps with a filter even12:14
=== zyga_ is now known as zyga
pedroniszyga: yes, I would like to do that, but at this point is a 22 goal12:15
pedronis(and maybe even that is optimistic)12:15
zygaas long as it's on the roadmap somewhere, I'm happy12:15
zygaI think we may have our hand forced at some point12:16
jdstrandpedronis and zyga (cc mvo and amurray): https://paste.ubuntu.com/p/d82JTRMdxz/12:21
jdstrandI'm going to file a bug12:22
jdstrandpedronis and zyga (cc mvo and amurray): https://bugs.launchpad.net/snapd/+bug/1898038. I've assigned amurray for the moment, but if someone else is going to jump on it, please let him know12:27
* jdstrand adds a 20.10 Ubuntu task too12:27
jdstrandstgraber: you might be interested in that bug too ^ (that said, I've been using lxd with apparmor3 for weeks and weeks and you seem ok)12:29
jdstrandamurray: we should update our test plan to include docker, lxd and multipass snaps12:32
pedroniszyga: I commented on #770012:33
cmatsuokamborzecki: done12:34
mborzeckicmatsuoka: thanks, let me land that and update the main branch12:34
pedronispstolowski: mvo: should I look myself at #9036 (snapshot import) or is it premature?12:35
pstolowskipedronis: 1 minute, i'm just finishing actual changes12:36
pstolowskiso far only pushed deconflicting changes12:36
jdstrandamurray: note, snap-confine has facilities for this sort of thing, but the core snaps don't ship /etc/apparmor and /etc/apparmor.d currently, so they'll need to be adjust to not strip those out12:38
* jdstrand steps away for a bit12:38
pstolowskipedronis: i pushed simplifications after changes in master. but this PR still needs more work to address all comments12:39
* zyga lunch12:40
zygapedronis thank you12:42
pedronispstolowski: made some small comments12:47
mvopedronis: thanks! how do you feel about it overall?12:49
pedronismvo: I don't know, I mostly looked at backend12:50
* pstolowski lunch13:56
pedronismborzecki: I did a pass on #944314:16
mborzeckipedronis: thanks!14:18
pedronispstolowski: how annoying would be to split the import PR into two ? (overlord/snapshotstate and the rest) it's not very big but it touches a lot files14:22
mvopedronis, pstolowski I can do that if it helps you, I can do the api bits maybe first? those look ok14:26
mvo(to me914:26
pedronisit would help a little bit my reviewing14:30
mvopedronis: sure, let me do this now, we may lose a bit of history unless I cherry pick stuff which will be super messy14:34
jdstrandSaviq: fyi, you are likely affected by https://bugs.launchpad.net/snapd/+bug/1898038. see backscroll starting ~2.5 hours ago14:49
pstolowskimvo, pedronis sorry, had a late lunch. sounds good. i'll focus on addressing the comments to backend part14:50
mvopstolowski: thank you!14:51
mvozyga: do you need anything from me?14:51
mvopedronis: I pushed 945014:51
zygamvo: a review on the notification thing would be great14:51
zygamvo: nothing else from the top of my head14:51
mvozyga: the one that actually sents the notifications (i.e. the latest one?)14:52
zygacachio I did a pass through most or all of your PRs14:58
cachiozyga, thanks!!14:58
cachioI'll take a look now14:58
ijohnsonhey folks14:59
* ijohnson is back finally14:59
ijohnsonmvo: jdstrand: sounds like docker on groovy stuff got sorted out and triaged at least ?15:01
zygaijohnson: I think we have a life jacket at least15:01
ijohnsonthat's nice15:01
ijohnsonbtw do layouts to put things from $SNAP onto /etc/... work ?15:02
zygaijohnson we should plan something more oomhpy over time and there are some proposals (bundle aa with snapd, stop using host's /etc, fix immediately with mount profiles when aa-capable interfaces are connected)15:02
ijohnsonlast time I tried that it didn't work because snap-update-ns died saying that trying to do the specified layout would modify the host15:02
ijohnsonbut maybe I was doing something else wrong15:02
zygaijohnson: we can do better, each interface that can use apparmor_parser could result in hiding real /etc/apparmor*15:02
ijohnsonzyga: yes that makes sense15:02
* zyga stops coding now, stops reviewing, need to take a break and work with the kids a little15:03
zygaI'll be back late to iterate on my own PRs a little15:03
jdstrandijohnson: layouts were designed to handle /etc... the idea is that the snap could ship its own /etc/apparmor and /etc/apparmor.d and then have a layout for it15:03
jdstrandijohnson: that is the escape hatch to unbreak things without snapd changes. snapd can do better though. and yes, it is triaged15:04
jdstrand(and assigned)15:04
jdstrandijohnson: hello btw :)15:04
* jdstrand wouldn't mind jumping on this himself...15:05
ijohnsonhi :-)15:05
zygajdstrand what do you think about the idea I mentioned?15:05
ijohnsonyeah I udnerstand that layouts are supposed to work on /etc, I just seem to remember some problem with them on /etc but perhaps I am mistaken15:05
zygaijohnson it IS a problem because etc is unpredictable15:05
zygaif a distribution provides /etc/apparmor -> /etc/apparmor-2.0 symlink15:05
zygawe are broken15:05
zygait's not a nice solution to force this on snap authors IMO15:06
jdstrandthat isn't the solution15:06
ijohnsonzyga: ah right I think that was what I hit, is that something on my host /etc/ was interfering with setting up the mimic15:06
jdstrandthat is just if the publisher insists on unbreaking themselves before the solution is in place15:06
zygaah, I misunderstood then15:06
zygayes, the publisher has an escape hatch as well15:07
jdstrandzyga: doing it in the interface could be nice since only two interfaces need this behavior. the interface code also understands what base is in use and can mount the correct one15:08
jdstrandzyga: perhaps add a comment to the bug how you think this might work for amurray?15:08
zygaand it's more open than a layout (which is by design more constrained)15:08
zygajdstrand I did :)15:08
* jdstrand reads15:09
zygawell, terse but so :)15:09
* zyga EPDs15:15
zygaEODs even15:17
* cachio lunch15:17
jdstrandzyga: ok, commented in the bug. enjoy your evening15:27
pedronismvo: I reviewed #945015:45
mvopedronis: thanks! working through it right now15:49
mvopstolowski: please push any WIP for the import pr before you EOD, I can pick up from that then :)15:49
pstolowskimvo: ok, i addressed the couple of minor points, but the big one is comparing hashes, pondering about that now15:52
pedronispstolowski: to be clear, that probanly can be/should be its own PR15:54
pedronisbut I didn't want us to forget about it15:55
pstolowskipedronis: ah, ok, thanks15:55
pedronisat least you can put a TODO in that fuction? is it even the place where we would do this?15:55
mvopstolowski, pedronis thanks, +1 for followup16:00
mvopedronis: looking at /v2/snapshots right now, I think that's what I will do16:01
pstolowskimvo: pedronis do we want to verify hashes at // XXX: check the snapshot hashes or in a followupt oo16:01
pedronispstolowski: that sounds like something we should to be sure the thing wasn't corrupted?16:02
pstolowskipedronis: indeed, mostly asking if we want it in this PR or followup16:03
pedronispstolowski: we have code for check-snapshot, maybe some part of it can be reused16:03
pedronisnot sure16:03
pstolowskiactually, Check16:03
mvopedronis: do you have a recommendation for the content-type? application/snapd-snapshot-v1 ? or too outlandish? versioning seems to be uncommon :/16:05
pstolowskiit would helped if Iter() wasn't assuming dirs.SnapshotsDir but again it probably should be a prerequisite (down a rabbit hole ;))16:08
pstolowskihmm, maybe not neccessary16:11
mvopedronis: thanks, just saw your updated review! I will address after I made dinner for the kids16:13
mvopedronis: I pushed a stawman for the content-type too now please comment in the PR :)16:13
pedronismvo: yea, I looked at what save does16:13
* mvo needs to vanish for ~45min or so16:13
pstolowskimvo: pushed a few more changes17:11
pstolowskimvo: added Check() and a very rudimentary test for it.. would love to have a good test, but it's tricky. i may take a look at it tomorrow with clear head17:13
pedronismvo: I changed my thinking slightly: https://github.com/snapcore/snapd/pull/9450#discussion_r498399310 to be consistent with the design as it was discussed17:17
mvopedronis: yeah, noticed and update17:51
pedronismvo: I made another comment (but maybe I'm confused), anyway I don't think #9450 needs my +1 at this point, my main concern were addressed, but I don't think I'm up to do a final close review right now18:04
pedronisif I'm making sense18:08
mvopedronis: sure, that's fine18:25
mvopedronis: I will ask the others tomorrow to review the details, thanks a bunch!18:25
ijohnsonpedronis: got a second for quick question on how to set a client.Client config option for cmd/snap ? I want to specify a Timeout to the http.Client we use for our snapd client.Client, because currently there is not one, but I only need it for the console-conf routine command, not for every command in snap18:32
ijohnsonpedronis: I was thinking about just tearing out the existing client from my Execute() and recreating a client, but that seems fragile and wrong18:32
ijohnsonor maybe it's okay to have a reasonable Timeout for all snap commands18:34
pedronisijohnson: there is kind of a timeout for all client requests, no?19:12
ijohnsonpedronis I don't see an http.Client Timeout set for the client that we use from clientMixIn19:13
ijohnsonMaybe I'm reading the wrong code19:13
pedronisijohnson: no, but client itself sets up a timeout via context, for everything except things that use client.raw directly19:14
ijohnsonAh right I forgot about contexts timeout19:14
* ijohnson goes to go look at that 19:15
pedronisdo uses doTimeout which goes into rawWithTimeout19:15
pedronisI mean client.do19:15
pedronisin client/client.go19:15
ijohnsonpedronis yes I see that now19:16
ijohnsonI was just looking at client.Config19:16
pedronisit's fixed19:17
pedronisexcept some things mostly dealing with downloading blobs from snapd don't use it19:17
pedronisdownloading or uploading19:17
ijohnsonOkay I think I just need to modify my call to Do() to use a different shorter timeout then19:18
ijohnsonProbably I could just call rawWithTimeout19:19
pedronisyes, most of the code in do seems only relevant for GET, plus the decoding but that is an helper as well, so you could call it yourself19:22
pedronisbut doSync does a bit more19:23
pedronisthis code honestly seems to need a cleanup, I think it was meant to be very standardized19:24
pedronisbut now we have grown different variant, and there's a bit of mix between params vs variants19:24
pedronisto ask for different behavior19:24
ijohnsonyes there's a few different ways to do subtly different versions of the same thing it seems19:29
pedronislike NoTimeout is weird19:32
ijohnsonI'm beginning to wonder if there's any part of our codebase that hasn't grown organically like this19:33
pedronisit's natural, but is also necessary to clean up once in a while19:34
pedronisthis one seems particularly bad though19:34
pedronisI don't think it has deep reasons to be like19:34
pedronisexcept to avoid changing call points19:34
pedronisbut all call points are inside client anyway19:34
pedronisit's even worse, some places do raw but set their own timeout19:41
pedronisI thing they preexist rawWithTimeout but anyway :/19:54
pedronisijohnson: I cleaned it up a bit20:35
ijohnsonpedronis thanks for that20:37
pedronisijohnson: you were doing doSync right?20:38
ijohnsonpedronis yes I was using doSync originally20:39
pedronisyea, now there's a doSync variant that can be passed a specific timeout20:39
ijohnsonGreat thanks20:40
ijohnsonmup seems to be mia20:40
pedronisijohnson: here: https://github.com/snapcore/snapd/pull/9454 if you could look at it and maybe pick it up tomorrow, it could use some tests, it's mostly a refactor but there are some changes to do, tbh lots of the new stuff didn't seem tested yet20:44
ijohnsonpedronis: sure I am going to work a bit extra tonight to make up for starting late today so I can try to clean that up20:44
ijohnsonalso hoping to get my first pr for the maintenance.json stuff up that is related to here20:45
pedronismaybe that was confusing: it's mostly a refactor but there are some changes to the "do" method20:59

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