| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2021-11-08 | |||
| 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? | |
| 20:08:25 | sean-k-mooney | right now if operator use extra specs for filtering and want to hide that form user there only option is to hide all the extra specs which is an interoperatty issue | |
| 20:09:15 | sean-k-mooney | gmann: good quetion i assuem configurble jsut for interop but admin only for some might make sense. anyway in the context of dans patch we shoudl nto change anythign form what we have today | |
| 20:09:57 | sean-k-mooney | gmann: i just dont really think the exsitg policy fully solve that use case but operators are not complaing so lets not change it for now | |
| 20:10:28 | gmann | sean-k-mooney: or they are not even know it :) as it is allow to show extra specs to everyone as default | |
| 20:10:51 | gmann | but yes if someone has overridden the policy then it make sense | |
| 20:11:05 | gmann | let's leave those as it is. | |
| 20:11:40 | gmann | dansmith: sean-k-mooney so in that case, we can leave it as single policy itself instead of granular i suggested earlier | |
| 20:12:22 | gmann | making them granular for flavor and servers response might confuse and make things complex | |
| 20:12:28 | dansmith | gmann: still need a different one for server/flavor but yep | |
| 20:12:38 | dansmith | oh, no we definitely need those two, | |
| 20:12:39 | gmann | dansmith: because of default ? | |
| 20:12:42 | dansmith | because of the scope_types | |
| 20:13:47 | sean-k-mooney | i can see where scope_types come into other issue by why is it relevent here? you want to not check scope types on the server endpoint? | |
| 20:14:10 | gmann | and with flavor one default as system-project-reader and server one as project-reader ? | |
| 20:14:25 | dansmith | flavor one is project,system but the embedded one is just project | |
| 20:14:43 | gmann | embedded in server reposne right? | |
| 20:14:55 | sean-k-mooney | isned the embeded on contole by the server detail policy | |
| 20:15:06 | sean-k-mooney | e.g. its only expose via server detail | |
| 20:15:13 | gmann | sean-k-mooney: no, as separate policy after detail policy | |
| 20:15:14 | sean-k-mooney | not /server/uuid/extra_specs right | |
| 20:15:30 | dansmith | sure, we could rely on that, but all these tests are disabling the parent check to obsess over the child one being right, | |
| 20:15:41 | dansmith | which would imply that they need to be different | |
| 20:16:11 | gmann | yeah, like if operator want to show server detail to their owner but not extra-specs | |
| 20:16:14 | dansmith | the test will thus assert that system:reader can view the embedded one, but it can't actually | |
| 20:16:43 | sean-k-mooney | gmann: right but if they showed the falavor name you could look it up or at least what it is now | |
| 20:17:00 | sean-k-mooney | you could not compare embeeded to currnt but not sure how much that is relevent | |
| 20:17:54 | gmann | as long as we allow project for both flavor or embedded it is ok i think | |
| 20:17:58 | sean-k-mooney | dansmith: i have not looked at the test so ill trust your judgement if you think havign two woudl be useful but im not sure when it would make sense to have tehm set differently | |
| 20:19:03 | gmann | sean-k-mooney: like we will not add system scope for embedded one as system cannot GET /servers/server-id anyways | |
| 20:19:21 | dansmith | getting the two to work is also making me hate life, so maybe a note in the test would be better | |
| 20:20:33 | gmann | so any extra things to show in sevrer reponse has to be moved to project=scoped only as parent policy is project-scoped only | |
| 20:20:35 | dansmith | these tests are hard to debug sometimes, figuring out which context can do a thing that shouldn't be allowed | |