| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2020-08-27 | |||
| 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 | |
| 18:15:52 | dansmith | right | |
| 18:16:49 | gmann | +1. CONF.policy_file's location in [opt_default, set_default ] as nova will do set_default to change the default. | |
| 18:18:59 | dansmith | ack | |
| 18:39:06 | openstackgerrit | Lee Yarwood proposed openstack/nova stable/stein: Improve CinderFixtureNewAttachFlow https://review.opendev.org/748509 | |
| 18:39:07 | openstackgerrit | Lee Yarwood proposed openstack/nova stable/stein: Robustify attachment tracking in CinderFixtureNewAttachFlow https://review.opendev.org/748510 | |
| 18:39:08 | openstackgerrit | Lee Yarwood proposed openstack/nova stable/stein: compute: Use source_bdms to reset attachment_ids during LM rollback https://review.opendev.org/748511 | |
| 18:39:09 | openstackgerrit | Lee Yarwood proposed openstack/nova stable/stein: compute: refactor volume bdm rollback error handling https://review.opendev.org/748512 | |
| 18:39:11 | openstack | bug 1889108 in OpenStack Compute (nova) train "failures during driver.pre_live_migration remove source attachments during rollback" [Undecided,In progress] https://launchpad.net/bugs/1889108 - Assigned to Lee Yarwood (lyarwood) | |
| 18:39:11 | openstackgerrit | Lee Yarwood proposed openstack/nova stable/stein: WIP Add regression tests for bug #1889108 https://review.opendev.org/748513 | |
| 18:39:12 | openstackgerrit | Lee Yarwood proposed openstack/nova stable/stein: compute: Don't delete the original attachment during pre LM rollback https://review.opendev.org/748514 | |
| 18:54:30 | lyarwood | melwitt: https://review.opendev.org/#/c/742415/ - can you look at this if you get time, it's holding up the CVE on stable/train | |