[05:06] morning [05:25] Hey [05:25] mborzecki: I found a hell of a bug yesterday [05:26] Working >12 hours but was worth it [05:29] zyga: hey, what bug? [05:31] Mount namespaces are seriously busted [05:32] This explains all the odd reports I got [05:32] I am tired a bit, still in bed [05:33] zyga: nice that you found it :) [05:34] I spent half of yesterday trying to nail what was going on [05:34] There are several bugs [05:34] I know of two for sure because those got fixed in my branch [05:35] The third one I know where it is but I was too tired to figure out where it is past 23:00 [05:35] Hehe [05:35] To figure out exactly where it is [05:43] Those are all serious enough I think we should do .2 [06:11] Hey mvo [06:11] I found a naaaasty bug last night [06:12] hey zyga ! good morning [06:12] zyga: uh, that sounds bad - can you tell me more please? [06:14] Mvo: mount namespaces have three bugs that cripple content sharing [06:14] 1) we unshare one too many times, this was fixed about 6 months ago but not merged, lost in a feature branch [06:15] 2) sharing is broken so user mount namespace does not get propagation correctly, breaking on refresh or reconnect depending on use [06:15] 3) ordering of mount operations is wrong, causing shadowing of past mounts [06:16] Lots of craze last night [06:16] I had a call with Jamie to discuss some of this [06:16] 1 is simple [06:17] 2 is todo, didn’t debug as I realized this was broken after 23:00 [06:17] 3 is complex and requires some algorithm work [06:18] Together those break gnome runtime sharing, theme sharing and perhaps other cases [06:18] zyga: uh, that sounds nasty [06:19] zyga: lets talk some more in some minutes, I make a cup of tea [06:19] Excellent idea :-) [06:27] mvo: morning [06:39] hey mborzecki [06:44] mvo: back in the office [06:52] slowly waking up [07:03] morning [07:03] could we get a 2nd review of #6775 [07:03] PR #6775: devicestate: make Remodel() return a state.Change === pstolowski|afk is now known as pstolowski [07:11] morning [07:12] pstolowski: hi, I have a question in #6793 [07:12] PR #6793: cmd/snap: support for --ensure argument for snap debug timings [07:16] pedronis: pstolowski: hey [07:17] pedronis: yes... you're right, i forgot about those [07:18] i mean about nesting them visually [07:24] PR snapd#6790 closed: interfaces: add login-session-control interface [07:26] zyga: 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 lyon [07:26] mvo: perhaps, I will try to devote an hour to it today [07:26] sorry for taking so long, yesterday was a marathon of debugging [07:27] zyga: I can look at it, I understand you are looking at important bugs [07:33] pstolowski: thanks, so I wasn't confused, I think we do the same with ^ as for the change. [07:33] pedronis: yes [07:33] pstolowski: let me make a comment in the PR itself [07:34] mborzecki: made a couple of comments in 6750 [07:34] one is not of immediate relevance [07:34] pedronis: thanks, reading through them now [07:36] pstolowski: for Ensure, is the label of level 0 usually the same as the "ensure" tag, or is just for some ? [07:37] Bug #1828175 opened: Lack of proxy support for snap prepare-image [07:40] pedronis: using refresh catalogs as an example: we have "ensure": "refresh-catalogs", then two nested measurements under it: with "get-sections" and "write-catalogs" labels [07:41] pstolowski: thx, noticed the same [07:41] pstolowski: I was thinking a bit about what to put in ID, but given that we don't want to make lines too long [07:41] "ensure" seems fine (it's a debugging thing and one should know what they asked) [07:42] zyga: I will push something shortly [07:48] pstolowski: if we can tweak the formatting of timing entries a bit, may we use ---^ or --- to fill the line before ^ sign? [08:05] pstolowski: http://paste.ubuntu.com/p/7dWSfRbTrc/ this is what i mean, the ^ dashes feel a bit hard to follow [08:05] pedronis: 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 line [08:06] Chipaca: welcome back [08:06] mborzecki: :) [08:08] mborzecki: 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:09] pstolowski: 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 it [08:11] pedronis: you around? [08:14] got a quick errand to run, back in ~30 [08:43] pedronis: pushed some changes + reply to your comment [08:45] pedronis: 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) today === ricab is now known as ricab|bbl [09:05] re [09:21] pstolowski: sorry was in meetings, interesting about the 0 level, maybe we should have an articial one [09:21] though maybe is just a wasted line, but it would clarify things [09:23] pedronis: yes, i can add this [09:24] pedronis: nb, can you merge master into your PRs? i'm about to do reviewing, will make diffs smaller [09:24] pedronis: eg #6822 (and it has a conflict) [09:25] PR #6822: overlord/devicestate: introduce registrationContext [09:29] pstolowski: basically the articial line should contain "ensure" and somewhere the ensure tag [09:29] pstolowski: can also be a follow up a this point though [09:30] pstolowski: yes, need a little break after the meetings but will look at my PRs, I'm also doing reviews [09:30] of some team PRs [09:41] pstolowski: btw you can review mvo 6775 and 6776 if you are wating to review mines [09:46] pedronis: ok. yep i'd prefer a followup [09:46] pedronis: did you see my message about the need of discussing snap unset? [09:50] anyone up for a 2nd review of https://github.com/snapcore/snapd/pull/6798 ? [09:51] PR #6798: gadget: introduce checkers for sanitizing structure updates [09:51] * zyga woke up again [09:51] sorry everyone, had a rough night [09:52] mvo: thank you for the tests [09:52] pstolowski: yes, are you blocked? I can do after lunch or immeditately after standup, but not sure when mvo can [09:53] pedronis: a bit blocked yes, but i'm doing reviews so it can wait for later today [10:24] PR snapd#6826 closed: tests: enable tests on centos 7 again === ricab|bbl is now known as ricab [10:33] * Chipaca afk for a while [11:26] man that's so darn annoying [11:26] I'm staring the bug in the face [11:26] I see it's wrong [11:27] but yet WHY eludes me === cachio_ is now known as cachio [11:35] mvo, hey [11:36] mvo, could you please merge #6694 to 2.39? I need that to finish snapd validation [11:36] PR #6694: tests: improve how snaps are cached [11:42] cachio: uh, 27 commits, let me see how we can do this [11:42] I can't cherry pick this indivudally [11:43] cachio: lets talk after (my) lunch, we get this in I might need to do it with conflicts instead of as cherry-picks [11:43] mvo, sure [11:43] thanks [11:45] brb [11:45] uhhhh [11:49] re [11:56] pstolowski: we could chat now but maybe you and mvo are having lunch atm === ricab is now known as ricab|lunch [12:10] pedronis: works for me if mvo is around [12:10] pstolowski: do we need mvo? [12:11] pstolowski: I'm in the standup channel [12:11] pedronis: ok, coming [12:12] PR snapcraft#2560 opened: meta: do not wrap commands by default [12:23] * Chipaca goes in quest of food [12:35] PR snapd#6821 closed: many: make which store to use contextual [12:39] Chipaca: ^ that provoked conflicts in your PRs I fear [12:39] I can look into that myself in a bit [12:46] niemeyer: do you prefer adapter: none or no adapter line at all in the gnome extension? [12:47] (sergiusens is changing that now and we can do it in either way) [12:47] q [12:52] #6828 can be reviewed (because of later pushes 6822 comes later in the chain now) [12:52] PR #6828: many: use a fake assertion model in the device contexts for tests [12:52] PR snapd#6840 opened: gadget: fix handling of positioning constrains for structures of MBR role [12:53] simple PR ^^ cmatsuoka pstolowski maybe? [12:54] mborzecki: checking that [12:54] cmatsuoka: thanks [12:56] pedronis, pstolowski sorry, had lunch indeed [12:57] mvo: np, i think the solution is uncontroversial, we will brief you in a momentr [12:59] pedronis: conflict is life [12:59] pstolowski: \o/ === ricab|lunch is now known as ricab [13:20] cmatsuoka: none or none at all seem similar [13:21] niemeyer: ack [13:22] cmatsuoka: But I'm still not sure I get what's going on there.. does adapter: full really mean no adapter? [13:22] That seems pretty absurd [13:22] From what I see there adapter: none means no adapter, "full" means command chain and "legacy" means wrappers [13:23] but sergiusens is changing the default to command chain [13:27] (I do agree that the choice of words isn't very clear) [14:03] off to school [14:10] * zyga is onto something! [14:11] cmatsuoka: 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 else [14:20] cachio: we got this: https://api.travis-ci.org/v3/job/529764900/log.txt [14:20] internal error: a core16 snap is installed, earlier test cleanup did not work [14:22] niemeyer: 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 easy [14:29] cachio: 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 both [14:30] mvo: I'll do the small fix pstolowski asked in your #6775 [14:30] PR #6775: devicestate: make Remodel() return a state.Change [14:32] sergiusens: We discussed that when introducing bases [14:33] sergiusens: We added the command chain concept at the same time so we would not break compatibility in the future [14:34] sergiusens: So I hope at least with a base, the default is no adapter.. is that the case? [14:36] Small? Large? [14:37] pedronis: thank you [14:37] mvo: done [14:37] pedronis, checking [14:37] pedronis: \o/ [14:38] pedronis, this is on master right? [14:38] because that should be fixed [14:39] cachio: one of my PRs [14:39] mvo, checking [14:39] but I had merge with master today I think [14:40] pedronis, in that case I'll need to take a look, that should be fixed, which is the PR? [14:41] cachio: this one: https://github.com/snapcore/snapd/pull/6828 (doesn't touch spread stuff) [14:42] PR #6828: many: use a fake assertion model in the device contexts for tests [14:42] pedronis, thanks [14:42] it's actually all unit tests changes I think [14:48] hi 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] mvo, this is giivng conflicts? git cherry-pick 2e10db98657923a58fd7461586d150cfa97fc5b2 [14:52] mvo, I could cherry pick all of them [14:53] cachio: this one looks fine, let me see if that is the one I was missing [14:54] mvo, the problem with this PR is that I tried many alternative during time until the last one was choosend [14:54] cachio: ohhhh, so only the last one is relevant? and that applies cleanly? [14:54] cachio: in this case we are good :) [14:55] mvo, no [14:55] mvo, it is a mix [14:56] zyga: a re-review of 6820 would be great, it already has a +0.9 from you :) [14:56] mvo, let me check which are needed [14:56] I'll give you a list [14:57] cachio: thanks [14:57] cachio: alternatively, feel free to branch off releae/2.39 and cherry-pick as needed [14:57] cachio: it seems like we just have to live with some conflicts when merigng back but thats ok [15:00] niemeyer: 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:01] mvo: looking [15:03] mvo: +1 [15:03] pedronis, this seems to be new, I'll try to reproduce it locally, thanks for the log [15:07] sergiusens: This is extremely unfortunate.. the whole point of command-chain was to avoid breaking compatibility in the future after bases were introduced [15:08] sergiusens: What's your plan now to make them the default and adapters gone without breaking people? [15:08] There's zero gain in command-chain if everybody has adapters [15:08] :/ [15:10] niemeyer: I will bring a proposal to discuss with you for next week, migrating is something we already discussed with kyrofa [15:17] mvo, pedronis hotplug mystery wrt core18 solved [15:18] mvo, 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:19] sil2100: hey where is the "official" repository for pi gadgets these days? is it https://github.com/snapcore/pi-gadget ? [15:20] pstolowski: so 2.39 fixes it? [15:20] ijohnson: this looks correct [15:21] mvo: cool thanks, we need to add the bt-serial interface back to the gadget there for uc18 [15:22] ijohnson: oh, that got lost? yeah, totally [15:22] * mvo wonders why :( [15:22] ijohnson: are you looking into this or sil2100 ? [15:22] I was just wondering because the snapcore repo is more active but there's also github.com/CanonicalLtd/pi-gadget [15:23] mvo: 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 reason [15:23] mvo: I was just going to file a PR to quick add it since it's 3 lines [15:24] mvo: but I'm not sure which repo is watched for PR's :-) [15:24] ijohnson: it should show up here :) [15:24] mvo: 2.39 has the serial-port changes so yes, should work. i'm going to verify that [15:24] ijohnson: mup should tell us [15:24] pstolowski: it will work once the gagdet is fixed? or today? [15:25] pstolowski: did you look whether system snap setting up is correct overall? [15:28] pedronis: 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 it [15:34] pstolowski: ok, I'll try to look at that myself (I don't remember the details) [15:41] ijohnson: yes, that's the right repository [15:41] ijohnson: (snapcore) [15:41] sil2100: great thanks for confirmation [15:42] ijohnson: I need to remove the CanonicalLtd ones, I created those when Foundations had no powers to manage the snapcore ones ;) [15:42] ah I see that makes sense [15:42] (sorry for the confusion) [15:42] np [15:42] ijohnson: just in case https://wiki.ubuntu.com/UbuntuCore/Development should be up-to-date with this things [15:43] thanks sil1200 that's really helpful I wasn't aware of that page [15:44] should be promoted on the forum and snapcraft docs [15:44] +1 [15:47] also, 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:48] the 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 pi2 [15:49] yeah, 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] ijohnson: I don't think we check/supress atm, we do trust the dgadget [15:49] (thats what i would expect actually) [15:49] it could be considered a bug [15:49] but that's current behavior [15:49] ouch [15:49] ok, do you want me to start a conversation on the forum for this? [15:50] yes [15:50] ack thanks [15:55] mvo, change cherry picked to 2.39 [15:57] mvo, pushed to branch on snapcore, is it ok or I should push to my own copy and then create a PR? [15:58] cachio: thanks! doing in a PR would be ideal but if travis is happy thats fine. thanks for doing this [15:58] mvo, ok, sorry, lets see if it works [15:59] cachio: aha, nice! it looks like it all merges back into master nicely, no conflict afaict. cool [15:59] cachio: yeah, I wasn't clear in my original question/request so no worries [16:00] mvo, I found another improvement to implement by reviewing the change :) [16:00] mvo, 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" check [16:00] ah [16:00] cachio: nice [16:01] pstolowski: ha! good catch [16:01] pedronis: posted as https://forum.snapcraft.io/t/gadget-slots-for-devices-that-dont-exist/11278 [16:01] thx [16:01] pstolowski: does this come from the base declaration or so something else? [16:02] I thought we made core mean snapd or core at that level [16:03] * cachio lunch [16:04] pedronis: no, we have a few helpers (sanitizeSlotReservedForOSOrGadget, sanitizeSlotReservedForOS...) that various interfaces call in their sanitize methods, they check SlotInfo.Snap.Type [16:05] ah, I think we should drop those if I understand things correctly [16:05] that job is done by the base declaration now [16:05] pedronis: hmm, interesting [16:05] ok, I'm going offline, ttfn ttyl [16:06] pedronis: right, slot-snap-type: in base decl does that [16:06] yes [16:06] I mean there might be reason to keep them [16:06] pedronis: does it treat "snapd" special though? [16:06] yes [16:06] as I said there is code to make core mean (core or snapd) [16:07] you can find it in policy I think [16:07] we should maybe sanitize that at some point but is not trivial [16:07] I mean switch to something like system instead of core [16:09] pedronis: i don't see a point in keeping these helpers anymore then [16:10] or they should special-case "snapd" if we want to keep them for a while [16:17] pstolowski: that might be a smaller change? [16:17] for now [16:17] not sure [16:21] pedronis: 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 regard [16:23] pedronis: for 2.39 though might be better to make a simple fix in the helpers, and then a followup which removes them all [16:24] pstolowski: yes [16:24] ok, will do [16:33] ok, i've a tentative fix, need to test it on pi3 though to see if that's the only issue, will do that tomorrow morning === pstolowski is now known as pstolowski|afk [16:38] PR snapd#6775 closed: devicestate: make Remodel() return a state.Change [16:39] 6776 is now a very simple review :) [16:52] PR snapcraft#2556 closed: cli: snapcraft promote [18:17] PR snapd#6820 closed: snap-confine: improve error when running on a not /home homedir [18:26] PR snapd#6840 closed: gadget: fix handling of positioning constrains for structures of MBR role [18:27] PR snapd#6828 closed: many: use a fake assertion model in the device contexts for tests [20:14] zyga, hey this is interesting https://paste.ubuntu.com/p/tG5MmWSH8n/ [20:14] base snaps mounted after snap remove [20:14] in the tests [20:36] PR snapd#6841 opened: overlord: make changes conflict with remodel [20:44] zyga, I already fixed the issue [21:07] PR snapd#6842 opened: tests: fix how the base snap are deleted when there are multiple to deleted on reset [22:20] PR snapcraft#2559 closed: manifest: expose snapcraft-started-at [22:23] PR snapd#6843 opened: tests: avoid removing snaps which are cached to speed up the prepare on boards