| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2021-08-04 | |||
| 12:29:07 | sean-k-mooney | gibi: i think we take an instace lock for volume stuff to avoid concurrent voluem attach/detach operation on the same instance | |
| 12:30:34 | sean-k-mooney | multi attach is interesting edgecase | |
| 12:30:40 | gibi | sean-k-mooney: how a reserve that only create a new bdm can race with an attach that already has the bdm created or a detach that will delete a dbm tha should already exists | |
| 12:31:09 | gibi | still, I'm not against keeping the instance.uuid lock | |
| 12:31:24 | sean-k-mooney | gibi: well i was just using attach as example not suggesting it would race with it | |
| 12:31:36 | gibi | it is a sensible safety measure | |
| 12:34:50 | opendevreview | Alexey Stupnikov proposed openstack/nova master: Fix incorrect reservation IDs in unit tests https://review.opendev.org/c/openstack/nova/+/803363 | |
| 13:09:05 | aarents | Hi nova, | |
| 13:09:26 | sean-k-mooney | o/ | |
| 13:09:31 | aarents | I'm a bit stuck with this change https://review.opendev.org/c/openstack/nova/+/799606 (see my self comment) which is only a partial fix to the bug https://bugs.launchpad.net/nova/+bug/1934742 I have in operation, | |
| 13:09:42 | aarents | I'm not sure how to properly fix that, if someone have idea I can dig.. | |
| 13:10:33 | sean-k-mooney | let me read but you having issue with pepole delete port via neutron? rather then port detach right | |
| 13:10:43 | sean-k-mooney | technially we dont really support that | |
| 13:11:10 | aarents | yep port deletion when attached | |
| 13:11:14 | sean-k-mooney | we have the network-vif-deleted event to try and help fix things | |
| 13:11:25 | sean-k-mooney | btu deletion when attached is strongly discuraged | |
| 13:12:03 | sean-k-mooney | the correct way to fix it would be to either block it in neutron or have nova call detach internally | |
| 13:14:28 | sean-k-mooney | aarents: stirckly speaking form a nova perpective deleting an interface that is attach to an instance via neutorn is a user error | |
| 13:15:10 | sean-k-mooney | and the code path you are looking at is a workaround we put in place to try an partly handel it | |
| 13:15:43 | aarents | So we can imagine calling detach_interface but it will try to to unbind and will probably got 404 from neutron which is not a big deal | |
| 13:16:23 | sean-k-mooney | we would have to handel that ya but its something we could handel | |
| 13:16:54 | sean-k-mooney | this came up a few weeks ago though in the neutron driver meeting an i was pushing to block deleteing attach ports in the neutron api | |
| 13:17:00 | sean-k-mooney | long term that is the correct approch | |
| 13:18:06 | sean-k-mooney | this is really a neutorn bug not a nova one but nova can do a better jobs at cleaning up when you incorrectly detelete attach ports | |
| 13:18:25 | sean-k-mooney | the same is true for attach volumes by the way | |
| 13:18:40 | sean-k-mooney | you should never try to delete an attach volume but in that case i belive cinder will protect you | |
| 13:19:50 | sean-k-mooney | gibi: you rememebr that conversation about 2-3 weeks ago in the drivers meeting right? i think the resolution was to write a neutron spec for the new extnsion | |
| 13:19:51 | aarents | sean-k-mooney: I see your point about not a nova issue, My issue is I'like to avoid this possible leak because it generates un live_migrable instances in infra.. | |
| 13:20:14 | sean-k-mooney | yep im sure it does and yes that is why we try to clean it up | |
| 13:20:36 | sean-k-mooney | aarents: it has other proplems it hte port that was delete uses sriov | |
| 13:20:52 | sean-k-mooney | since the pci device will not be released properly | |
| 13:21:12 | aarents | I see | |
| 13:21:32 | sean-k-mooney | aarents: sriov deattach does not work properly in any release before wallaby | |
| 13:22:09 | sean-k-mooney | and i think it sitll wont work properly if you delete an attach neturon port although ill admit i have not looked at that code path to check | |
| 13:22:35 | sean-k-mooney | i just suspect that we did not test that when we added sriov attach/detach support | |
| 13:23:01 | sean-k-mooney | aarents: anyway your partial fix is to just add a lock | |
| 13:24:34 | sean-k-mooney | that might help i in limited cases but i dont think it will in general fix this | |
| 13:24:47 | aarents | I aggre it is not enought | |
| 13:30:12 | aarents | sean-k-mooney: thks! for the usefull info, I will dig more on blocking or try a detach | |
| 13:30:44 | sean-k-mooney | aarents: by the way if we cant use detach directly because all files cant be constructed we have 2 options. | |
| 13:31:14 | sean-k-mooney | we could have neutron send the full port info in the detach payload or we could contocut as much data as we can and pass incompelte objects | |
| 13:31:43 | sean-k-mooney | but blocking it in nueton would be the best approch or having neutron call detach which is rahter messy but would work better | |
| 13:32:15 | aarents | "neutron send the full port info in the detach payload" is interesting yes | |
| 13:32:37 | sean-k-mooney | one thing we disucssed was moving when neutron sends the event | |
| 13:32:56 | sean-k-mooney | if neutorn sent the deleted event and waited for nova to ack it | |
| 13:33:01 | sean-k-mooney | then that would also fix it | |
| 13:33:06 | sean-k-mooney | to a degree at least | |
| 13:33:38 | sean-k-mooney | anyway this is a know issue that we just have not focused on so sorry your are hitting it | |
| 13:34:16 | aarents | "could contocut as much data as we can and pass incompelte objects" <- I try that but it needs details like vif_type bridge name this info are no more available when port is deleted, I don't know how regenerate that | |
| 13:35:03 | sean-k-mooney | ah yes we do need those | |
| 13:35:25 | sean-k-mooney | which may or may not be still in the info cache as you have found | |
| 13:35:59 | opendevreview | Merged openstack/nova master: Add functional test for bug 1937375 https://review.opendev.org/c/openstack/nova/+/802011 | |
| 13:36:27 | sean-k-mooney | its its a hack but we might be able to reconstuct the required data by inspecting the libvirt xml but i would prefer to avoid that | |
| 13:38:38 | aarents | sean-k-mooney: yes.. it was the workaround I was considering, kind of detach_device_by_mac, we intereate on interfaces nad deleted the good one | |
| 13:39:27 | sean-k-mooney | ya by mac works for most things excption sriov pfs but in that case you can kind of still figure it out in some cases | |
| 13:39:47 | sean-k-mooney | it really depend on how much of the db info we have acess too | |
| 13:40:33 | sean-k-mooney | we cant get the pci address form the port since its gone and the info cache may be empty but we recently started recoring the neutron port uuid as the requester id in the pci_devices table i belive | |
| 13:41:13 | aarents | the info we can grab in db are entry in virtuales_interfaces: | |
| 13:41:14 | sean-k-mooney | which means we should be able to get the pci adress by the port uuid | |
| 13:41:30 | aarents | +---------------------+------------+------------+----+--------------------------------------------------------+------------+--------------------------------------+--------------------------------------+---------+------+ | |
| 13:41:35 | aarents | | created_at | updated_at | deleted_at | id | address | network_id | uuid | instance_uuid | deleted | tag | | |
| 13:41:39 | aarents | +---------------------+------------+------------+----+--------------------------------------------------------+------------+--------------------------------------+--------------------------------------+---------+------+ | |
| 13:41:43 | aarents | | 2021-08-03 15:44:23 | NULL | NULL | 62 | fa:16:3e:fb:2d:7b/4ae693fd-23fb-46c6-a8a5-369270029fc6 | NULL | 4ae693fd-23fb-46c6-a8a5-369270029fc6 | d07df8d0-fc98-4087-9b25-3b4a55cdcac4 | 0 | NULL | | |
| 13:41:47 | aarents | +---------------------+------------+------------+----+--------------------------------------------------------+------------+--------------------------------------+--------------------------------------+---------+------+ | |
| 13:41:54 | sean-k-mooney | yep | |
| 13:42:01 | opendevreview | Sylvain Bauza proposed openstack/nova master: Expose the mdev class https://review.opendev.org/c/openstack/nova/+/801743 | |
| 13:42:01 | opendevreview | Sylvain Bauza proposed openstack/nova master: WIP: Cleanup GPU vs. mdev wording https://review.opendev.org/c/openstack/nova/+/803379 | |
| 13:42:51 | sean-k-mooney | so with that info and the pci_devices table we can with some effort constuct most of what we need to inpect the xml and then figure out what the vif_type ectra was to unplug | |
| 13:43:12 | sean-k-mooney | but since the virt dirvers are not really ment to tlak to the db that is tricky to do | |
| 13:43:41 | sean-k-mooney | we would have to implement a new fuction in the compute manager ot do that more then likely | |
| 13:43:58 | sean-k-mooney | and then extend thet virt driver api | |
| 13:44:18 | sean-k-mooney | well it depend on how we approch it | |
| 13:54:33 | aarents | sean-k-mooney: it is good to know the topic is open with neutron, in short term I will find a hack to detach interface with few info I have in db | |
| 13:55:16 | aarents | sean-k-mooney: thks! | |
| 14:00:01 | kashyap | sean-k-mooney: Remind me again, why did you suggest to leave this as-is to 'cirrus'? - https://review.opendev.org/c/openstack/nova/+/798680/3/nova/virt/libvirt/config.py | |
| 14:00:23 | sean-k-mooney | because we only want to change the behavior for new instnaces | |
| 14:00:39 | sean-k-mooney | also for most code paths this get overriden | |
| 14:01:13 | sean-k-mooney | none might actully be a better default | |
| 14:01:31 | sean-k-mooney | i dont think this will ever get used currently at least not on x86 | |
| 14:01:57 | kashyap | sean-k-mooney: I'm actually going to do a bunch of migration tests with a variant of this patch to see if there's *actual* breakage or are we talking only theoretical stuff | |
| 14:03:26 | sean-k-mooney | well what we approved ot proceed was no change to existing instnaces | |
| 14:03:47 | sean-k-mooney | if you want to change the default of exisitng instance then we should disssu this cahgne again with the wider team | |
| 14:03:57 | kashyap | Well, let's see what breaks, if anything. "What was approved" was all based on theory. I'd like to see some actual evidence | |
| 14:03:59 | sean-k-mooney | so i think you should leave it at cirrus or defer this to yoga | |
| 14:04:52 | kashyap | Don't worry, I am concerned as much as you to not break any valid cases or upgrades, etc. | |
| 14:09:04 | kashyap | Oh, totally forgot: I said this befoere but I've gotten a Red Hat QE to do some tests w/ Cirrus and VirtIO to change for existing instances - for Windows and Linux. | |
| 14:09:25 | kashyap | I'll add a note in the review | |
| 14:10:02 | sean-k-mooney | they would need to test both migration and hard reboots after the fact withthe updated xmls | |
| 14:10:38 | sean-k-mooney | for live migration obviolys we cant change teh device model, it could change on a hard reboot after the migration but we generally want to avoid that | |
| 14:10:46 | kashyap | sean-k-mooney: Right, the test that was done so far was this: | |
| 14:10:55 | kashyap | Have a Linux and Windows guest on source with 'cirrus', change the video model to 'virtio'; reboot the guest, and then live-migrate -- it all succeeds | |
| 14:11:59 | sean-k-mooney | ack. we had discussed that in some cases default to virtio might be fine due to the vga fallback | |
| 14:12:10 | sean-k-mooney | have you test this where we have specifed vram | |
| 14:12:17 | sean-k-mooney | or other extra specs | |
| 14:12:34 | sean-k-mooney | * other image properteis | |
| 14:12:38 | kashyap | And on hard-reboot, a guest can pick up new device-related bits some times; can't avoid it in some cases | |
| 14:12:58 | kashyap | sean-k-mooney: Can you spell out a bit more on what do you want tested with the vRAM? | |
| 14:13:02 | sean-k-mooney | right personally i feel like that is a bug when that happens | |
| 14:13:25 | kashyap | (I mean, it's a bug if it breaks anything user-visible; if not, I'd say it's fine) | |
| 14:13:45 | sean-k-mooney | depends on who you talk too | |