| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2023-09-01 | |||
| 14:22:42 | bauzas | dansmith: example with shutdown https://github.com/openstack/nova/blob/master/nova/compute/manager.py#L3173-L3178 | |
| 14:23:04 | dansmith | bauzas: yep, we could just wrap all of that function in that check and move on.. perhaps that's safest | |
| 14:23:27 | bauzas | that's what I originally ask to auniyal but then sean-k-mooney and I discussed on having a better helper | |
| 14:23:29 | dansmith | but we need to not log that on every single reboot for people that have no cinder at all, which is what sean-k-mooney's patch will currently do | |
| 14:23:48 | sean-k-mooney | ah i can drop the log | |
| 14:23:56 | sean-k-mooney | i didnt have it orgianlly | |
| 14:23:59 | bauzas | dansmith: to not help the problem, we don't cache the cinder client | |
| 14:24:16 | bauzas | dansmith: we're really instantiating it at every call we make | |
| 14:24:35 | sean-k-mooney | dansmith: this is the log you are refering too right https://review.opendev.org/c/openstack/nova/+/893502/4/nova/compute/manager.py#4193 | |
| 14:24:37 | dansmith | bauzas: are we? | |
| 14:24:41 | bauzas | yes | |
| 14:24:50 | dansmith | sean-k-mooney: the one with the ugly parens, yeah :) | |
| 14:25:11 | bauzas | dansmith: eg. https://github.com/openstack/nova/blob/master/nova/volume/cinder.py#L863 | |
| 14:25:44 | dansmith | bauzas: ack | |
| 14:26:56 | dansmith | bauzas: you need to re-+W the ironic manage one | |
| 14:27:35 | opendevreview | Danylo Vodopianov proposed openstack/nova master: Packed virtqueue support was added. https://review.opendev.org/c/openstack/nova/+/876075 | |
| 14:28:41 | opendevreview | sean mooney proposed openstack/nova master: only attempt to clean up dangling bdm if cinder is installed https://review.opendev.org/c/openstack/nova/+/893502 | |
| 14:28:41 | bauzas | dansmith: ack, will | |
| 14:30:31 | dansmith | man I hate wasting vertical space on dangling parens | |
| 14:31:14 | sean-k-mooney | i have kind of started to do that because of ifs but ya i prefer not to wast it either | |
| 14:31:33 | dansmith | man it is SO ugly | |
| 14:33:00 | bauzas | black syntax kills python :) | |
| 14:33:10 | bauzas | it's Friday I can duck | |
| 14:40:18 | dansmith | bauzas: ++ | |
| 14:40:43 | bauzas | well, now I know that we don't have jobs without cinder | |
| 14:41:14 | dansmith | I'm really shocked tbh, and makes me think we have some room for trimming if that's the case ;) | |
| 14:45:53 | bauzas | yeah that's actually good news for the gate | |
| 14:47:15 | sean-k-mooney | before i go fix the test are we ok with this https://termbin.com/d2q2 | |
| 14:50:22 | auniyal | sean-k-mooney, should we catch one more i.e only Exception in manager.py | |
| 14:50:40 | auniyal | after EndpointNotFound | |
| 14:50:56 | bauzas | sean-k-mooney: looking | |
| 14:51:19 | sean-k-mooney | auniyal: no we shoud avoid catching Exception in general | |
| 14:51:27 | bauzas | yeah | |
| 14:51:35 | sean-k-mooney | i think we even have a hacking check to prevent you doing that without a #noqa line | |
| 14:51:47 | sean-k-mooney | s/line/comment/ | |
| 14:52:04 | bauzas | no AFAIR | |
| 14:52:10 | bauzas | but meh, anyway | |
| 14:52:25 | auniyal | sean-k-mooney ack | |
| 14:52:55 | bauzas | sean-k-mooneydoesn't see in your patch where you're moving the attachment_get_call(), just see it added at first | |
| 14:53:11 | bauzas | but I assume you're moving the call, not copying it | |
| 14:53:13 | auniyal | actually I was saying it because its here https://github.com/openstack/nova/blob/master/nova/compute/manager.py#L3184 | |
| 14:53:26 | sean-k-mooney | im moving it im still editing but i just wanted to confirm that was the general direction | |
| 14:53:38 | auniyal | but yes, it should be avoided | |
| 14:53:38 | bauzas | yeah, that's the direction we agreed | |
| 14:53:47 | bauzas | make it the first call in the method and bail out early | |
| 14:54:03 | bauzas | this way, non-cinder envs would meh this method | |
| 14:54:07 | sean-k-mooney | yep and im adding a second early our for | |
| 14:54:12 | sean-k-mooney | len(bdms_to_delete) == 0: | |
| 14:54:23 | sean-k-mooney | after the first loop | |
| 14:54:30 | bauzas | well, we're looping, right ? | |
| 14:55:10 | bauzas | ah | |
| 14:55:25 | bauzas | then after the loop, my bad, you just told it :) | |
| 14:55:59 | sean-k-mooney | https://termbin.com/a7sc | |
| 14:56:13 | sean-k-mooney | ok im goign to start fixign the test and ill push somthing up in a bit | |
| 14:57:04 | dansmith | sean-k-mooney: I'd be okay with that other than the dangling paren | |
| 14:57:24 | sean-k-mooney | for the excpet | |
| 14:57:28 | dansmith | and the unnecessary wrapping of the except list | |
| 14:57:31 | dansmith | that could be two lines but is four | |
| 14:58:05 | sean-k-mooney | well our convention is if you need to wrap you move all args | |
| 14:58:29 | dansmith | no it's not | |
| 14:59:01 | sean-k-mooney | it depend on if you want to dedent the args | |
| 14:59:02 | dansmith | that's black convention that has been sneaking into nova over years | |
| 14:59:10 | sean-k-mooney | either we dont denent them or we do and then move all | |
| 14:59:43 | dansmith | first example I found in the same file: https://review.opendev.org/c/openstack/nova/+/893489/1/nova/compute/manager.py#3881 | |
| 14:59:54 | dansmith | there's another right above it | |
| 15:00:11 | sean-k-mooney | yep i know we have been converting but fine i have swap back to that | |
| 15:00:38 | dansmith | "we" have not been converting, AFAIK.. "some people" have been converting :) | |
| 15:00:39 | sean-k-mooney | i dont really mind doing that in this case as it not hevvily indented | |
| 15:01:08 | sean-k-mooney | i really hate when we have really indeneted arges across like 10 lins | |
| 15:01:22 | sean-k-mooney | when it can be 4 if we dedent and wrap properly | |
| 15:01:43 | dansmith | well, regardless, this is not that case | |
| 15:01:47 | sean-k-mooney | we are both trying not to have lots of wasted virtical space so im ok to do it the other way here | |
| 15:04:51 | dansmith | yep | |
| 15:41:58 | sean-k-mooney | ok i think i have things wokring now | |
| 15:42:28 | sean-k-mooney | i cant do the second early out | |
| 15:42:58 | opendevreview | sean mooney proposed openstack/nova master: only attempt to clean up dangling bdm if cinder is installed https://review.opendev.org/c/openstack/nova/+/893502 | |
| 15:47:08 | sean-k-mooney | ok i need to run now so dansmith bauzas can ye take this from here. | |
| 15:47:20 | bauzas | ack | |
| 16:20:40 | bauzas | dansmith: can you please validate https://review.opendev.org/c/openstack/nova/+/893502 ? | |
| 16:23:48 | dansmith | bauzas: +W.. we might want to get a depends-on running against it in parallel to make sure it really solves the problem | |
| 16:23:59 | dansmith | was that a devstack base job that hit the original problem? | |
| 16:24:10 | bauzas | dansmith: yeah, some neutron ones | |
| 16:24:18 | bauzas | https://bugs.launchpad.net/tempest/+bug/2033752 | |
| 16:24:48 | dansmith | neutron ones or devstack ones? | |
| 16:25:11 | dansmith | ykarel: still around? | |
| 16:25:12 | bauzas | sorry https://zuul.openstack.org/job/neutron-ovn-base | |
| 16:25:47 | ykarel | dansmith, yes kind off | |
| 16:25:48 | bauzas | https://zuul.openstack.org/job/tempest-integrated-networking doesn't skip cinder, but his child above skips it | |
| 16:26:03 | dansmith | ykarel: I'll submit a DNM neutron patch to test | |
| 16:26:44 | dansmith | ykarel: https://review.opendev.org/c/openstack/neutron/+/893538 | |
| 16:26:51 | ykarel | dansmith, ack | |
| 16:27:33 | bauzas | I'll bail out now | |
| 16:27:39 | dansmith | bauzas: ack | |
| 16:27:41 | bauzas | long day for a long week | |
| 16:28:00 | dansmith | bauzas: puh lease man.. back for one week after gone for what, three? | |
| 16:28:11 | dansmith | I'm going to really enjoy that ONE day off I have next week ;) | |
| 16:28:15 | bauzas | 3 and a half :) | |
| 16:28:21 | dansmith | pssh | |
| 16:28:32 | bauzas | yeah, but going back on the ff week, lovin' <3 | |
| 16:29:04 | dansmith | bauzas: go on, get out of here.. I don't want to over stress your system :) | |