=== chihchun_afk is now known as chihchun [05:08] morning [05:43] PR snapd#5761 closed: store: use stable instance key in store refresh requests [05:51] mvo: hi [05:54] mborzecki: hey, good morning [05:57] mvo: could you take a look at https://github.com/snapcore/snapd/pull/5758 ? [05:57] PR #5758: overlord/snapstate, snap: handle shared snap directories when installing/remove snaps with instance key [05:59] mborzecki: sure, I'm just looking into the 18.10 upgrade hang, I will check this one in parallel [05:59] mvo: thank you [06:11] wrz 10 08:11:10 galeon snapd[9337]: retry.go:52: DEBUG: The retry loop for https://api.snapcraft.io/api/v1/snaps/names?confinement=strict%2Cclassic finished after 1 retries, elapsed time=303.098089ms, status: cannot decode new commands catalog: got unexpected HTTP status code 403 via GET to "https://api.snapcraft.io/api/v1/snaps/names?confinement=strict%2Cclassic" [06:11] 403? [06:18] good morning [06:19] zyga: hey [06:32] ok, breakfast done, time to get to work [06:32] zyga: hey [06:33] hey [06:33] I didn't do much work over weekend [06:33] I'm snapshotting now, going to upgrade to cosmic [06:37] mvo: is this expected? [06:37] Upgrades to the development release are only [06:37] available from the latest supported release. [06:37] I can update manually but I was expecting do-release-upgrade -d to work [06:38] zyga: yes, this should work. you could try change /etc/update-manager/release-upgrades from "Prompt=lts" to "prompt=normal" [06:39] mmm, let's try [06:39] indeed, thanks! [06:40] zyga: I'm confused, why is mkdirAllChown racy? it is that we create mkdirAllChown(/prefix/foo) and mkdirAllChown(/prefix/bar) with different users? [06:44] mvo: two concurrently running instances will eventually race with each other to mv a file over [06:45] (well, a directory) [06:45] zyga: aha, thats the one [06:45] mvo: and only one will win [06:45] zyga: and the other one will get a EEXITS? [06:45] yes [06:45] mvo: the version in snap-update-ns is not racy because it chooses to construct the directory in-place and chown it split second later [06:45] as such it already handles EEXIST [06:45] as a normal operation [06:45] it's a trade-off [06:46] trading temporary visibility of incorrect owner [06:46] for robustness [06:46] zyga: ok [06:46] zyga: thanks, that makes sense now [06:46] E:Problem executing scripts APT::Update::Post-Invoke-Success 'if [06:46] /usr/bin/test -w /var/cache/app-info -a -e /usr/bin/appstreamcli; [06:46] then appstreamcli refresh-cache > /dev/null; fi', E:Sub-process [06:46] returned an error code [06:47] mvo: I saw that before but I don't know how I fixed that [06:47] let me just upgrade manually [06:48] zyga: the appastream thing is harmless [06:48] zyga: just ignore it [06:48] it broke the upgrade [06:48] as in, do-release-upgrade failed [06:51] upgrading the old way now [06:51] that appstream thing is not very robust [06:52] zyga: ok [06:52] zyga: I'm surprised that the appstream thing prevented the upgrade [06:52] apt hook magic [06:52] zyga: but yeah, not super important, I'm keen to learn what you find [06:55] PR snapd#5787 closed: tests: update the listing expression to support core from different channels [07:04] mvo: upgrade complete [07:04] no issues [07:05] do you know if the upgrade was headless/ [07:05] perhaps that's the key ingredient === pstolowski|eow is now known as pstolowski [07:05] heyas [07:05] hey pawel [07:07] cosmic has new looks! [07:07] I was expecting to stay on bionic but ubuntu is like drugs, you just want more and more and more fresh software [07:10] mvo: do you want me to repeat the test under different conditions? [07:10] zyga: hm hm, I had no luck reproducing the issue myself earlier too [07:11] zyga: I think there is soemthing missing no need for you too keep trying [07:12] OK [07:29] hmm, it seems gnome-shell in 18.10 no longer logs errors whenever some window activity happens [07:37] https://bugs.launchpad.net/snapd/+bug/1791587 [07:37] Bug #1791587: [2.34.2] snapd ignores proxy settings [07:41] zyga: cannot be, wonder if they finally fixed it upstream [07:41] anyways, super annoying [07:42] mborzecki: I'm much happier now, it was always annoying to watch journal output with gnome-shell spamming everything [07:43] mborzecki: I left a bunch of comments in the PR, tl;dr its fine but I have quibbles with some of the naming (yay bike-sheed!) [07:44] mborzecki: hope its still useful [07:45] mvo: thanks, will look in a minute once i'm done with the autotools madness [07:46] mborzecki: no rush, I'm back at apt upgrade madness [07:50] PR snapd#5800 opened: cmd: use systemdsystemgeneratorsdir, cleanup automake complaints, tweaks [07:50] mborzecki: nice one [07:50] mvo: duh, i need coffee now, i'm still shaking :P [07:50] * mvo hugs mborzecki [07:54] mborzecki: "Not it's at the right place to begin with" ? [07:55] s/Not/Now/ ? [07:55] PR core18#70 opened: hooks: add rfkill [07:55] heh, right :) [07:56] zyga: fixed the description [07:57] mborzecki: very nice [07:59] zyga: o/ [07:59] hey [08:00] zyga: one question i had left over from friday was: do we really need to loadmountinfo of /proc/self/mountinfo so many times (in quick succession)? if these are all part of the same operation, can't we load it once and pass it around? [08:01] Chipaca: yeah, we could; it's somewhat "tricky" because interface backends have no state across invocations [08:02] mborzecki: https://github.com/snapcore/snapd/pull/5801 <- last of the removal PRs [08:02] PR snapd#5801 opened: cmd/snap-update-ns: remove the unused Secure type [08:02] PR #5801: cmd/snap-update-ns: remove the unused Secure type [08:02] mborzecki: after that it's just the additions; hopefully with a way that lets us decide if the names are ok and if the way they are passed around is ok [08:02] zyga: these 2k+ calls a re all from separate interface backend invocations? [08:02] is forum.snapcraft.com failing for anyone else right now? [08:02] yes [08:02] err .io [08:02] Chipaca: if we had a "mount manager" that could keep a representative state and share it [08:03] Chipaca: (e.g. use inotify) [08:03] diddledan: yes for me as well [08:03] Chipaca: we could just call it once per mount op [08:03] diddledan: same here [08:03] Chipaca: there's a not well known fact that you can poll on /proc/self/mountinfo [08:03] and get events when it changes :) [08:03] diddledan: and it's back [08:03] diddledan: so _that_'s why the morning was nice and quiet [08:04] :-) [08:04] hrm, its back and it ate the reply I was typing [08:04] *grumpf* [08:04] nomnomnom [08:04] mvo: i thought those were in cookies [08:04] or, well, local state in any case [08:04] boo [08:05] zyga: hmmm [08:05] Chipaca: maybe I had a bad cookie [08:05] or not enough milk [08:05] ¯\_(ツ)_/¯ computers are hard, let's go do something easy like rocket surgery [08:06] Chipaca: do you think we can sit down and work on this at the sprint? [08:06] zyga: yes [08:06] zyga: if it's all interfaces, then we probably can do the cache+dnotify from interface manager [08:06] yes, I think so [08:07] zyga: before stopping myself and going off on friday i had it down to where the biggest single memory chunk was bolt [08:08] zyga: this was via a hackish have-a-global-mountinfo that expired after a second, and changing asserts/findwildcard to not load the whole directory every time [08:08] Chipaca: so after mountinfo bolt is the biggest heavyweight? [08:09] did someone say rocket surgery? https://youtu.be/fg58hVEY5Og?t=36 [08:09] zyga: on my machine, the asserts db is the next one, after that it's bolt [08:09] diddledan: … no, nobody. You must be having bad dreams again. [08:10] zyga: bah. There's a json.Unmarshal of *json.RawMessage in there [08:10] but I ignored that because I'd have to dig into upgrading our go [08:10] and i'd rather not dream [08:11] :-) [08:12] Chipaca: I probably missed context, what are you looking into? memory usage? [08:13] pedronis: on friday i idlye looked at a memory profile of my snapd after it started up and did 5 things (refresh, find, list, changes, info) [08:14] pedronis: and two things stood out: osutil's strings.Split, and asserts's directory reading [08:14] pedronis: osutil's is just because it's called eleventy billion times in quick succession and re-reads the whole thing every time [08:14] Chipaca: yea, I would expect snap-revision directory to be growing over time, we will need to do something about it [08:15] pedronis: that one's an easy and clean refactor actually, i could split it out [08:15] at least if i understood the code correctly -- the tests were green :-) [08:16] ship it [08:16] diddledan: no because then mvo gets nasty emails to his personal inbox [08:16] test green means everything is perfect. right? [08:17] diddledan: they _should_ though :-) [08:17] :-p [08:17] Chipaca: will need to gc snap-revisions as well at some point, but that can wait a bit longer [08:18] er [08:18] pedronis: directory lookup will become an issue before space, i fear [08:21] any idea what issue may this guy have with the CLA? https://github.com/snapcore/snapd/pull/5799 [08:21] PR #5799: Install snap-failure binary on Fedora [08:21] Chipaca: ? [08:21] isn't what I said [08:21] that gc can wait longer [08:22] pedronis: i meant kernel performance of finding things in a yuge directory [08:22] pedronis: this is separate from go's memory usage from reading a whole big directory into memory [08:22] Chipaca: we scan them no [08:23] anyway [08:23] Chipaca: are you saying we need to sue a real index? [08:23] sooner or later [08:23] pedronis: I'll push the refactor in a bit [08:24] Chipaca: we already too many PRs :) [08:24] pedronis: I'm saying if we delay gc a lot more, we should think of moving to a two-level directory layout [08:35] so the hang on 18.10 is caused by incomplete seeding. the gtk-common-themes is seeded after the snaps that use it [08:36] * mvo scratches head [08:36] mvo: yes, they dediced that we should fix it in snapd [08:36] they only fixed 18.04 [08:37] pedronis: oh, looks like I did not get the memo [08:37] but are waiting for our fix for 18.10 [08:37] ok [08:37] it's in the bug [08:38] mvo: we discussed a fix I think [08:38] it's not a matter of sorting, we need to add some logic to autoconnect in ifacestate [08:40] pedronis: well, I think its a terrible choice to let users suffer when there is a trivial workaround availalbe but *shrug*. let me dive into the issue [08:40] pedronis: yeah [08:51] mvo: quick workaround: on cosmic, ignore the seed [08:53] Chipaca: ? [08:54] pedronis: a joke [08:54] heh [08:54] pedronis: calling out the cavalier attitude towards breaking users [08:56] Chipaca: I was actually thinking to have a preinst to reoder things, I [08:56] Chipaca: anyway, looking into the real fix now [08:59] Chipaca: is your comment about lookup still relevant with ext4 dir_index, I see here that firefox cache is flat and ext4 is uses a hash directory for it [09:02] Chipaca: yea, same for /var/lib/snapd/assertions/asserts-v0/snap-revision/ [09:04] pedronis: I think the hashe dir made it fine until ~100k entries, but I'd have to look that up [09:06] Chipaca: bit unclear if space will become a problem before or after then, it's not only the size of directory, there is the size of the assertions too [09:06] though snap-revisions are small === chihchun is now known as chihchun_afk [09:09] also each asssertion is a dir [09:13] mvo: apt hooks test unhappy? [09:13] the test runs forever (49 minutes and times out) [09:13] https://api.travis-ci.org/v3/job/426565106/log.txt [09:14] mvo: https://bugs.launchpad.net/ubuntu/+source/ubuntu-meta/+bug/1772844/comments/21 is the comment [09:14] Bug #1772844: snapd didn't initialize all the seeded snaps

