| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2021-08-27 | |||
| 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 | |
| 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: Provide and use other RCs for mdevs if needed https://review.opendev.org/c/openstack/nova/+/803233 | |
| 14:19:54 | opendevreview | sean mooney proposed openstack/nova master: Expose the mdev class https://review.opendev.org/c/openstack/nova/+/801743 | |
| 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? | |