| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2020-08-27 | |||
| 17:23:12 | dansmith | gmann: we can do that too, but we're saying it would be better if we can make it default to yaml now, but fall back to json if need b | |
| 17:23:14 | dansmith | *be | |
| 17:24:17 | gmann | and remove fallback in next cycle? | |
| 17:24:25 | sean-k-mooney | yep | |
| 17:24:32 | dansmith | potentially..some future cycle | |
| 17:24:37 | sean-k-mooney | basicaly treat this as a deprecation | |
| 17:24:48 | sean-k-mooney | we normally remove in a cycle or two | |
| 17:24:51 | gmann | ok. yeah if at some point we can remove fallback then it is fine | |
| 17:24:59 | dansmith | gmann: where is policy.init called from the actual code? | |
| 17:25:02 | dansmith | I only see it from cmd/status | |
| 17:25:38 | gmann | from manage also and at the first call of context.can( | |
| 17:25:51 | sean-k-mooney | you mean in the api wsgi app | |
| 17:25:52 | dansmith | ah, context.can runs it if not run yet? | |
| 17:26:54 | gmann | yeah | |
| 17:26:57 | gmann | https://github.com/openstack/nova/blob/f521f4dbace0e35bedd089369da6f6969da5ca32/nova/policy.py#L168 | |
| 17:27:12 | gmann | which is called from context.can() | |
| 17:27:47 | sean-k-mooney | i was expecting to see it in here https://github.com/openstack/nova/blob/master/nova/api/wsgi.py but ok | |
| 17:28:35 | dansmith | gmann: we're suggesting this: https://pastebin.com/WASufpac | |
| 17:28:44 | dansmith | but less crappy and working.. I just hacked that up like pseudocode | |
| 17:29:37 | sean-k-mooney | ya something like that | |
| 17:29:38 | dansmith | make sense? | |
| 17:31:07 | gmann | yeah but we should only replace replace('yaml', 'json') if default file, 'policy.json' and not any other name | |
| 17:31:37 | sean-k-mooney | thats why there is CONF.policy_file == DEFAULT_POLICY_FILE | |
| 17:31:38 | gmann | if CONF.policy_file == OLD_DEFAULT | |
| 17:32:01 | dansmith | gmann: no, | |
| 17:32:06 | openstack | bug 1889108 in OpenStack Compute (nova) train "failures during driver.pre_live_migration remove source attachments during rollback" [Undecided,New] https://launchpad.net/bugs/1889108 | |
| 17:32:06 | openstackgerrit | Lee Yarwood proposed openstack/nova stable/train: WIP Add regression tests for bug #1889108 https://review.opendev.org/748494 | |
| 17:32:07 | dansmith | gmann: only do this if they haven't touched their config file | |
| 17:32:07 | openstackgerrit | Lee Yarwood proposed openstack/nova stable/train: compute: Don't delete the original attachment during pre LM rollback https://review.opendev.org/748496 | |
| 17:32:07 | gmann | DEFAULT_POLICY_FILE will be policy.yaml right | |
| 17:32:07 | openstackgerrit | Lee Yarwood proposed openstack/nova stable/train: compute: refactor volume bdm rollback error handling https://review.opendev.org/748495 | |
| 17:32:11 | sean-k-mooney | we only do hte fall back if the value is the default | |
| 17:32:14 | dansmith | right | |
| 17:32:32 | sean-k-mooney | dansmith: actully isnt there a way to check if a config value has been set or is the default | |
| 17:32:37 | dansmith | because if they're relying on the default file being policy.json, only THEN do we fall back to the old default | |
| 17:32:53 | dansmith | sean-k-mooney: yes, I'm just doing the easy thing without having to look that up :) | |
| 17:32:55 | gmann | else part change overridden policy file also | |
| 17:33:07 | gmann | xyv_policy_nova.json | |
| 17:33:13 | sean-k-mooney | ok cool i think i have used it before | |
| 17:33:23 | dansmith | gmann: if they have overridden it in the config, then we take what they have set | |
| 17:33:39 | dansmith | gmann: we only need to look for the old file if they haven't, but are assuming the default is the old thing | |
| 17:35:40 | gmann | that is what i am saying. which need to change else: -> elif CONF.policy_file == OLD_DEFAULT_POLICY_FILE | |
| 17:36:08 | dansmith | NO | |
| 17:36:09 | dansmith | :) | |
| 17:36:32 | dansmith | we don't need to do anything if they have said their CONF.policy_file to anything, including the old default | |
| 17:36:39 | dansmith | because of L13 | |
| 17:37:27 | dansmith | wait, maybe I'm missing which else you're talking about.. gmann maybe update the pastebin with what you mean? | |
| 17:37:37 | gmann | if they set CONF.policy_file=nova_policy.json | |
| 17:37:43 | gmann | ok | |
| 17:37:58 | sean-k-mooney | if they set it at all we just use what they set | |
| 17:38:07 | dansmith | sean-k-mooney: right | |
| 17:38:46 | sean-k-mooney | if they dont set it. we check the new default and if the file does not exists check the old default. | |
| 17:39:00 | dansmith | we never need to check to see if the conf is set to the old default, | |
| 17:39:01 | sean-k-mooney | that is what dansmith's pastebin does | |
| 17:39:14 | sean-k-mooney | correct | |
| 17:39:16 | dansmith | we only need to do something different if the conf is unchanged, i.e. set to the NEW default | |
| 17:39:38 | dansmith | and knowing if it's overridden is better than knowing if it's set to the default | |
| 17:39:48 | dansmith | because if it's just set to the default in their config, we should *only* honor that file | |
| 17:40:13 | gmann | https://pastebin.com/GN7mz5tE | |
| 17:40:30 | dansmith | gmann: no, that's not what we want | |
| 17:41:15 | dansmith | if it's set to the old value, then we just use the old value, end of story | |
| 17:41:35 | dansmith | if it's not overridden, then we need to check the new value, and if not present, check the old value..that's it | |
| 17:41:39 | sean-k-mooney | gmann: that woudl expand to 'policy.json'.replace('yaml', 'json') on line 14 | |
| 17:42:08 | dansmith | right | |
| 17:44:04 | gmann | in case of overridden to any new-name.json also else block is executed right? | |
| 17:44:30 | dansmith | if the value is set to *anything* other than the new default in config, then we just honor that | |
| 17:45:10 | dansmith | the ONLY thing we need to handle is the case where the value is not overridden and the operator or packager assumes the filename is the old value | |
| 17:45:22 | dansmith | i.e. "I don't need to set it in my config because I use the default filename" | |
| 17:45:33 | sean-k-mooney | gmann: did you miss the early return on line 11 https://pastebin.com/WASufpac | |
| 17:45:43 | dansmith | we break those people if we change the default filename | |
| 17:46:17 | dansmith | we we'd like to change the default filename, but still honor the old one IF-AND-ONLY-IF they're relying on the default and have not specified a value in their config file | |
| 17:46:50 | sean-k-mooney | im going to go have food o/ | |
| 17:46:51 | gmann | yeah but i was thinking to change the existing default policy file instead if cONF default based on what exist | |
| 17:47:25 | dansmith | gmann: I don't understand that last statement | |
| 17:51:28 | gmann | overall goal i was thinking is to warn with upgrade checks and change default to yaml so that oslo_policy can remove the json format support at some point. | |
| 17:51:54 | gmann | but having fallback is kind of same situation with what we have currently. | |
| 17:52:08 | dansmith | we were discussion two options and maybe we confused you, so let me reiterate the two options: | |
| 17:52:23 | dansmith | 1. Leave the default as json now, warn in nova status, change default later and break people that didn't notice | |
| 17:52:33 | gmann | yeah this one | |
| 17:52:54 | dansmith | 2. Switch default to yaml now, but fall back to json file for a release or two and warn in nova-status | |
| 17:53:29 | dansmith | if we do #2, which is my pastebin, then we can remove json more aggressively because we have been defaulting to yaml for one more release (i.e. default to yaml starts in V) | |
| 17:53:48 | dansmith | sean-k-mooney and I prefer #2, more aggressive, without breaking people, so we can say json goes away sooner | |
| 17:54:04 | gmann | but operator would not notice the default change as fallback is there. | |
| 17:54:10 | dansmith | if we do #1, then we will want to wait longer | |
| 17:54:12 | gmann | unless we fail upgrade checks | |
| 17:54:30 | dansmith | the upgrade checks can be the same in both cases | |
| 17:54:32 | gmann | i was thinking in option1, fail upgrade checks in W | |
| 17:55:02 | dansmith | gmann: we can fail upgrade checks in V if we do #2, we just need to fall back to the old filename because not everyone runs upgrade checks | |
| 17:55:53 | gmann | humm, you mean fail upgrade check but also fallback? does not that make upgrade checks a false info? | |
| 17:56:23 | gmann | upgrade checks should fail at the same time we start rejecting the json file in both otion | |
| 17:56:25 | gmann | option | |
| 17:56:56 | dansmith | the upgrade checks are informational anyway.. we have "warning" and "failure" in upgrade checks, we can warn now and fail on the release where we remove it in either case | |
| 17:57:34 | gmann | yeah fail later seems more correct and what our code will behave | |
| 17:57:35 | dansmith | if we do #2, then we get all new users to yaml right now, which means we can dump json support sooner | |
| 17:57:47 | dansmith | that's what we're going for | |
| 17:57:54 | dansmith | it doesn't change the signaling now at all | |
| 17:58:02 | dansmith | it just means we can get all new users on yaml right away, | |
| 17:58:05 | gmann | yeah that is good i think. | |
| 17:58:15 | dansmith | instead of new users being on yaml later when everyone else if forced to do the same | |
| 17:58:48 | gmann | yeah. it is better way | |