| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-07-18 | |||
| 15:12:17 | mriedem | *compute | |
| 15:12:19 | mriedem | not the scheduler | |
| 15:12:21 | mriedem | jaypipes: ^ | |
| 15:12:52 | keerthi | Thanks mriedem. i will look in to this | |
| 15:19:23 | mriedem | jaypipes: not-tags-any is tested in https://review.openstack.org/#/c/469800/ | |
| 15:19:35 | mriedem | https://review.openstack.org/#/c/469800/35/nova/tests/functional/wsgi/test_servers.py@237 | |
| 15:19:49 | mriedem | the 4 filters are tested in the same functional test, there just need to be more wrinkles it sounds like | |
| 15:20:19 | mriedem | and we can't do any of this in sql | |
| 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 :) | |