| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2020-08-27 | |||
| 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 | openstackgerrit | Lee Yarwood proposed openstack/nova stable/stein: WIP Add regression tests for bug #1889108 https://review.opendev.org/748513 | |
| 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: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 | |
| 18:54:49 | lyarwood | melwitt: well the release at least | |
| 19:03:29 | ganso | hello Nova folks! I just logged bug: https://bugs.launchpad.net/nova/+bug/1893263 after reproducing the issue on master. I am planning to work on the fix myself, however, I am curious to know if anyone here knows the reason why plug_tap() needs to run, as if the instance is ever being rebooted, the tap device goes away and is re-created by libvirt instead (therefore plug_tap() seems to have never needed to run at all) | |
| 19:03:29 | openstack | Launchpad bug 1893263 in OpenStack Compute (nova) "Cannot create instance with multiqueue image and vif_type=tap (calico)" [Undecided,New] | |
| 19:04:08 | ganso | because one of the possible fixes is just to remove plug_tap() and be happy | |
| 19:07:49 | openstackgerrit | Lee Yarwood proposed openstack/nova stable/train: Add regression tests for bug #1889108 https://review.opendev.org/748494 | |
| 19:07:49 | 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) | |
| 19:07:50 | openstackgerrit | Lee Yarwood proposed openstack/nova stable/train: compute: refactor volume bdm rollback error handling https://review.opendev.org/748495 | |
| 19:07:50 | openstackgerrit | Lee Yarwood proposed openstack/nova stable/train: compute: Don't delete the original attachment during pre LM rollback https://review.opendev.org/748496 | |
| 19:11:33 | openstackgerrit | Lee Yarwood proposed openstack/nova stable/stein: Add regression tests for bug #1889108 https://review.opendev.org/748513 | |
| 19:11:33 | 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) | |
| 19:11:34 | openstackgerrit | Lee Yarwood proposed openstack/nova stable/stein: compute: Don't delete the original attachment during pre LM rollback https://review.opendev.org/748514 | |
| 19:26:05 | melwitt | lyarwood: ack will do | |
| 19:30:22 | lyarwood | melwitt: thanks! :) | |
| 19:31:32 | lyarwood | artom: https://review.opendev.org/#/c/747451/ - LGTM, I've got a few questions but I'm not comfortable +2+W'ing networking stuff so even with these answered I'm going to defer to other cores on this one | |
| 19:31:55 | melwitt | sean-k-mooney: fyi I added you to review https://review.opendev.org/747451 please. iirc you have said using the cache is fine but it would be good if you could ack it | |
| 19:32:14 | lyarwood | ah there we go \o/ | |