| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2022-04-28 | |||
| 12:09:41 | sean-k-mooney | oh is it | |
| 12:09:56 | sean-k-mooney | no third i just have the wrong tab open | |
| 12:10:16 | sean-k-mooney | the first patch also hass the same issue | |
| 12:10:49 | gibi | hm, you have a point about cloning | |
| 12:11:06 | gibi | if the id is dropped by our db code then that is now a global change on the db dict | |
| 12:11:21 | sean-k-mooney | yep | |
| 12:11:30 | sean-k-mooney | nova.tests.unit.pci.test_manager.PciDevTrackerTestCase.test_set_hvdevs_unavailable_pf_removed is failing in the first patch | |
| 12:12:48 | sean-k-mooney | you need to do | |
| 12:12:52 | sean-k-mooney | fake_pci_devs = [copy.deepcopy(fake_pci), copy.deepcopy(fake_pci_2), | |
| 12:12:54 | sean-k-mooney | copy.deepcopy(fake_pci_3)] | |
| 12:14:27 | gibi | but non of the test does it so probably the rest of test can be broken too | |
| 12:14:37 | gibi | I can add the deepcopy to _fake_get_pci_devices to fix them all | |
| 12:14:39 | sean-k-mooney | some do | |
| 12:14:59 | sean-k-mooney | like test_set_hvdev_changed_stal | |
| 12:15:38 | sean-k-mooney | many do all_devs = fake_db_devs_tree[:] | |
| 12:15:44 | gibi | that is a shallow copy | |
| 12:15:44 | sean-k-mooney | self._create_tracker(all_devs) | |
| 12:15:47 | gibi | only duplicate the list | |
| 12:15:54 | sean-k-mooney | it is yes | |
| 12:16:25 | sean-k-mooney | but i guess they dont currently modify thigns i guess. | |
| 12:16:26 | gibi | but that does not duplicat the dict having the id field | |
| 12:16:51 | sean-k-mooney | we likely do need to fix other test but i think we have just got lucky | |
| 12:17:43 | sean-k-mooney | thre are certenly sevel test that explcitly do a deepcopy | |
| 12:19:03 | sean-k-mooney | i dont think it s the _create_tracker that copl;es the global state | |
| 12:19:36 | sean-k-mooney | i think its self.tracker._set_hvdevs([fake_db_dev_3, fake_db_dev_4]) | |
| 12:20:52 | sean-k-mooney | _create_tracker does not actully pass the fake devs to the tracker | |
| 12:21:39 | sean-k-mooney | https://github.com/openstack/nova/blob/master/nova/tests/unit/pci/test_manager.py#L144-L147= | |
| 12:22:59 | gibi | yeah you are right | |
| 12:23:09 | gibi | the set_hvdev does the coupling | |
| 12:23:29 | sean-k-mooney | yep so you are not actully initalisign the tracker with the devices you created before using them | |
| 12:26:37 | gibi | I will push a fix | |
| 12:26:50 | gibi | for the new test cases | |
| 12:27:13 | gibi | and a separate commit for the existing ones not having deepcopy before _set_hvdevs | |
| 12:27:55 | sean-k-mooney | so i tihnk it was "working" becaue of this stub https://github.com/openstack/nova/blob/5f5551448dcfcde26095963e223f973b90e6f637/nova/tests/unit/pci/test_manager.py#L153-L154 | |
| 12:28:16 | sean-k-mooney | i was just trying to fiture out how the the tracker had any devices when it was not called | |
| 12:28:53 | sean-k-mooney | that just returns self.fake_devs https://github.com/openstack/nova/blob/5f5551448dcfcde26095963e223f973b90e6f637/nova/tests/unit/pci/test_manager.py#L153-L154 | |
| 12:29:06 | sean-k-mooney | so ya anything not doign a deep copy was sharing state | |
| 12:32:28 | trident | We have had a few policy overrides to allow regular users to create flavors. At some point they seem to have stopped working (probably upgrade to Victoria) - Neither a policy.json with the old format or a policy.yaml seem to help. | |
| 12:33:10 | trident | Any known reasons why a policy override like "os_compute_api:os-flavor-manage:create": "rule:admin_or_owner" (and the same for update and delete) would not work nowadays? | |
| 12:34:17 | trident | Version 22.3.2 | |
| 12:41:55 | sean-k-mooney | it should be possibel to configure that today | |
| 12:42:03 | sean-k-mooney | not advised but possible | |
| 12:45:05 | trident | Weird. I still get a 403 Policy doesn't allow os_compute_api:os-flavor-manage:create to be performed back... | |
| 12:48:38 | trident | sean-k-mooney: You say "not advised" - what would be the advised way to accomplish that? | |
| 12:49:00 | sean-k-mooney | you said it was victoria | |
| 12:49:06 | sean-k-mooney | 22.3.2 | |
| 12:49:41 | sean-k-mooney | 22.3.2 does not exist in git | |
| 12:49:59 | sean-k-mooney | on 22 | |
| 12:50:25 | sean-k-mooney | aslo no does not exist | |
| 12:50:28 | sean-k-mooney | https://github.com/openstack/nova/tree/22.3.2/nova | |
| 12:51:05 | sean-k-mooney | https://github.com/openstack/nova/tree/22.3.0/nova is the most recent release i see | |
| 12:51:42 | sean-k-mooney | same via opendev https://opendev.org/openstack/nova/src/tag/22.3.0 | |
| 12:52:37 | sean-k-mooney | https://opendev.org/openstack/nova/src/tag/22.3.0/nova/policies/flavor_manage.py#L22-L59 | |
| 12:52:57 | sean-k-mooney | this looks correct | |
| 12:53:13 | sean-k-mooney | am you have not turned on scope enforement have you | |
| 12:53:34 | sean-k-mooney | this requires a system scope token | |
| 12:53:59 | sean-k-mooney | trident: you cannot use a project scope token if you have scope enfroment enabled | |
| 12:54:55 | sean-k-mooney | so normal project_member tokens would not work even i fyou udated teh check_str unless you also added 'project' to the scope_types | |
| 12:56:23 | sean-k-mooney | trident: by the way im not sure admin_or_ower shoudl work in this context | |
| 12:56:42 | sean-k-mooney | trident: the resouce does not exsit yet so you cant be an ower of it | |
| 12:57:07 | sean-k-mooney | you woudl want something likse we use for server create | |
| 12:57:09 | sean-k-mooney | https://opendev.org/openstack/nova/src/tag/22.3.0/nova/policies/servers.py#L166-L175 | |
| 12:57:15 | sean-k-mooney | which is just project_member | |
| 12:58:12 | sean-k-mooney | actully no | |
| 12:58:14 | sean-k-mooney | https://opendev.org/openstack/nova/src/tag/22.3.0/nova/policies/create_backup.py#L24-L36 | |
| 12:58:17 | sean-k-mooney | creat backup is better | |
| 12:58:31 | sean-k-mooney | trident: you woudl want PROJECT_MEMBER_OR_SYSTEM_ADMIN | |
| 12:58:44 | sean-k-mooney | with both scope types | |
| 12:59:44 | sean-k-mooney | again we dont really recommend doing this but if i was to do that personally i woudl create role and then in the custom polciy string allow peopel with admin or the "create_flavor" role to create flavors | |
| 13:00:14 | sean-k-mooney | that way you can atleast limit it to a subset of peopel but admin_or_owner shoudl not work for create | |
| 13:01:02 | trident | Hm, yeah, that makes sense - as you say, it doesn't exist, so owner doesn't make sense at the time of creation. I'm however pretty sure those rules have worked previously. Probably at least on train. | |
| 13:01:21 | sean-k-mooney | admin_or_ower was defiend as 'is_admin:True or project_id:%(project_id)s' | |
| 13:01:31 | sean-k-mooney | but flavors are not part of projects | |
| 13:01:39 | sean-k-mooney | so that woudl not work in anycase | |
| 13:02:26 | trident | Thanks for the advise! I'll do some testing and see what I end up with. :) | |
| 13:02:38 | sean-k-mooney | on train it was more or less the same https://opendev.org/openstack/nova/src/tag/train-em/nova/policies/base.py#L27-L30 | |
| 13:13:08 | gibi | sean-k-mooney: I can reproduce the keyerror on master too with a bit of change and I think I see the issue. _create_tracker should take dict that come from the db with id field but _set_hvdevs should take dict that come from the hypervisor (no id field). My new tests (and one existing test) passed db dict to _set_hvdevs causing the db dict to get corrupted | |
| 13:13:52 | gibi | https://github.com/openstack/nova/blob/028b3bca16c750f6c7edf1b389ed6c79a2c9843d/nova/tests/unit/pci/test_manager.py#L361 | |
| 13:13:52 | sean-k-mooney | ya so that can work if its a copy | |
| 13:14:03 | sean-k-mooney | since it wont affect the gloabl sate but ya | |
| 13:14:25 | gibi | so we need the copy in L361 as that already corrupts the global state | |
| 13:14:34 | gibi | even on master | |
| 13:14:44 | sean-k-mooney | yep | |
| 13:15:02 | sean-k-mooney | well | |
| 13:15:11 | sean-k-mooney | we shoudl do the copy on line 347 | |
| 13:15:23 | sean-k-mooney | or we shoudl hvae _create_tracker do the copy internally | |
| 13:15:37 | sean-k-mooney | and then work on self.fake devs if need | |
| 13:16:24 | sean-k-mooney | but ya in anycase i wonder why we are not seeign this fail in general | |
| 13:16:38 | sean-k-mooney | theses test are not flaky in my experince | |
| 13:18:30 | gibi | we only have one test on master that does the mistake to pass a db dict with id to _sethvdevs. That corrupts the global state but no other test depends on that. Then I added another test that did this mistake and blow if the two test run in the same executor | |
| 13:18:41 | gibi | you can reproduce the issue by duplicating test_set_hvdev_remove_tree_maintained_with_allocations on master | |
| 13:18:49 | sean-k-mooney | ack ok | |
| 13:18:57 | gibi | I will try to clean this up | |
| 13:19:25 | sean-k-mooney | i wonder if we shoudl jsut have _create_tracker deep copy in genreal | |
| 13:20:03 | sean-k-mooney | we can still explictly do it but that would remove the need to do it in the default case | |
| 13:20:27 | sean-k-mooney | only if you call set_hvdevs | |
| 13:21:19 | sean-k-mooney | gibi: glad we caught that before it was merged | |
| 13:21:33 | sean-k-mooney | that kind of think is a pain to debug in the gate when it only failes ocationally | |
| 13:21:48 | gibi | yes, it was a good catch | |
| 13:21:59 | gibi | nobody likes interfeering test cases :) | |