| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-07-18 | |||
| 15:20:25 | mriedem | which sucks, but it is what it is | |
| 15:21:17 | jaypipes | mriedem: ok, fair enough. will remove my -1. | |
| 15:21:41 | gibi | jaypipes: with alex_xu's https://review.openstack.org/#/c/480379/ I still get the same stack trace. logs are here http://paste.openstack.org/show/615747/ | |
| 15:22:00 | mriedem | thanks | |
| 15:22:00 | gibi | jaypipes: I will try to combine cdent's and alex_xu's patch together | |
| 15:22:11 | jaypipes | gibi: k | |
| 15:29:45 | openstackgerrit | Matt Riedemann proposed openstack/nova master: DNM: Test changes with multiple cells https://review.openstack.org/467383 | |
| 15:29:48 | jaypipes | gibi: hmm, ok I have an idea... | |
| 15:30:33 | jaypipes | gibi: can you do me a favor? | |
| 15:30:37 | jaypipes | gibi: these two lines: https://github.com/openstack/nova/blob/master/nova/objects/resource_provider.py#L2393-L2394 | |
| 15:30:47 | gibi | jaypipes: sure | |
| 15:30:55 | jaypipes | gibi: can you change them to have the usage fields on the *right* side of the condition? | |
| 15:31:21 | jaypipes | gibi: in other words, they should be: inv.c.resource_provider_id == usage.c.resource_provider_id | |
| 15:31:29 | jaypipes | and the same for the resource_class_id columns. | |
| 15:31:34 | openstackgerrit | Gábor Antal proposed openstack/nova master: Transform libvirt.error notification https://review.openstack.org/484851 | |
| 15:31:35 | gibi | jaypipes: sure I can do that. Meanwhile the two combined patches resulted the same HTTP 500 | |
| 15:31:58 | gibi | jaypipes: do you need that change top of master or top of some bugfixes? | |
| 15:32:05 | gibi | jaypipes: or doesnt matter | |
| 15:32:10 | jaypipes | gibi: I'm wondering if SQLAlchemy is constructing the LEFT JOIN verbatim with that order (which is an incorrect join order) | |
| 15:32:25 | jaypipes | gibi: just make it locally on whatever code you have running. | |
| 15:32:31 | gibi | jaypipes: OK | |
| 15:33:35 | jaypipes | zzzeek: hey Mike, does SA rewrite outerjoins to be the "correct" join condition column order if there's a mistake in the order of the join condition? :) see https://github.com/openstack/nova/blob/master/nova/objects/resource_provider.py#L2393-L2394 | |
| 15:33:39 | mgiles | mriedem ildikov I took a pass as the grenade tests that stvnoyes was going to work on. See: https://review.openstack.org/#/c/484469/ | |
| 15:33:52 | jaypipes | zzzeek: it would be awesome if SA solved all my mistakes for me :P | |
| 15:35:11 | ildikov | mgiles: great, thank you! | |
| 15:36:22 | zzzeek | jaypipes: column order....i dont think so? you mean "A LEFT OUTER JOIN B" and not "B LEFT OUTER JOIN A" ? | |
| 15:37:49 | zzzeek | jaypipes: like you want the two conditions to be in some order to satisfy an index or something ? | |
| 15:39:47 | jaypipes | zzzeek: no, I was mostly joking with you :) I have the SQL correct in the code comment above there but have the column order wrong in the SQLalchemy join. | |
| 15:40:25 | zzzeek | jaypipes: the SQL is rendering differently from what you specify ? | |
| 15:40:51 | ildikov | stephenfin: happy to try to answer questions if you can take a look at the live_migrate patch :) | |
| 15:41:15 | jaypipes | zzzeek: not sure, but I suspect it would be. I mean, I'm asking SQLalchemy to do a LEFT JOIN using the wrong order of columns in the join condition. | |
| 15:41:47 | jaypipes | zzzeek: it's my mistake, not SA's :) I was just joking with you about having SA read my mind ;) | |
| 15:42:14 | zzzeek | jaypipes: i still don't understand what "wrong order of columns" means | |
| 15:42:59 | jaypipes | zzzeek: oh, I'm saying that I should be doing a LEFT JOIN b ON a.col = b.col, but I'm asking SA to do a LEFT JOIN b ON b.col = a.col | |
| 15:43:14 | zzzeek | jaypipes: OK you mean in the condition around an operator | |
| 15:43:18 | jaypipes | and maybe SA is doing b LEFT JOIN a ON b.col = a.col | |
| 15:43:24 | jaypipes | zzzeek: ya | |
| 15:43:25 | zzzeek | jaypipes: if you are doing "col <operator> col" it should preserve that order | |
| 15:43:40 | jaypipes | zzzeek: right, and that order is wrong :) | |
| 15:43:46 | zzzeek | jaypipes: only if you have "<some literal python thing> <operator> col" might it switch things because the __eq__ operator is on the right | |
| 15:43:48 | jaypipes | zzzeek: thus me saying it was my mistake ;) | |
| 15:44:19 | gibi | jaypipes: I'm getting the same stacktrace after reodering the condition. git diff is on the top of the logs: http://paste.openstack.org/show/615749/ | |
| 15:44:20 | zzzeek | jaypipes: OK so, if you have "col <operator> othercol" that left/right is maintained | |
| 15:44:43 | zzzeek | jaypipes: also, it....shouldnt matter? unless you're trying to hit an index on oracle | |
| 15:45:22 | zzzeek | jaypipes: == operator is commutative... | |
| 15:45:50 | jaypipes | gibi: :( ok, back to the drawing board. I really don't know why a KeyError is being raised there. the root provider ID should be in the summaries dict since _get_usages_by_rp_and_rc() should be returning a record for that rp | |
| 15:46:12 | gibi | jaypipes: is there any log I can turn on to help? | |
| 15:46:34 | gibi | jaypipes: or if you provide a patch with extra LOGs then I can apply that | |
| 15:46:35 | jaypipes | zzzeek: right, but I was thinking maybe SA saw the == operator column order and maybe made the expression b LEFT JOIN a instead of a LEFT JOIN b. | |
| 15:46:41 | jaypipes | zzzeek: apparently not, though | |
| 15:46:55 | jaypipes | gibi: I'll do the latter | |
| 15:47:25 | gibi | jaypipes: OK. I'm still around for an hour or so then I can continue tomorrow | |
| 15:47:32 | gibi | jaypipes: thank again for helping | |
| 15:47:38 | jaypipes | gibi: I'll have a patch up in 5 mins. | |
| 15:47:44 | zzzeek | jaypipes: ah. no way :) | |
| 15:47:55 | gibi | jaypipes: I will test that! | |
| 15:48:26 | openstackgerrit | Matt Riedemann proposed openstack/nova master: DNM: test new style cinder attach with upgrades https://review.openstack.org/484860 | |
| 15:48:27 | mriedem | mgiles: thanks, testing it here ^ | |
| 15:49:41 | mgiles | miredem: great! thanks | |
| 15:49:56 | mgiles | mriedem ^ | |
| 15:51:03 | melwitt | mriedem: I think we've glossed over that in the review and the thought was, we can't count those atomically together with instances because they're in the API DB ... | |
| 15:51:26 | openstackgerrit | Jay Pipes proposed openstack/nova master: TESTING - DO NOT MERGE https://review.openstack.org/484862 | |
| 15:51:30 | jaypipes | gibi: ^^ | |
| 15:53:30 | jaypipes | dtantsur: looking at that devstack custom RCs patch now... | |
| 15:53:41 | mriedem | melwitt: do we count instances atomically across multiple cells? | |
| 15:53:42 | dtantsur | thanks! | |
| 15:53:52 | gibi | jaypipes: looking... | |
| 15:53:55 | mriedem | melwitt: don't we have a separate session for each cell db? | |
| 15:53:59 | melwitt | mriedem: no, each cell is atomic | |
| 15:54:17 | dansmith | mriedem: we can't count atomically across cells | |
| 15:55:03 | mriedem | right, | |
| 15:55:14 | mriedem | so my point is, saying we can't do it atomically b/c of the api db is kind of a cop out | |
| 15:55:25 | mriedem | since we can't do it atomically for multiple cells either | |
| 15:55:29 | dansmith | well, | |
| 15:55:34 | dansmith | but the cells don't overlap with each other | |
| 15:55:40 | mriedem | true, yes | |
| 15:55:41 | dansmith | the api db and the cell dbs do overlap | |
| 15:55:44 | mriedem | the race window is a problem | |
| 15:55:47 | mriedem | between build requests and instances | |
| 15:56:12 | mriedem | again, i don't think i'm advocating trying to account for build requests in flight at the same time as counting the instances | |
| 15:56:21 | mriedem | i just want to make sure we thought about it and are ok with it | |
| 15:57:54 | jaypipes | dtantsur: answered. | |
| 15:58:09 | melwitt | mriedem: yeah, I mean, I was thinking ideally we should count them because they are parts of instances | |
| 15:58:46 | dtantsur | jaypipes: thanks! I wonder if we should enroll nodes after nova-compute is started then | |
| 15:58:58 | dtantsur | jaypipes: to better emulate how things work in actual production | |
| 15:59:00 | dtantsur | wdyt? | |
| 15:59:03 | mriedem | the "for x in 5; nova boot --min-count 20...." worries me | |
| 15:59:39 | jaypipes | dtantsur: well, they automatically get enrolled when the nova-compute node starts, but it doesn't look like that has run by the time the test tries to boot an instance. | |
| 16:00:04 | dtantsur | this is suspicious.. okay, thanks for the hints. I'll take a deeper look tomorrow | |
| 16:00:17 | jaypipes | dtantsur: looks to be just a simple ordering issue to me. | |
| 16:00:37 | jaypipes | dtantsur: I will look further into it as well and leave comments on the patch. | |
| 16:00:45 | dtantsur | thanks :) | |
| 16:14:55 | melwitt | mriedem: yeah. the bad thing about counting both is it could get too much usage if you say, count build requests and then count instances, some of those instances could have been build requests a split second ago, and then you get too much usage after you add them together | |
| 16:17:16 | gibi | jaypipes: here is the log http://paste.openstack.org/show/615753/ | |
| 16:20:21 | jaypipes | gibi: excellent, that helps a lot, thank you! | |
| 16:20:40 | jaypipes | gibi: the usage information isn't being returned for rp 1 for some reason. | |
| 16:21:37 | gibi | jaypipes: rp 1 is the compute provider? | |
| 16:21:48 | jaypipes | gibi: yeah | |
| 16:22:11 | melwitt | mriedem, dansmith: apparently sqlalchemy supports two-phase commit. do you think that's something we could use to get atomic across databases? http://docs.sqlalchemy.org/en/latest/orm/session_transaction.html#enabling-two-phase-commit | |
| 16:22:41 | jaypipes | melwitt: I wouldn't go that route. It's a pain in the ass, frankly. | |