[09:14]

[09:14] zyga: not sure [09:15] Chipaca: anyway playing with debug2fs wasn't actually my morning plan [09:15] pedronis: thanks, I remembered and found it, I still disagree but it seems like the intended effect was reached [09:15] pedronis: sary [09:16] Chipaca: anyway 100000 dirs with 4k blocks are 400 mb ? [09:16] mvo: does this mean #1776622 is a dupe of #1772844 ? [09:16] Bug #1776622: snapd on cosmic never finishes installing/updating. Can't install any other updates.

[09:16] Bug #1772844: snapd didn't initialize all the seeded snaps

[09:16]

=== chihchun_afk is now known as chihchun [09:21] Chipaca: too much stuff is too much stuff (tm) [09:24] Chipaca: yes [09:25] Chipaca: well sort of but if the ordering in the seed or our code would be fixed then the bug would be fixed [09:25] Chipaca: I mean the bug that users see the hang on an upgrade [09:28] mvo: we order bases now, but we don't want to order content providers (also because that's too specific for seed code) [09:29] pedronis: yeah, I'm building a unit test right now [09:29] pedronis: once that is done iirc you suggested to fix doAutoConnect in ifstate:handlers.go [09:29] pedronis: so that it would check the wait chain [09:29] yes [09:29] correct [09:29] but tests is the first order of things [09:29] pedronis: and if there is a install of the content provider in the wait chain just do nothing (indead of retrying) [09:30] yes [09:31] wtf, on arch .dirstamp is created in the $(builddir)/snap-mgmt, on ubuntu it's in $(srcdir)/snap-mgmt and generating snap-mgmt obviously fails [09:31] pedronis: great, yeah, working on a test now that models the current cosmmic problem closely and once that is done I look at the other bits [09:31] (s/other bits/fix/ [09:31] ) [09:32] mvo: then the other related issue (but not related to the bug) would be to teach image to at least warn if there are missing content providers [09:32] like it does for bases now [09:32] * mvo nods [09:34] mvo: #5797 green after adding comment as requested; can i merge? [09:34] PR #5797: osutil, o/snapshotstate, o/sss/backend: quick fixes [09:36] Chipaca: nice! out of curiosity, why does runuser fail as root? pardon my ignorance on this [09:36] mvo: because the user doesn't exist [09:36] it's faked [09:36] Chipaca: ok [09:36] mvo: if you're not root, runuser doesn't come into the picture so it's not an issue [09:37] aha, now I get it [09:37] mvo: the test could be changed to fake runuser [09:37] PR snapd#5797 closed: osutil, o/snapshotstate, o/sss/backend: quick fixes [09:37] mvo: but it's diminishing returns land [09:38] Chipaca: no worries, just wanted to understand it better, thats fine [09:39] Chipaca: do we run our unit tests as root in spread? or do we use a test user there [09:39] spread tests default to root [09:40] pedronis: the unit tests are run as the test user afaik [09:40] pedronis: yep [09:40] pedronis: su -l -c "[...] ./run-checks --unit" test [09:41] mborzecki: the meat [09:41] Chipaca: good [09:41] pedronis: the gccgo tests are run differently, but still as test user: su - -c "cd $GOHOME/src/github.com/snapcore/snapd && dpkg-buildpackage -tc -Zgzip" test [09:41] we run the tests as root during package build [09:41] mvo: is that actual root, or fake root? (both would confuse runuser) [09:42] fakeroot [09:42] well [09:42] usually [09:42] but iirc in pbuilder it will be real root [09:42] mborzecki: https://github.com/snapcore/snapd/pull/5802 [09:42] PR #5802: cmd/snap-update-ns: introduce trespassing state tracking [09:43] mborzecki: I staged it so that we can review the algorithm without discussing how the assumptions/restrictions are passed around [09:43] PR snapd#5802 opened: cmd/snap-update-ns: introduce trespassing state tracking [09:43] mborzecki: so that we can separately make that decision in the next branch, either as a new argument or as a helper that holds the state and has MkDir... methods [09:43] mborzecki: but all of the essential new bits are here now [09:44] mborzecki: have a look please and let me know, after this I will have the RFC branch that just passes Restrictions as an optional argument (as in the code you read last week) [09:45] mborzecki: and keeps tracks of Assumptions in main and in a few places below [09:45] ok [09:45] mborzecki: I made unit tests 100% cover the new module [09:45] mborzecki: and moved a pair of helpers there (and made them private since they only exist to fix trespassing issues) [09:46] mborzecki: one more thing to ask is if we should move this to osutils now [09:46] mborzecki: or not [09:50] zyga: iirc there was some concern if we move security sensitive code to osutils, changes tehre may go under the radar or sth [09:50] mborzecki: yeah but as I said on Friday during the call, I think we're past that and we just need to be mindful about it; we use mount code from osutil already because keeping two copies was less useful [09:51] zyga: aha, in that case i'm all for it [09:52] mborzecki: thanks, I'd like Chipaca to weigh in as well (and mvo) [10:05] PR snapd#5801 closed: cmd/snap-update-ns: remove the unused Secure type [10:24] seems like the automated review failed again for VLC, " [10:24] Task a7261f59-ffc0-4a32-b732-3e9d16f8457b failed. [10:32] brb, break [10:53] * zyga never does breaks [10:53] mborzecki: https://github.com/zyga/snapd/commit/7ab0f9f3344e2306779695aba0c355c767d8b0ee [10:53] that's the final cherry on top [10:53] mvo: ^ CC that's what I mentioned [10:53] mborzecki: now the question is, is that sensible or is there an easier way to pass the required state around [10:56] actually https://github.com/zyga/snapd/commit/75f47d017634159b6415246901a088e308c54cc2 is the better link [10:56] anyway [10:56] break for real now :) [10:56] coffee and dog [10:56] ttyl [11:10] PR snapd#5803 opened: ifacestate: fix hang when retrying content providers [11:31] mvo: ^ nice, thanks for that === chihchun is now known as chihchun_afk [11:38] pstolowski: thanks, my pleasure [11:41] mvo: thank you, couple initial comments [11:44] pedronis: aha, good one, will address right after lunch [11:44] pedronis: thank you! [11:45] PR snapd#5804 opened: ifacestate: fix hang when retrying content providers (2.35) [11:48] off to pick up the kids [11:52] zyga: need karma: https://bodhi.fedoraproject.org/updates/snapd-2.35-1.fc29 [11:53] Son_Goku: I'll test on my box today [11:54] thanks! [11:54] PR snapd#5800 closed: cmd: use systemdsystemgeneratorsdir, cleanup automake complaints, tweaks [11:57] zyga, mborzecki, also one of you guys should package the new deps added to snapd [11:57] for Fedora [12:11] is there a way to order services across snaps? like start my service after another snap's service? [12:14] mborzecki: I'll incorporate your review of 5760 into the last branch and close it, ok? [12:14] Saviq: no, there is no such feature [12:25] PR snapcraft#2252 closed: build providers: shell in provider if debug is used [12:34] PR snapd#5760 closed: cmd/snap-update-ns: don't trespass on host filesystem in /etc and other places [12:36] PR snapd#5788 closed: tests: add publisher regex to fix the snap-info test pass on sru [12:40] PR snapd#5514 closed: daemon, overlord/state: warnings pipeline [12:42] re [12:42] Son_Goku: which new deps? juju/ratelimit? [12:42] mborzecki, yes [12:42] Son_Goku: iirc it's already packaged [12:42] in Fedora? [12:42] well, then [12:42] apparently it is [12:43]