| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2020-08-27 | |||
| 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 | |
| 18:00:10 | gmann | and this warning in doc still valid for option2 also - https://review.opendev.org/#/c/748059/1/doc/source/configuration/policy-concepts.rst | |
| 18:00:29 | gmann | or we should add default value things also here? | |
| 18:01:03 | dansmith | warning applies to either for sure, I wouldn't add the json fallback here | |
| 18:01:16 | gmann | ok | |
| 18:01:18 | dansmith | I would only mention the fallback in the reno | |
| 18:02:19 | gmann | will update patch with option2 once oslo.policy is released. thanks dansmith sean-k-mooney . | |
| 18:08:15 | dansmith | cool, thanks gmann ! | |
| 18:08:27 | gmann | dansmith: but i will add "elif CONF.policy_file == OLD_POLICY_FILE:" for case - policy.json exist but an old, not supposed to be used file and CONF.policy_file =nova_policy.* means overridden file | |
| 18:09:07 | dansmith | gmann: then I will -1 it :) | |
| 18:09:13 | gmann | so that we do not pick not-supposed-to-use but existing policy.json | |
| 18:09:15 | gmann | :) | |
| 18:09:30 | dansmith | if CONF.policy_file is set to anything, then we should do what they ask | |
| 18:10:04 | dansmith | but go ahead and propose what you want and let sean-k-mooney and I have a chance to -1 with discussion and we can do it there, which might be easier since we can reference actual code and lines | |
| 18:10:59 | gmann | but where you are checking override file? | |
| 18:11:04 | gmann | ok | |
| 18:13:12 | dansmith | bnemec: apologize for asking this again, but.. there's some oslo_config method for determining if the conf option has been set or not right? | |
| 18:13:59 | mriedem | set vs the value coming from a default? | |
| 18:14:00 | dansmith | bnemec: is it just opt.value == opt.default ? | |
| 18:14:06 | dansmith | mriedem: yeah | |
| 18:14:12 | mriedem | there is some location thing i thought | |
| 18:14:19 | mriedem | which says where the value comes from | |
| 18:14:48 | mriedem | https://github.com/openstack/oslo.config/blob/master/oslo_config/cfg.py#L57 | |
| 18:14:58 | bnemec | Yes | |
| 18:15:06 | dansmith | okay, perfect | |
| 18:15:34 | bnemec | Docs are here: https://docs.openstack.org/oslo.config/latest/reference/locations.html | |
| 18:15:41 | dansmith | gmann: for sean-k-mooney and my desired behavior, the *only* thing you need to check is if CONF.policy_file's location == opt_default :) | |
| 18:15:43 | gmann | +1, that can save any accidental pick of file if both extensions exist | |