[01:28] wgrant: i ran into some issues with by branch but finally got them sorted. it would be great if you could review it [01:28] https://code.launchpad.net/~wallyworld/launchpad/create-aag-privacy-transitions2-1009364/+merge/110706 [01:41] wallyworld_: Looking. [01:41] thanks [01:43] wallyworld_: That bugtasksearch change is worrisome. [01:44] wallyworld_: eg. it breaks TextualBugTaskSearchListingView [01:44] i didn't run all the tests [01:45] we can leave it as it was but it will continue to return duplicates [01:45] Normal bug searches return duplicates too. [01:45] which is unfortunate [01:45] how so ? [01:45] It just happens that this method only returns the bug ID, so they're more obvious. [01:46] lifeless: since when would it be expected a search result should be ok to return dups? [01:46] They're not strictly duplicates. [01:46] they are if they are ids [01:46] In that particular format they are. [01:47] I'm not sure it's useful to suppress them. [01:47] i can use a set instead of a list when i consume them [01:47] And the way you've done it there breaks textual searches and kills performance. [01:48] so no problem, i'll revert and work around it [01:48] I agree the method's current result is less than optimal. [01:48] But it's all-round easier and not too much uglier to work around it using set() [01:48] apart from that, ok? [01:49] + if information_type in PRIVATE_INFORMATION_TYPES: [01:49] shouldn't that check that it's coming from something public? [01:49] probably [01:50] but maybe not [01:50] since i person may have had access before but not now [01:50] due to the new info type [01:50] Ah, true. [01:50] eg they had an APG [01:50] Yep, you are completely correct. [01:50] But a comment to that effect might be handy. [01:51] ok, will d. this stuff can hurt one's brain [01:51] Yup [01:51] wgrant: Can? Does. [01:51] Er, wallyworld_, even. [01:51] yep [01:55] wallyworld_: findPeopleWithoutAccess doesn't seem to have a good reason to be on APGF, and it doesn't seem to check policy grants. [01:56] APGF is only flattened in one direction, not the other. [01:56] A grant for an artifact shows up in APGF linked to all the artifact's policies. [01:56] But a grant for a policy doesn't show up linked to all of the artifacts, because that can be hundreds of thousands of rows. [01:56] And that set is much quicker to query the other way. [01:57] wgrant: it's there because it queries the aggf table, hence is with the other methods which do such queries [01:57] wallyworld_: It queries the subset of APGF that is AAG. [01:58] APGF is good for getting all the grants related to a policy. [01:58] It *doesn't* work for getting all the grants related to an artifact. [01:59] hmm. i will look at reworking it [01:59] I'm pretty sure that CTE also just overcomplicates things. [02:00] it makes the core query a bit simpler to grok [02:00] It should be a pretty simple query through TP, AAG, APA and APG [02:00] which you are saying you would prefer in the service? [02:01] wallyworld_: so its not that duplicates in that method are desired, its that; a) forcing distinct in the DB forces a sort and merge, and sometimes we don't want that; secondly bug search is a horrid engine today, and keeping it sane is tricky [02:01] I think it's probably best in the service. [02:01] Given it's sort of over-arching and doesn't fit in any of the others.' [02:01] lifeless: yeah, understood, i was just responding to my assumption that you were questioning my view on whether duplicates in search results were good or bad [02:02] ah [02:02] so, on general searches, we have a discontinuity in the UI [02:02] we show tasks [02:02] but claim to show bugs [02:02] Well [02:02] yeah [02:02] Tasks don't exist. [02:02] this confuses folk :) and is where the 'duplicates' come from [02:02] The UI deliberately never mentions tasks. [02:03] lifeless: sure, but in this case, i invoke the api that gives ids, and those are indeed duplicated [02:03] wgrant: abstractions leak; this leaks. [02:03] * StevenK stabs Django [02:03] lifeless: Of course. [02:03] wallyworld_: Apart from that method being wrong, and the bug search thing, this looks good, thanks. [02:04] wgrant: cool, already fixed the search method, will move the other code, thanks for looking [02:04] We need a couple of nice big cleanup pass through all this once the legacy stuff is removed. [02:04] Great. [02:11] wgrant: http://pastebin.ubuntu.com/1046567/ [02:17] lifeless: I can now import auditor.manage and auditor.settings in bin/auditor-manage, but it now can't find the module auditlog :-( [02:22] thats auditor.auditlog, right ? [02:25] StevenK: Hm, you didn't check security.py-cleanliness before you landed? [02:34] lifeless: Yeah, but loading that in bin/auditor-manage doesn't help. [02:34] wgrant: Obviously not. :-( [02:34] StevenK: Anyway, should be fine. Land a fix now, but the default EXECUTE should be sufficient to make it all work for tonight. [02:35] wgrant: Where should the three of them go? launchpad_main ? [02:35] (in postgres, functions default to granting EXECUTE to PUBLIC) [02:35] StevenK: public, with the ret. [02:35] st [02:35] rest [02:35] Pretty much every function should be in public. [02:35] wgrant: Right, land to db-devel? [02:36] StevenK: Indeed. [02:36] As long as nothing further has landed there after your patch. [02:36] (other than merges from stable) [02:36] * StevenK re-checks the deployment report. [02:37] Looking good on that front. [02:42] wgrant: Some of the functions are listed with no rights. How do I tell if the new functions should be no rights? [02:44] StevenK: The ones that don't need any rights should have no rights :) [02:44] Things that are only called as helpers from SECURITY DEFINER functions don't need to be executable by normal users. [02:45] wgrant: http://pastebin.ubuntu.com/1046591/ [02:45] It's not a problem for all of these methods to be publicly executable. [02:45] But they don't all need to be. [02:46] StevenK: That should break. [02:46] accessartifact_denorm_to_artifacts is called by accessartifact_maintain_denorm_to_artifacts_trig [02:46] Which is not SECURITY DEFINER. [02:47] * StevenK adds EXECUTE rights to it. [02:47] branch_denorm_access too [02:47] build_access_cache can be EXECUTE if you want, but I don't believe it's necessary. [02:48] wgrant: Hah, maintain_transitively_private() isn't in security.cfg. [02:51] StevenK: It's a trigger function. [02:55] wgrant: Those security.cfg changes don't cause any tests in test_branch_access_policy_triggers, test_bugtaskflat_triggers and test_bug_mirror_access_triggers to fail. [02:55] That's a start. [02:56] wgrant: Shall I push it up? [02:56] Might as well. [02:56] Throw it off to ec2. [02:56] Oh, stub needs to sticky beak it? [02:56] nup [02:56] Not for security.cfg, no [03:03] wgrant: https://code.launchpad.net/~stevenk/launchpad/db-branch-nap-security/+merge/110709 and it's currently in the middle of setting up ec2 test [03:09] StevenK: Thanks. [03:14] Except it's testing against devel. Fail. [03:15] ec2 kill needs to die early if you don't specify an instance id rather than failing with an obtuse traceback. [03:16] StevenK: Probably doesn't matter, if your branch is from db-devel in the first place. [03:17] Mmmmmm [03:17] Too late, was the cry [03:24] wgrant: i've pushed the changes to my branch [03:24] wallyworld_: Looking [03:24] thanks [03:25] wallyworld_: The branch got cursed again? [03:25] yep [03:25] Excellent. [03:25] Maybe we can work out what it's hanging on. [03:25] webops: Hi [03:25] yo [03:26] spm: The branch scanner hates some branches sometimes, hanging for more than 5 minutes before timing out. I'd like to trigger a hang and then get pg_stat_activity checked to see what it's doing. [03:26] can we do that in staging? [03:27] We've never reproduced it there. [03:27] Or on dev. [03:27] have we ever tried? :-) [03:27] Yup [03:27] just checking, your wording could have been taken as very cautiously spaken [03:27] Heh [03:27] True. [03:28] so we know which branches do this on a regular basis? it's not the on disk bzr side perhaps? [03:29] Nothing does it on a regular basis, AFAICT. It's normally a new branch that gets permanently cursed. [03:29] Pushing it up under another name works. [03:29] huh, weird. [03:29] Precisely. [03:29] how do you propose to trigger? and do we know which DB server(s) we need stats from? is it worth doing a * * * * * pg_stat_dump > logfile? [03:30] spm: branchscanner user on wildcherry [03:30] We trigger by wallyworld forcing a scan on that branch. [03:30] With a push [03:30] is wallyworld_ planning on promising cake to go with this debug? cause, you know, cake. [03:30] wgrant: you want me to do that now? [03:31] spm: i have no cake - the dog ate it [03:31] so I hear [03:32] wgrant: select * from pg_stat_activity where usename='branchscanner'; or do you want the full thing, jic? [03:32] spm: That'll do [03:32] wallyworld_: Please. [03:33] I have my rsynctail running to observe the hang. [03:33] wgrant: just pushed -r-2 and then -r-1 [03:34] It be scanning. [03:35] spm: Can you check pg_stat_activity now? [03:35] been grabbing heaps the past min or so [03:35] Great. [03:35] Been going for a minute now, which is itself a good sign that it's hung. [03:35] Should normally take about 50s. [03:35] probably should have while looped, but anyway [03:36] yay, it has hung. [03:36] appears stuck in a SELECT [03:36] First time we've caught it in the act. [03:36] What is the SELECT? [03:36] it's a secret, that will be exposed on production of cake [03:37] Sneaky. [03:37] indeed. a cunning plan even [03:37] wgrant: https://pastebin.canonical.com/68304/ [03:38] still at the same place nearly a minute later too [03:38] Aha [03:38] Thanks. [03:39] Should time out in 20s or so [03:39] so, code review + http://okfnlabs.org/annotator/demo/ [03:39] still holding atm [03:42] spm: It's done, btw. [03:42] timed out, as expected. [03:42] and yes, cleared here as well [03:42] spm: any more interesting queries? [03:42] not for branchsscanner [03:42] I mean from before the SELECT of Doom. [03:43] I can paste you a rather "long" session history, if you're interested? [03:43] That might be handy, thanks. [03:43] in transaction <== a fair bit [03:43] Better than having to do this again. [03:43] Yeah [03:44] wgrant: https://pastebin.canonical.com/68305/ [03:45] spm: Thanks! [03:47] It has traditionally been suspected that bzr slowness was the issue here, but this proves otherwise :) [03:48] Now, it doesn't actually tell us how it's being so slow, but at least it's something. [03:48] wgrant: I'd like to point out that your email address is in that query; QED, it's all your fault. <== advanced sysadmin debug. [03:48] That's good solid logic you have there :) [03:48] I'm glad you approve [04:05] Hmmm [04:05] So the branch has a full set of BranchRevisions, but no last_scanned_id [04:06] This is causing subsequent runs to try to clear the entire history. [04:06] But it doesn't explain what happened during the initial failure. [04:08] wallyworld_: How soon after pushing the branch did you create the MP? [04:09] wgrant: almost straight away [04:09] A saw a WIP MP pretty soon after the push [04:09] Looks like that is how to repro it. [04:10] I wonder if we're deadlocked with the MPJ runner. [04:10] plausible [04:10] 2 minutes so far... [04:10] That means we have two bugs. [04:10] Fail [04:11] Took 2.5 minutes, but it succeeded [04:12] branch revision has a lot to answer for [04:12] * wallyworld_ -> school and back [04:27] wallyworld_: Why not do the grants after _reconcileAccess, so you can use APAs rather than having getPeopleWithoutAccess duplicate the pillar determination logic? [04:40] wallyworld_: So, QA? :-) [04:40] wgrant: Hmm, so you think MPJ and the scanner lock? [04:40] Plausible, indeed. [04:42] Discounted in this case, it seems. [04:42] The first logged MPJ was 12 minutes after the scan timed out. [05:28] wallyworld_: https://bugs.launchpad.net/launchpad/+bug/808930/comments/20 is the result of the debugging, btw. [05:28] <_mup_> Bug #808930: Timeout running branch scanner job < https://launchpad.net/bugs/808930 > [05:43] wgrant: so we really want to delete BranchRevision :-) [05:45] wallyworld_: But it's my friend :( [05:48] wgrant: i'm not sure i follow your review suggestion above [05:48] i do call ensureAccessGrants after reconcileAccess [05:49] wallyworld_: Right, then you don't need to manually determine the pillars for bugs and branches. [05:49] You can just use the APAs [05:52] wgrant: so, i use IAccessPolicyArtifactSource.find() to fill out the policies_to_check array? [05:53] wallyworld_: AccessPolicyArtifact.artifact = abstract_artifact.id AND AccessPolicyGrant.policy = AccessPolicyArtifact.policy AND AccessPolicyGrant.grantee = TeamParticipation.team [05:54] Python never needs to see the policies. [05:54] They're purely used by the join from AccessArtifact to AccessPolicyGrant. [05:54] ah ok. [05:55] gary_poster: btw, trunk testrepository has a --concurrency option for you [05:56] wallyworld_: Oh, also, that whole thing is pretty strange. Your top-level query has "Person.id IN person_ids AND Person.id NOT IN policy_grantees AND Person.id NOT IN artifact_grantees", but then policy_grantees and artifact_grantees also filter on person_ids. [05:57] wgrant: i was attempting to limit the size of the in lists [05:58] the top level In(Person.id, person_ids) is because we only want people withotu access from those that are specified [05:59] wallyworld_: They're not lists, they're subquery expressions :) [05:59] It optimises through them. [05:59] sure, but the subsquery still returns results [05:59] Not really, no. [06:00] internally [06:00] Again, not really :) [06:00] Theoretically perhaps. [06:00] so if i limit the size of the subquery, it's more efficient [06:00] it is with oracle anyway [06:00] Not necessarily. [06:00] Also not necessarily. [06:00] It depends on how you're limiting the size. [06:00] So [06:01] I'd pull the TP join out. [06:01] There's no reason to have it all the way down there. [06:03] If I were postgres, I'd plan that as a nested loop, and the person_filter inside the subqueries would have no effect whatsoever. [06:03] ok, i'll take a look. maybe a run on dogfood to check ite performance is needed [06:03] Any reasonably query planner can see that. [06:08] The planner is smarter than you, except when it isn't. [06:08] In this case it should be :) [06:10] wgrant: this should be ok i think https://pastebin.canonical.com/68312/ [06:10] except for flat maybe [06:11] yes, flat is gone [06:13] https://pastebin.canonical.com/68313/ [06:27] wallyworld_: If I were writing it, I'd phrase it as the set difference between the requested people and the result of https://pastebin.canonical.com/68315/ [06:27] But your new version looks correct. [06:27] (although the distincts on the subqueries are pointless) [06:28] wgrant: looks like your version returns the oppoiste of what is wanted? [06:30] wallyworld_: Right, positive queries are much easier to write directly. [06:31] Anyway, yours is OK if you drop the distinct=Trues [06:31] Mine is much faster, but we shouldn't have performance issues here anyway. [06:31] Since the dataset is small. [06:33] wgrant: distinct on subquery is a planner hint [06:33] wgrant: as is offset 0; [06:33] s/hint/sledgehammer/ [06:34] wgrant: i've removed the distincts, i can rework to match your logic [06:37] lifeless: Maybe in postgres 6.1 [06:37] lifeless: But IN implies distinct on the subquery nowadays [06:37] Plus we have no reason to use planner deception techniques here. [06:38] You should use them once it's shown that you need them; not before :) [06:38] wallyworld_: The rework's not critical, but I wonder if it makes more sense as a getPeopleWithAccess anyway [06:38] wallyworld_: eg. for determining who gets notified. [06:39] (for later structural subscription enablement) [06:39] wgrant: we can add a method for that, but i'd rather use the db for now to find people without access [06:40] You're probably better off having a single method and doing integral set difference in Python. [06:40] i used distinct onthe sub queries from prior experience, but whio know what postgres dowesa [06:41] wgrant: i was trying to avoid anyting in python [06:41] It's almost always prettier to do set difference in Python than it is to describe non-trivial negative assertions in SQL :) [06:41] Perhaps. [06:41] StevenK: Going to qa-ok your db-stable rev? [06:44] I got distracted by auditor [06:45] wgrant: db-branch-nap-security => db-devel [OK] (up for 3:29:40) i-b2f88acb [06:45] That's encouraging. [06:49] wgrant: are you ok to +1 the mp and i'll land after i've had a look at rejigging the query to you your logic? i'd still like to keep everything in the db for now [06:49] Just +1ing now, yep. [06:49] and we can pull out common query expr for notofications later if needed [06:49] thanks [06:49] If loggerhead wakes up [06:51] There we go [06:51] Thanks. [06:54] wgrant: thanks. btw, the bloody bug subscription removal job still oopes on qas :-( [06:54] wallyworld_: No idea why? [06:54] error reading 'infoformation types' from job metadata [06:54] wut [06:54] examining log [06:54] yeah, haven't looked too much nto it [06:55] the should should only be able to be constructed such that metadata is written with info type key [06:55] Oh [06:55] What if it's old data? [06:55] From before the jobs were merged. [06:55] ah, that is it i think [06:56] It's certainly what it looks like. [06:56] so, i'll do a branch to make the code robnust [06:56] Mmm [06:56] No [06:56] It shouldn't be robust against information_types being missing. [06:56] Oh, if it's in getOperationDescription, hm, maybe. [06:56] it's trivial to do though [06:56] It is, yeah [06:56] missing = [] [06:57] so close to being able to turn all this shit on [06:57] Heh [06:57] Yeah [06:57] finally [06:59] StevenK: fdt requested [07:00] \o/ [07:00] wgrant: What shall we do with this security.cfg branch, then? [07:00] StevenK: It'll be landable on db-devel before this is deployed. [07:00] Land it on db-devel before this is deployed. [07:01] We can then merge db-devel instead of the relevant rev of db-stable. [07:01] Since nothing further of consequence has landed on db-devel to stop us from doing that. [07:01] Keeps the MP happy. [07:02] wgrant: Sounds like a good plan. [07:43] wgrant: db-devel r11688 [07:45] good morning [07:45] StevenK: Thanks. === almaisan-away is now known as al-maisan [08:27] wgrant: in 8.4 distinct made a different in subqueries, FWIW. Haven't tested in 9.1 yet. [09:24] jml: have to ask, what headset do you use and do you recommend it ? [09:24] czajkowski: I use my Bose QC3 headphones with a mic cable. I've successfully used Logitech USB headsets in the past. [09:25] czajkowski: ultimately, what matters is that it's well-tested and that it doesn't leak noise into the mic. [09:26] perhaps it's inevitable when you have > N people, but it always seems as if someone is on a combination of hardware/software that they are only using for the first time that day. [10:34] wgrant: I'm planning on re starting the fix and then copy translations for Q today. I was hoping to get more insight into how it failed on friday. In case it is more of a systemic issue. [10:34] What process was it that you thought was adding data? (User filling out translations for P and that somehow ending up in Q?) [10:35] jam: Translations are shared between series. [10:35] So a translation in Precise can show up in Quantal. [10:35] Or even a translation in an upstream project. [10:36] wgrant: so I understand that they can be shared, and that is my understanding of why we are running a 'copy' script in the first place. But I'm trying to understand how/what decides to put a Q entry in when submitting a P entry. [10:36] jam: The normal Translations model code in the webapp. [11:06] wgrant: I'm poking through the code base, and I'm noticing a garbo script that touches the PO tables. Is that something we should be stopping along with the cron scripts? [11:07] The script is ScrubPOFileTranslator [11:09] anyone else able to answer how and when garbo runs and how likely it is to be inserting data into the db for a table we're trying to clean up? [11:10] jam: That's in garbo-daily. [11:10] It was introduced by jtv recently as part of this work. [11:10] It maintains POFileTranslator, both adding and deleting [11:10] wgrant: right, it was something he wanted to move into a refresh-the-db stuff so that it doesn't block the copy stuff [11:11] but I don't know if we need to deactivate it, etc. [11:11] 5 23 * * * $LP_PY /srv/launchpad.net/production/launchpad/cronscripts/garbo-daily.py --abort-script=72000 -q --log-file=INFO:/srv/launchpad.net/production-logs/garbo-daily.log [11:11] It died with fastdowntime, so it is not a concern. [11:11] And it would have been dead on Friday too. [11:11] wgrant: died with fastdowntime? [11:12] as in, garbo-daily isn't working correctly? [11:12] right now [11:12] jam: fastdowntime killed its DB connection, as it was meant to. [11:12] It's working fine, but not running atm since we killed it 72 minutes ago [11:12] ah, and so won't be running again until tomorrow/tonight at 23:05 [11:12] UTC? [11:12] Right. [11:17] jelmer: vila mgz could one of you look at https://answers.launchpad.net/launchpad/+question/200576 please. [11:18] czajkowski: on it [11:18] jelmer: lovely thanks [11:18] am looking forward to reading that fo my reference [11:19] wgrant: another small thing (hopefully), we were getting email spam while the cron jobs were stopped. Is that something we can supress since we know that we manually stopped them? [11:20] jam: You'll need to ask ops to disable the relevant script-monitor cronjobs. === matsubara is now known as matsubara-lunch [11:21] cjwatson: on the installer does it ask you which drive to install Ubuntu on ? [11:21] Hi, since I don't have commit access but I have an approved branch, could someone kick off the test + check in cycle on this please: https://code.launchpad.net/~dooferlad/launchpad/workitems-bugfix/+merge/110563 [11:28] wgrant: so the table that was getting new data was POFileTranslator (IIRC). However, I see this in the db: [11:28] https://pastebin.canonical.com/68246/ [11:28] sorry bad paste [11:28] http://paste.ubuntu.com/1047080/ [11:28] so why is new data being inserted there if it is considered 5-year old legac [11:28] legacy. [11:29] jamestunnicliffe: not sure who does that is it a reviewer [11:29] czajkowski: You should get to choose in the event that you have multiple drives, last I checked, yes [11:29] maybe pofiletranslator is still new, but the others are not? [11:29] cjwatson: that's what I thought, but do kinda feel bad for https://answers.launchpad.net/ubuntu/+question/200739 [11:30] cjwatson: last time I just installed ubuntu and wiped the machine when I bought it. [11:30] yeah, that looks right. As it adds a function that does pretty much the same thing, to update POFileTranslator, but doing so from different source tables. [11:30] jam: Huh? [11:30] jam: That's dropping two functions to work with old tables. [11:30] wgrant: I was misreading a comment. It was saying a table was legacy. [11:30] Right. [11:31] I thought it was saying POFileTranslator [11:31] was the legacy table [11:31] but it was saying the other ones were [11:31] Right, no :) [11:32] jam: Anyway, I'd just try again [11:32] See what happens. [11:32] yeah, I'm still waiting on ops, so I figured I'd try to at least understand how things are working together. [11:33] To be clear, Translations is by far my weakest point. [11:33] I don't really know how it works. [11:33] Only very vaguely. [11:33] If a db table is getting updates while doing COPY that seems a good way for copy to break [11:33] It may not be the specific problem we've been running into. [11:33] But it was the problem we ran into on Friday. [11:33] for the delete step [11:33]