| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2021-08-27 | |||
| 13:28:16 | gibi | sean-k-mooney: it is about interface attach test cases that started to pass earlier (and therefore not mentioned in the code at all) than expected | |
| 13:28:17 | sean-k-mooney | right but im wondering why they were previouly mared as exped failure | |
| 13:28:28 | sean-k-mooney | was that because of resouce requests? | |
| 13:28:33 | gibi | sean-k-mooney: yepp, | |
| 13:28:38 | sean-k-mooney | ah ok | |
| 13:28:57 | gibi | sean-k-mooney: we had first patches to reject every operation with the new extended resource request format | |
| 13:29:09 | gibi | sean-k-mooney: then this patch adds the impl for them and remove the rejection | |
| 13:29:16 | sean-k-mooney | got it | |
| 13:29:31 | sean-k-mooney | ok ill leave it for not sicne i would have to start at the begining of the serise to review properly | |
| 13:30:02 | gibi | sean-k-mooney: yeah it is a long one :) | |
| 13:30:39 | sean-k-mooney | well its more if i jump in mid seriese without context i wont really understand what your doing so it wont be helpful | |
| 13:31:00 | sean-k-mooney | and i dont have time today unfortunetly to load all that context | |
| 13:31:16 | sean-k-mooney | but if there is anything you want me to look at just let me know | |
| 13:31:17 | gibi | sean-k-mooney: no worries, I got good reviews from stephenfin and lyarwood on the series | |
| 13:31:39 | gibi | sean-k-mooney: it is better to spread our effort, like making the mdev series land as well | |
| 13:32:17 | sean-k-mooney | ack yep thats why im working on it now | |
| 13:32:22 | gibi | cool | |
| 13:32:31 | sean-k-mooney | im hoping unified limits can also make it | |
| 13:33:02 | gibi | does the dansmith's comments have been resolved there? | |
| 13:33:15 | dansmith | not that I've seen, | |
| 13:33:16 | sean-k-mooney | i think melwitt was working on that | |
| 13:33:23 | gibi | ack | |
| 13:33:26 | dansmith | and I think unified limits had a long way to go to be landable even before | |
| 13:33:57 | dansmith | i.e. it's still reaching into oslo library internals and such, last I saw | |
| 13:34:15 | sean-k-mooney | oh ok melwitt raised it as posibel at risk for this cycle but i think she was hopeful it could still be ready in time | |
| 13:34:45 | dansmith | wow, okay I... would be surprised | |
| 13:35:29 | sean-k-mooney | ill take your word for it since i have not been following the details of it | |
| 13:35:52 | dansmith | I didn't see tempest tests for it, | |
| 13:36:16 | dansmith | but I definitely shook out some stuff from the glance implementation when I wrote tempest tests for it | |
| 13:36:35 | dansmith | I don't see docs, and I would think it needs docs | |
| 13:36:38 | sean-k-mooney | there are some https://review.opendev.org/q/topic:%22bp%252Funified-limits-nova%22+(status:open%20OR%20status:merged) | |
| 13:36:57 | sean-k-mooney | https://review.opendev.org/c/openstack/tempest/+/790186 and https://review.opendev.org/c/openstack/tempest/+/804311 | |
| 13:37:05 | dansmith | I see gibi has a bunch of +2s over the patch I'm -1 on, but I think some of those have code that reaches into oslo.limit | |
| 13:38:01 | dansmith | ah okay cool, glad to see those tempest patches | |
| 13:38:18 | dansmith | missed them being WIP I guess | |
| 13:38:32 | gibi | dansmith: as far as I understand even if we land unified limits it will be a sort of experimental optional feature | |
| 13:38:58 | gibi | there was no API impact so I considered it safe to experiment | |
| 13:39:09 | dansmith | mmmkay :) | |
| 13:39:19 | gibi | :) | |
| 13:40:40 | sean-k-mooney | https://www.youtube.com/watch?v=6wJXBUfcIOE | |
| 13:43:39 | dansmith | gibi: did you see my comment on this patch you have +Wd? https://review.opendev.org/c/openstack/nova/+/712139 | |
| 13:43:46 | dansmith | I think those exceptions are wrong, no? | |
| 13:45:44 | stephenfin | gibi: I'm +2 on basically the whole QoS series now, bar the things you've already talked about and the last two patches (small issues there) | |
| 13:46:13 | gibi | dansmith: could be a bit more chatty yes, but ValueError tend to be used to point out which formal parameter got an unexpected value in the functin call | |
| 13:47:01 | gibi | stephenfin: thanks, much appreciated | |
| 13:47:26 | dansmith | gibi: sure, but is that caught and logged appropriately? | |
| 13:48:13 | dansmith | gibi: usually that sort of thing ends up as not a trace in a log line where you just see "ValueError: entity_type" and never know what was going on when it happen | |
| 13:48:17 | dansmith | but if so, cool | |
| 13:48:53 | gibi | dansmith: thanks for pointing that out. I dropped my vote. I have to go back and see how that error is propagated | |
| 13:49:26 | dansmith | gibi: I see there's a test for the specific string, so I guess it's intentional, but I still would like to know that it's turned into something useful :) | |
| 13:50:35 | gibi | dansmith: I agree with your concerns | |
| 13:53:26 | dansmith | btw, is the plan really to just outright revert the consumer types stuff because of that bug? | |
| 13:53:30 | gibi | it felt for multiple cycles that this work is just sitting there waiting, I'm happy that now it got enough attention that we can make progress with it | |
| 13:54:22 | dansmith | gibi: definitely been sidelined and I'm very happy to see it moving. having just implemented this for glance I've also got context on it, which is helpful since nova's situation is obviously more complex | |
| 13:54:37 | sean-k-mooney | dansmith: i think we are going with the force flag instead of a full revert | |
| 13:54:59 | sean-k-mooney | dansmith: although melwitt has also proposed patches for the revert if we decied that is what we want to do | |
| 13:55:35 | dansmith | orly | |
| 13:55:44 | sean-k-mooney | sorry melwitt revert patches are for the delete of the allocation using put | |
| 13:56:10 | sean-k-mooney | is ther a reason to revert the consumer types if the force flag patch merges | |
| 13:56:27 | dansmith | is the force flag safe? | |
| 13:56:31 | sean-k-mooney | gibi: that already making its way through the gate correct | |
| 13:56:51 | dansmith | that conflict is specifically to avoid deleting or changing data you don't realize has changed right? | |
| 13:57:09 | sean-k-mooney | dansmith: it is but im not sure the orginial premis of this was valid | |
| 13:57:11 | gibi | sean-k-mooney: I had to respin due to couple of missed tests | |
| 13:57:18 | gibi | sean-k-mooney: so it is not on the gate but no passed check queue | |
| 13:57:43 | dansmith | sean-k-mooney: premise of what, the force flag or the premise of the conflict? | |
| 13:57:59 | sean-k-mooney | the conflict on allocation delete | |
| 13:58:08 | sean-k-mooney | or rahter the precident in hte non soft delete case | |
| 13:59:08 | sean-k-mooney | if we got a delete form the user i think that shoudl have taken precendce over allocation changes in general | |
| 13:59:34 | sean-k-mooney | gibi raised a vaild concern with soft delete which the force flag patch adresses | |
| 13:59:56 | sean-k-mooney | e.g. restore should take precidence over soft delete | |
| 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 | |