Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2022-04-28
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 :)
13:56:01 opendevreview Balazs Gibizer proposed openstack/nova master: Isolate PCI tracker unit tests https://review.opendev.org/c/openstack/nova/+/839766
13:56:20 gibi sean-k-mooney: ^^ I will base the current series on top of this
14:02:46 opendevreview Balazs Gibizer proposed openstack/nova master: Isolate PCI tracker unit tests https://review.opendev.org/c/openstack/nova/+/839766
14:02:46 opendevreview Balazs Gibizer proposed openstack/nova master: Remove unavailable but not reported PCI devices at startup https://review.opendev.org/c/openstack/nova/+/838553
14:02:47 opendevreview Balazs Gibizer proposed openstack/nova master: Simulate bug 1969496 https://review.opendev.org/c/openstack/nova/+/838554
14:02:47 opendevreview Balazs Gibizer proposed openstack/nova master: Allow claiming PCI PF if child VF is unavailable https://review.opendev.org/c/openstack/nova/+/838555
14:13:30 sean-k-mooney gibi: ack ok that makes sesne
15:21:08 opendevreview Elod Illes proposed openstack/nova master: [CI] Install dependencies for docs target https://review.opendev.org/c/openstack/nova/+/839781
15:29:36 elodilles gibi bauzas : another stable gate fix to review ^^^ when you have time o:)
15:30:26 elodilles (today's broken oslo.policy release showed an error in our tox docs target as the job is failing for stable branches due to a release on zed)
15:46:05 clarkb elodilles: gibi bauzas email was sent about that problem a few weeks ago http://lists.openstack.org/pipermail/openstack-discuss/2022-April/028016.html there is a good chance that much of openstack needs that sort of update
15:47:04 gibi elodilles, clarkb: thanks I'm +2 on it
15:49:29 sean-k-mooney when did that get remvoed
15:49:42 sean-k-mooney we used to install requiremets.txt
15:50:27 clarkb sean-k-mooney: a while back there was a big push to switch to trimming the doc requirements down so you didn't have to install everything. What that missed was that the doc builds depended on the projects to collect cli command output and such. Basically I think it was docs having their own requirements that introduced the bug
15:50:40 clarkb the intent was good, but no one realized that this flaw existed
15:51:17 sean-k-mooney ah so it was applied genericly
15:51:26 sean-k-mooney i just did not recally this patch going in
15:51:55 sean-k-mooney we might also need test-requiremetns in some cases but in generaly not
15:53:54 elodilles clarkb: thanks, i'll try to check other projects as i've seen +24 broken stable-periodic tox-docs job today (neutron has already a similar patch on the gate right now)
15:54:13 elodilles (this one: https://review.opendev.org/c/openstack/neutron/+/839777 )
15:59:01 sean-k-mooney clarkb: so it would b enice to have included the change id of the change that remvoed it but i dont think we shoudl hold this up for that so ill review it now
15:59:25 clarkb I mean its not my change. I just helped debug a similar problem a few weeks ago and we told everyone about it hoping they would audit and fix their repos
15:59:30 clarkb seems that didn't happen hence the current situation
15:59:50 sean-k-mooney i never new this happend i must have missed the mail

Earlier   Later