Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2021-11-08
15:59:15 dansmith I know you do, but today regular users can't right?
15:59:22 sean-k-mooney they can
15:59:40 sean-k-mooney its systrem_reader_or_project_reader i think today
15:59:56 dansmith oh, maybe that explains why the test is so weird
16:00:07 sean-k-mooney https://github.com/openstack/nova/blob/master/nova/policies/flavor_extra_specs.py#L78
16:00:29 dansmith heh yeah okay
16:00:46 sean-k-mooney adding/removing obviously should be system scoped i think
16:01:06 dansmith the test conflates a lot of stuff, I guess for that reason, so I'll have to split things apart a bit because system can't see the servers, and thus can't see the embedded flavor
16:01:29 sean-k-mooney ah right
16:12:25 dansmith hrm,
16:12:43 dansmith pretty sure there's a bug in the create test that is testing index perms for create
16:13:32 dansmith https://github.com/openstack/nova/blob/171138146a648d22474b7021ac730e26f03455f8/nova/tests/unit/policies/test_flavor_extra_specs.py#L235-L254
16:13:43 dansmith the test purports to check update,
16:13:52 melwitt gibi: ack, will look
16:13:58 dansmith but it's actually checking index, which is rule_name instead of rule
16:14:13 dansmith and it's stubbing out update for everybody instead of index
16:14:32 opendevreview Merged openstack/nova stable/victoria: Reproduce bug 1944759 https://review.opendev.org/c/openstack/nova/+/810914
16:15:16 dansmith and the test actually asserts that system reader can update extra specs
16:18:28 lyarwood elodilles / gibi ; https://review.opendev.org/q/I26b2a14e0b91c0ab77299c3e4fbed5f7916fe8cf do either of you recall why we don't need this on >= stable/victoria ?
16:21:26 lyarwood seeing some weird behaviour downstream where we hit the 2to3 issues on py39 thanks to virtualenv and setuptools versions but upstream it looks like UC is downgrading setuptools for us during the run
16:22:09 kashyap frickler: lyarwood: Unrelated: just for info: MichalP from libvirt has completed both things: switching to '-accel' by default, and wiring up tb-cache. I tested his v2 series, and looks good: https://listman.redhat.com/archives/libvir-list/2021-November/msg00236.html
16:23:41 lyarwood awesome thanks
16:31:53 elodilles lyarwood: i think it is related to victoria and newer runs over focal on gate and we haven't encounter the use_2to3 errors there
16:36:04 lyarwood elodilles: interesting, I get the feeling this might have something to do with the pip version tbh
16:36:26 lyarwood elodilles: the weirdness downstream where we don't downgrade setuptools during the run as we do upstream that is
16:53:04 gmann dansmith: we need to have separate policy now for this. for listing via 1. GET /flavors/{flavor_id}/os-extra_specs/ - system-reader-or-project-reader 2. showing extraspec in GET /servers APIs response we need project_reader 3. PUT/rebuild /servers we need to add project_member
16:53:17 dansmith gmann: yeah, but I think the existing tests are wrong
16:53:21 elodilles lyarwood: so the issue in nova was with the l-c job as it used decorator==3.4.0, which uses the use_2to3 from setuptools. In victoria the l-c.txt has decorator==4.1.0 set, which is not having the use_2to3 anymore
16:53:23 dansmith gmann: for both create and update
16:53:23 gmann is it?
16:53:46 dansmith gmann: if not I need help understanding this: https://github.com/openstack/nova/blob/171138146a648d22474b7021ac730e26f03455f8/nova/tests/unit/policies/test_flavor_extra_specs.py#L235-L254
16:53:58 dansmith gmann: we're checking update, but we're making policy for update be @
16:54:17 dansmith gmann: and we're running the check against index
16:54:40 dansmith if I run the check against update and don't stub update with @, I get a fail
16:56:26 lyarwood elodilles: sorry was on a call
16:56:35 gmann dansmith: yeah, so we are checking this policy https://github.com/openstack/nova/blob/master/nova/api/openstack/compute/flavor_manage.py#L124
16:56:44 lyarwood elodilles: https://bugs.launchpad.net/designate/+bug/1946340 is the issue that we are hitting downstream, same root cause as the decorator problem just a different package
16:56:45 gmann dansmith: so first policy check of update has to be @
16:57:23 lyarwood elodilles: and with stable/wallaby upstream we don't appear to be hitting it because our upper constraints correctly downgrades setuptools during a run
16:57:23 gmann dansmith: so that we allow update policy for everyone and see if flavor extraspec in flavor update API response is included as per extra spec policy
16:57:28 dansmith gmann: ...but then you're just asserting that the user can run index, not that update is properly checking the thing right?
16:57:37 lyarwood elodilles: I'm just lost as to why this isn't happening downstream
16:58:04 dansmith gmann: the test is asserting that project reader can create/update flavor extra specs
16:58:14 johnsom lyarwood We removed the EOL driver that needed suds-jurko from Designate: https://review.opendev.org/c/openstack/designate/+/813380
16:58:49 gmann dansmith: this test is for 'updating flavor return the extra specs if policy allow'
16:58:50 johnsom stable branches will need to be pinned
16:59:08 lyarwood johnsom: right it's a dep of oslo.vmware as well so it's still pulled in by nova during a unit/functional run
16:59:17 gmann dansmith: for create/update flavor is separate tests
16:59:58 dansmith gmann: okay I see test_update_flavor_extra_specs_policy and test_flavor_update_with_extra_specs_policy
17:00:10 dansmith gmann: are you saying the latter is for the "and can see the result" variant?
17:00:16 dansmith if so, that's majorly confusing :)
17:00:58 gmann dansmith: yeah, i should have name it something like test_flavor_update_return_extra_specs_policy
17:01:07 gmann or more clear
17:02:22 dansmith I have a hard time reasoning about these tests, with very few comments and sparse naming,
17:02:34 dansmith because they're trying to replicate things super deep in an api request (as in this case)
17:03:23 gmann dansmith: yeah,multi-policy operation it is confusing but I agree I should have add more comments there
17:12:23 elodilles lyarwood: nova had only this l-c job failure with 'decorator', but other projects had different other packages that failed and had to be replaced/updated. Most probably some packages' version are older at your downstream job than the upstream.
17:59:59 kevko sean-k-mooney: is this related to mi issue ? :/ https://paste.opendev.org/show/810855/ ?
18:51:53 sean-k-mooney kevko: you get the error if the port has been created in ovs but the tapdevice is not present on the host in the same network namespace as ovs
18:52:21 sean-k-mooney actully no that is a slightly idffernt error
18:52:34 sean-k-mooney in this case the port has been revomed form the ovsdb.
18:52:43 sean-k-mooney this might be a race with the vm deletion
19:33:53 dansmith gmann: are you okay with this? https://pastebin.com/hGzKL2ap
19:34:12 dansmith gmann: it makes the tests much less verbose, easier to reason about (IMHO) and easier to refactor when we move things around
19:39:23 gmann dansmith: sure, looks good to me.
19:39:37 dansmith gmann: okay cool :)
19:49:20 dansmith gmann: why is this index and not show? https://github.com/openstack/nova/blob/171138146a648d22474b7021ac730e26f03455f8/nova/api/openstack/compute/views/servers.py#L236-L237
19:50:50 gmann dansmith: because we used single policy for listing all extraspecs in flavor APi and in server API response also.
19:51:17 gmann but with new granular one we can name them like servers:show:flavor_extraspecs
19:51:20 dansmith gmann: right, but index means "can you list" and show means "can you see the actual thing" right?
19:51:30 gmann dansmith: yes
19:51:47 dansmith seems like the server-embedded flavor is more of a "show" than "index" to me
19:53:12 dansmith ah,
19:53:30 dansmith I guess it's because a show on a flavor including extra specs means "listing" the extra specs inside
19:53:37 gmann dansmith: but that is list right? not single extraspec. we usually used show for getting single resource details and index for list
19:53:54 dansmith I think that's probably just an implementation detail about how we store those..seems confusing from the outside, but I see now
19:54:41 gmann this one https://github.com/openstack/nova/blob/171138146a648d22474b7021ac730e26f03455f8/nova/api/openstack/compute/flavors_extraspecs.py#L59
19:55:19 dansmith yep, I understand now
19:56:06 dansmith makes sense if you consider extra specs their own thing and a flavor as a container of them. I know that's how we do it, it just a little confusing if you think of the flavor as a whole thing :)
19:56:52 gmann yeah
19:57:37 gmann dansmith: i am thinking do we need separate policy for showing extra-specs?
19:58:01 dansmith showing individual specs?
19:59:16 gmann I mean if flavor can be seen by anyone then we show extraspecs also to them or have policy to show flavor detail itself?
19:59:53 dansmith oh, I thought that came from some people wanting to be able to exclude extra_specs from the view of regular users, without removing the rest of the flavor
19:59:54 dansmith sean-k-mooney: ?
20:00:51 sean-k-mooney dansmith: no i was suggesting we might want to allow that in the futre currently some public cloud hide all extra specs form there users there is not filterign today
20:01:28 dansmith sean-k-mooney: there's a separate rule for showing them now
20:01:58 sean-k-mooney in the server reponce or in flavor list
20:02:04 sean-k-mooney i tought there was just one for both
20:02:08 gmann in flavor and server response both
20:02:18 gmann yeah currently one for both
20:02:52 gmann and default to reader so kind of everyone as default
20:03:00 sean-k-mooney what i think woudl be nice at some point woudl be to be able to filter out any non namespace extra spec an any namespaced on related to filtering
20:03:31 sean-k-mooney that way operators that want to hide infra detail can do so but still expsoe the rest fo the extra specs
20:04:02 sean-k-mooney some extra specs ar thigns a user really want to know like does this have a gpu
20:04:24 sean-k-mooney other are really things that are important to the admin and schduler only
20:06:36 gmann that is little complex to handle but anyways let's leave them as it is then if it get updated in future
20:07:32 sean-k-mooney gmann: well that would not be implemeted as a policy it would likely need to be a new api microverions
20:07:45 sean-k-mooney or at least a code change that would implement the filtering
20:08:15 gmann sean-k-mooney: but you said that should be configurable right not hard-coded is_admin check?

Earlier   Later