| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-10-15 | |||
| 14:56:23 | dansmith | yeah | |
| 14:56:24 | mriedem | logstash only goes back 10 days | |
| 14:56:30 | mriedem | i reported that bug 78 days ago | |
| 14:56:30 | openstack | bug 78 in Baz (deprecated) "When asking you to sign something; baz should tell you what" [Medium,Won't fix] https://launchpad.net/bugs/78 | |
| 14:56:49 | mriedem | heh bug 666 | |
| 14:56:49 | openstack | bug 666 in Launchpad itself "can't file a bug on Ubuntu" [Medium,Invalid] https://launchpad.net/bugs/666 | |
| 14:56:49 | dansmith | yeah | |
| 15:05:08 | mriedem | it looks like the dict keys are at least sorted so i can do: | |
| 15:05:11 | mriedem | http://logstash.openstack.org/#dashboard/file/logstash.json?query=message%3A%5C%22actual%20%20%20%20%3D%20%7B'attachments'%3A%20%5B%5D%2C%5C%22%20AND%20tags%3A%5C%22console%5C%22&from=7d | |
| 15:06:03 | dansmith | hmm, that only shows two hits/ | |
| 15:06:14 | dansmith | maybe those are the two I looked at | |
| 15:07:57 | stephenfin | mriedem: https://review.opendev.org/688731 | |
| 15:08:31 | mriedem | dansmith: yeah it's on 627891 | |
| 15:08:36 | mriedem | it's just rare | |
| 15:08:37 | dansmith | yeah | |
| 15:10:21 | mriedem | stephenfin: ack i'll make my devstack change depend on that | |
| 15:10:59 | mriedem | oh i lost all of my beautiful meaningless +1s......what a world what a world | |
| 15:11:15 | stephenfin | Cool. Looks like a fix in pip _is_ underway but it could be months/years before that lands https://pradyunsg.me/blog/2019/06/23/oss-update-1/ | |
| 15:12:12 | mriedem | cool like what? write a dep resolver for pip? | |
| 15:12:40 | mriedem | fwiw i think lifeless shed many years from his life working on a dep resolver for pip.... | |
| 15:13:01 | mriedem | before or during just saying f it and doing the constraints stuff in openstack | |
| 15:13:48 | dansmith | ah the good old days of openstack | |
| 15:14:26 | sean-k-mooney | mdbooth: is this https://review.opendev.org/#/c/663382/ the patch form stephenfin you planned to add functional test too? if not i might add one in a few days | |
| 15:20:38 | sean-k-mooney | stephenfin: by the way we still need to land this https://review.opendev.org/#/c/675776/ and backport it to train | |
| 15:21:42 | sean-k-mooney | although it looks like it has lots of other change mixed in | |
| 15:24:00 | sean-k-mooney | so ya we shoudl merge v5 https://review.opendev.org/#/c/675776/5 not v6 ill -1 the patch and ask for the patch to be fixed. | |
| 15:49:56 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Remove compute compat checks for aborting queued live migrations https://review.opendev.org/688409 | |
| 17:54:20 | mriedem | i'm finding that writing a functional test for bug 1836754 is so hacky to order the events that it's probably not worth it and we should just throw a retry decorator on delete_allocation_for_instance when we hit a consumer generation conflict | |
| 17:54:20 | openstack | bug 1836754 in OpenStack Compute (nova) "Conflict when deleting allocations for an instance that hasn't finished building" [Low,Confirmed] https://launchpad.net/bugs/1836754 | |
| 17:55:27 | mriedem | efried: also conductor deletes the allocations created by the scheduler if the server is gone by the time we get back from scheduling https://github.com/openstack/nova/blob/149327a3abb12418cdf65316e7c1d4924767bfdf/nova/conductor/manager.py#L1402 | |
| 17:55:30 | mriedem | so that's covered | |
| 17:59:54 | artom | mriedem, we never hit it that often, did we? | |
| 17:59:56 | artom | And only in CI | |
| 18:00:00 | artom | So I think no func test is fine | |
| 18:00:17 | artom | We can use the e-r query to see if merging the commit makes the hits disappear | |
| 18:01:25 | openstackgerrit | Dan Smith proposed openstack/python-novaclient master: Add aggregate-cache-images command and client routines https://review.opendev.org/687141 | |
| 18:15:56 | mriedem | artom: yeah it's very rare | |
| 18:34:35 | openstackgerrit | Merged openstack/nova master: Filter migrations by user_id/project_id https://review.opendev.org/674243 | |
| 18:55:29 | efried | mriedem: are you working on the forced alloc delete rn? | |
| 19:01:19 | mriedem | yeah, need to write a test | |
| 19:01:56 | efried | mriedem: imo we should use DELETE when we're serious rather than using a retry. | |
| 19:02:08 | efried | and indicate seriousness explicitly from the caller | |
| 19:02:41 | mriedem | why was it using PUT with an empty allocations dict to begin with then? | |
| 19:03:41 | efried | mriedem: exactly so we can capture 409s. | |
| 19:03:50 | efried | point being, we only want to do that some of the time. | |
| 19:04:04 | efried | IIRC we used to use DELETE until I made a stink about it. | |
| 19:05:09 | efried | mriedem: I was about to dig in; if you want to write a test, I can take a swing at the patch. | |
| 19:05:18 | efried | unless you're doing both already | |
| 19:05:27 | mriedem | so you must have gotten stinky here https://review.opendev.org/#/c/591597/ | |
| 19:06:52 | efried | yeah, that was a step on the path. I think I remember complaining about the fact that that wasn't really helping us, because of the tininess of the window we were leaving. | |
| 19:07:02 | efried | though clearly we're hitting that window, hence the bug. | |
| 19:08:21 | efried | mriedem: is this something you think we should backport? Because I'd like to do a series where we split the existing one into forced-or-normal, also getting rid of @safe_connect. But can do a "tactical" version for backport if that's a consideration. | |
| 19:09:00 | efried | artom: did you see this bug in real life or just in CI? | |
| 19:09:04 | mriedem | reading some of the comments on that patch it sounds like you were pushing for keeping DELETE rather than going with PUT and consumer generation handling: | |
| 19:09:05 | mriedem | "This patch does not make a conflict visible unless it happens to occur in the teeny window *within* the method itself. IMO introducing this code could make one *think* we're doing so, which is bad. Because the vast, vast majority of cases where such conflicts occur (allocation gets mucked with between when the instance is created and when the deletion process begins) will go completely unnoticed." | |
| 19:09:24 | efried | wise words. Who said them? | |
| 19:09:32 | mriedem | you | |
| 19:09:41 | mriedem | but you said above you made a stink about using DELETE | |
| 19:10:08 | efried | Yes, I pushed for s/DELETE/PUT{...generation}/ in, I think, Dublin. | |
| 19:10:21 | mriedem | because you wanted the caller to decide what to do, | |
| 19:10:25 | efried | because the way it *should* be done is that the GET happens somewhere earlier in the process | |
| 19:10:36 | mriedem | so in this case the API would catch the conflict and retry rather than blindly adding a @retries decorator to the method | |
| 19:10:56 | efried | right, that would be one way to approach it. | |
| 19:11:40 | efried | the other way would be, when we don't care about conflicts and really just want to do the delete, we use DELETE (again, via a new reportclient method, or a flag to the existing, which amounts to the same thing) and never have to do multiple calls. | |
| 19:12:34 | efried | I would advocate for the latter, because this isn't the only place we're going to want to force from, and it's easier to force=True than to write the same retry logic from multiple places, not to mention the fewer calls. | |
| 19:12:37 | mriedem | going back to your other question i would not mangle up removing @safe_connect and all that in the same patch that resolves the bug | |
| 19:13:06 | mriedem | speaking for artom (i'll take the liberty here) i think it's seen in CI only but that doesn't mean it's not in the real world | |
| 19:13:12 | efried | yeah, I was going to do a bottom patch with @safe_connect backward compat and then kill it after the bugfix. | |
| 19:13:35 | mriedem | in fact, if you see this in real life i'm not sure a normal user can even retry the delete if we changed the task_state and didn't reset it on error | |
| 19:14:00 | efried | right, you have to heal allocations via nova-manage or whatever. | |
| 19:14:09 | efried | I think | |
| 19:14:10 | mriedem | no, | |
| 19:14:26 | mriedem | i mean you as a user should be able to just retry the delete of the server since you got a 409 from the compute API (not a 500) | |
| 19:14:45 | efried | right, you *should* be able to, but you can't, you have to use nova-manage today. | |
| 19:15:08 | efried | I gotta chauffeur a kid, back in 20. | |
| 19:15:20 | mriedem | i think we're talking past each other but ok | |
| 19:33:19 | efried | mriedem: I get what you're saying now -- in this particular path we don't actually leak the allocation, we just fail to finish deleting the instance. | |
| 19:33:30 | mriedem | correct | |
| 19:34:25 | mriedem | i'm working something up and will post it so you can see before working on tests | |
| 19:35:35 | efried | okay. be warned I'm likely to be -1 on a solution involving retrying. | |
| 19:36:11 | mriedem | i'm not retrying | |
| 19:36:19 | efried | cool | |
| 19:44:24 | artom | efried, mriedem, I... don't remember? | |
| 19:47:09 | artom | I went IRC log diving | |
| 19:47:10 | artom | Jul 16 10:07:27 |
|
| 19:47:19 | artom | Jul 16 10:08:33 <artom> efried, there aren't that many hits, but yeah, our theory from last night is pretty much confirmed | |
| 19:47:57 | efried | okay, well anyway, mriedem is working on a fix. | |
| 19:48:18 | artom | Jul 15 16:05:11 |
|
| 19:48:37 | artom | So, I think we first hit this in CI back when we were testing that hybrid plug revert resize thing | |
| 19:48:40 | artom | So yeah, CI only | |
| 19:48:46 | efried | ack | |
| 19:48:49 | artom | That we know of, anyways | |
| 19:49:02 | mriedem | it's an extremely tight window between GETing the allocations and PUTing them back with allocations={} | |
| 19:49:04 | efried | I would think it would be fairly tough to hit otherwise | |
| 19:49:19 | mriedem | tempest creates a server and then immediately deletes it | |
| 19:49:21 | efried | yeah, there's basically nothing in that window. | |
| 19:49:21 | mriedem | when we hit this | |
| 19:51:06 | artom | Right, it's coming back | |
| 19:51:10 | artom | It's a specific tempest tes | |
| 19:56:21 | openstackgerrit | Matt Riedemann proposed openstack/nova master: WIP: Add force kwarg to delete_allocation_for_instance https://review.opendev.org/688802 | |