| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2021-08-27 | |||
| 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 | |
| 14:37:23 | sean-k-mooney | ah so we dont have 2 indepented allcoations | |
| 14:37:36 | dansmith | right | |
| 14:37:37 | sean-k-mooney | i tought we used the migration uuid to create an indepent allcoation | |
| 14:37:45 | dansmith | oh we do no, | |
| 14:37:46 | dansmith | *now* | |
| 14:37:58 | sean-k-mooney | ah ok | |
| 14:38:05 | dansmith | I'm saying before we did that, we were creating allocations for the instance against both computes | |
| 14:38:06 | sean-k-mooney | you were discibing your initall implemenation | |
| 14:38:10 | sean-k-mooney | before you refactored it | |
| 14:38:14 | dansmith | which required both to modify it during/after the migration | |
| 14:38:24 | dansmith | not mine, but the, yes | |
| 14:38:30 | dansmith | mine was the "use migration allocation" change | |
| 14:38:54 | sean-k-mooney | right ok that aligns with what i recall | |
| 14:39:20 | sean-k-mooney | so you think this was really only need before the move to seperate allocations? | |
| 14:39:33 | sean-k-mooney | or do you think its still useful now. the put instead of delete | |
| 14:40:35 | dansmith | I think the PUT came after we were already using two separate allocations, but not positive | |
| 14:40:37 | sean-k-mooney | for migrations that is | |
| 14:41:03 | sean-k-mooney | ok im not sure of the time line but that sound right | |
| 14:41:04 | dansmith | gibi wrote that initial patch you linked above to move *away* from delete specifically to get generation stuff, | |
| 14:41:10 | dansmith | and it was part of "nested allocation candidates" | |
| 14:41:20 | sean-k-mooney | the migration allcotion change was a long time ago relitivly speaking | |
| 14:41:34 | dansmith | right | |
| 14:41:50 | sean-k-mooney | oh it was for nested resouce providers | |
| 14:42:08 | sean-k-mooney | so maybe for port resouce requests? | |
| 14:42:34 | dansmith | well, that's kinda what I was eluding to above, | |
| 14:42:43 | dansmith | with multiple things accessing here | |
| 14:42:57 | sean-k-mooney | ack | |
| 14:43:05 | dansmith | like does PUT {} avoid nuking a sub-allocation that neutron hasn't yet cleaned up? | |
| 14:43:30 | sean-k-mooney | i dont think we technially support netron modifying the allocation directly | |
| 14:43:43 | sean-k-mooney | i think nova always handels that on its behalf | |
| 14:43:51 | sean-k-mooney | but valid question | |
| 14:44:05 | dansmith | okay I thought in some cases it would.. wasn't that the point of that blueprint? | |
| 14:44:19 | dansmith | I wasn't really involved in it so I dunno | |
| 14:44:26 | gibi | it was definitely before port resource request as nested a_c is a prerequisite for that | |
| 14:44:30 | sean-k-mooney | so there was the open question about modifying QOS policies for bound ports | |
| 14:44:45 | sean-k-mooney | which could change the allcoaitons | |
| 14:44:46 | gibi | the only case neutorn modifies allocation ^^ | |
| 14:44:55 | sean-k-mooney | but i dont think we support that yet do we | |
| 14:45:01 | gibi | we do | |
| 14:45:05 | sean-k-mooney | oh ok | |
| 14:45:28 | gibi | you can change QoS policy on a bound port, neutron will try to amend the instance allocation according to the new policy | |
| 14:45:35 | sean-k-mooney | so ya change the bandwith or pps qos amount on a bound port would be the only time neutron could modify the allocation i think | |
| 14:45:43 | gibi | yepp I think too | |
| 14:45:44 | dansmith | okay, but this was for nested A-C, meaning another allocation against another provider coming back from placement, not any sort of actual nesting of allocations as stored right? | |
| 14:46:20 | sean-k-mooney | yes i dont think there is really a concept of nested allocations | |
| 14:46:27 | gibi | dansmith: right, we dont have sub allocations we have multiple RPs in an allocaiton | |
| 14:46:31 | sean-k-mooney | jsut a singel allcoation with resouce form multple RPs today | |
| 14:47:04 | dansmith | gibi: so you wrote this initial patch to move from delete to put specifically for "benefit" of generations.. are you thinking that was wrong now? | |
| 14:48:03 | gibi | dansmith: I tried to figure out this mornign the reason why I had that patch | |
| 14:48:43 | gibi | the only thing I know is that bumping the placemenet api to use nested a_cs meant that we got generations as well | |
| 14:48:51 | gibi | so we had to do something with generations | |
| 14:49:29 | gibi | i.e. the nested a_c support was in a bigger placement microversion than the generation support | |
| 14:50:26 | gibi | 1.28 and 1.29 | |
| 14:51:53 | gibi | if the request from the user is to delete the instance but that deletion fails due to a parallel operation then I think now that the delete operation should be the one that wins | |
| 14:52:47 | dansmith | I don't think this patch was required just because we did generations and nested a_c, because you're updating the version you pass there, | |
| 14:52:48 | gibi | in the other hand if the user requested restore of soft deleted instance and the timer on the soft delete -> hard delete is racing with that request then the restore should win | |
| 14:52:53 | dansmith | and each call uses whatever version it wants right? | |
| 14:53:00 | sean-k-mooney | gibi: microversion are per cal though | |
| 14:53:03 | dansmith | right | |
| 14:53:24 | gibi | right | |
| 14:54:30 | gibi | I think the warning at the end of https://docs.openstack.org/placement/latest/placement-api-microversion-history.html#consumer-generation-support shows that at that time we was pretty serious about using generations strictly | |
| 14:55:44 | sean-k-mooney | gibi: for the soft delete case we are expectign that this will help because? resotre will update allocation generation and the perodici will fail? | |
| 14:56:23 | sean-k-mooney | for soft delete i assume we continue to claim the resouce with the allocation until the actul hard delete happens | |
| 14:56:34 | sean-k-mooney | so would there be a need to update the allocation at all | |
| 14:56:40 | sean-k-mooney | for either soft delete or restore | |
| 14:56:49 | gibi | sean-k-mooney: if you are right then there could no be race between restore and soft -> hard delete | |
| 14:56:50 | sean-k-mooney | e.g. will the generation actuly cahnge | |
| 14:57:01 | gibi | btw the microversion history doc states | |
| 14:57:01 | dansmith | gibi: I haven't mapped it out, but is there some reason we can't specifically opt-into the nuke-and-destroy behavior when we're deleting an instance and still fail for other cases? | |
| 14:57:02 | gibi | Passing an empty allocations object along with a consumer_generation makes PUT /allocations/{consumer_uuid} a safe way to delete allocations for a consumer. | |
| 14:57:57 | gibi | dansmith: you mean the user should be able to opt into the force? | |
| 14:58:03 | dansmith | no | |
| 14:58:06 | sean-k-mooney | dansmith: am i think the reason is that technially deletes are not allowd to have request bodies and we did not want to use a query arg | |
| 14:58:08 | dansmith | you're force=True by default | |
| 14:58:22 | sean-k-mooney | oh you mean the force default | |
| 14:58:27 | dansmith | yes | |
| 14:58:31 | gibi | dansmith: so just inverting the default | |
| 14:58:37 | gibi | dansmith: that is OK to me | |
| 14:58:40 | sean-k-mooney | we can set that to false and jsut pass true for a user delete | |
| 14:58:50 | sean-k-mooney | ya im fine with inverting too | |
| 14:59:08 | gibi | I think at some point melwitt stated that she used force=True as default as that was the more frequent call | |
| 14:59:09 | dansmith | right, I think that's better signaling, and also better for whoever comes along later and uses that to delete an allocation | |
| 14:59:33 | dansmith | I think making the dangerous activity explicit is worth the verbosity | |
| 14:59:39 | sean-k-mooney | i tihnk its pretty close either way | |
| 15:00:01 | sean-k-mooney | in terms of calls | |
| 15:00:21 | dansmith | ack | |
| 15:40:03 | dansmith | gibi: sean-k-mooney: do we know why the consumer types thing suddenly means we need to do this btw? I imagine the answer is just "more chance for the generation to change" but.. why specifically? | |
| 15:42:45 | gibi | dansmith: exactly the consumer_types series merged the two transaction handling the allocation update into one, causing a longer transaction and hence a longer race window | |
| 15:43:40 | dansmith | really? I wouldn't expect that to be meaningfully long | |
| 15:44:15 | dansmith | as in, I would expect people running this in a larger cloud with lots of stuff going on would have hit this by now and not just in a gate run with few instances at any one point | |
| 15:44:29 | dansmith | I saw that analysis from melwitt in the bug and wasn't so sure | |
| 15:44:58 | dansmith | but I guess if it's really related to that change only then that has to be it | |
| 15:45:47 | gibi | there is a correlation with the consumer_type series landed and the uptick in the conflict failures in the gate | |
| 15:46:20 | gibi | I know correlation is not causation but still it feel relevant | |
| 15:46:34 | gibi | and the consumer_type series had this transaction logic change | |
| 15:46:36 | dansmith | hrm | |
| 15:46:50 | gibi | i have no better explanation at the moment | |