| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2021-08-27 | |||
| 14:02:28 | dansmith | the bug discusses a retry, which is what I expect to do when I get a 409 from placement | |
| 14:02:35 | dansmith | but the proposed solution is to force? | |
| 14:03:41 | sean-k-mooney | perhaps its better for gibi to surmiarse or look at his patch so i dont butcher the answer :) | |
| 14:03:43 | gibi | retry here means read the new allocatin with the new generation and null it out in the response | |
| 14:04:03 | gibi | so for me that logically the same as ignore the current allocaiton and force the delete | |
| 14:04:07 | dansmith | null it out in the request? | |
| 14:04:24 | gibi | dansmith: yepp the current code deletes an allocation with an empty PUT | |
| 14:04:36 | gibi | "empty" as there is the generation | |
| 14:05:09 | dansmith | okay, the current proposed patch does a delete with no generation right off the bat if force=true, never tries "nicely" once AFAICT | |
| 14:06:10 | gibi | dansmith: correct | |
| 14:06:17 | sean-k-mooney | so i think doing a http delete is the corrct thing if your ar enot using soft delete and the user request the instnace to be deleted | |
| 14:06:31 | dansmith | okay, confused about the "retry" terminology then :) | |
| 14:06:32 | sean-k-mooney | which is one of the case we will pass force=true | |
| 14:07:33 | gibi | dansmith: we can only do a meaningful retry if our original intention depends on the current state of the resource we are updated | |
| 14:07:50 | gibi | in many delete case we don't care about the current state we just want to delete | |
| 14:08:35 | dansmith | gibi: yeah, technically now we're just nulling out the allocations and PUTing the thing back as-is, and generation + retry would be the right thing to do in that case, | |
| 14:08:48 | dansmith | but I understand, if we're deleting the thing then maybe delete is the right answer | |
| 14:11:39 | dansmith | the thing I was saying above was that it seemed like you (all) were saying try the put, and if the put fails, delete to override | |
| 14:11:44 | dansmith | but I see that's not what the patch does | |
| 14:12:15 | sean-k-mooney | right no the patch adds a force flag and we are selectivly setting it depening on which behavior we want | |
| 14:12:38 | sean-k-mooney | force=true to jsut delete regardless of state and force=false if we want an operturnity to reconider | |
| 14:12:50 | gibi | dansmith: if we know we force on failure then what the point to try first nicely? | |
| 14:13:21 | dansmith | gibi: I'm not arguing that we should, I was just trying to understand the proposed new behavior | |
| 14:13:29 | gibi | ack | |
| 14:14:46 | dansmith | I guess I'm wondering why we did the PUT with empty allocations in the first place instead of delete | |
| 14:15:02 | sean-k-mooney | we used to do delete | |
| 14:15:09 | sean-k-mooney | we started doing put when we added generations | |
| 14:15:16 | dansmith | well, right :) | |
| 14:15:39 | sean-k-mooney | but that is why we also have the straight revert patches as an option to consider | |
| 14:15:55 | sean-k-mooney | melwitt: myslef and possibel other were wondering the same thing | |
| 14:16:32 | sean-k-mooney | the force flag patch is kind of a middle ground | |
| 14:16:44 | dansmith | it also seems super odd to have force=True as the default | |
| 14:17:03 | dansmith | I'm guessing that's just for expediency, but.. | |
| 14:17:23 | sean-k-mooney | i think in most case we really do just want to delete not put | |
| 14:17:40 | sean-k-mooney | althogh callign it force is perhaps questioable in that case | |
| 14:17:43 | dansmith | right | |
| 14:18:04 | dansmith | inverting the logic to "extra_checks=False" or something if you opt into it might be good, | |
| 14:18:07 | dansmith | but again, | |
| 14:18:21 | dansmith | we're opting out of the force for things like move and soft delete, | |
| 14:18:28 | dansmith | which I kinda expect to exist more places than actual instance delete | |
| 14:18:56 | dansmith | anyway, I didn't mean to come in here and throw a wrench in things, I just wanted to understand what as going on | |
| 14:19:18 | dansmith | I saw 409s in failed runs yesterday, but I assumed those were just retries and didn't give it much thought | |
| 14:19:53 | opendevreview | sean mooney proposed openstack/nova master: Provide the mdev class for every PCI device https://review.opendev.org/c/openstack/nova/+/802918 | |
| 14:19:54 | opendevreview | sean mooney proposed openstack/nova master: Expose the mdev class https://review.opendev.org/c/openstack/nova/+/801743 | |
| 14:19:54 | opendevreview | sean mooney proposed openstack/nova master: Provide and use other RCs for mdevs if needed https://review.opendev.org/c/openstack/nova/+/803233 | |
| 14:19:55 | opendevreview | sean mooney proposed openstack/nova master: [WIP] update vgpu docs to account for generic mdev support https://review.opendev.org/c/openstack/nova/+/806412 | |
| 14:21:29 | artom | Oooohhhh | |
| 14:21:53 | sean-k-mooney | this will be good. why so | |
| 14:21:58 | artom | If something's changed between PSN and PSN+1 that's not actually part of the change itself, it'll show the diff in yellow, not green | |
| 14:22:08 | sean-k-mooney | ah yes | |
| 14:22:10 | sean-k-mooney | it does | |
| 14:22:31 | dansmith | and sometimes blue right? | |
| 14:22:34 | sean-k-mooney | the colours depend on light vs dark mode | |
| 14:22:37 | dansmith | I haven't figured out the pattern | |
| 14:22:39 | dansmith | ah | |
| 14:22:45 | artom | I've only seen yellow so far | |
| 14:23:10 | dansmith | yeah, probably the dark switch, I had one machine in dark the other day | |
| 14:23:19 | artom | Reading the scrollback, what *was* the logic behind PUTing empty allocations instead of outright DELETE? | |
| 14:23:27 | artom | We had consumer generations, so... just because we could? | |
| 14:23:34 | artom | Like, we're deleting, why do races matter? | |
| 14:23:41 | dansmith | artom: well, I'm kinda wondering if it's related to races during move and such | |
| 14:23:53 | dansmith | artom: we delete allocations for all kinds of reasons other than deleting an instance | |
| 14:24:31 | artom | So the idea is - before we delete, make sure we have the latest version so that we can then POST it to a new RP? | |
| 14:24:50 | dansmith | which is why it seems to me like maybe we should only force=true on instance delete, and default to not everywhere else, instead of what is proposed which is always force | |
| 14:25:32 | sean-k-mooney | so just invert the default for force | |
| 14:25:39 | dansmith | we're not really checking the allocations though, so, probably not worth it | |
| 14:26:04 | dansmith | anyway, it just feels like we're removing a fence because it's currently getting in the way :) | |
| 14:26:23 | artom | Yeah - hence the question - why did we put the fence there in the first place? | |
| 14:27:02 | dansmith | well, generations went in to avoid us clobbering allocations all the time | |
| 14:27:10 | dansmith | during things like moves | |
| 14:28:36 | sean-k-mooney | artom: this is the orginial commit that added the put behaivor https://review.opendev.org/c/openstack/nova/+/591597 | |
| 14:28:40 | artom | So, I'm sure there's a legitimate reason for it, but I don't grok it. Even during a move it, why would there be 2 bits of code trying to update allocations concurrently? | |
| 14:28:47 | artom | *move operation | |
| 14:28:54 | sean-k-mooney | it has some motivation for it in the commit but not a fully explanation | |
| 14:30:01 | sean-k-mooney | the main motivation i got form that was to intneionally be able to put the instance into error on delete if it was in an inconsitent state | |
| 14:30:16 | sean-k-mooney | to presumable give the operator or user a change to investigate | |
| 14:30:39 | sean-k-mooney | *chance | |
| 14:30:48 | dansmith | originally before we were using migration allocations I think both sides fought over who was going to claim the resources and delete the other | |
| 14:30:49 | artom | sean-k-mooney, that's for instance deletion, dansmith is talking about moves needing generations | |
| 14:31:12 | dansmith | and of course same-host migrations made that hard | |
| 14:31:19 | artom | ... so we did a thing in placement because Nova was dumb? | |
| 14:31:36 | artom | Ah, resize to same host... | |
| 14:31:38 | sean-k-mooney | isnt that why placment was created :) | |
| 14:31:42 | dansmith | artom: no dude, placement was/is accessed by multiple services at once | |
| 14:31:45 | artom | Hah | |
| 14:32:39 | sean-k-mooney | but here this is a distibuted lock problem | |
| 14:32:50 | sean-k-mooney | in that we dont have one | |
| 14:32:58 | artom | dansmith, right, I know that. So was this something like Nova updating the VCPUs or whatever, while Neutron concurrently tries to update something about the port? | |
| 14:33:00 | dansmith | well, generation is the lock | |
| 14:33:07 | sean-k-mooney | so generations were added ^ yep | |
| 14:33:12 | dansmith | I'm trying to remember how this works, but is the generation on the consumer actually? | |
| 14:33:20 | dansmith | such that one instance moving between two RPs shares a generation? | |
| 14:33:40 | sean-k-mooney | i think the generation is on the allocation | |
| 14:34:02 | sean-k-mooney | i dont know if they share the same generation when we have both in a migration case | |
| 14:35:01 | dansmith | we don't when migration is holding the resources on the other, | |
| 14:35:26 | dansmith | but the generation is on the allocation for the consumer across both compute nodes (if you do it that way) | |
| 14:35:46 | dansmith | so even with both compute nodes being "separate" they both had to update the allocation for the instance, | |
| 14:35:57 | dansmith | to claim resources on incoming and drop the resources from the outgoing | |
| 14:36:26 | dansmith | but this delete is actually _deleting_ the allocation | |
| 14:36:48 | dansmith | so this is from after that concept of a single allocation against both providers | |