| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2023-09-01 | |||
| 14:00:48 | sean-k-mooney | dansmith: can you take a look at this https://review.opendev.org/c/openstack/nova/+/893502 | |
| 14:01:19 | sean-k-mooney | merging the dangeling bdms serise broke reboot for deploymetns without cinder | |
| 14:01:20 | bauzas | dansmith: shouldn't need to update node, right? | |
| 14:01:27 | bauzas | dansmith: it's not changing | |
| 14:01:47 | bauzas | but, compute_id, mmmm, yeah you're right :( | |
| 14:02:04 | dvo-plv | I would like to talk regarding our proposal | |
| 14:02:22 | dansmith | bauzas: if compute_id is changing then node is changing, however, we're changing host to point to a different node, right? | |
| 14:02:40 | dvo-plv | Currently I see two option. 1 we move back to the separate mech driver. 2 we generate socket in the OpenStack and pass it to the dpdk | |
| 14:03:02 | bauzas | dansmith: well, the compute node uuid is the ironic node uuid | |
| 14:03:13 | bauzas | so, it shouldn't be changing, right? | |
| 14:03:17 | dvo-plv | I belive that 2 option is more suitable for you | |
| 14:03:55 | dansmith | okay I see, we're moving the node between services... | |
| 14:04:04 | dvo-plv | So, how do you see it in terms of the OpenStack, out dpdk team currently analyze this approach with our pmd driver | |
| 14:04:33 | bauzas | dansmith: correct, that being said, now I wonder about compute_id | |
| 14:04:58 | dansmith | bauzas: compute_id should match the node, so only change one if the other | |
| 14:05:15 | bauzas | ah, I was about looking on your series | |
| 14:05:35 | dvo-plv | I belive that using common option vhost-server-path is not acceptable in case dpdk port type, so we should create the new one, and it also will be appropriate parameter | |
| 14:05:53 | bauzas | (b/c while I reviewed it, honestly, I didn't thought about Ironic) | |
| 14:06:59 | sean-k-mooney | dvo-plv: ya so option 2 woudl be my prefernce. but add a new dpdkarg or extra_config to hold the path | |
| 14:07:29 | sean-k-mooney | dvo-plv: we have kind of a chicken and egg problem | |
| 14:07:41 | dansmith | sean-k-mooney: really? we have no jobs without cinder? | |
| 14:07:48 | sean-k-mooney | dansmith: apprently | |
| 14:07:59 | sean-k-mooney | dansmith: its enabled by default in the devstack base job | |
| 14:08:24 | dansmith | sean-k-mooney: that doesn't make sense to me.. if we have no cinder, we should have no bdms with type==volume | |
| 14:09:07 | sean-k-mooney | dvo-plv: os-vif does not have a way to return data ot nova, nova need to knwo the socket path to put it in the libvirt xml so qemu cna create it, and neutron does not knwo what vendor nic is being used | |
| 14:09:13 | bauzas | dansmith: ok, indeed compute_id == compute_node.id (found it in the code) so are you +W then ? | |
| 14:09:28 | bauzas | dansmith: see https://bugs.launchpad.net/nova/+bug/2033752 | |
| 14:09:43 | dansmith | bauzas: it was already +W, I just -Wd to make sure we weren't missing something | |
| 14:10:15 | sean-k-mooney | dansmith: hum... that is a very good point | |
| 14:10:22 | sean-k-mooney | oh | |
| 14:10:33 | sean-k-mooney | https://review.opendev.org/c/openstack/nova/+/893502/4/nova/compute/manager.py#4217 | |
| 14:10:42 | sean-k-mooney | dansmith: that line is alwasy called | |
| 14:10:47 | dansmith | ah | |
| 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 | |