| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2020-08-27 | |||
| 17:16:02 | sean-k-mooney | we can do import os; os.path.exist(filename) | |
| 17:16:03 | dansmith | which means we have to warn this time before we can switch the default | |
| 17:16:16 | gmann | yeah | |
| 17:16:26 | dansmith | it would be better if we could make nova prefer one over the other, and I think it's possible to do that, but not required | |
| 17:16:57 | dansmith | gmann: nova could check for the file and if not existing, parse the config opt, check for the .json equivalent and reset the default right? | |
| 17:17:38 | sean-k-mooney | right instead of hardcoding DEFAULT_POLICY_FILE = 'policy.yaml' | |
| 17:17:46 | dansmith | like if CONF.policy_file == DEFAULT_POLICY_FILE and os.path.exists(CONF.policy_file.replace('yaml', 'json')): cfg.set_defaults(...) | |
| 17:18:00 | gmann | first one is not right thing to do as any existing file should be consider as policy file | |
| 17:18:08 | dansmith | sean-k-mooney: no, we have to hardcode it | |
| 17:18:09 | dansmith | but we can be flexible in our use of it | |
| 17:18:25 | sean-k-mooney | ya what you sugges seam sane to me too | |
| 17:18:40 | sean-k-mooney | its more or less what i ment | |
| 17:18:59 | gmann | you mean just change the extension name? | |
| 17:19:04 | sean-k-mooney | yep | |
| 17:19:09 | sean-k-mooney | check if the file exists | |
| 17:19:15 | sean-k-mooney | if not change extnetion | |
| 17:19:22 | sean-k-mooney | and check again if found use it | |
| 17:19:36 | dansmith | gmann: yeah my str.replace was just an example, but basically if the config is not overridden, and the file does not exist but the s/yaml/json/ one does, change the default | |
| 17:20:49 | gmann | i think policy.json -> policy.yaml should be converted via new tool which acutally convert the JSON formatted to YAM L formatted. | |
| 17:21:04 | gmann | otherwise policy.yaml in JSON format is confusing | |
| 17:21:14 | dansmith | they should, but we can't make them | |
| 17:21:27 | dansmith | putting json in a .yaml file is not a good idea, nor what we're suggesting | |
| 17:21:32 | sean-k-mooney | well this is not assumign yaml in an .json | |
| 17:22:13 | sean-k-mooney | we are assumign that are on ussuri upgradeing to victoria | |
| 17:22:23 | sean-k-mooney | so they have a policy.json with json in it now | |
| 17:22:39 | gmann | i was thinking warn now give them time to properly convert the json to yaml and in next cycle or so we just change default | |
| 17:22:43 | sean-k-mooney | and we want to make sure we can load that in victoria without them modifying it in any way | |
| 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 | openstackgerrit | Lee Yarwood proposed openstack/nova stable/train: WIP Add regression tests for bug #1889108 https://review.opendev.org/748494 | |
| 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:07 | openstackgerrit | Lee Yarwood proposed openstack/nova stable/train: compute: refactor volume bdm rollback error handling https://review.opendev.org/748495 | |
| 17:32:07 | gmann | DEFAULT_POLICY_FILE will be policy.yaml right | |
| 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 | dansmith | gmann: only do this if they haven't touched their config file | |
| 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 | |