| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-07-31 | |||
| 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 | dansmith | yeah | |
| 15:38:55 | jaypipes | dansmith: ya | |
| 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 | 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:11 | mriedem | or was the security issue just that it was completely unbounded to begin with? | |
| 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 | cdent | https://bugs.launchpad.net/nova/+bug/1707669 | |
| 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: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 | |
| 16:09:25 | edleafe | cdent: ok, that's what I was asking above about "or just for the consumer/RP combo?" | |
| 16:09:33 | edleafe | I'm following now | |
| 16:09:39 | edleafe | Is this being worked on? | |
| 16:09:45 | edleafe | IOW, can I help? | |
| 16:09:47 | cdent | yes, jay's integrating it in his stack | |
| 16:09:59 | jaypipes | just running new tests now.. | |
| 16:10:00 | edleafe | ok cool | |
| 16:10:10 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Clean variable names and docs around neutron allocate_for_instance https://review.openstack.org/489267 | |
| 16:10:11 | cdent | edleafe: I think the main thing to do is try to break stuff | |
| 16:10:11 | mriedem | i hate the allocate_for_instance code ^ | |
| 16:11:27 | edleafe | cdent: roger that. | |
| 16:15:54 | mdbooth | gibi_: Just looking at https://review.openstack.org/#/c/487958/4/nova/tests/functional/test_servers.py | |
| 16:16:02 | mdbooth | gibi_: Any idea how close that might be to landing? | |
| 16:16:52 | mdbooth | gibi_: I need to add a test for https://review.openstack.org/#/c/462521/ and ServerMovingTests looks like an obvious place for it | |
| 16:17:38 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Clean variable names and docs around neutron allocate_for_instance https://review.openstack.org/489267 | |
| 16:20:42 | cdent | mdbooth: I think gibi_'s gone. We could potentially land that code soon if dansmith and jaypipes think it belongs alongside jay's stack, but it is primarily for testing how allocations are handled, not verifying reverts etc. Not sure if that makes it better or worse. | |
| 16:21:42 | jaypipes | cdent: five minutes. | |
| 16:22:03 | mdbooth | cdent: Well the setUp there creates an environment with 2 computes sufficient for running resize(), which is exactly what I need | |
| 16:22:50 | cdent | mdbooth: yeah, it apparently also fixes some issues with doing that | |
| 16:23:11 | mdbooth | cdent: I'm not surprised in the slightest there are dragons. | |
| 16:23:24 | mdbooth | Didn't want duplicate the effort slaying them. | |
| 16:24:31 | cdent | indeed | |
| 16:32:14 | openstackgerrit | Jay Pipes proposed openstack/nova master: placement: don't allocate on compute nodes https://review.openstack.org/488595 | |
| 16:32:15 | openstackgerrit | Jay Pipes proposed openstack/nova master: remove source provider allocs in confirm_resize() https://review.openstack.org/488510 | |
| 16:32:15 | openstackgerrit | Jay Pipes proposed openstack/nova master: placement: remove existing allocs when set allocs https://review.openstack.org/489273 | |
| 16:32:27 | jaypipes | dansmith, edleafe, cdent, gibi_: ok dokey ^ | |
| 16:33:16 | cdent | word | |
| 16:33:40 | jaypipes | the bird. | |