Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-10-15
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 efried, hah, see where else that error popped up: http://logs.openstack.org/09/666409/8/check/tempest-full-py3/38bf84e/job-output.txt#_2019-07-14_17_19_10_677555
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 artom: This is interesting, the failure on that skip patch http://logs.openstack.org/48/670848/1/check/neutron-tempest-dvr/ed2b81c/testr_results.html.gz
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 mriedem when we hit this
19:49:21 efried yeah, there's basically nothing in that window.
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
19:56:30 mriedem i'm a bit troubled by the various places that should use force=True in ^
19:56:50 mriedem meaning, wtf did we change the behavior of delete_allocation_for_instance in the first place? it feels like we did because we could.
20:00:25 melwitt efried, gibi, mriedem: can confirm I ran into the bug but didn't file one (and I saw in the backscroll that artom filed one)
20:00:59 efried mriedem: agreed, given the change we actually ended up with, we would have been better off not doing it.
20:03:40 mriedem i've annoted that places i'm using force=True now to try and justify the reasoning
20:03:45 mriedem *the places
20:03:58 mriedem maybe gibi can say "no we shouldn't force b/c resource requests" or something, idk
20:04:04 efried *for spawn, we should always be using NULL, and if we get a conflict, it means we're racing with some other operation (delete, resize, etc) and should abort the spawn
20:04:04 efried mriedem: if we're going to use generation-based allocation management at all, we should really be doing the GET early in the flow (except for spawn*) so that the race window actually means what it should.
20:04:05 mriedem *port resource requests and nested allocations
20:04:26 efried but the existing use of generations is worse than useless.
20:04:50 efried (I should have objected harder instead of +2ing that patch on the promise of "we'll improve it later")
20:08:35 mriedem yeah idk, i don't remember being very involved in this, in irc, or the meetings. i didn't comment in the ML thread and i didn't get into the "why"s in the patch, just reviewed it, likely to keep the series moving and trust gibi and everyone else's decisions on this (since you, chris, gibi and jay were all involved)
20:09:04 mriedem going back to the feeling of "we did it because we could"
20:11:35 efried we did it because we *should*, but we should do it right, and were planning on doing so eventually.
20:12:50 mriedem that makes more sense in the other patches in that series which dealt with PUTing allocations with updates rather than removing them when deleting a server
20:24:06 openstackgerrit Dustin Cowles proposed openstack/nova master: Provider Config File: YAML file loading and schema validation https://review.opendev.org/673341
20:24:07 openstackgerrit Dustin Cowles proposed openstack/nova master: Provider Config File: Merge provider configs to provider tree https://review.opendev.org/676522
20:24:07 openstackgerrit Dustin Cowles proposed openstack/nova master: Provider Config File: Function to further validate and retrieve configs https://review.opendev.org/676029
20:24:13 mriedem dansmith: a few things on https://review.opendev.org/#/c/687140/
20:24:17 mriedem did you intend to drop the release note?
20:25:17 dansmith mriedem: ah thanks. No, I tried to get all smart with the renumber on the microversion which caused me to lose several things.. it was a real timesaver
20:26:36 KeithMnemonic melwitt, mriedem, that patch from hemna finally verified. Thanks for all of you help. Reviews when someone gets time are appreciated in advance https://review.opendev.org/#/c/683008/
20:27:33 melwitt thanks for the heads up
20:30:04 efried what's the ironic ring thing called?
20:30:33 efried rebalance puts your node in another.... "X"?
20:31:11 efried yeah, it's a "ring", I'm not too crazy.
20:31:20 openstackgerrit Dan Smith proposed openstack/nova master: Add image caching API for aggregates https://review.opendev.org/687140
20:31:22 dansmith mriedem: ^
20:31:32 dansmith I gotta step away for a bit..got a raging headache
20:31:54 dansmith and no matter what my wife says, it is NOT because I emptied a whole can of Brakleen on my valve cover last night in the closed-up garage
20:32:56 mriedem some are saying fumes are good for the brain
20:33:01 mriedem efried: yeah hashring
20:33:13 efried thx
20:33:31 KeithMnemonic files accessibility /var/lib/nova/instances/... nova/virt/libvirt/driver.py:8893 " my guess is maybe a permission or something but was wondering if anyone ever ran into something like this
20:33:31 KeithMnemonic looking for some tips/suggestions on another odd issue I am investigating. This is Rocky with ceph backed instances. i.e instance boots from ceph directly. per https://docs.ceph.com/docs/master/rbd/rbd-openstack/ . When doing an evacuate from a compute that is powered off it fails with "Invalid state of instance files on shared storage" it looks like somewhere here it is failing on this "Checking instance
20:34:17 KeithMnemonic the instance ran fine of the source compute. my next step is to try and see if the same happens with a migrate
20:34:32 KeithMnemonic and other instances in the same ceph pool are running on the target

Earlier   Later