| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2020-03-31 | |||
| 22:30:59 | sean-k-mooney | dansmith: if you are about can you read the question i left in https://review.opendev.org/#/c/715326/4. basicaily im wondering if i should be binding the arqs in teh conductor during evacuate or on the destinaiton node. and if the conducrot are we ok with the rpc change that will required like we did in build_and_run_instance | |
| 22:31:30 | sean-k-mooney | dansmith: im done for the day so no rush but ill try and rework that tomorrow | |
| 23:18:19 | gmann | melwitt: dansmith lgtm. I will propose the disable warnings when I cut the releasenotes and doc for new defautls. | |
| 23:18:59 | gmann | i think gibi planning this in ussuri highlights also which also good enough signals | |
| 23:26:05 | openstackgerrit | Ghanshyam Mann proposed openstack/nova master: Pass the actual target in limits policy https://review.opendev.org/715761 | |
| #openstack-nova - 2020-04-01 | |||
| 00:14:32 | openstackgerrit | Merged openstack/nova master: Add test coverage of existing hypervisors policies https://review.opendev.org/715029 | |
| 00:18:58 | brinzhang_ | gibi, dansmith: I have reviewed your talking, and saw comment in the latest patch, I will update the update_volume_attachment_v285 to a new schema as raw from gmann suggestion | |
| 00:24:49 | brinzhang_ | dansmith: I will update the comments left by gmann to the latest patch. | |
| 00:25:35 | brinzhang_ | dansmith: I will add you to the core-author, thanks | |
| 00:27:10 | lbragstad_ | we decided to OR deprecated policies with the new defaults so that we could allow for a smoother migration to the new policy enforcement concept | |
| 00:28:41 | lbragstad | we felt compelled to add deprecation warnings so that operators would know if/when a user's permissions adhered to the old policy concepts, and not the new way | |
| 00:30:14 | lbragstad | ideally, the operator would take that as an action to either adjust the users permissions to fit the new concept (oh, they're an operator they need the 'admin' role on the system) or evaluate if that user really needed that authorization | |
| 00:32:24 | lbragstad | but, i can totally empathize with warning overload and i can understand how that might teach users the wrong thing by being overly noisy | |
| 00:38:35 | lbragstad | maybe we could introduce a simpler way of opting into the new concept, instead of telling people to write policies back into a file for warnings to go away (only to remove them later if they're fine with the new defaults) | |
| 00:44:03 | gmann | lbragstad: yeah, i am thinking to add warning disable flag per rule in DocumentedRuleDefault so that we can 1. skip the common default change policy to log warning 2. but at same time rule name change keep adding warning. | |
| 00:44:42 | gmann | i mean instead of global flag, support per rule flag and policy implementor decide which rule they want to add warning. | |
| 00:45:30 | gmann | because if rule name is changed and rule is in policy file we should add warning. | |
| 00:45:45 | gmann | that is case in nova case when granularity is added to adopt the new defaults | |
| 01:01:14 | openstackgerrit | Brin Zhang proposed openstack/nova master: Allow PUT volume attachments API to modify delete_on_termination https://review.opendev.org/693828 | |
| 01:01:52 | brinzhang_ | dansmith: gmann: https://review.opendev.org/#/c/693828/ addressed your comments | |
| 01:06:09 | lbragstad | gmann warnings for policy removal/renames make sense | |
| 01:07:02 | lbragstad | that functionality could also be incorporated into another oslo.policy cli tool (if it isn't already?) | |
| 01:14:19 | gmann | lbragstad: or we can suppress warning based on global flags (new flag) but policy renaming always log warning . keeping this one always and rest all based on flag - https://github.com/openstack/oslo.policy/blob/c483dee1f306b698448fbee9169038159209e916/oslo_policy/policy.py#L670 | |
| 01:14:55 | gmann | brinzhang_: thanks i will check | |
| 01:15:26 | lbragstad | gmann yeah - that makes sense | |
| 01:15:41 | brinzhang_ | gmann: what do you thinf of this change? https://review.opendev.org/#/c/693828/23/nova/api/openstack/compute/volumes.py@495 | |
| 01:15:55 | lbragstad | gmann how much policy work does nova have left? | |
| 01:16:18 | lbragstad | the reviews have been on my list, but i clearly haven't gotten around to them | |
| 01:17:24 | brinzhang_ | update an attachments not swap, we will update the attachment firstly, then check the attachment_id !=volumeId, then do swap, that will all admin_or_owner firstly, and then do admin policy | |
| 01:17:25 | gmann | lbragstad: i think ~18 policy are left | |
| 01:17:34 | lbragstad | total?! | |
| 01:17:45 | gmann | counting :) | |
| 01:18:06 | lbragstad | wow - nice work, gmann | |
| 01:18:31 | gmann | lbragstad: around 50 | |
| 01:18:50 | brinzhang_ | dansmith, gmann: this looks so strange, is it? | |
| 01:19:33 | gmann | lbragstad: let me propose warning thing in this week and you can check if that make sense especially from other project point of view | |
| 01:20:10 | lbragstad | gmann ok - do we want to think about a different way for people to opt into the new concept, though? | |
| 01:20:49 | gmann | lbragstad: did not get ? | |
| 01:21:50 | lbragstad | today, if i upgrade to Ussuri and nova has all these new defaults, i need to override all of them to opt into the new way of doing things | |
| 01:21:57 | gmann | brinzhang_: you mean we are going to 403 late if non-admin try to update_swap ? | |
| 01:22:30 | lbragstad | gmann do we want to make that easier by not having operators write policies into a file | |
| 01:22:37 | brinzhang_ | gmann: 403? | |
| 01:22:50 | lbragstad | but still allow them to opt into the new system | |
| 01:23:12 | lbragstad | like what dansmith and melwitt were saying earlier | |
| 01:23:32 | gmann | lbragstad: ohk. they do not need to write policy. if they enable scope and add new defaults in their system then they adopt the new policy | |
| 01:23:55 | gmann | or you are saying new things only and no old things supported ? | |
| 01:24:37 | lbragstad | let's say i want to adopt this right away and i don't want policies OR'd | |
| 01:24:53 | gmann | brinzhang_: i mean policy unauthorize error for non-admin doing update + swap. | |
| 01:25:08 | lbragstad | that requires me (as the operator) to go and update my nova policy file to override all the policies with their new defaults, right? | |
| 01:25:09 | brinzhang_ | gmann: yes | |
| 01:25:36 | gmann | yeah if they want to stop old tokens/role then yes they need to override | |
| 01:25:40 | brinzhang_ | gmann: maybe we should move L494 to L497, what do you think? | |
| 01:25:49 | gmann | but in nova case it is easy as we are controlling those via common base rules | |
| 01:26:46 | gmann | lbragstad: we are testing that simulation also- basically operator needs to do this - https://github.com/openstack/nova/blob/c31de903dd73de1c0a41bcfe4c867daa6eb10f85/nova/tests/unit/policies/base.py#L103 | |
| 01:28:03 | gmann | brinzhang_: we cannot do that becasue swap operation is async and might not be completed before we update deleet flag. | |
| 01:28:40 | gmann | brinzhang_: we can move the both policy checks in starting. i mean check id !=volume_id then check swap policy also. | |
| 01:30:22 | lbragstad | gmann i wonder if we could come up with a way to opt in without having to write to the policy file (after reading the scrollback, i can see how that might be misleading to operators if we've told them not to do that in the past) | |
| 01:30:24 | gmann | brinzhang_: anyways that part we can do in https://review.opendev.org/#/c/711194/10 | |
| 01:31:42 | gmann | lbragstad: i see your point. how about doing it via enforce_scope ? if it is true along with scope enable we remove the ORing old rules ? | |
| 01:32:10 | lbragstad | gmann yeah - i think that would work | |
| 01:32:34 | gmann | basically enforce_scope flag is new policy though we kept new defaults roles separate but still someone want to adopt both at same time | |
| 01:33:15 | brinzhang_ | gmann: I have an doubt, if the swaped volume is a new volume, maybe we will get bdm raised exception.VolumeBDMNotFound, that we dont do _update_volume_regular(), and cannot completed the swap operation, right? | |
| 01:33:22 | lbragstad | we originally intended that to be an all-or-nothing option | |
| 01:33:36 | gmann | lbragstad: for existing project who have already exposed reno or doc which is keystone only :). does that flag scope change is bad for user ? | |
| 01:33:54 | lbragstad | so - operators would only set it once they 1.) updated their policies (which might not be needed anymore) and 2.) audited their users to make sure the ones that need system scope have it | |
| 01:34:07 | gmann | yeah. | |
| 01:34:32 | lbragstad | gmann i need to think through it a bit more | |
| 01:35:37 | lbragstad | i doubt anyone is running enforce_scope in production, yet? | |
| 01:36:59 | gmann | ok, in that way it make sense to make it all-or-nothing. | |
| 01:38:19 | gmann | because new flag controlling scope and defaults roles separately does not make sense. | |
| 01:38:40 | lbragstad | yeah - you mean using the old policies and setting enforce scope to True? | |
| 01:38:41 | gmann | and overahead for us also to maintain the old deprecated things | |
| 01:38:59 | gmann | yeah. | |
| 01:39:12 | lbragstad | i agree - but i'm not an operator :) | |
| 01:39:30 | gmann | :) me too, johnthetubaguy can answer this better | |
| 01:41:02 | gmann | that is how we did the tests for new system. scope + new rules - https://github.com/openstack/nova/blob/c31de903dd73de1c0a41bcfe4c867daa6eb10f85/nova/tests/unit/policies/test_deferred_delete.py#L121 | |
| 01:42:22 | gmann | that was what johnthetubaguy idea to see how operator will use the new policy but he can tell if any operator want to do scope + old rule | |
| 01:42:36 | lbragstad | gmann ah - yeah, we took a similar approach | |
| 01:42:39 | lbragstad | in keystone | |
| 01:42:58 | gmann | nice | |
| 01:43:01 | lbragstad | using a config option would make that easier though | |
| 01:43:06 | lbragstad | and cleaner | |
| 01:48:59 | gmann | brinzhang_: so swap-only is all ok. and if anyone requesting update + swap and update fail (say VolumeBDMNotFound) then it is ok to fail before doing swap. I mean request is for two operations so we either do both with success otherwise fail and failing before swap is good. | |
| 01:49:26 | gmann | otherwise we end up doing multi-success things which is bad | |
| 01:51:16 | brinzhang_ | gmann: I know, but there is an issue, when microversion >=2.85, if the volumeId != id in the request body, I think this will greatly affect the functional requirements of the swap volume. | |
| 01:52:20 | brinzhang_ | if this is only a policy check issue, it is easy to resolve, but it is not | |
| 01:53:43 | gmann | brinzhang_: ohk, you mean user do not have swap-only options with >2.85 ? | |
| 01:54:36 | brinzhang_ | gmann: I will leave comments of this concern, wait for gibi, and dansmith with together consider of that. | |
| 01:55:09 | gmann | brinzhang_: i think we are ok here as swap_volume also does get bdm - https://github.com/openstack/nova/blob/c31de903dd73de1c0a41bcfe4c867daa6eb10f85/nova/compute/api.py#L4777 | |
| 01:55:20 | brinzhang_ | gmann: no, I mean, if the user want to do swap volume | |
| 01:55:21 | gmann | so there is extra things before swap | |
| 01:56:06 | brinzhang_ | gmann, can you refer you link to https://opendev.org/openstack/nova/src/branch/master/nova/compute/api.py? | |
| 01:56:20 | brinzhang_ | github is so slowly for me | |
| 01:56:49 | gmann | brinzhang_: _update_volume_regular does not do any extra things what swap_volume does | |
| 01:57:36 | gmann | brinzhang_: https://opendev.org/openstack/nova/src/commit/c31de903dd73de1c0a41bcfe4c867daa6eb10f85/nova/compute/api.py#L4777 | |
| 01:57:45 | brinzhang_ | gmann: my concern is bdm = objects.BlockDeviceMapping.get_by_volume_and_instance(context, volume_id, instance.uuid), I am afarid this code failed | |
| 01:58:11 | gmann | brinzhang_: but that is what swap_volume also does. above link | |
| 02:02:51 | brinzhang_ | gmann: the old volume's bdm, will be copied to the new volume's bdm? | |
| 02:07:15 | brinzhang_ | gmann: do you think we can get this exception? https://opendev.org/openstack/nova/src/branch/master/nova/objects/block_device.py#L281 | |
| 02:09:55 | gmann | brinzhang_: it can but it can be raised from swap_volume also so updating failing on this first for update+swap request is ok. | |