| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2020-03-31 | |||
| 21:41:21 | bnemec | That's fair, we've actually shut off the policy deprecations in some cli tools and unit tests. | |
| 21:41:24 | dansmith | *that* is not okay | |
| 21:42:23 | dansmith | if we have no way to distinguish then we need to squelch the warning until we've fixed all our defaults | |
| 21:42:35 | gmann | i think waning should be added once during api service only | |
| 21:42:45 | dansmith | and, we shouldn't be making those warnings on nova-manage invocations | |
| 21:42:47 | dansmith | gmann: not for our own defaults | |
| 21:43:03 | dansmith | if we're telling the user something is wrong and the thing that is wrong is our default, we're teaching them to ignore our deprecation warnings | |
| 21:43:48 | bnemec | But the user needs to take action. They _shouldn't_ ignore these messages. | |
| 21:44:13 | gmann | own default but still operator rely on those and new default can change the behaviour if they have new roles like read only etc | |
| 21:44:29 | dansmith | gmann: that's what release notes are for | |
| 21:44:30 | gmann | scope is disabled by default so no issue there | |
| 21:44:48 | dansmith | logging that once per startup is totally unreasonable, IMHO | |
| 21:45:14 | owalsh | could nova status upgrade check validate any policy overrides if they exist? | |
| 21:45:38 | dansmith | also that | |
| 21:45:42 | bnemec | If a policy is overridden then you don't get the message. | |
| 21:45:51 | dansmith | nova-status is supposed to be a dynamic release note checker | |
| 21:46:00 | gmann | for this case, i agree on that because it is for every rule and lot of warnings | |
| 21:46:49 | bnemec | cmurphy did have a patch up to further consolidate the deprecation messages, but I feel like there were other concerns with it. | |
| 21:48:14 | gmann | yeah, if rule is override there is no warning for default change. it warn only if rule name change | |
| 21:48:46 | dansmith | we tell people not to override every rule, and almost nobody would override everything, | |
| 21:48:47 | melwitt | gmann: what is happening here is that the defaults are being deprecated and new defaults will be activated in one or two cycles right? maybe a Upgrade release note saying "the default policies are going to change in the W release, please review them" is good enough? | |
| 21:48:52 | dansmith | which means everyone will receive that warning | |
| 21:49:04 | dansmith | melwitt: ++ | |
| 21:49:31 | dansmith | this is precisely what nova-status and renos are for | |
| 21:50:09 | gmann | yeah new defaults are not enforced by default. | |
| 21:50:13 | bnemec | The new defaults are active already. They're just OR'd with the old defaults (if an explicit override is not set) to make sure that the rules are at least as permissive as the old rule so nobody is broken without notice. | |
| 21:50:25 | dansmith | I have to run to a thing, | |
| 21:50:54 | dansmith | but tldr of my opinion is.. there *has* to be a way to make this message go away.. I'd prefer reno/status. If not, then there has to be some way to say "OKAY I GOT IT" | |
| 21:50:55 | melwitt | gmann: and include instructions on how to set enforce_scope = True to see and try the new defaults? also instructions on how to dump the new defaults to review | |
| 21:50:58 | gmann | dansmith: melwitt reno is planned at the end. | |
| 21:51:18 | bnemec | There is. You explicitly set the new rule in your policy file and the deprecation warning goes away. | |
| 21:51:30 | dansmith | bnemec: but we've spent years telling people NOT to do that | |
| 21:51:39 | gmann | bnemec: but that is not expected | |
| 21:51:47 | gmann | yeah what dansmith mentioned | |
| 21:51:48 | melwitt | yeah ... that seems backwards. shouldn't default mean an empty policy file? | |
| 21:51:54 | dansmith | yes | |
| 21:51:55 | bnemec | It's essentially the operator saying, yes, we've looked at this change and it's fine. | |
| 21:52:04 | melwitt | that's what we made a big deal about policy in code back when that was added | |
| 21:52:11 | gmann | true | |
| 21:52:13 | dansmith | copying defaults into the policy file to shut it the eff up moves us backwards many years | |
| 21:52:39 | melwitt | not sure I'd want to confuse users by saying "ok now go set every single rule in your policy file" that seems counterintuitive | |
| 21:53:08 | melwitt | I would imagine their takeaway from that is we've removed policy-in-code and we're back to the old way | |
| 21:53:51 | dansmith | right, because they won't go to the trouble of understanding why, | |
| 21:54:02 | dansmith | they will just hear that is the solution to make it stop warning | |
| 21:54:03 | bnemec | I will say I don't think the policy deprecation mechanism was designed for deprecating the entire policy file at once, like we're doing now. | |
| 21:54:42 | melwitt | yes, I agree this is not the usual thing | |
| 21:54:50 | gmann | yeah, in this case where all defaults are changing warning is too much. | |
| 21:55:00 | bnemec | Also, the redundant rule tool was designed to let them clean up their policy files once the deprecation process is over. | |
| 21:55:00 | gmann | for one or two policy change and name etc then it is fine | |
| 21:55:13 | bnemec | It will tell them that they have rules in their policy file that don't need to be there anymore. | |
| 21:55:36 | gmann | yup that wanring make sense. | |
| 21:56:12 | bnemec | I wonder if we should make that a runtime check instead of a separate tool. | |
| 21:56:13 | gmann | if rule name changing | |
| 21:57:14 | bnemec | He knows way more about why things work the way they do than I do. | |
| 22:04:09 | melwitt | taking some steps back, I feel like we have changed the defaults for a reason right, that we think they are better defaults. when I say new defaults become "active" I mean when they become the only default, when we're no longer OR'ing them. I feel like maybe we don't need to sound an alarm about it, but rather let the notice in the release notes explain the upcoming change and how to dump the new defaults to review. that is our normal | |
| 22:04:09 | melwitt | way to communicate upcoming changes to our operators | |
| 22:22:09 | openstackgerrit | melanie witt proposed openstack/nova master: Add info about affinity requests to the troubleshooting doc https://review.opendev.org/715092 | |
| 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 | |