[01:19] wgrant: Looks like there are 18 or so legitimate failures, most of them related to query counts. [01:20] And one very strange one where the sharing service seems to generate a completly bong query. [01:20] wallyworld: ^ Let me get you a traceback [01:20] ok [01:23] wallyworld: http://pastebin.ubuntu.com/1077406/ [01:23] * wallyworld looks [01:26] StevenK: so, you have code that i don't, but the callto getPeopleWithoutAccess in trantitionToInfoTyoe in branch is being done with people=[] [01:27] blind_subscribers = service.getPeopleWithoutAccess( [01:27] self, self.subscribers) [01:27] so self.subscribers must be empty [01:28] or so it seems [01:29] + Subscribers: [] [01:30] from the sharing service code: [01:30] # Find the people who can see the artifacts. [01:30] person_ids = [person.id for person in people] [01:30] the onbly way person_ids can be [] is if people is empty [01:31] so i'm not sure what's happening [01:31] can you pastebin your code? [01:31] + Subscribers: [] [01:31] + person_ids = [243652] [01:32] (It's a doctest, so pdb debugging makes me want to kill people.) [01:32] That's probably the wrong call. [01:32] Bah, so it is. [01:32] ah, i may have found it [01:32] wallyworld: https://code.launchpad.net/~stevenk/launchpad/new-branch-visibility/+merge/113484 [01:33] there's also clauses in the result set In(TeamParticipation.teamID, policy_grantees) [01:33] maybe policy_grantees is empty [01:33] or artifact_grantees [01:34] That should be a subselect, shouldn't it? [01:34] So it won't appear as an empty sequence in the textual statement. [01:34] perhaps, but it doesn't appear to be coded that way [01:35] It's a subselect [01:35] In(Person.id, person_ids)) [01:35] It has to be that line [01:36] why not this line In(TeamParticipation.teamID, policy_grantees) [01:36] if not person_ids: return [] ? [01:36] Yeah [01:36] or this one In(TeamParticipation.teamID, artifact_grantees) [01:36] wallyworld: No. [01:36] they use the same construct [01:36] That is on teamparticipation.team, and it uses a subselect [01:36] Whereas the error is about person.id without a subselect [01:37] wgrant, wallyworld: http://pastebin.ubuntu.com/1077416/ is the full statement [01:37] launchpad_dev=# SELECT COUNT(*) FROM branchsubscription WHERE branch = (SELECT id FROM branch WHERE unique_name='~name12/firefox/main'); [01:37] yes, i have read the code again and person ids doesn't use the sub select [01:37] count [01:37] ------- [01:37] 0 [01:37] StevenK: There is the problem [01:37] (1 row) [01:38] StevenK: So you'll want to fix the sharing service to not crash when given no people, but you might also need to fix sampledata. [01:38] i don't think the service should be called with no people [01:38] if no people, don't call it [01:39] but StevenK said above there were people [01:39] + Subscribers: [] [01:39] + person_ids = [243652] [01:39] Different call, as wgrant surmised [01:39] When a statement from a human is in conflict with a traceback, disregard the human statement :) [01:40] ok. i believed what i was told :-) [01:40] authorised_people = [self.branch.owner] [01:40] Unauthorized: (, 'owner', 'launchpad.View') [01:40] so, the design of getPeopleWithoutAccess was to be called with a non empty list [01:40] * StevenK stabs things [01:41] i guess i should have added an assert [01:42] Right, it changes the information type, reconciles access, tries to render Branch:+index, and fails hard because no one has access to the branch. [01:44] that also happended at some point with bugs [01:44] i think reassigning a private bug [01:44] i can't recall the solution, perhaps redirect to the project page with a message i think it was [01:46] wgrant: The branch in question is PUBLIC, I should create a branchsubscription for the owner anyway? [01:46] StevenK: All new branches have a subscription created for the owner. [01:46] They can remove it, of course. [01:47] But the sample data is a little anomalous. [01:47] Right, so let's create a subscription [02:15] * wallyworld -> buy coffee [03:40] * mwhudson stabs the branch vocabulary [03:57] stub: hi [03:57] stub: can we catch up soon ? [after morning cofee :)] [03:57] lifeless: hi [03:57] lifeless: Coffeed up already, been here a while; just offline on IRC [03:58] cool, let me just change rooms [04:00] stub: skype or g+; just sent you a g+ invite but skype is running too ... whatever you prefer [04:11] wgrant: So, query counts? [04:13] StevenK: Can you paste a failure? [04:15] * StevenK fixes another test by use of rSP [04:16] wgrant: http://pastebin.ubuntu.com/1077548/ [04:18] StevenK: Oh, I guess we still need the shortcircuit for public branches [04:18] To avoid tripping query tests like that :) [04:21] wgrant: In IBranch.visibleByUser? [04:22] Hm, I wonder if this evil hack fixes it [04:22] StevenK: Yeah [04:22] wgrant: http://pastebin.ubuntu.com/1077551/ [04:23] StevenK: Exactly. [04:23] Wow, you approving of code I wrote in anger. :-P [04:25] wgrant: I'm also getting a wierd failure in lib/lp/registry/browser/tests/poll-views_0.txt [04:25] Oh? [04:26] wgrant: http://pastebin.ubuntu.com/1077553/ [04:33] StevenK: Did that fail on ec2? [04:33] And locally [04:34] StevenK: I don't care about locally. ec2 is all that's interesting :) [04:34] Oh, hah. It failed on ec2 with [] [04:34] Yeah [04:34] * StevenK ignores it at spurious 2.7 garbage [04:34] s/at/as/ [04:34] Nah [04:35] wgrant: And thoughts on http://pastebin.ubuntu.com/1077556/ ? [04:35] StevenK: It's because of the local timezone. [04:35] Haha [04:35] Polls fail [04:35] Test fail [04:36] It uses datetime.now() and assumes launchpad will interpret it the same way. [04:36] Polls need to die horribly anyway [04:39] StevenK: wgrant: I note you both always want features to die horribly. have you considered maybe having them be tastefully removed instead? [04:40] spm: They don't deserve it. [04:41] that wasn't my implication [04:41] I might have been alluding to deep and disturbing psychological problems that are currently assailing two of my close colleagues. might. [04:42] there's probably room for a "trolling" in that description too [04:42] spm: Polls are special. They have been removed once, and then it turned out a few community teams required them to elect new members, so they were added back. [04:42] lovely [04:45] spm: You're not implying that wgrant and I are closet psychopaths? :-) [04:45] sure I was [04:45] Closet!? [04:45] point [04:45] it could be a very BIG closet [04:51] wgrant: Can has look at pastebin? [04:56] StevenK: Something may have been preloading subscriptions before. [04:56] StevenK: Check the old query log to see what's different [05:03] wgrant: bin/test -m test_pillar_sharing -t test_view_query_count still runs four tests, can I nail it down further? [05:03] StevenK: -t specifies a regex [05:03] Multiple regexes are ORed [05:04] So -t test_pillar_sharing.*test_view_query_count might work [05:04] Otherwise -t test_pillar_sharing.TestProductSharingDetailsView.test_view_query_count should be pretty specific [05:09] wgrant: http://pastebin.ubuntu.com/1077582/ is the test on devel, all 2500 lines of output [05:11] StevenK: I was more interested in just the interesting bit. [05:11] Yeah, I'm just looking how to get the statement recorder to print out what it saw. [05:13] StevenK: Reduce the limit to 1 [05:13] Haha, that would do it too, I suppose [05:14] wgrant: Interesting bit: http://pastebin.ubuntu.com/1077587/ [05:19] StevenK: Ah, I was looking at the wrong test [05:20] StevenK: Given that it's viewing +sharing, it's probably logged in as the product owner [05:20] StevenK: And you'll never guess who owns the branches that it creates. [05:20] Hahah [05:20] So the test is buggy [05:20] It verifies that the query count is constant, but only in a very exceptional case. [05:21] def makeArtifactGrantee( [05:21] self, grantee=None, with_bug=True, [05:21] with_branch=False, security=False): [05:21] * StevenK stabs wallyworld [05:21] why? [05:21] For crimes against PEP8 [05:22] could be worse [05:30] yeah. could be stabbed for not providing cake to deserving webops staff [05:31] spm: Hahaha [05:32] wgrant: So what do you think the plan is? [05:33] StevenK: Well, see what the query count is if the owner is someone else. I suspect it'll be the same with the new and old code. [05:33] StevenK: That's an important bug, but not a regression. [05:45] wgrant: Modulo being distracted by Sarah, http://pastebin.ubuntu.com/1077619/ [05:46] spm: but i gave you some cake [05:46] yeah. could be stabbed for not providing non-virtual cake to deserving webops staff [05:46] fixed. [05:47] if i had one of those "beam me up scotty" things i could send you some [05:49] 🎂 [05:51] wallyworld: Money can be exchanged for goods, such as cake, and services, such as cake delivery to Canberra. [05:51] StevenK: ssshhh [05:52] StevenK: I wonder if the user might be an admin, then. [05:52] StevenK: It doesn't explicitly log in, AFAICT [05:52] So it might be running as some user tha happens to leak from the factory. [05:53] Hahahaha [05:53] TestProductSharingDetailsView.setUp calls login_person(self.owner) [05:54] StevenK: Right, that's what I expected. But then I wonder why the test works even after you change the branch owner. [05:54] Well, it's only a query count [05:55] I've not looked deeply into what that view is doing [06:00] StevenK: Well, it's somehow doing an access check in 0 queries. [06:00] wgrant: Shall we just bump the test from 12 to 26 and move on for the time being? [06:07] StevenK: No. You need to work out why there are 0 queries initially. [06:07] There can't be zero. Is no understand [06:08] StevenK: http://pastebin.ubuntu.com/1077619/ shows 0 access checking queries. [06:10] wgrant: What about the last one that UNIONs against BranchSubscription? [06:13] StevenK: Right, it only finds visible branches, same as with your changes, but then it doesn't do any subsequent checks. [06:13] I can't see any code to prefill the authorization cache. [06:14] So I don't know what's going on [06:15] So where is there the actual view code? I've yet to find it [06:18] StevenK: lp.registry.browser.pillar [06:19] WTF? That code doesn't help at all [06:20] Hm? [06:20] PillarPersonSharingView is what you're looking for [06:25] Indeed, how does this even work? [06:28] wallyworld: Maybe you could help? How is PillarPersonSharingView managing to do its work in 0 queries? [06:29] context? [06:29] when did this start happening? [06:30] wallyworld: http://pastebin.ubuntu.com/1077619/ and http://pastebin.ubuntu.com/1077587/ and http://pastebin.ubuntu.com/1077556/ === matsubara-afk is now known as matsubara [06:34] StevenK: the one with 25 queries, what changes were done for that? [06:34] wallyworld: That branch is my new-branch-visibility branch that uses APG and AAG to determine branch access rather than subscriptions [06:34] wallyworld: The other two pastebins are against devel [06:35] ok [06:37] so the question you want to know i think is why changing the branch owner to some other person other than the pillar owner reduces the query count [06:38] StevenK: do all the other tests pass, other than the query count one? [06:40] wallyworld: bin/test -vvt test_pillar_sharing == Ran 40 tests with 1 failures and 0 errors in 1 minutes 4.142 seconds. [06:40] StevenK: so looking at the devel branch with owner=someperson, the query count has just been reduced by 1, no? [06:40] ie down to 10 [06:41] Which makes no sense to both wgrant and I [06:41] what's your concern? [06:42] (save me trying to grok the sql) [06:42] The issue is not that changing the owner reduces the query count. [06:42] It's that it doesn't increase it. [06:42] Somehow the old code is taking 0 queries to do security proxy access checks. [06:42] The new code is doing one per branch. [06:44] i see that the old code has an extra person select [06:44] at the end [06:45] so where are the SP checks you are expecting? [06:46] the work to load the data is primarily done by accesspolicy stuff at the db level [06:49] wallyworld: Where at the DB level? [06:49] wallyworld: It's not the DB level that's the problem [06:49] It fetches the branches from the DB, then the securityproxies need to verify launchpad.View [06:49] In devel at present the launchpad.View checks take no queries [06:49] StevenK: in the queries performed by findArtifactsByGrantee [06:50] wgrant: but the current code return no branches [06:50] unless i am wrong [06:51] the initial tests were written only for bugs [06:51] since branches didn't support info type etc then [06:52] wallyworld: That's actually not a crazy thought. [06:52] Except that this should have broken last week when we started adding AAGs. [06:53] why? [06:53] if the tests didn't create any branches [06:53] then nothing would have broken [06:53] Oh, did the test change? [06:53] not afaik. [06:54] the test never created any branches to check [06:54] i think [06:54] Does so. [06:54] there's a bunch of test modules that need to have extra tests for branhces added [06:54] def makeArtifactGrantee( [06:54] self, grantee=None, with_bug=True, [06:54] with_branch=False, security=False): [06:54] for x in range(0, 15): [06:54] self.makeArtifactGrantee(person, True, True, False) [06:55] So it makes 15 bugs and branches [06:55] And then dies when the query count increases by 15. [06:55] but are any of those branches included in the results [06:55] Not until last week [06:55] right ok [06:56] But I don't see how this branch could change the set of branches that is found. [06:57] But it doesn't? The queries that are added are all access checks [06:57] If Branch and BranchSubscription are cached by earlier queries which sounds likely, the query count would be low [06:57] i'm still not sure why we expect changing the owner will change the results returned [06:57] StevenK: BranchSubscription doesn't appear to be cached. [06:57] It's not even cachable. [06:58] StevenK: It's used as part of the query's internal access check, but it's not returned. [06:58] wallyworld: I was expecting that we were hitting the branch owner short-circuit. [06:58] But StevenK reports that the query count remains low even when a non-owner is used. [06:58] well it dropped from 11 to 10 [06:59] Yes, defined as 'remains low' [06:59] Rather than 26 [06:59] so with bugs, they are bulk loaded [06:59] not sure about branches [06:59] Right, bug searches generally automatically populate the access cache. [06:59] that's why this query count test was added [07:00] since initially the query count was linear [07:00] just like it appears to be now for branches [07:01] Exactly. The question is how that happens, and why it stops working. [07:02] I suspect there isn't actually any prepopulation of the access cache. We're just hitting another short-circuit somewhere. [07:02] So the test is buggy. [07:03] why is the test buggy? [07:03] it verifies that the query count is independent of nr of bugs [07:03] It's asserting query count linearity, but it looks like it's only linear in a special case that the test hits, rather than the general case. [07:04] it used to fail though before changes were made [07:04] to fix it [07:04] fix the code i mean [07:04] i can't recall what was done though [07:05] it's not asserting linearity, it's ensuring there is no linearity [07:05] Er, yeah, that. [07:05] Sub-linearity :) [07:08] well, the intent is that the query count doesn't increase as the number of artifacts increases [07:08] who is to say it can't decrease by 1 or 2 [07:08] depending on what the exact data is [07:08] wallyworld: That's not the issue [07:08] That issue is that somehow the old code is not issuing extra queries [07:08] The new code is [07:08] The new code needs to not [07:09] But we can't work out how the old code manages to not [07:09] so fix the new code :-) [07:09] So we can't make the new code not. [07:09] StevenK: Breaking on visibleByUser in the old code didn't reveal anything useful? [07:09] wgrant: i'm not sure how did old code did its thing, too long ag [07:09] ago [07:10] Indeed, I didn't really expect you to remember :) [07:10] wgrant: bugtasks = list(getUtility(IBugTaskSet).search(param)) [07:10] that's what the old code does to load the bugs [07:11] where param has the bug ids [07:11] param = BugTaskSearchParams(user=user, bug=any(*bug_ids)) [07:11] iso it's whatever IBugTaskSet does [07:11] Yes, bugtaskset precaches authorization [07:11] But branchcollection appears not to [07:12] yes [07:12] so isn't that the answer? [07:12] the old code for branches [07:12] all_branches = getUtility(IAllBranches) [07:12] wanted_branches = all_branches.visibleByUser(user).withIds( [07:12] *branch_ids) [07:12] branches = list(wanted_branches.getBranches()) [07:13] Hmmm? [07:13] The answer to how it avoids queries is not "it's not precached" [07:13] That's the anti-answer :) [07:13] no, i meran bugtaskset precahces, so eliminates queries [07:13] branch collection does not [07:13] so queries increase [07:14] So, I think the 10th query in the devel test is IBranchCollection.visibleByUser() [07:14] But *something* manages to avoid an access query for the 15 branches in the old code! [07:14] The key thing is to identify what that is. [07:14] I'm not certain why it is called 15 times, but only generates one query [07:14] Well, I'm certain why it's called [07:14] so it must be in branchcollection [07:14] branchcollection gives me a headache [07:14] It's horrible [07:14] yep [07:19] StevenK: So how many times is the visibleByUser called in the problematic context, in devel? [07:19] wgrant: 15 [07:20] StevenK: And what does stepping through it show it doing? [07:21] wgrant: Hold on a moment, visibleByUser returns a collection [07:21] So it probably returns all fifteen in one query [07:21] StevenK: I mean Branch.visibleByUser, not *BranchCollection.visibleByUser [07:21] Branch.visibleByUser is not called [07:26] wgrant: http://pastebin.ubuntu.com/1077708/ [07:28] StevenK: Hm [07:28] StevenK: Of course, the alternate explanation is that the user can't actually see any of the branches in the devel test. [07:28] StevenK: So there's nothing returned to check access on. [07:28] StevenK: Can you check that hypothesis? [07:29] The project owner will be able to see them in your branch, due to the APG [07:32] wgrant: self.branches in PPSV is [] [07:32] (for devel) [07:33] StevenK: Ha ha hahhah [07:33] That explains it. [07:33] [, ...] for my branch [07:34] So it caches nothing and was a useless test [07:34] It's good for bugs. [07:34] Just not branches. [07:34] it was designed primarily for bugs [07:34] branches weren't done yet [07:34] So it's awesome that we've wasted a few hours bashing our head against it [07:34] Which is unsurprising, given that branches didn't know about the access schema when it was written [07:35] wgrant: So, 12 -> 26 and move on? Please? [07:35] StevenK: Do file a critical bug about it, but yes. [07:35] good morning [07:38] wgrant: https://bugs.launchpad.net/launchpad/+bug/1021622 [07:38] <_mup_> Bug #1021622: PillarPersonSharingView uses O(n) queries for branch access < https://launchpad.net/bugs/1021622 > [07:38] StevenK: Thanks. [08:08] morning === almaisan-away is now known as al-maisan [08:42] Err, I just got http://paste.ubuntu.com/1077762/ from an ec2 land run - should I be concerned? === jamestunnicliff_ is now known as jamestunnicliffe === adeuring changed the topic of #launchpad-dev to: http://dev.launchpad.net/ | On call reviewer: adeuring | Firefighting: - | Critical bugs: 4.0*10^2 [09:49] Oh, well, it didn't do it again on a retry ... [10:29] cjwatson, then it does mean you should be worried :) === al-maisan is now known as almaisan-away === matsubara is now known as matsubara-afk [11:34] * cjwatson starts to understand the layout of .../browser/ and .../templates/ and feels something else leaking out of his ears as counterbalance [13:32] adeuring, https://plus.google.com/hangouts/_/977b4ffbaf4a8c42a0f41a913e0632048bf13e25?authuser=0&hl=en [13:49] benji: you landed some stuff in the lazr code (and launchpadlib) that seem to be dealing with httplib2 0.7+ (which enables ssl validation by default). [13:49] I'm trying to land some of that in launchpad itself (because I want a patch to launchpadlib's test suite to pass on py2.7) [13:50] however, httplib 0.7 breaks launchpad's test suite because of the ssl validation. [13:50] is it sane to just set the env var for the test suite/ [13:50] ? [13:51] jam: I'm trying to remember that branch... oh, I think that is the one by jml; yeah, setting the env. variable for the test suite would work. [13:58] benji: where/when would that get set in the lp test suite? [13:59] jam: hmm, let me look [13:59] maybe in a layer? [13:59] LPFunctionalLayer is the one the specific test I'm looking at right now. [14:01] jam: that might work; if you wanted to set it globally, doing it in buildout-templates/bin/test.in would make sense [14:07] What's a sensible way to write a test that attempts to open a URL on the appserver that traverses to one on the librarian? [14:07] I can use setupBrowserForUser to get something that knows how to open URLs on the appserver, and urlopen to open librarian files, but I can't seem to find something that works for both [14:07] (I'm in LaunchpadFunctionalLayer) [14:16] benji, bac, jcsackett: I just committed and uploaded launchpadlib-0.10.2 (as approved). Can you upload it to pypi? I can do everything but that step. [14:16] jam, benji: let me know if I can help. [14:17] jam: if you're going to be making releases, then you should have pypi access; give me your pypi user name and I'll give you access [14:19] jam: to clarify, we don't upload releases to pypi, we just register them (although, we should upload them in my opinion, but that's a seperate issue) [14:20] benji: I did mean register, and my pypi name is jameinel, let me double check, though. [14:20] benji: yeah, jameinel [14:21] jam: you're all set [14:21] jml: so the patch you requested ends up *requiring* httplib2 0.7+ because it is passing a parameter that doesn't exist in 0.6 [14:21] however, using 0.7 means that the test suite aborts all over the place because by default the ssl certificate is checked and fails [14:22] I'm guessing we want to push forward, and fix that, rather than changing the patch to be backward compatible? [14:22] jam: I think so. 0.6 isn't even the version in lucid. [14:23] jml: it is the version in versions.cfg :) [14:24] jml: the problem right now is that I set the env var in bin/test, and if I drop into pdb I see it set, but the test still fails [14:25] jam: interesting. I had to make changes both to launchpadlib and to lazr.restfulclient. Perhaps only one is up-to-date? [14:26] jml, what's "LoC neutrality"? Is it removing as many LoC as you add? [14:26] mpt: yes. [14:27] jml: something is stripping the env variable in "launchpad_for() [14:27] jam: remind me which code-base that function is in? [14:27] as in, it is set, I break on _request and then it is no longer set [14:27] lazr.restfulclient._browser.Browser._request is the chain that ends up failing [14:27] which you updated to support the flag [14:28] the test I'm running is: lp.bugs.browser.tests.test_bugattachment_file_access.TestWebserviceAccessToBugAttachmentFiles.test_user_access_to_private_bug_attachment [14:32] I'm sure I'm running updated code, as I'm getting the error: SSLHandshakeError: [Errno 1] _ssl.c:504: error:14090086:SSL routines:SSL3_GET_SERVER_CERTIFICATE:certificate verify failed: perhaps set LP_DISABLE_SSL_CERTIFICATE_VALIDATION if certificate validation failed and you genuinely trust the remote server. [14:32] which means it asks me to set the variable I have set. [14:33] jam: pulling latest stable [14:34] jam: what's launchpad_for() ? [14:35] oh launchpadlib_for [14:36] jml: so... I can see that the value is being seen inside _browser [14:37] self.disable_ssl_certificate_validation is True [14:37] and I'm still getting an SSL handshake failure... [14:37] jam: perhaps I missed a place in my patch? can you paste the full stack? [14:38] jam: also, I thought you were reporting that the env var is unset at the time of error? [14:39] jml: lp:///~jameinel/launchpad/py27-introduction-1020667 [14:39] jml: I thought that was the problem, as I was still getting the error. [14:39]