| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-12-20 | |||
| 15:56:15 | mriedem | bauzas: bottom 4 nova-net removal changes are +2ed and ready to go https://review.opendev.org/#/c/696510/ | |
| 15:56:18 | mriedem | starting at ^ | |
| 15:56:31 | bauzas | mriedem: thanks ! | |
| 15:56:42 | bauzas | I'm finally back upstream this week FWIW | |
| 16:00:08 | TheJulia | mriedem: rats, I was hopeful :) | |
| 16:01:14 | openstackgerrit | Lee Yarwood proposed openstack/nova master: guestfs: Log a useful error when the required vmlinuz isn't found https://review.opendev.org/700193 | |
| 16:10:37 | openstackgerrit | Merged openstack/nova stable/train: Remove 'test_cold_migrate_with_physnet_fails' test https://review.opendev.org/700021 | |
| 16:12:54 | mriedem | gibi_pto: nits in https://review.opendev.org/#/c/699015/ for when you want to start a FUP patch for that series | |
| 16:16:45 | mriedem | bauzas: also, the bottom few changes in the live migration + qos ports series ^ are +2ed and need another core | |
| 16:17:08 | mriedem | per usual gibi has done a lot of good functional testing | |
| 16:33:02 | bauzas | mriedem: ack, will review it too on monday | |
| 16:36:54 | mriedem | johnthetubaguy: are you ok with me fixing https://review.opendev.org/#/c/697162/7/nova/tests/unit/compute/test_shelve.py@168 in a follow up? | |
| 16:37:05 | johnthetubaguy | mriedem: yeah | |
| 16:37:53 | johnthetubaguy | I assumed I miss-read it, no worries | |
| 16:40:25 | mriedem | thanks | |
| 16:43:59 | openstackgerrit | Matt Riedemann proposed openstack/nova master: WIP: Implement reschedule logic for cross-cell resize/migrate https://review.opendev.org/696213 | |
| 16:43:59 | openstackgerrit | Matt Riedemann proposed openstack/nova master: WIP: Add negative test to delete server during cross-cell resize claim https://review.opendev.org/688832 | |
| 16:44:00 | openstackgerrit | Matt Riedemann proposed openstack/nova master: FUP to Iff8194c868580facb1cc81b5567d66d4093c5274 https://review.opendev.org/700202 | |
| 17:04:35 | gmann | johnthetubaguy: i can change the default target logic in policy.py. cannot remember what case i thought it can break if moving to common place - https://review.opendev.org/#/c/676688/5 | |
| 17:05:18 | gmann | johnthetubaguy: also, do you have time to discuss the admin policy case - https://review.opendev.org/#/c/676682/5 | |
| 17:10:19 | johnthetubaguy | gmann: sure | |
| 17:12:14 | gmann | johnthetubaguy: in this case, new default (base.SYSTEM_ADMIN) has special string 'system=all' which will not match with existing project admin even enforce_scope is flase | |
| 17:12:45 | gmann | that is why i think we need to deprecate it to support the existing user's token keep working | |
| 17:13:57 | johnthetubaguy | but I am not sure we need to add it yet | |
| 17:14:14 | johnthetubaguy | in the admin_only case, that is | |
| 17:15:03 | johnthetubaguy | but... I get your point for admin_or_owner, hmm | |
| 17:15:10 | gmann | you mean 'system=all' ? | |
| 17:15:55 | johnthetubaguy | yeah | |
| 17:16:18 | johnthetubaguy | because in the admin_only case, we already check via the scope check | |
| 17:16:19 | gmann | that is needed for the window when we remove the deprecated rule and make enforce_scope=True by default. in that window project admin should not be able to do system admin operation. | |
| 17:16:47 | gmann | if enforce_scope if false then scope check is not checked right. | |
| 17:17:04 | johnthetubaguy | yeah, I just assumed those two things happened at the same time | |
| 17:18:18 | johnthetubaguy | gmann: maybe we should do the admin_password API, its admin_or_owner | |
| 17:18:40 | johnthetubaguy | I think what works there we probably want to apply to the admin api too | |
| 17:19:22 | johnthetubaguy | I think the points you made above, I totally agree with you for the admin_or_owner case | |
| 17:19:36 | gmann | if both things happen at same time then yes i agree with your point for admin case. | |
| 17:20:21 | gmann | yeah admin_or_owner and any lower role like member or reader we need that string. | |
| 17:20:54 | gmann | so that we do not expose any project level reader to system level reader | |
| 17:21:13 | johnthetubaguy | what about doing the deprecation at this level: https://review.opendev.org/#/c/645452/17/nova/policies/base.py | |
| 17:21:22 | gmann | s/system level reader to project level reader | |
| 17:21:59 | johnthetubaguy | we spoke about someone wanting to use this sooner they can override the base policy rule to always to the system check, if they want to opt into the new world early | |
| 17:22:51 | gmann | yeah | |
| 17:23:14 | gmann | did not get your last point on deprecation at base level ? | |
| 17:23:33 | johnthetubaguy | so thinking about this: https://github.com/openstack/nova/blob/b953db09e8b9195c6a922c03a57f44cd790adcb6/nova/policies/base.py#L79 | |
| 17:23:50 | johnthetubaguy | what if we move the APIs over to the new rule | |
| 17:24:18 | johnthetubaguy | but we add a deprecated bit to the new rule, so it falls back to allowing the old check in there? | |
| 17:24:33 | johnthetubaguy | ... i am not explaining this well | |
| 17:25:30 | gmann | i think i got your point and deprecate to RULE_ADMIN_API | |
| 17:25:37 | johnthetubaguy | yeah | |
| 17:25:58 | johnthetubaguy | so when an operator wants the new world, its only a few rules they change | |
| 17:26:18 | gmann | but what if any API rule changing from RULE_ADMIN_OR_OWNER - > SYSTEM_ADMIN ? i think no.. | |
| 17:27:08 | gmann | it will be -> either PROJECT_MEMBER_OR_SYSTEM_ADMIN or PROJECT_READER_OR_SYSTEM_READER that what we have in table | |
| 17:27:39 | johnthetubaguy | yeah, that is the only option I believe | |
| 17:27:53 | johnthetubaguy | and it should work OK for those two | |
| 17:28:03 | johnthetubaguy | hopefully the unit tests will keep us honest here anyways | |
| 17:28:16 | johnthetubaguy | once we have added them :) | |
| 17:28:21 | gmann | johnthetubaguy: then all case we can do in base rules as per this tabel? -https://github.com/openstack/nova/blob/b953db09e8b9195c6a922c03a57f44cd790adcb6/nova/policies/base.py#L41 | |
| 17:28:36 | johnthetubaguy | yeah, I hope so | |
| 17:29:01 | johnthetubaguy | so if we go through the cases... | |
| 17:29:08 | gmann | until we encounter any such policy conflicting that table but we can check that if we encounter such case | |
| 17:29:25 | johnthetubaguy | 1) no overrides, just use default, they keep working | |
| 17:29:43 | gmann | ahh 1 min | |
| 17:29:52 | gmann | so override would not work in that case | |
| 17:30:02 | johnthetubaguy | ah, why is that? | |
| 17:30:28 | johnthetubaguy | I think if you override, it skips doing an or with the deprecated rule | |
| 17:30:34 | gmann | because override is done with specific policy rules not base rules | |
| 17:30:51 | johnthetubaguy | right, but I think that is OK | |
| 17:31:16 | johnthetubaguy | in the sense that if the override it to something special, that something special stays the same | |
| 17:32:30 | gmann | yeah override rule is first pick in oslo policy. | |
| 17:33:03 | johnthetubaguy | maybe another question... what upgrade check should we when we want to make enforce_scope = true? | |
| 17:33:31 | johnthetubaguy | we could warn if they have any policy rules overriden that they need checking, but not sure what else we can do really | |
| 17:33:51 | gmann | we need deprecation at specific policy level when we do granularity where rule name change like- https://review.opendev.org/#/c/648480/23/nova/policies/services.py | |
| 17:33:53 | johnthetubaguy | this is more for "next" release, after we make the initial backwards compatible change | |
| 17:34:04 | johnthetubaguy | gmann: +1 | |
| 17:34:11 | johnthetubaguy | that case we totally need that | |
| 17:34:58 | johnthetubaguy | but that is a sort of independent issue, its because we added more granularity | |
| 17:35:07 | gmann | yeah | |
| 17:35:11 | johnthetubaguy | and folks make have overriden the old rule | |
| 17:35:28 | johnthetubaguy | cool, does that mean we are all set? | |
| 17:36:33 | gmann | for upgrade check, i was thinking auto correction but it need more thought. | |
| 17:36:59 | johnthetubaguy | yeah, although I don't think it blocks this release's work a such | |
| 17:37:21 | johnthetubaguy | the simplest check is probably to see if the override is the old default | |
| 17:37:37 | johnthetubaguy | in case people haven't deleted their old policy files they always used to deploy | |
| 17:37:58 | johnthetubaguy | there is an ops meetup after christmas, I could ask around there if policy comes up | |
| 17:38:17 | gmann | +1, that will be great. | |
| 17:38:50 | johnthetubaguy | cool, happy holidays when that happens | |
| 17:38:52 | gmann | seems all set for now. thanks for discussion. | |
| 17:39:17 | johnthetubaguy | no problem, thanks for pushing this, so many of our customers are wanting to see this move forward | |
| 17:39:19 | gmann | you too. i will try to make other policy ready once you are back | |
| 17:39:29 | johnthetubaguy | cool | |
| 17:40:21 | gmann | also waiting for stephenfin to reply on this, i could not understand his query (if something interesting to care at initial level)- https://review.opendev.org/#/c/657698/14 | |
| 17:43:30 | johnthetubaguy | gmann: I think stephenfin was meaning we could have left the unit tests where they were, under api, rather than "moving" to policy | |
| 17:44:12 | gmann | ohk | |
| 17:45:13 | johnthetubaguy | personally, I had assumed we were putting the unit tests under api/ before I saw your patches, but I wasn't against moving it | |
| 17:45:14 | gmann | i think doing under separate dir is more explicit and easy to check coverage. inside API it was very hidden kind of testing. but may be it just me ? | |
| 17:45:40 | johnthetubaguy | the other thing, is when you cut and paste all the tests when adding a new API, you might not see the policy tests | |
| 17:45:57 | johnthetubaguy | not that people do that, of course (cough) | |
| 17:45:58 | gmann | i actually went with the code structure. policies is not under API code | |
| 17:46:31 | johnthetubaguy | but we are testing the context.can call, and that is under the API code | |
| 17:46:59 | johnthetubaguy | I should be clear, my +2 stands, I don't mind the idea, for the reasons you just said | |