| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-07-31 | |||
| 15:17:49 | cdent | gibi_: so the problem is somewhere near here: https://review.openstack.org/#/c/488510/4/nova/compute/resource_tracker.py@1083 | |
| 15:18:16 | cdent | sorry, not there | |
| 15:18:36 | cdent | 476 | |
| 15:19:33 | gibi_ | but that is the piece of code that generates our PUT | |
| 15:19:40 | gibi_ | to me | |
| 15:19:40 | gibi_ | and that PUT seems correct | |
| 15:19:45 | cdent | true | |
| 15:26:47 | gibi_ | I added code to https://review.openstack.org/#/c/488510/4/nova/scheduler/client/report.py@1079 to read back the allocations the code just PUT-ed | |
| 15:26:54 | gibi_ | http://paste.openstack.org/show/617031/ | |
| 15:27:15 | gibi_ | and the GET returns both allocations after the PUT | |
| 15:27:31 | gibi_ | so the problem is in the placement I think | |
| 15:28:04 | cdent | wow | |
| 15:28:15 | cdent | that will be an exciting bug if so | |
| 15:28:46 | dansmith | I'm missing the obvious thing | |
| 15:29:00 | cdent | gibi_: yeah | |
| 15:29:03 | dansmith | oh, we can't read back the allocations we just wrote in a particular place? | |
| 15:29:17 | cdent | it only deletes where rp uuid and consume uuid == | |
| 15:29:21 | cdent | not just consumer uuid | |
| 15:29:47 | bauzas | interesting | |
| 15:30:16 | cdent | https://github.com/openstack/nova/blob/master/nova/objects/resource_provider.py#L1509-L1520 | |
| 15:30:43 | cdent | gibi_: see what happens if you get rid of the first == in the and_ | |
| 15:30:52 | gibi_ | checking | |
| 15:31:01 | bauzas | yeah that will work | |
| 15:31:09 | bauzas | obviously | |
| 15:31:17 | cdent | dansmith: we had in our brains that we were removing all allocations for the consumer | |
| 15:31:24 | cdent | but the code is only removing some of them | |
| 15:32:33 | dansmith | meaning, the api is such that all should be removed, but some bug in the db side of placement prevents that from happening? | |
| 15:32:50 | cdent | yeah | |
| 15:32:59 | dansmith | that seems ungood | |
| 15:33:15 | cdent | it looks like the original goal was to _only_ remove exact matches (not sure why) | |
| 15:33:22 | cdent | (the comment says as much) | |
| 15:33:27 | dansmith | hmm | |
| 15:33:41 | dansmith | jaypipes seemed to think that should be fully atomic, so that seems weird | |
| 15:33:59 | gibi_ | I can confirm that removing the rp == from the db code makes the placement API behave as expected in this particular case http://paste.openstack.org/show/617034/ | |
| 15:34:16 | cdent | and we had three authors on that particular change, so is probably going to be hard to remember the whys and wherefores | |
| 15:34:18 | cdent | gibi_: nice | |
| 15:34:56 | cdent | dansmith: since we've been assuming all this time that the behavior is one thing and not the other, I think we should just change it | |
| 15:35:12 | cdent | also I don't think there is much risk, because we haven't been doing any dual provider allocations up til now | |
| 15:35:39 | cdent | edleafe: you listening ^ ? | |
| 15:35:42 | cdent | brb | |
| 15:36:19 | gibi_ | I'll let you guys to report a bug and propose a fix as my workday is over soon. | |
| 15:36:46 | cdent | gibi_: right on, thanks very much for all your digging | |
| 15:37:16 | jaypipes | rock on, thanks gibi | |
| 15:37:25 | dansmith | cdent: right, and I think that's the thing the api espouses anyway | |
| 15:38:10 | edleafe | cdent: sorry, distracted by meeting. Trying to follow, though | |
| 15:38:29 | jaypipes | dansmith, cdent: so shall I remove that == rp_uuid line in a separate patch or in the same confirm_resize() patch? | |
| 15:38:33 | cdent | edleafe: no worries, just wanted your memory if anything | |
| 15:38:39 | dansmith | jaypipes: definitely separate | |
| 15:38:49 | dansmith | jaypipes: we probably need to backport that right? | |
| 15:38:51 | jaypipes | dansmith: ahead of confirm_resize() yeah? | |
| 15:38:55 | jaypipes | dansmith: ya | |
| 15:38:55 | dansmith | yeah | |
| 15:39:00 | jaypipes | ok, I'm on it. | |
| 15:39:35 | bauzas | jaypipes: if we remove the == rp, wouldn't that be a problem for racy calls ? | |
| 15:39:55 | jaypipes | bauzas: no | |
| 15:39:57 | bauzas | jaypipes: because previously we were just supposing the generation bit to be updated | |
| 15:40:06 | bauzas | given that generation is per RP | |
| 15:40:18 | jaypipes | bauzas: the generation is on the RP. this code is removing allocation records. | |
| 15:40:43 | jaypipes | bauzas: this is leftover code that was assuming a single RP for an allocation | |
| 15:41:14 | jaypipes | 1509gg | |
| 15:41:17 | jaypipes | ffs | |
| 15:43:02 | cdent | the bug: https://bugs.launchpad.net/nova/+bug/1707669 | |
| 15:43:04 | openstack | Launchpad bug 1707669 in OpenStack Compute (nova) "[placement] put allocations does not do a full overwrite of existing allocations" [High,Triaged] | |
| 15:44:34 | jaypipes | cdent: danke | |
| 15:50:59 | sdague | 2 easy bugs to close - https://review.openstack.org/#/c/486642/ | |
| 15:51:07 | sdague | https://review.openstack.org/#/c/488530/ | |
| 15:51:39 | sdague | plus mriedem here is my nova-manage patch to do list_cells with urls by default - https://review.openstack.org/#/c/487860/ | |
| 15:58:00 | mriedem | sdague: wasn't the limits stuff added here https://review.openstack.org/#/c/486642/ for a security bug? | |
| 15:58:11 | mriedem | or was the security issue just that it was completely unbounded to begin with? | |
| 15:58:11 | edleafe | cdent: ok, catching up. Looking through the long paste, I don't see where we clean up the allocations for the source host | |
| 15:58:22 | sdague | mriedem: yes, the security issue was completely unbounded | |
| 15:58:28 | sdague | it was originally set to 2 seconds | |
| 15:58:41 | edleafe | We PUT the allocations for the target, but don't seem to remove the source allocs | |
| 15:58:43 | sdague | then after that actually failed in the gate some times, it was bounced to 8 | |
| 15:58:45 | cdent | edleafe: it was the third of three PUTs | |
| 15:58:54 | sdague | but there are real world reports that times out in legit cases | |
| 15:59:28 | sdague | it still gives us a backstop though so you can't make a malicious image that eats all the cpu forever | |
| 15:59:46 | jaypipes | sdague: done | |
| 16:00:20 | sdague | jaypipes: thank you | |
| 16:01:07 | edleafe | cdent: ok, I see that now. So is that PUT not doing a true PUT? | |
| 16:01:25 | edleafe | cdent: or just for the consumer/RP combo? | |
| 16:01:50 | cdent | it's not doing a true put | |
| 16:01:59 | openstack | Launchpad bug 1707669 in OpenStack Compute (nova) "[placement] put allocations does not do a full overwrite of existing allocations" [High,Triaged] | |
| 16:01:59 | cdent | https://bugs.launchpad.net/nova/+bug/1707669 | |
| 16:02:09 | cdent | server-side is borked | |
| 16:02:43 | edleafe | cdent: what I'm asking is if it's just doing a true PUT for RP/consumer | |
| 16:02:54 | edleafe | cdent: and not for consumer | |
| 16:02:58 | edleafe | *just | |
| 16:03:29 | cdent | edleafe: sorry, I'm not parsing you | |
| 16:03:37 | mriedem | sdague: comments on more testing in https://review.openstack.org/#/c/487860/3/nova/tests/unit/test_nova_manage.py | |
| 16:03:46 | mriedem | the db url parsing has broken a few times | |
| 16:03:52 | cdent | the bodies of the PUTs, all three, are correct, according to our expectations of how the api behaves. the api doesn't not behave as we expect | |
| 16:03:58 | mriedem | and we have an outstanding bug for db urls with TLS info in them | |
| 16:05:44 | sdague | mriedem: so, using urlparse we're never going to hit that, but I'm fine putting some more in there | |
| 16:06:21 | mriedem | we were originally using urlparse | |
| 16:06:24 | mriedem | which was causing problems | |
| 16:07:00 | edleafe | cdent: yes I see that. Is the disconnect due to the fact that we expect when we PUT allocations for a consumer, it first removes any existing allocs for that consumer? | |
| 16:07:33 | ralonsoh_ | stephenfin: hi, can you take a look at my question in https://review.openstack.org/#/c/427145/? PS7 | |
| 16:07:53 | cdent | edleafe: that's the bug. we expect that, but it does not do that. what it was doing is removes only those allocations where rp uuid and consumer uuid match | |
| 16:07:57 | cdent | which is not enough | |