Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2020-08-27
16:58:25 gmann yeah. i hope at least with upgrade checks, changing the default value would not break operators but dansmith or johnthetubaguy can corret me
16:58:48 gmann gibi: yeah that is the plan. i think we can merge the olso change by tomorrow
16:59:06 gibi cool, let's get back to this tomorrow or next week
16:59:12 gibi I have to drop now
16:59:14 gmann yup
16:59:51 gibi o/
17:00:23 dansmith gmann: json still works, right?
17:00:57 gmann dansmith: yeah it will work but default value of policy_file will change. https://review.opendev.org/#/c/748059/1/nova/policy.py
17:01:02 dansmith yaml is the default for the generation, right? I wouldn't think any problem on upgrade that wouldn't happen if we overwrote their file with fresh jsson anyway
17:01:08 dansmith oh
17:01:24 gmann i mean is anyone has policy.json then it has to be converted to policy,yaml
17:01:44 dansmith wait, they *have* to convert to yaml?
17:03:00 gmann yeah because of extension name change in default value. json formatted still work though
17:03:16 dansmith you mean if they take the default
17:03:18 dansmith file name
17:03:21 gmann yes
17:03:23 sean-k-mooney you need to use yaml to use some of the feautures
17:03:35 dansmith sean-k-mooney: yeah that's not what we're talking about
17:03:42 sean-k-mooney ah ok
17:04:01 gmann converting json formatted to yaml is separate thing and they can do in their own time and we provide tool also for that.
17:04:12 dansmith gmann: ah so, wait, we don't have a policy file conf, is that right?
17:04:27 gmann we do have
17:04:47 sean-k-mooney gmann: i assume the tool just does yaml.dump(json.loads(file))
17:04:52 sean-k-mooney or similar
17:04:53 dansmith gmann: where is it?
17:05:05 sean-k-mooney e.g. parse the profile then dump it in the other format
17:05:22 gmann sean-k-mooney: more than that - https://review.opendev.org/#/c/748055/4//COMMIT_MSG@17
17:05:23 dansmith sean-k-mooney: we're talking about breaking existing people, not generation right now
17:05:55 sean-k-mooney we kind of need to do this in a falback way
17:06:09 sean-k-mooney check for policy.json then check for policy.yaml right
17:06:18 sean-k-mooney and fail if we find both
17:06:36 dansmith sean-k-mooney: exactly, that's what I'm getting at
17:06:38 gmann dansmith: this is per service option - https://github.com/openstack/oslo.policy/blob/a626ad12fe5a3abd49d70e3e5b95589d279ab578/oslo_policy/opts.py#L48
17:06:41 dansmith prefer yaml, but fall back to json
17:07:08 sean-k-mooney just prefer or hard error if both
17:07:08 dansmith gmann: ack, so where is that DEFAULT_POLICY_FILE thing used?
17:07:21 dansmith oh jeez, I see
17:07:26 dansmith right under it, sorry :P
17:07:34 dansmith but yeah, we *have* to fall back IMHO
17:08:01 sean-k-mooney to fultile on upgrade we dont change config right
17:08:16 sean-k-mooney that part of the grenade theory of upgrades workflow
17:08:18 gmann dansmith: this will change the default for nova - https://github.com/openstack/oslo.policy/blob/a626ad12fe5a3abd49d70e3e5b95589d279ab578/oslo_policy/opts.py#L121
17:08:21 dansmith sean-k-mooney: right
17:08:37 dansmith gmann: right, I see it now
17:08:56 dansmith gmann: but we can't just change the default and break everyone, even if the workaround is to change the config or copy the file, IMHO
17:09:25 gmann dansmith: but at some point we have to do that right? may be upgrade check in this cycle with warning and next cycle change default ?
17:09:30 dansmith that passes grenade because we don't override any policy I imagine, but if we did it'd be broken
17:09:51 dansmith gmann: yep, we can do that, warn now switch next
17:10:40 gmann ok. and with new tools operator have time for converting to yaml file too other than default one.
17:11:13 sean-k-mooney we shoudl add a nova status check too
17:11:40 sean-k-mooney because tehy should convert on victoria before upgrading idealy
17:11:45 gmann yeah its there but with failure which i can convert to warning for this cycle
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

Earlier   Later