| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2023-09-01 | |||
| 10:40:30 | bauzas | we just instantiate a client at every call | |
| 10:41:00 | sean-k-mooney | ya that is somehtign we can fix when we replace the client with the sdk | |
| 10:41:10 | bauzas | yup | |
| 10:41:45 | bauzas | anyway, my daughters are starving, I shall start to cook something or they'll burn the house | |
| 10:42:01 | bauzas | fortunately, the school is starting again next week | |
| 10:50:12 | opendevreview | Amit Uniyal proposed openstack/nova master: Addis a try-catch for cinder unavailability. https://review.opendev.org/c/openstack/nova/+/893489 | |
| 11:48:25 | opendevreview | sean mooney proposed openstack/nova master: only attempt to clean up dangeling bdm if cinder is installed https://review.opendev.org/c/openstack/nova/+/893502 | |
| 11:48:50 | sean-k-mooney | bauzas: auniyal ^ is how i woudl fix it | |
| 11:50:13 | opendevreview | sean mooney proposed openstack/nova master: only attempt to clean up dangeling bdm if cinder is installed https://review.opendev.org/c/openstack/nova/+/893502 | |
| 11:50:33 | auniyal | yeah, its looks cleaner than https://review.opendev.org/c/openstack/nova/+/893489 | |
| 11:51:27 | sean-k-mooney | jsut fixing some typos | |
| 11:51:31 | sean-k-mooney | is there a bug for this | |
| 11:51:49 | sean-k-mooney | ah https://bugs.launchpad.net/tempest/+bug/2033752 | |
| 11:52:01 | auniyal | yes | |
| 11:53:23 | opendevreview | sean mooney proposed openstack/nova master: only attempt to clean up dangeling bdm if cinder is installed https://review.opendev.org/c/openstack/nova/+/893502 | |
| 12:07:41 | bauzas | sorry folks, was with kids | |
| 12:08:29 | auniyal | sean-k-mooney, I added 2 comments in patch, its same what I was trying to do locally | |
| 12:08:54 | auniyal | *your patch - https://review.opendev.org/c/openstack/nova/+/893502/3 | |
| 12:15:13 | stephenfin | Uggla: https://review.opendev.org/c/openstack/openstacksdk/+/893505 | |
| 12:15:25 | stephenfin | I'm off for the rest of the afternoon, so hopefully that does the trick for you :) | |
| 12:16:10 | Uggla | oh thank you stephenfin, I'll have a look | |
| 12:16:28 | Uggla | stephenfin, have a great day | |
| 12:21:46 | stephenfin | Also published here https://gist.github.com/stephenfin/5834e4bd3213a2b9a9313ee3a7f56946 | |
| 12:21:50 | stephenfin | Uggla: Cheers | |
| 12:24:42 | Uggla | stephenfin, damned I'have again forgotten to register the options. :( It needs more testing but that looks good. | |
| 12:24:56 | Uggla | stephenfin, thank you so much | |
| 12:51:08 | 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 | |
| 12:53:02 | sean-k-mooney | bauzas: ^ i think i have adressed your nits stephenfin if your around can you take a look quickly too | |
| 12:53:52 | bauzas | sean-k-mooney: cool do | |
| 13:48:17 | dvo-plv | sean-k-mooney, Hello | |
| 13:48:46 | sean-k-mooney | hi | |
| 14:00:01 | dansmith | johnthetubaguy: comment here if you could look quickly: https://review.opendev.org/c/openstack/nova/+/886989 | |
| 14:00:30 | dansmith | also bauzas, since you were +2 | |
| 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 | |