/srv/irclogs.ubuntu.com/2019/05/08/#snappy.txt

mborzeckimorning05:06
zygaHey05:25
zygamborzecki: I found a hell of a bug yesterday05:25
zygaWorking >12 hours but was worth it05:26
mborzeckizyga: hey, what bug?05:29
zygaMount namespaces are seriously busted05:31
zygaThis explains all the odd reports I got05:32
zygaI am tired a bit, still in bed05:32
mborzeckizyga: nice that you found it :)05:33
zygaI spent half of yesterday trying to nail what was going on05:34
zygaThere are several bugs05:34
zygaI know of two for sure because those got fixed in my branch05:34
zygaThe third one I know where it is but I was too tired to figure out where it is past 23:0005:35
zygaHehe05:35
zygaTo figure out exactly where it is05:35
zygaThose are all serious enough I think we should do .205:43
zygaHey mvo06:11
zygaI found a naaaasty bug last night06:11
mvohey zyga ! good morning06:12
mvozyga: uh, that sounds bad - can you tell me more please?06:12
zygaMvo: mount namespaces have three bugs that cripple content sharing06:14
zyga1) we unshare one too many times, this was fixed about 6 months ago but not merged, lost in a feature branch06:14
zyga2) sharing is broken so user mount namespace does not get propagation correctly, breaking on refresh or reconnect depending on use06:15
zyga3) ordering of mount operations is wrong, causing shadowing of past mounts06:15
zygaLots of craze last night06:16
zygaI had a call with Jamie to discuss some of this06:16
zyga1 is simple06:16
zyga2 is todo, didn’t debug as I realized this was broken after 23:0006:17
zyga3 is complex and requires some algorithm work06:17
zygaTogether those break gnome runtime sharing, theme sharing and perhaps other cases06:18
mvozyga: uh, that sounds nasty06:18
mvozyga: lets talk some more in some minutes, I make a cup of tea06:19
zygaExcellent idea :-)06:19
mborzeckimvo: morning06:27
mvohey mborzecki06:39
zygamvo: back in the office06:44
zygaslowly waking up06:52
pedronis morning07:03
pedroniscould we get a 2nd review of #677507:03
mupPR #6775: devicestate: make Remodel() return a state.Change <Created by mvo5> <https://github.com/snapcore/snapd/pull/6775>07:03
=== pstolowski|afk is now known as pstolowski
pstolowskimorning07:11
pedronispstolowski: hi, I have a question in #679307:12
mupPR #6793: cmd/snap: support for --ensure argument for snap debug timings <Created by stolowski> <https://github.com/snapcore/snapd/pull/6793>07:12
mborzeckipedronis: pstolowski: hey07:16
pstolowskipedronis: yes... you're right, i forgot about those07:17
pstolowskii mean about nesting them visually07:18
mupPR snapd#6790 closed: interfaces: add login-session-control interface <Created by AlanGriffiths> <Merged by mvo5> <https://github.com/snapcore/snapd/pull/6790>07:24
mvozyga: was wondering about 6751, it seems it just misses a unit test to go into master, can I help you with that? I would love to land this before lyon07:26
zygamvo: perhaps, I will try to devote an hour to it today07:26
zygasorry for taking so long, yesterday was a marathon of debugging07:26
mvozyga: I can look at it, I understand you are looking at important bugs07:27
pedronispstolowski: thanks, so  I wasn't confused, I think we do the same with ^ as for the change.07:33
pstolowskipedronis: yes07:33
pedronispstolowski: let me make a comment in the PR itself07:33
pedronismborzecki: made a couple of comments in 675007:34
pedronisone is not of immediate relevance07:34
mborzeckipedronis: thanks, reading through them now07:34
pedronispstolowski: for Ensure, is the label of level 0 usually the same as the "ensure" tag, or is just for some ?07:36
mupBug #1828175 opened: Lack of proxy support for snap prepare-image <Snappy:New> <https://launchpad.net/bugs/1828175>07:37
pstolowskipedronis: using refresh catalogs as an example: we have "ensure": "refresh-catalogs", then two nested measurements under it: with "get-sections" and "write-catalogs" labels07:40
pedronispstolowski: thx, noticed the same07:41
pedronispstolowski: I was thinking a bit about what to put in ID, but given that we don't want to make lines too long07:41
pedronis"ensure" seems fine  (it's a debugging thing and one should know what they asked)07:41
mvozyga: I will push something shortly07:42
mborzeckipstolowski: if we can tweak the formatting of timing entries a bit, may we use ---^ or --- to fill the line before ^ sign?07:48
mborzeckipstolowski: http://paste.ubuntu.com/p/7dWSfRbTrc/ this is what i mean, the ^ dashes feel a bit hard to follow08:05
pstolowskipedronis: actually, we won't see "^" nesting for ensures right now, the refresh-catalogs is the root node with tags, then get-sections and write-catalogs are at level 0. for tasks level 0 measurements appear nested visually because we put them under task line08:05
mborzeckiChipaca: welcome back08:06
Chipacamborzecki: :)08:06
pstolowskimborzecki: yep, i tend to agree, but i'd leave it for a potential followup, this had already been disputed about in one of the first PR in these series ;)08:08
mborzeckipstolowski: mhm, sounds good to me, i don't have an immediate suggestion now either, but maybe we'll come up with something once people start using it08:09
Chipacapedronis: you around?08:11
mborzeckigot a quick errand to run, back in ~3008:14
pstolowskipedronis: pushed some changes + reply to your comment08:43
pstolowskipedronis: also, i need to discuss with you some aspects of snap unset implementation that i touched breifly with mvo yesterday in the standup; would be great to meet (three of us) today08:45
=== ricab is now known as ricab|bbl
mborzeckire09:05
pedronispstolowski: sorry was in meetings,  interesting about the 0 level, maybe we should have an articial one09:21
pedronisthough maybe is just a wasted line, but it would clarify things09:21
pstolowskipedronis: yes, i can add this09:23
pstolowskipedronis: nb, can you merge master into your PRs? i'm about to do reviewing, will make diffs smaller09:24
pstolowskipedronis: eg #6822 (and it has a conflict)09:24
mupPR #6822: overlord/devicestate: introduce registrationContext <Created by pedronis> <https://github.com/snapcore/snapd/pull/6822>09:25
pedronispstolowski: basically the articial line should contain  "ensure" and somewhere the ensure tag09:29
pedronispstolowski: can also be a follow up a this point though09:29
pedronispstolowski: yes, need a little break after the meetings but will look at my PRs, I'm also doing reviews09:30
pedronisof some team PRs09:30
pedronispstolowski: btw you can review mvo 6775 and 6776 if you are wating to review mines09:41
pstolowskipedronis: ok. yep i'd prefer a followup09:46
pstolowskipedronis: did you see my message about the need of discussing snap unset?09:46
mborzeckianyone up for a 2nd review of https://github.com/snapcore/snapd/pull/6798 ?09:50
mupPR #6798: gadget: introduce checkers for sanitizing structure updates <Gadget update> <Created by bboozzoo> <https://github.com/snapcore/snapd/pull/6798>09:51
* zyga woke up again09:51
zygasorry everyone, had a rough night09:51
zygamvo: thank you for the tests09:52
pedronispstolowski: yes, are you blocked?  I can do after lunch or immeditately after standup, but not sure when mvo can09:52
pstolowskipedronis: a bit blocked yes, but i'm doing reviews so it can wait for later today09:53
mupPR snapd#6826 closed: tests: enable tests on centos 7 again <Created by sergiocazzolato> <Merged by bboozzoo> <https://github.com/snapcore/snapd/pull/6826>10:24
=== ricab|bbl is now known as ricab
* Chipaca afk for a while10:33
zygaman that's so darn annoying11:26
zygaI'm staring the bug in the face11:26
zygaI see it's wrong11:26
zygabut yet WHY eludes me11:27
=== cachio_ is now known as cachio
cachiomvo, hey11:35
cachiomvo, could you please merge #6694 to 2.39? I need that to finish snapd validation11:36
mupPR #6694: tests: improve how snaps are cached <Created by sergiocazzolato> <Merged by sergiocazzolato> <https://github.com/snapcore/snapd/pull/6694>11:36
mvocachio: uh, 27 commits, let me see how we can do this11:42
mvoI can't cherry pick this indivudally11:42
mvocachio: lets talk after (my) lunch, we get this in I might need to do it with conflicts instead of as cherry-picks11:43
cachiomvo, sure11:43
cachiothanks11:43
zygabrb11:45
zygauhhhh11:45
zygare11:49
pedronispstolowski: we could chat now but maybe you and mvo are having lunch atm11:56
=== ricab is now known as ricab|lunch
pstolowskipedronis: works for me if mvo is around12:10
pedronispstolowski: do we need mvo?12:10
pedronispstolowski: I'm in the standup channel12:11
pstolowskipedronis: ok, coming12:11
mupPR snapcraft#2560 opened: meta: do not wrap commands by default <Created by sergiusens> <https://github.com/snapcore/snapcraft/pull/2560>12:12
* Chipaca goes in quest of food12:23
mupPR snapd#6821 closed: many: make which store to use contextual  <Created by pedronis> <Merged by pedronis> <https://github.com/snapcore/snapd/pull/6821>12:35
pedronisChipaca: ^ that provoked conflicts in your PRs I fear12:39
pedronisI can look into that myself in a bit12:39
cmatsuokaniemeyer: do you prefer adapter: none or no adapter line at all in the gnome extension?12:46
cmatsuoka(sergiusens is changing that now and we can do it in either way)12:47
cachioq12:47
pedronis#6828 can be reviewed  (because of later pushes 6822 comes later in the chain now)12:52
mupPR #6828: many: use a fake assertion model in the device contexts for tests <Created by pedronis> <https://github.com/snapcore/snapd/pull/6828>12:52
mupPR snapd#6840 opened: gadget: fix handling of positioning constrains for structures of MBR role <Gadget update> <Simple 😃> <Created by bboozzoo> <https://github.com/snapcore/snapd/pull/6840>12:52
mborzeckisimple PR ^^ cmatsuoka pstolowski maybe?12:53
cmatsuokamborzecki: checking that12:54
mborzeckicmatsuoka: thanks12:54
mvopedronis, pstolowski sorry, had lunch indeed12:56
pstolowskimvo: np, i think the solution is uncontroversial, we will brief you in a momentr12:57
Chipacapedronis: conflict is life12:59
mvopstolowski: \o/12:59
=== ricab|lunch is now known as ricab
niemeyercmatsuoka: none or none at all seem similar13:20
cmatsuokaniemeyer: ack13:21
niemeyercmatsuoka: But I'm still not sure I get what's going on there.. does adapter: full really mean no adapter?13:22
niemeyerThat seems pretty absurd13:22
cmatsuokaFrom what I see there adapter: none means no adapter, "full" means command chain and "legacy" means wrappers13:22
cmatsuokabut sergiusens is changing the default to command chain13:23
cmatsuoka(I do agree that the choice of words isn't very clear)13:27
mborzeckioff to school14:03
* zyga is onto something!14:10
niemeyercmatsuoka: Yeah, the command chain isn't an adapter.. it's also under the control of the command-chain field itself, so there's no need to say anything else14:11
pedroniscachio: we got this:  https://api.travis-ci.org/v3/job/529764900/log.txt14:20
pedronisinternal error: a core16 snap is installed, earlier test cleanup did not work14:20
sergiusensniemeyer: as absurd as it feels now, it is was we concluded with kyrofa on https://forum.snapcraft.io/t/proposal-add-command-chain-to-apps-instead-of-generating-opaque-wrappers/6018 ... I'll see how we can get out of that, but it won't be as easy14:22
mvocachio: I looked at 6694 but cherry-picking it in isolation to release/2.39 shows conflicts on the first patch already so I think there is another PR before that that modified this part of the prepare.sh. do you remember which one that might be? did we had something similar before? we may need to cherry-pick both14:29
pedronismvo: I'll do the small fix pstolowski asked in your #677514:30
mupPR #6775: devicestate: make Remodel() return a state.Change <Created by mvo5> <https://github.com/snapcore/snapd/pull/6775>14:30
niemeyersergiusens: We discussed that when introducing bases14:32
niemeyersergiusens: We added the command chain concept at the same time so we would  not break compatibility in the future14:33
niemeyersergiusens: So I hope at least with a base, the default is no adapter.. is that the case?14:34
niemeyerSmall? Large?14:36
mvopedronis: thank you14:37
pedronismvo: done14:37
cachiopedronis, checking14:37
mvopedronis: \o/14:37
cachiopedronis, this is on master right?14:38
cachiobecause that should be fixed14:38
pedroniscachio: one of my PRs14:39
cachiomvo, checking14:39
pedronisbut I had merge with master today I think14:39
cachiopedronis, in that case I'll need to take a look, that should be fixed, which is the PR?14:40
pedroniscachio: this one:  https://github.com/snapcore/snapd/pull/6828  (doesn't touch spread stuff)14:41
mupPR #6828: many: use a fake assertion model in the device contexts for tests <Remodel :train:> <Created by pedronis> <https://github.com/snapcore/snapd/pull/6828>14:42
cachiopedronis, thanks14:42
pedronisit's actually all unit tests changes I think14:42
ijohnsonhi folks, what's the status of building core18 snaps on travis (especially with respect to using build-snaps)? is launching a docker container still the thing to do on travis?14:48
cachiomvo, this is giivng conflicts?  git cherry-pick 2e10db98657923a58fd7461586d150cfa97fc5b214:48
cachiomvo, I could cherry pick all of them14:52
mvocachio: this one looks fine, let me see if that is the one I was missing14:53
cachiomvo, the problem with this PR is that I tried many alternative during time until the last one was choosend14:54
mvocachio: ohhhh, so only the last one is relevant? and that applies cleanly?14:54
mvocachio: in this case we are good :)14:54
cachiomvo, no14:55
cachiomvo, it is a mix14:55
mvozyga: a re-review of 6820 would be great, it already has a +0.9 from you :)14:56
cachiomvo, let me check which are needed14:56
cachioI'll give you a list14:56
mvocachio: thanks14:57
mvocachio: alternatively, feel free to branch off releae/2.39 and cherry-pick as needed14:57
mvocachio: it seems like we just have to live with some conflicts when merigng back but thats ok14:57
sergiusensniemeyer: it is not, we can discuss the reasons next week; we did discuss this with bases, but at the end of last cycle, command-chain had not made the release cut in snapd ...15:00
zygamvo: looking15:01
zygamvo: +115:03
cachiopedronis, this seems to be new, I'll try to reproduce it locally, thanks for the log15:03
niemeyersergiusens: This is extremely unfortunate.. the whole point of command-chain was to avoid breaking compatibility in the future after bases were introduced15:07
niemeyersergiusens: What's your plan now to make them the default and adapters gone without breaking people?15:08
niemeyerThere's zero gain in command-chain if everybody has adapters15:08
niemeyer:/15:08
sergiusensniemeyer: I will bring a proposal to discuss with you for next week, migrating is something we already discussed with kyrofa15:10
pstolowskimvo, pedronis hotplug mystery wrt core18 solved15:17
pstolowskimvo, pedronis serial-port interface doesn't contain hotplug changes in 2.38.1. hotplug subsystem is triggered, just 0 interfaces support it. i have no idea how that happened that it didn't land in release brach :(15:18
ijohnsonsil2100: hey where is the "official" repository for pi gadgets these days? is it https://github.com/snapcore/pi-gadget ?15:19
mvopstolowski: so 2.39 fixes it?15:20
mvoijohnson: this looks correct15:20
ijohnsonmvo: cool thanks, we need to add the bt-serial interface back to the gadget there for uc1815:21
mvoijohnson: oh, that got lost? yeah, totally15:22
* mvo wonders why :(15:22
mvoijohnson: are you looking into this or sil2100 ?15:22
ijohnsonI was just wondering because the snapcore repo is more active but there's also github.com/CanonicalLtd/pi-gadget15:22
ijohnsonmvo: I'm actually looking into this because I wasn't able to use hotplug for the bluetooth adapter serial port, then learned that the gadget is supposed to include a slot for that and it's missing from the gadget on uc18 for some reason15:23
ijohnsonmvo: I was just going to file a PR to quick add it since it's 3 lines15:23
ijohnsonmvo: but I'm not sure which repo is watched for PR's :-)15:24
mvoijohnson: it should show up here :)15:24
pstolowskimvo: 2.39 has the serial-port changes so yes, should work. i'm going to verify that15:24
mvoijohnson: mup should tell us15:24
mvopstolowski: it will work once the gagdet is fixed? or today?15:24
pedronispstolowski: did you look whether system snap setting up is correct overall?15:25
pstolowskipedronis: yes, it looks ok (mapper says it's "snapd"). however i haven't checked seeding etc. i'm not sure how my manual testing affects it15:28
pedronispstolowski: ok, I'll try to look at that myself (I don't remember the details)15:34
sil2100ijohnson: yes, that's the right repository15:41
sil2100ijohnson: (snapcore)15:41
ijohnsonsil2100: great thanks for confirmation15:41
sil2100ijohnson: I need to remove the CanonicalLtd ones, I created those when Foundations had no powers to manage the snapcore ones ;)15:42
ijohnsonah I see that makes sense15:42
sil2100(sorry for the confusion)15:42
ijohnsonnp15:42
sil2100ijohnson: just in case https://wiki.ubuntu.com/UbuntuCore/Development should be up-to-date with this things15:42
ijohnsonthanks sil1200 that's really helpful I wasn't aware of that page15:43
ograshould be promoted on the forum and snapcraft docs15:44
ijohnson+115:44
ijohnsonalso, question for mvo and pedronis if it's quick (if it's not quick/easy let me know and I'll start a forum post instead), how does snapd handle gadget slots for devices like a serial port that doesn't actually exist?15:47
ijohnsonthe idea here being that we declare a serial port slot on a gadget for pi2 and pi3, but the serial port actually only exists on the pi3 and not on the pi215:48
ograyeah, specifically with interfaces that define a device path in their interface declaration, is snapd checking the device actually exists before exposing the interface ?15:49
pedronisijohnson: I don't think we check/supress atm, we do trust the dgadget15:49
ogra(thats what i would expect actually)15:49
pedronisit could be considered a bug15:49
pedronisbut that's current behavior15:49
ograouch15:49
ijohnsonok, do you want me to start a conversation on the forum for this?15:49
pedronisyes15:50
ijohnsonack thanks15:50
cachiomvo, change cherry picked to 2.3915:55
cachiomvo, pushed to branch on snapcore, is it ok or I should push to my own copy and then create a PR?15:57
mvocachio: thanks! doing in a PR would be ideal but if travis is happy thats fine. thanks for doing this15:58
cachiomvo, ok, sorry, lets see if it works15:58
mvocachio: aha, nice! it looks like it all merges back into master nicely, no conflict afaict. cool15:59
mvocachio: yeah, I wasn't clear in my original question/request so no worries15:59
cachiomvo, I found another improvement to implement by reviewing the change :)16:00
pstolowskimvo, pedronis: there is a bug though with core18 related to snapd transition and hotplug; serial-port hotplug slot doesn't pass sanitization because of "if slot.Snap.Type != snap.TypeOS && slot.Snap.Type != snap.TypeGadget" check16:00
pedronisah16:00
mvocachio: nice16:00
mvopstolowski: ha! good catch16:01
ijohnsonpedronis: posted as https://forum.snapcraft.io/t/gadget-slots-for-devices-that-dont-exist/1127816:01
pedronisthx16:01
pedronispstolowski: does this come from the base declaration or so something else?16:01
pedronisI thought we made core mean snapd or core at that level16:02
* cachio lunch16:03
pstolowskipedronis: no, we have a few helpers (sanitizeSlotReservedForOSOrGadget, sanitizeSlotReservedForOS...) that various interfaces call in their sanitize methods, they check SlotInfo.Snap.Type16:04
pedronisah, I think we should drop those if I understand things correctly16:05
pedronisthat job is done by the base declaration now16:05
pstolowskipedronis: hmm, interesting16:05
Chipacaok, I'm going offline, ttfn ttyl16:05
pstolowskipedronis: right, slot-snap-type: in base decl does that16:06
pedronisyes16:06
pedronisI mean there might be reason to keep them16:06
pstolowskipedronis: does it treat "snapd" special though?16:06
pedronisyes16:06
pedronisas I said there is code to make core mean (core or snapd)16:06
pedronisyou can find it in policy I think16:07
pedroniswe should maybe sanitize that at some point but is not trivial16:07
pedronisI mean switch to something like system instead of core16:07
pstolowskipedronis: i don't see a point in keeping these helpers anymore then16:09
pstolowskior they should special-case "snapd" if we want to keep them for a while16:10
pedronispstolowski: that might be a smaller change?16:17
pedronisfor now16:17
pedronisnot sure16:17
pstolowskipedronis: around 12 interface files call them, but then over 80 test files are affected too (because of common interface). the change should be pretty mechanical though, as long as base declarations for them are complete in that regard16:21
pstolowskipedronis: for 2.39 though might be better to make a simple fix in the helpers, and then a followup which removes them all16:23
pedronispstolowski: yes16:24
pstolowskiok, will do16:24
pstolowskiok, i've a tentative fix, need to test it on pi3 though to see if that's the only issue, will do that tomorrow morning16:33
=== pstolowski is now known as pstolowski|afk
mupPR snapd#6775 closed: devicestate: make Remodel() return a state.Change <Remodel :train:> <Created by mvo5> <Merged by mvo5> <https://github.com/snapcore/snapd/pull/6775>16:38
mvo6776 is now a very simple review :)16:39
mupPR snapcraft#2556 closed: cli: snapcraft promote <Created by sergiusens> <Merged by sergiusens> <https://github.com/snapcore/snapcraft/pull/2556>16:52
mupPR snapd#6820 closed: snap-confine: improve error when running on a not /home homedir <Created by mvo5> <Merged by mvo5> <https://github.com/snapcore/snapd/pull/6820>18:17
mupPR snapd#6840 closed: gadget: fix handling of positioning constrains for structures of MBR role <Gadget update> <Simple 😃> <Created by bboozzoo> <Merged by mvo5> <https://github.com/snapcore/snapd/pull/6840>18:26
mupPR snapd#6828 closed: many: use a fake assertion model in the device contexts for tests <Remodel :train:> <Created by pedronis> <Merged by mvo5> <https://github.com/snapcore/snapd/pull/6828>18:27
cachiozyga, hey this is interesting https://paste.ubuntu.com/p/tG5MmWSH8n/20:14
cachiobase snaps mounted after snap remove20:14
cachioin the tests20:14
mupPR snapd#6841 opened: overlord: make changes conflict with remodel <Created by pedronis> <https://github.com/snapcore/snapd/pull/6841>20:36
cachiozyga, I already fixed the issue20:44
mupPR snapd#6842 opened: tests: fix how the base snap are deleted when there are multiple to deleted on reset <Created by sergiocazzolato> <https://github.com/snapcore/snapd/pull/6842>21:07
mupPR snapcraft#2559 closed: manifest: expose snapcraft-started-at <Created by sergiusens> <Merged by sergiusens> <https://github.com/snapcore/snapcraft/pull/2559>22:20
mupPR snapd#6843 opened: tests: avoid removing snaps which are cached to speed up the prepare on boards <Created by sergiocazzolato> <https://github.com/snapcore/snapd/pull/6843>22:23

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