| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2021-08-27 | |||
| 12:24:54 | gibi | yes mypy only runs in the pep8 target not in the fast8 one | |
| 12:28:24 | stephenfin | sean-k-mooney: small nits on https://review.opendev.org/c/openstack/nova/+/806394 | |
| 12:34:10 | opendevreview | Ghanshyam proposed openstack/nova master: Convert features not supported error to HTTPBadRequest https://review.opendev.org/c/openstack/nova/+/806294 | |
| 12:38:43 | sean-k-mooney | stephenfin: yep just saw them come in. ill gix those up once i finish with bauzas mdev series | |
| 12:56:35 | opendevreview | Merged openstack/nova master: [func test] move port resource request tests https://review.opendev.org/c/openstack/nova/+/801815 | |
| 13:00:42 | opendevreview | Merged openstack/nova master: [func test] create pps resource on OVS agent RP https://review.opendev.org/c/openstack/nova/+/787205 | |
| 13:02:32 | gibi | lyarwood, stephenfin: I'm not sure I get https://review.opendev.org/c/openstack/nova/+/800086/16/nova/tests/functional/test_servers_resource_request.py#b1669 | |
| 13:03:31 | stephenfin | gibi: lyarwood is looking at your earlier statement that those tests are passing even though they wouldn't work in real life, and is saying the tests are therefore misleading | |
| 13:03:59 | stephenfin | and therefore it would be better to comment them out until such a time as their real-world equivalent scenario would pass | |
| 13:04:00 | lyarwood | stephenfin / gibi ; well I'm saying removing them isn't the way to document that | |
| 13:04:08 | lyarwood | right | |
| 13:04:31 | lyarwood | just seems odd to add things, remove them because they are passing when they shouldn't be and then later add coverage back in again | |
| 13:04:38 | gibi | stephenfin: ahh so it is about the interface attach tests that are started passing earlier than the actual implementation? | |
| 13:04:45 | lyarwood | gibi: right | |
| 13:04:51 | lyarwood | it's a nit | |
| 13:04:51 | stephenfin | yup, iiuc | |
| 13:05:17 | lyarwood | I shouldn't really -1 for it, it's just confusing as a reviewer tbh as opposed to leaving them commented out with a note | |
| 13:05:27 | lyarwood | IMHO | |
| 13:21:06 | gibi | sorry , I got distracted with a call | |
| 13:24:10 | gibi | OK, I got confused as you marked code lines that are about booting VMs not about interface attach | |
| 13:24:16 | gibi | anyhow | |
| 13:24:38 | gibi | those interface attach tests that are passing are not false positives | |
| 13:24:43 | gibi | those sceanrios really works | |
| 13:24:49 | gibi | but those scenarios are error scenarios | |
| 13:25:20 | gibi | so they are pretty useless for the end user | |
| 13:26:17 | gibi | I will respin the patches to move the service version checks to the compute.api | |
| 13:26:37 | gibi | but I'm not sure what to do about the interface attach tests you commented on | |
| 13:26:48 | gibi | I cannot comment them out as they are in the base class where they pass | |
| 13:27:18 | lyarwood | ah crap right sorry, ignore that then | |
| 13:27:25 | gibi | I can redefine them with empty body and a comment but that feels more confusin than actually doing nothing | |
| 13:27:33 | sean-k-mooney | you can boot with one ovs and one seriov prot today | |
| 13:27:48 | gibi | sean-k-mooney: ignore the context of the comment the comment is not about those test cases | |
| 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 | |