| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2023-09-01 | |||
| 14:10:49 | bauzas | dansmith: the problem is with https://github.com/openstack/nova/blob/master/nova/compute/manager.py#L4209 | |
| 14:11:09 | bauzas | everytime we call it | |
| 14:11:16 | dansmith | so let's put a check to not do anything else if no bdms_to_delete right? | |
| 14:11:36 | dansmith | if not bdms_to_delete: return | |
| 14:11:37 | bauzas | no, because we could have none BDMs | |
| 14:11:49 | bauzas | while there could be dangling volume attachments | |
| 14:11:54 | sean-k-mooney | i mean i can adda that as well | |
| 14:12:00 | bauzas | but, I looked at shutdown | |
| 14:12:07 | sean-k-mooney | are you suggesting removing the check for if cinder exists | |
| 14:12:11 | dansmith | bauzas: that's to delete attachments cinder thinks we have? | |
| 14:12:21 | sean-k-mooney | or just also adding a second check on line 4216 | |
| 14:12:32 | dvo-plv | sean-k-mooney, If we extend this field with socket, it will be acceptable, right ? https://github.com/openstack/os-vif/blob/master/vif_plug_ovs/ovsdb/ovsdb_lib.py#L184 | |
| 14:12:38 | dansmith | bauzas: isn't that wrong anyway? | |
| 14:12:59 | dansmith | bauzas: what if we have a multi-attach volume, with one instance and one standalone attachment for an externally-managed database server or something? | |
| 14:13:15 | sean-k-mooney | dvo-plv: i think that would be the correct approch yes | |
| 14:13:29 | sean-k-mooney | and if other vendors then want to integrate they just need to supprot that too | |
| 14:14:07 | bauzas | dansmith: on the first way, I'm okay with the verification : if we have nova attachments, then we can verify whether cinder also has them | |
| 14:14:07 | dvo-plv | okay, great, we will rework this solution. Do I need to udpate spec file ? | |
| 14:14:30 | bauzas | dansmith: on the other way, we could have cinder attachments while we wouldn't see BDMs | |
| 14:14:43 | dansmith | bauzas: right, but that's valid in the real world right? | |
| 14:15:05 | sean-k-mooney | dvo-plv: yes but not entirly because of this. last week was the non-client lib freeze so os-vif is frozen for bobcat and yesterday was the feature freeze for nova so the spec need to be reposposed for next cycle | |
| 14:15:13 | dansmith | bauzas: or, I guess we're filtering by only attachments for that instance, so maybe that's okay | |
| 14:15:33 | sean-k-mooney | dvo-plv: so it would be nice to note this in that when you reporpose the spec | |
| 14:15:46 | bauzas | dansmith: well, if you see my comments, I was torn | |
| 14:16:10 | bauzas | I wonder whether it's possible to have dangled volume attachments for this instance while we no longer have the BDMs | |
| 14:16:29 | bauzas | in order to remove the stale cinder attachments | |
| 14:16:42 | dansmith | bauzas: we should only do that if they specify our instance in the attachment | |
| 14:16:54 | sean-k-mooney | bauzas: if you try to attach a volume to an isntace via cinder it will create that case | |
| 14:17:10 | sean-k-mooney | fortunetly that is now blokcked without a service token | |
| 14:18:09 | sean-k-mooney | i.e. if a attachment extis for a voluem that we do not have a bdm for. you either did a db restore or messed with the nova db or you use cidner api to create it directly which we never supported | |
| 14:19:03 | sean-k-mooney | dansmith: bauzas: i have not been fully following your conversation but im planning to finish in the next hour or so | |
| 14:19:14 | sean-k-mooney | with that in mind is there a direction we want to take to unblock things | |
| 14:19:34 | sean-k-mooney | revert? proceed with patch? or is there a change you woudl like me to make quickly? | |
| 14:19:43 | dansmith | stale attachments are the only *legit* way to get there now, but that can certainly happen | |
| 14:19:45 | sean-k-mooney | alternitivly im fine with someone else takeing over this | |
| 14:19:47 | dvo-plv | okay, I got it | |
| 14:19:50 | dvo-plv | thanks | |
| 14:20:09 | dansmith | sean-k-mooney: bauzas I'd be okay with revert and fix, but if not, sean-k-mooney's patch needs some change I think | |
| 14:20:21 | dansmith | I can make the change if bauzas is around to approve, but melwitt is out today and monday all of the US is off | |
| 14:20:26 | dansmith | so a fast revert might be more prudent | |
| 14:20:38 | bauzas | dansmith: that's what it's doing now, right? we ask Cinder to tell us if we have volume attachments for this instance | |
| 14:20:57 | dansmith | bauzas: yeah, I'm just now reading it.. that wasn't in the version I last reviewed | |
| 14:21:09 | bauzas | but, how can we make this call conditional? I don't see | |
| 14:21:30 | bauzas | that's why wrapping the exception seems the easiest path to me | |
| 14:21:30 | dansmith | well, one thing we could do is just not do that healing at all if the instance has no volume bdms | |
| 14:21:49 | bauzas | dansmith: but we wouldn't fix the problem of a stale volume attach | |
| 14:22:10 | bauzas | and looking at other pieces of code, we just blindly catch the keystone exception actually | |
| 14:22:12 | sean-k-mooney | bauzas: in the event there are no other cinder voluem on the instance | |
| 14:22:13 | dansmith | bauzas: yep, we just need to make a call | |
| 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 | |