| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2022-08-24 | |||
| 10:26:32 | sean-k-mooney | so this will auto split on traits and resouce class | |
| 10:26:50 | gibi | hehe :) | |
| 10:26:50 | sean-k-mooney | technially we allow operator provided tags too but they were never uable for anything | |
| 10:26:53 | sean-k-mooney | we just ignore them | |
| 10:27:46 | sean-k-mooney | at one point there was talk of allowign the pci alias to match on extra tags | |
| 10:27:51 | gibi | I had to ignore traits and resource_class https://review.opendev.org/c/openstack/nova/+/853316/4/nova/pci/stats.py to keep the pool matching work | |
| 10:28:55 | sean-k-mooney | ah ok hehe | |
| 10:29:10 | gibi | it is mostly ther to keep _filter_pools_for_spec happy as the request contains the traits tag but the pool will not mapped to traits just to RPs | |
| 10:29:35 | sean-k-mooney | ya thats proably fine | |
| 10:31:25 | sean-k-mooney | will we have the tags in the pools | |
| 10:31:28 | sean-k-mooney | *traits | |
| 10:31:50 | sean-k-mooney | i assume the intent is just to relay on placment to do the trait/rc filtering | |
| 10:32:07 | sean-k-mooney | and then we use the rp id to corralate the pools with the allcoation candaate | |
| 10:32:12 | sean-k-mooney | so we dont need to check them in the filters | |
| 10:32:27 | sean-k-mooney | so we can just not put them in the pools | |
| 10:33:17 | gibi | we use the rp_uuid to correlate the request with the pool | |
| 10:33:23 | gibi | the rest is doen in placemnet | |
| 10:34:16 | sean-k-mooney | yep that is what i was expecting | |
| 10:34:36 | gibi | the pooling logic uses all the dev_spec tags automatically so I needed to explicity ignore traits and resource_class there | |
| 10:34:47 | gibi | to not to put them into the pool | |
| 10:34:50 | gibi | as we don't need them | |
| 10:35:04 | gibi | and it also won't match with the request in generic way | |
| 10:35:09 | sean-k-mooney | so your going to do two change right. 1 split the pools now also by parent adress and 2 split the alias requests into multipel instance_pci_request object if the alias request more then one of something | |
| 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 | |