=== chihchun is now known as chihchun_afk === ErichEickmeyer is now known as Eickmeyer === tedg_ is now known as tedg === bluesabre_ is now known as bluesabre === coreycb_ is now known as coreycb === bashfulrobot_ is now known as bashfulrobot === chihchun_afk is now known as chihchun === chihchun is now known as chihchun_afk === chihchun_afk is now known as chihchun === ShibaInu is now known as Shibe [06:20] morning [06:38] Hey :-) [06:38] I still owe you that review [06:40] zyga: hey :) haha [06:44] zyga: heh love how unpredictable our spread runs are [06:48] hey mborzecki and zyga [06:48] mvo: hey [06:50] Hey mvo [06:53] seems no mup [06:53] hi [06:53] mvo: hi, https://github.com/snapcore/snapd/pull/6747 [06:53] mup_: ping [06:54] mvo: read the description as well, hope it makes sense [06:54] pedronis: yeah, thank you! [06:54] pedronis: I'm reading it now (but just started only in 3rd file or so) [06:55] mvo: TBH, not surprisingly tests were most of the work [06:55] * mvo nods === chihchun is now known as chihchun_afk [07:13] morning [07:19] mvo: I answered the comment, could you reply? [07:20] zyga: interesting find, on centos, when snapd runs runuser, it somehow triggers keyctl() === chihchun_afk is now known as chihchun [07:22] pedronis: yes, that makes sense [07:22] pedronis: its more a warning for the future, that makes sense now [07:23] mvo: ok, I'll change it, and rewrap a couple of doc comments [07:23] which I noticed are bit too long one liners atm [07:28] done [07:29] * mvo nods [07:29] 3 of my PRs need a 2nd review, one probably is best to wait of the prereq to land [07:29] * pedronis is going back to do some reviews too [07:33] mborzecki: keyctl, that's interesting [07:33] zyga: mhm, found that they adjusted the core policy around rhel 6.5 release, to account for logrotate calling runuser too [07:36] I will be in the office in 20 [07:38] zyga: quick question - does 6717 also addresses the bug 1819318 ? the htop implicit slots/plugs? [07:41] mvo: one moment, let me look [07:41] mvo: yes, I don't think I need to review closely 6741 [07:42] mvo: no, it is not related [07:44] zyga: ok, trying to understand the remaining blockers [07:45] zyga: thanks, I understand now I think [07:45] zyga: so the fix is to install the snapd snap automatically when the first snap gets installed, thats fine [07:46] I would say the condition is: [07:46] mvo: ah, that bug, yes [07:46] No core snap, no snapd snap, any app snap: install snapd [07:46] Remember that systems in the field are broken [07:49] zyga: yes [07:50] zyga: I think we can do that right after 6741 landed [07:51] zyga: probably interesting for you too: https://github.com/snapcore/snapd/pull/6748 [07:52] zyga: still, makes me wonder how / got devpts_t label [07:56] pedronis: just to double check - for the remodel use-case I just add code to DeviceCtx(.., task, ...) that gets it from the task (and hang it on the right task in devicestate.Remodel() (?) [07:57] mborzecki: what does -i do? [07:57] zyga: --interpret [07:58] zyga: tehre's an example in PR description [07:58] (and a commit message too) [08:04] back now [08:06] mvo: not quite like that [08:06] mvo: where do you need the info again? [08:07] pedronis: I think I have ideas now, I can pastebin in 5-10min what I have. I need the info in the mount-snap handler in snapstate [08:07] mvo: I still think we should set the model on the change [08:07] pedronis: my idea was to define "ModelForTask" (similar to ModelFromTask) that adds a new remodelDeviceContext [08:08] pedronis: aha, ok [08:08] mvo: you should not need new helper [08:08] s [08:08] I did it wrong if you do [08:08] you need more code in devicestate [08:09] pedronis: I was thinking I need a new "remodelDeviceContext" struct and then need to add it as data to task/change, yes? [08:09] okay [08:09] * zyga checks backlog first [08:09] then reviews [08:10] mvo: yes, you need a remodelDeviceContext correct [08:10] mvo: then you need to teach devicestate.DeviceCtx that if task is passed in [08:10] and it's change has new-model data attr, to make a remodelDeviceContext [08:11] pedronis: cool, so I add remodelDeviceContext and change Remodel() to return a change and in the handlers will get the remodel from the change (via DeviceCtx) [08:11] mvo: because Install etc also might check model [08:11] you also want to make remodelDeviceContext by hand inside Remodel [08:11] and pass to Install Update etc [08:11] pedronis: right [08:12] as I explained in the PR description its a simple change [08:12] there's an example of what should happen to install for example [08:12] we need to do that only for snapstate functions that we plan to use in the remodel [08:12] * mvo nods [08:12] mvo: this change will need some kind of unit test though === chihchun is now known as chihchun_afk [08:14] mvo: when there will be a temporary store as well this changes will also help [08:14] together with the changes I will do around the use of snapstate.Store [08:14] pedronis: indeed [08:15] pedronis: for my current PR (allow different kernel) I *may* not need to change Install() as we do the checks late in mount-snap if we can install the kenrel or not. I can still change the signtuare or keep it for the initial PR, what do you prefer? [08:16] mvo: as you prefer, if you don't do the change please add a TODO at least [08:16] though [08:16] pedronis: thanks, will do - I dive deeper now [08:16] (I might have forgotten everything after my vacation ;) ) [08:17] pedronis: heh :) [08:19] mvo: I forgot about ifacestate (because it's using directly devicestate) [08:20] mvo: it's actually clear that except some form for tests, devicestate.Device/SetDevice/Model/Serial should go away [08:20] as public methods [08:21] https://bugs.launchpad.net/ubuntu/+source/snapd/+bug/1825298 [08:22] mvo: daemon etc can use the methods on the DeviceManager itself [08:22] mvo: to be clear it's not a blocker for your work, just shows there is a bit more to do [08:22] pedronis: ok, I think we need to add this to a card somewhere [08:23] and that some aspects of the old devicestate api is dangerous now [08:23] because what is the relevant model is not so a fixed concept [08:26] * pstolowski doh @ get_journal_log [08:33] mvo: don't know if noticed but seems we need to increase the timeout on debian-sid-64:tests/main/sbuild [08:34] I have seen it fail timing out quite often [08:34] https://api.travis-ci.org/v3/job/521593407/log.txt [08:35] (maybe is just a symptom, anyway9 === chihchun_afk is now known as chihchun [08:59] Chipaca: degville: some wondering from me that you might consider in my comments to #6669, also Chipaca that one is something for you to review when you have a bit of time [08:59] pedronis: I wasjust about to say [08:59] pedronis: 'expiration' is a date [08:59] not a duration [08:59] yes, not a fan [08:59] and obviously it makes no sense for this to be a date [09:00] no [09:01] automatic-snapshots.keep ? [09:01] then it could be a duration or "no" [09:01] that is too terse [09:01] for me [09:02] Chipaca: we are trying to get rid of automatic-snapshots [09:02] it needs to be snapshots.something [09:02] also [09:02] right [09:02] snapshot.retention [09:03] auto-saves-retention=2h or no [09:03] would seem ok (a bit clunky but ok) [09:03] I don't want to just have retention because it doesn't apply [09:03] to other snapshots [09:03] mmm [09:03] snapshots.auto-saves-retention seems a'ight [09:04] snapshots.automatic.retention? [09:04] ooh [09:04] schnazzy [09:04] nesting [09:04] losting [09:04] losting? :) [09:04] snapshots.automatic.on-removal=no [09:04] :-) [09:04] I'm 50/50 whether I want more nesting or not [09:05] flip a coin [09:05] I take tails [09:05] not today [09:05] * Chipaca watches it land on its edge [09:05] no coin flipping before holidays [09:05] shouldn't've used a pound coin [09:05] hard rule [09:05] "god does not throw a coin" said someone, somewhere [09:06] zyga: *play dice [09:06] I know :D [09:06] in absence of dies you use dimes ;) [09:06] zyga: clearly einstein was a lousy DM [09:06] Chipaca: do we have anything already with two levels? [09:08] yes [09:08] pedronis: service.rsyslog.disable [09:09] pedronis: and ssh, same [09:22] pedronis: Chipaca: ...ah, I've only just seen this discussion :) for some reason my IRC session was stuck a few pages up. Actually, my nick had disconnected. [09:24] degville: the internets. because just simple networks were too reliable. [09:24] Chipaca: pstolowski: let's go with snapshots.automatic.retention [09:25] \o/ [09:25] woot [09:25] * zyga helped name something :) [09:26] pedronis: sounds good [09:26] and gives room for more options under "automatic" [09:27] sounds good! [09:32] mborzecki: looking at the volumes PR again [09:32] zyga: great, thanks! === chihchun is now known as chihchun_afk [09:41] Chipaca: if the string value is relevant then simply explain that [09:41] Chipaca: my point was that it is a magic string [09:41] so some context is useufl [09:41] *useful [09:41] # magic [09:42] zyga: noted [09:42] wait was your comment on the 'snap download --cohort' test done in the create-cohort pr [09:43] yes [09:43] I didn't notice there are two [09:43] :-) === sparkieg` is now known as sparkiegeek [10:15] I'm off to do some exercise again, bbiab [10:21] * Chipaca really goes now [10:38] fun fact. the semantics of (some) network errors in Go are different between 16.04 & 18.04 - at least in the case i'm interested wrt to auto-refresh issue :/ [10:43] that's the little unexpected dragon hiding there after all [10:46] mborzecki: sent partial review on gadget validation [10:48] pstolowski: https://github.com/snapcore/snapd/pull/6749 one for you, mainly to check if you agree with the rationale [10:49] pstolowski: and if you are in review mood, maybe also https://github.com/snapcore/snapd/pull/6717 [10:49] zyga: yeah i remember about that one, sorry i didn't get to it yet. it needs full focus [10:49] pstolowski: no apologies needed [10:50] the watch is telling me to get up but I sprained my ankle yesterday [10:50] eh === chihchun_afk is now known as chihchun === amurray` is now known as amurray [10:57] zyga: hm the offset thing is inability of current structure to distinguish a case when there's explicit offset:0 and when no offset was set at all, i have a patch for that in another branch, maybe i should include it in the PR [10:57] please [10:57] zyga: fwiw it's kind of ugly :/ things become pointers and so on [10:58] yes, unavoidably so [10:58] wish we could do better but not with Go [10:58] or start to use magic -1 ;) [10:58] haha :P [10:58] how would we do better? [10:59] zyga: we'd need to have a none like thing [10:59] that's *exactly* a pointer :) [11:00] well, not exactly, but i won't start that without a glass of beer :P [11:01] mborzecki: value vs reference but yeah [11:01] actually beer sounds good regardlesss ;) [11:16] zyga: +1 with one remark [11:17] pstolowski: thanks [11:17] just to clarify here before I commit to the code: they are optional in the sense that we don't die on absence of a cookie [11:17] unless you think that should change [11:20] zyga: no, that is fine; we need to have a clue if that happens, and snapctl should give us one (but that shouldn't happen anymore) [11:22] * pstolowski lunch [11:44] zyga: I still dislike the solution in 6117, as did originally [11:44] hmm? [11:45] probably wrong PR number [11:45] 6717 [11:45] ah [11:45] because of the public data piece (Scoped)? [11:45] yes [11:45] I can make it private and write a PublicDeepEquals [11:45] it's really only public because of unit tests [11:46] that doesn't sound better [11:46] hmm [11:46] so is it the data piece of the access to the data piece that is problematic in your eyes? [11:47] zyga: the data, we use Plug/SlotInfo too much as standalone things [11:47] also it makes public a semantics detail [11:47] of which nothing later should care about afaik [11:47] hmm, not sure I follow, the standalone aspect is that they are too standlone or? [11:48] I still prefer some kind of private maps on Info [11:48] counterpoint: maps take more memory, this is literally a bit of information (well, a word because go) [11:50] pedronis: please add a comment to the PR, I can rework it to use private maps if you strongly prefer that [11:51] though personally I think this is okay, I don't see the risk of storing it this way (I only dislike that it is public) [11:52] strictly speaking we don't even need to attach the maps to the struct [11:52] interesting [11:52] I think the calling order is a bit unfortunate but with some shuffling I think you are right [11:53] perhaps I remember wrong but AFAIR implicit hooks are added in a separate pass at a later stage so we'd have to return the maps to that stage to proceed [11:53] infoFromSnapYamlWithSideInfo [11:53] is internal [11:54] so we can change it's signature [11:54] yeah [11:54] I will also explore the idea to load the hooks first [11:54] to return a map or take one [11:54] that would be easier, as the maps would be totally internal [11:54] that might be painful I fear [11:55] anyway all that code is a bit strange (evolved very organically) [11:55] I agree :) [11:55] and under weird constraints (some we cannot get rid of though) [11:55] I'm happy to rework it, probably tomorrow because today I need to break for a doctor visit [11:56] it would be nice to rework to something that uses canonical yaml [11:56] and a series of passes that transform the input yaml [11:56] and yaml is used freely here, it's just the yaml* types that we have there [11:56] load yaml, (processing passes), single analysis pass that reads "canonical form" [11:57] we are still trying to fix a bug, not rework the all thing over 3 months [11:57] yes :) [11:57] I'm only making a point on how it could be cleaned up from the organic growth === chihchun is now known as chihchun_afk [11:57] we could merge as-is and iterate [11:57] the bug would be fixed for 2.39 [11:59] were to we check that a plug and a slot cannot have the same name? [11:59] *where [11:59] *where do we [11:59] pedronis: we have a checker for that [11:59] * zyga looks [11:59] ohh [11:59] maybe we only do in repo [11:59] let's check [11:59] we shouldn't do it only that late [11:59] fwiw [11:59] if that's the case [12:00] plugsSlotsUniqueNames [12:00] fortunately we do validate early === cachio_ is now known as cachio [12:03] zyga, hey, I found a problem in the test core16-provided-by-core and I think it is realted to ns [12:04] cachio: tell me [12:04] zyga, if you run spread -debug -repeat 2 google:ubuntu-core-16-64:tests/main/core16-provided-by-core [12:04] zyga, it fails the second execution [12:04] bacause it is not falling back to core [12:04] zyga: it's in validate though, so we do it after everything else [12:05] anyway that's fine [12:05] I was debugging this and the problem is about how we reset [12:05] cachio: how do we reset? [12:05] cachio: (please tell me all you know) [12:06] zyga, the reset.sh uninstall all the snaps [12:06] but core, kernel. etc [12:06] and then [12:06] discard all mount namespaces and active mount profiles [12:06] active mount profiles? [12:07] zyga, https://paste.ubuntu.com/p/8tW5M7Bcgy/ [12:07] is it doing that [12:07] ah, the comment is confusing, I understand [12:08] when I run this it makes the test fail [12:08] in fact the test is failing master with the same error [12:08] depending of the tests which run before [12:09] how does the test fail? do you understand the problem and know what do do about it or are sharing information to get support in fixing it? [12:09] zyga, I don't really understand why it is happening [12:10] I took the look to the code and I think it is failing in snap-confine-invocation.c [12:10] how is it failing? [12:11] I mean there is where it should fall back to core and it is skipping that part [12:11] but not sure [12:11] perhaps you do :) [12:11] no, I don't [12:12] that part is never skipped [12:12] I bet that the problem is indeed in how we reset [12:12] zyga: I'm loooking at it myself, I don't think reworking it from the current state should be a lot of work (last famous words) [12:12] and that if we look at the before / after state (before 1st run, after reset on 1st run) you will see the difference [12:12] pedronis: +1 [12:13] pedronis: do push if you have a patch, my remark was really only about that it is fixing the bug as-is and it's green; it's certainly something we can rework [12:13] zyga, ok, I'll add some debug to the code to see if that helps [12:13] cachio: please do, my recommendation is really to start checking for leftovers [12:14] if the code fails on 2nd run reliably [12:14] it is clearly because the environment is no longer the same [12:14] right? [12:15] zyga, I suppose that [12:15] I already compared that [12:15] what state did you compare and what did you find? [12:15] and I couldnt detect the diff [12:16] everything inside /run/snapd/ns/ [12:16] that's not sufficient, the code looks at /snap/* [12:16] ok [12:16] my advice is to compare: /snap/* , /var/lib/snapd/* and /run/snapd/ns [12:16] zyga, I'll compare that too [12:16] I'm sure that whatever you will surely find will fix not only this test [12:17] but many others [12:17] zyga, great [12:17] cachio: you can also compare /proc/self/mountinfo [12:17] to see if we are leaking something [12:17] perhaps also look inside existing mount namespaces, among the set we are not discarding [12:17] good luck [12:17] zyga, nice [12:18] zyga, thanks, I'll continue with this [12:18] thank you! [12:50] zyga: I have the rework, to you want me to push it separately or on the PR ? [12:53] ergh. I wish the build.snapcraft.io would tell me why it thinks the yaml is invalid!! [12:55] how can I fix it if it won't tell me what's wrong?! [13:14] pedronis: push it directly there please [13:15] zyga: in meetings, but will do, it seems to work nicely, but it was quick hacking so names etc can maybe be tweaked [13:15] sure [13:15] thank you! === ricab is now known as ricab|lunch [13:56] zyga, well I found the problem [13:56] zyga, well, almost [13:56] after reset [13:56] zyga, https://paste.ubuntu.com/p/S25ntst2xQ/ [13:57] core16 is not installed but it remains in /snap [13:57] zyga, https://paste.ubuntu.com/p/NqdzDvjKVg/ [13:59] I am trygin a fix now === tinwood_ is now known as tinwood === chihchun_afk is now known as chihchun [14:27] zyga: I pushed it === chihchun is now known as chihchun_afk [14:32] zyga: pushed updates to cross structure validation PR [14:33] pedronis, mborzecki: ack [14:35] pedronis: I opened trivial https://github.com/snapcore/snapd/pull/6751 [14:36] it's the whole feature sans the tests [14:36] at +16,-3 it's the smallest feature branch I can recall :) === ricab|lunch is now known as ricab [14:47] off-topic: being in snap world where releases happen often and when they are ready I find the current craze around 19.04 release oddly quaint :) [14:48] non the less, time to update [15:00] * Chipaca wanders off [15:03] zyga, found the issue [15:03] https://paste.ubuntu.com/p/CwPtSXqcdV/ [15:03] where is that code? in the test? [15:03] we need to trim the variable when there is just 1 base snap to remove [15:03] aah [15:03] heh [15:03] I added logs to reset.sh [15:03] I see [15:03] hehehe [15:03] how was that not broken? [15:03] as in, not breaking the execution before [15:04] is the error ignored? [15:04] cachio: in any case, super nice find, thank you for chasing this! :) [15:04] set -e -x [15:04] I'll fix it [15:05] zyga, and also I'll add a new pr to print the tree for the state and compare it with the initial one [15:05] thanks to that I found the problem [15:05] cachio: please coordinate with Chipaca on that work, he's started this under the invariants theme [15:06] zyga, yes [15:19] pstolowski: I added some comments to your timings PRs [15:19] pedronis: ty [15:19] pedronis: btw are you off tomorrow? [15:19] yes [15:19] and then on vacation? [15:21] yes, back on April 30 [15:22] ok [15:26] mborzecki: 2nd pass on 6688 [15:54] zyga, this is the fix #6753 [15:55] mvo: https://github.com/snapcore/snapd/pull/6753 needs a 2nd review [15:55] * zyga EODs [15:55] ttyl everyone [15:55] zyga, thanks for the review [16:03] zyga, #6744 updated [16:04] thanls [16:04] * cachio lunch === pstolowski is now known as pstolowski|afk [16:17] zyga: thank you [16:26] a second review for 6720 would be great [16:39] cachio: I will release 2.39~pre1 to beta hopefully later tonight, its already building in the ppa [17:31] cachio: can we move the 2.38.1 snapd snap to candidate? or is there more validation required? [17:44] mvo, my validation is done for 2.38.1 [17:44] I was waiting for cert [17:44] let me check [17:44] cachio: ta [19:31] Hi! Is there someone I could ping here to get that done quickly https://forum.snapcraft.io/t/please-transfer-flatc/10980 [19:32] @popey ^^ [19:43] om26er: hi I can help [19:44] roadmr thanks for verification https://github.com/google/flatbuffers/pull/5293#issuecomment-484654914 [19:45] om26er: a github conversation is super helpful because it lets me verify the recipient's acceptance and identity in addition to verifying your email in github matches your snap store developer email :) [19:47] roadmr heh, true. I added a comment on the forum :-) [19:52] done, om26er ✅ [19:53] roadmr super! thank you. So what would be the next step here, should I delete the autobuild entry I had created in https://build.snapcraft.io/user/om26er ? [19:54] om26er: hm, not very sure about what happens with build.snapcraft.io but I think it would be sane for you to delete it and let upstream set up auto-builds [19:55] Chipaca: https://eng.uber.com/optimizing-m3/ <- interesting, also in terms of our state engine and memory usage [19:56] roadmr ok, so if he setups the autobuild, will I be a "collaborator" by default or would he need to add me again ? [19:57] om26er you're already a collaborator, this is set up at the snap store level and is done automatically when a snap is transferred [19:57] om26er: as a collab, in principle you could own the auto-build on build.snapcraft.io (you have access to the upstream source, and can publish/release revisions for the snap) [19:58] om26er: so in principle, what you have *should* work, but because the ownership changed, maybe the authentication credentials will go stale and you might need to reauthenticate or something [20:00] roadmr ok, thanks a lot for the help :-) [20:01] np, happy to help!