Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2022-08-24
10:35:18 gibi yes
10:35:21 gibi the later is already up
10:35:28 gibi I doing the former now
10:35:55 gibi https://review.opendev.org/c/openstack/nova/+/852771/6/nova/objects/request_spec.py#540
10:36:03 gibi this is the request splitting ^^
10:37:22 sean-k-mooney cool ill try and review more of the seriese today
10:37:39 gibi thanks
10:37:51 sean-k-mooney i have some coments on the ones i reviewd but im +2 on all the ones i have reviewed so far
10:38:00 gibi I will go through your comment
10:38:24 sean-k-mooney i had a littele bit of consern with https://review.opendev.org/c/openstack/nova/+/846470
10:38:34 gibi I just want to make the functionality complete first. If you see some dealbreaker in the series then use -1 so I will stop and go back to it
10:38:42 sean-k-mooney but i think the pci tracker will be sufficent to protect us
10:38:55 sean-k-mooney ack
10:39:22 sean-k-mooney so i just want to highligh a subtle behavior that you may or may not be aware of
10:39:40 sean-k-mooney if a pci device has a claim against it in the pci_devices table
10:39:53 sean-k-mooney and you remove it form the pci whitelist/dev spec
10:40:04 sean-k-mooney we do not remove it form teh pci tracker until the vm is delete or moved
10:40:19 sean-k-mooney that is to prevent you form currupting your db
10:40:31 gibi yes
10:40:35 sean-k-mooney by typoing the config
10:40:41 sean-k-mooney so we need to make sure we dont break that
10:40:45 gibi I follow that logic in the placement side as much as I can
10:40:52 sean-k-mooney ack
10:40:57 sean-k-mooney that is what i was wondering
10:41:10 sean-k-mooney i was hoping that we woudl not remove RPs if they had allcoations
10:41:13 gibi the commit message has an edge case described when nova will fail to start though https://review.opendev.org/c/openstack/nova/+/852397/5//COMMIT_MSG
10:41:54 sean-k-mooney this else branch https://review.opendev.org/c/openstack/nova/+/846470/15/nova/compute/pci_placement_translator.py#320
10:42:02 gibi but other than that the PCI RP will be kept until the PCIDevice is in the nova DB
10:42:18 sean-k-mooney is for the case where its in the pci tracker with a claim agaisnt it but removed form the config right
10:42:54 gibi at that point in the series we have no allocations against PCI RPs. so we just ignore the device without spec
10:43:14 sean-k-mooney we ignore removign it
10:43:35 sean-k-mooney so the rp stays there whiel the device is not deleted in the pci tracker
10:44:01 sean-k-mooney i guess it will just stay there
10:44:09 sean-k-mooney ok ill review what you have later anyway
10:44:20 sean-k-mooney since you have a patch for the case i was really worried about
10:45:27 sean-k-mooney but ya until we have allocations it really does not matter anyway
10:45:32 gibi later in the series we have the patch for reconf with allocations
10:45:36 gibi https://review.opendev.org/c/openstack/nova/+/852397/5/nova/compute/pci_placement_translator.py#419
10:46:30 sean-k-mooney perfect you even tell them how to fix it in the warning
10:48:06 gibi and here is a test case for it https://review.opendev.org/c/openstack/nova/+/852397/5/nova/tests/functional/libvirt/test_pci_in_placement.py#760
10:48:39 gibi below that there is the test case for reconfiguring by removing a PF but adding its VFs. That is a hard stop
10:49:46 gibi that would lead to a dependent device config which we explicitly not support
10:56:47 sean-k-mooney can we add a test for creating an instnace then removing the device form the config and restarting the agent
10:57:06 sean-k-mooney that shoudl allow the agent to start but complain loudly if we want to keep the old behaivr
10:57:15 sean-k-mooney or fail to start
10:57:30 sean-k-mooney but in either case the RP should not be updated and the allcoation should be kept in placment
11:01:33 gibi yes here is the test case that warns https://review.opendev.org/c/openstack/nova/+/852397/5/nova/tests/functional/libvirt/test_pci_in_placement.py#760
11:01:46 gibi and here is the case where nova refuse to start
11:01:47 gibi https://review.opendev.org/c/openstack/nova/+/852397/5/nova/tests/functional/libvirt/test_pci_in_placement.py#802
11:01:57 gibi as the reconf would create dependent device situation
11:02:36 gibi a simple config removal only cause a warning but RP and allocation is kept
11:02:58 gibi a config change that removes an allocated PF and configures its VF will be a hard stop
11:03:26 sean-k-mooney ack that sound like the semantics we want to have
11:03:46 sean-k-mooney the warning behavior is the same as we had today
11:03:57 sean-k-mooney and you are also catching a failure mode we dont prevent today
11:04:07 sean-k-mooney well it will be prevented differntrly
11:04:18 sean-k-mooney the current case if you add the pf and remove the vfs
11:04:24 sean-k-mooney i think will result in the pf goign to unaviaable
11:04:36 sean-k-mooney since the child device is claimed
11:04:52 sean-k-mooney so its a slight behavior delta but not nessisarly a bad one
11:05:21 gibi it is a delta becase we explicitly not support dependent device config with PCI in placement and that will be documented :)
11:05:59 gibi this is the delta https://review.opendev.org/c/openstack/nova/+/846435/19/doc/source/admin/pci-passthrough.rst#370
11:06:34 gibi I probably need to mention the extra case in the doc about reconf
11:07:04 gibi ohh I did https://review.opendev.org/c/openstack/nova/+/852397/5/doc/source/admin/pci-passthrough.rst#407
12:15:01 gmann gibi: dansmith_ : re on RBAC, I am +2 on dansmith_ patch https://review.opendev.org/c/openstack/nova/+/848021/2
12:15:31 gmann but little confused with the error in my patch, https://review.opendev.org/c/openstack/nova/+/849209/4
12:20:47 opendevreview Merged openstack/nova master: Add source dev parsing for vdpa interfaces https://review.opendev.org/c/openstack/nova/+/841016
12:20:54 opendevreview Merged openstack/nova master: Fix suspend for non hostdev sriov ports https://review.opendev.org/c/openstack/nova/+/841017
12:21:33 sean-k-mooney yay
12:22:00 sean-k-mooney stephenfin: when you have time could you review the last vdpa patch https://review.opendev.org/c/openstack/nova/+/853704/7
13:06:17 Uggla sean-k-mooney, gibi is https://review.opendev.org/c/openstack/nova/+/854355 ok for you. Or you absolutely don't want to change NovaPersistentObject ? I think it is clearer in the way, but I don't mind changing if you think it is better.
13:06:39 gibi gmann: left a note about the test failure in https://review.opendev.org/c/openstack/nova/+/849209/4
13:09:59 gmann gibi: that is what I am not getting why it is failing as there is no reference of PROJECT_ADMIN. locally all test passing
13:11:15 gmann PS3 passed successfully which was rebased with no change in that patch
13:12:18 opendevreview Ghanshyam proposed openstack/nova master: Remove system scope from all APIs https://review.opendev.org/c/openstack/nova/+/848021
13:12:27 opendevreview Ghanshyam proposed openstack/nova master: Keep legacy admin behaviour in new RBAC https://review.opendev.org/c/openstack/nova/+/849209
13:12:36 gmann rebasing on master, let's see
13:15:04 gibi Uggla: I made some notes in https://review.opendev.org/c/openstack/nova/+/854355 . I can accept the direction you are proposing.
13:16:23 Uggla gibi, thx I'm gonna look at the comments.
13:16:42 gibi gmann: that feels like some weird rebase issue then in zuul
13:17:09 gmann gibi: yeah seems so. it should pass now, let's see
13:17:33 gibi yeah we will see
13:47:49 sean-k-mooney Uggla: i have not done a full review but that is proably workable providere there is not api or db impact
13:48:32 opendevreview Rico Lin proposed openstack/nova master: Add locked_memory extra spec and image property https://review.opendev.org/c/openstack/nova/+/778347
13:48:33 opendevreview Rico Lin proposed openstack/nova master: Add traits for viommu model https://review.opendev.org/c/openstack/nova/+/844507
13:48:33 opendevreview Rico Lin proposed openstack/nova master: libvirt: Add vIOMMU device to guest https://review.opendev.org/c/openstack/nova/+/830646
13:50:27 Uggla sean-k-mooney, yep I just want the minimal change. Mostly to avoid next contributors "confusion" creating new objects.
14:58:21 auniyal__ Hi All
14:58:26 auniyal__ please review these
14:58:31 auniyal__ https://review.opendev.org/c/openstack/nova/+/852171
14:59:23 auniyal__ https://review.opendev.org/c/openstack/nova/+/853811
14:59:35 auniyal__ https://review.opendev.org/c/openstack/nova/+/853812
16:33:29 opendevreview Ghanshyam proposed openstack/nova master: Keep legacy admin behaviour in new RBAC https://review.opendev.org/c/openstack/nova/+/849209
16:35:21 gibi sean-k-mooney: am I correct that in nova there is no implemented preference between consuming a PF (for a direct-phyiscal port) that has VFs over consuming a PF that has no VFs?
16:38:06 gibi there is a single func test that assumes that we consume the PF without the VFs instead of the PF with VFs. But I think it was a simple coincidence that nova actually consumed that
16:39:04 gibi with the pools splitted the order of the pools changed and that test fails now.
16:39:29 gibi I will try to trick nova to change the order back...
16:47:59 gmann gibi: 849209 should pass now, actually unshelve_to_host policy merged in between and RBAC patches were not rebases to master. after rebasing to master the unshelve_to_host new policy shows up in gerrit display which needs to be corrected in RBAC changes.
16:48:07 gmann gibi: I fixed that and it in gate now
16:49:25 gibi gmann: ahh, true, that explains it. I always forget that zuul rebases a patch even in the check queue

Earlier   Later