| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2020-08-27 | |||
| 17:12:20 | gmann | https://review.opendev.org/#/c/748059/1/nova/cmd/status.py | |
| 17:13:00 | sean-k-mooney | so i would propose the following. check for the existine of both policy.yaml and policy.json. error if you find both and warn if you finde policy.json but use either policy.json or policy.yaml if you find one but not the other | |
| 17:14:23 | gmann | all except 'error if you find both' s part of this oslo spec. | |
| 17:14:52 | sean-k-mooney | right didn oslo say we should treat that as an error | |
| 17:14:54 | gmann | 'existine of both policy.yaml and policy.json' is not possible as file can be detected via config potion only | |
| 17:15:05 | gmann | which one, finding both? | |
| 17:15:11 | sean-k-mooney | yes | |
| 17:15:27 | sean-k-mooney | didnt they call that out as an error as we dont know which policy to follow | |
| 17:15:30 | gmann | we cannot find both as we can only look for conf.oslo_policy.policy_file | |
| 17:15:34 | sean-k-mooney | so we shoudl hard error | |
| 17:15:54 | dansmith | sean-k-mooney: he's saying that there is only one option and we will hard fail in both cases, | |
| 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 | 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 | |