Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2020-02-05
16:47:42 efried Agree: fold port & device resource requests (and any others in the future) together prior to from_components. ++
16:48:57 efried so yeah -- how early can we fold those together? The earlier the better.
16:49:21 efried Maybe as early as the construction of base_options. Need to see how the supports_port_bandwith_requests business plays in.
16:49:25 dansmith that's not my preference, I'm just saying if we're still going to call this legacy method to set a single attribute, then they should be combined
16:49:47 dansmith what I'd *rather* is he leave what he has here and I follow up and move that port_resource_requests outside of from_components where it belongs in the first place, IMHO
16:50:19 efried I just don't love that from_components is initializing resource_requests, and then we're extending it after, outside of that method.
16:50:31 dansmith ack, that's legit
16:50:55 efried But you're right, we could easily accept what's here and refactor later.
16:50:55 dansmith so can I follow up after his set and make PRR set after from_components() like he is doing here?
16:51:11 dansmith if so I shall commit to it in writing
16:51:32 efried so, remove that as a param from from_components() entirely?
16:51:52 dansmith yeah
16:51:57 efried I don't have the big picture on from_components; you hinted we should be able to get rid of it entirely?
16:52:20 dansmith it was supposed to be bridge code to get us to objects and removed in mitaka or something
16:52:34 efried oh, didn't mriedem propose a WIP that started doing that?
16:52:46 dansmith he complained about it a lot, so probably
16:53:26 efried https://review.opendev.org/#/c/697686/ ?
16:53:38 efried no, not quite
16:53:49 dansmith well, that's what you're thinking of probably
16:54:11 efried yeah
16:54:15 dansmith regardless, as you can see, from_components is just "set a bunch of things and no other logic"
16:54:49 efried yeah. but it's common to both build and cold migrate
16:55:24 dansmith to what end?
16:55:33 efried just DRY
16:55:39 dansmith it's not really though
16:55:46 efried like, I'm not sure I see a better split logically. Whether we want to get rid of the filter_properties in some way, that's fine
16:55:49 dansmith because the actual constructor can take field values
16:56:20 efried There's a *little* bit of logic in there.
16:56:24 efried All these _from_* methods
16:57:03 dansmith okay, I guess so
16:57:20 dansmith but that would also kinda mean that the setting of a field that doesn't need any handling shouldn't be in here
16:58:55 efried reqspec.fold_in_other_shit(filter_properties, maybe_others, ...)
16:58:55 efried for all foo/bar that are static, and then
16:58:55 efried reqspec = RequestSpec(foo=bar, ...)
16:58:55 efried So your refactor would be like the caller doing
16:59:42 efried where the latter is from_components with all the static setters removed?
17:00:31 dansmith well, that's a potential bigger refactor, because from_components is a classmethod
17:00:50 dansmith and, I'm not sure that would really work with the way the _from things work.. they may expect to "go first"
17:01:24 dansmith so I would just take the req_spec that comes from from_components, and follow up with the direct things
17:01:36 dansmith req_spec = from_components(..); req_spec.requested_resources = port_requested_resources + accel_requested_resources; etc
17:06:08 efried I guess I don't see the benefit of assigning some fields in the classmethod and some afterwards. Similar reasoning as mentioned previously: then you always have to go make sure you're assigning the right pieces the right way on the right side of the method boundary.
17:06:29 efried but I don't feel strongly enough to -1 that.
17:06:44 mriedem beware the request spec quagmire
17:06:54 efried I think we're already quagged.
17:07:58 mriedem if you go one up that stack https://review.opendev.org/#/c/697697/ and read my comments, unwinding a bunch of that stuff is going to be dependent on doing a major conductor rpc api version
17:08:12 mriedem because there is backward compat code to handle the older version
17:08:20 mriedem which relies on a lot of this bridge code
17:08:36 dansmith efried: I commented and I'm late for another meeting
17:11:00 stephenfin melwitt: Does my reply at https://review.opendev.org/#/c/705654/ make sense?
17:11:03 stephenfin before I rewrite the thing
17:12:18 larsks asd
17:15:44 melwitt stephenfin: oh, wow. so ... we are ok to remove the helper method so long as the minimum python version is 3.6 because the native method is doing the same thing? are we (deployers) guaranteed to not be running <= 3.6 at this point?
17:16:15 stephenfin melwitt: yup https://github.com/openstack/nova/blob/master/setup.cfg#L9
17:16:25 stephenfin you can't install nova on anything less
17:16:30 melwitt stephenfin: cool, thanks
17:16:53 melwitt yup, that all makes sense then
17:17:20 stephenfin appears to have been the behavior since Python 3.3
17:17:29 stephenfin https://github.com/python/cpython/blob/v3.3.0/Lib/http/server.py#L562-L565
17:17:39 stephenfin cool, I'll rework that so. Thanks for the review
17:28:19 melwitt stephenfin: I just looked this up, apparently the original commit you referenced _would_ fix the issue because the reverse dns lookup was happening as a result of simply logging a message (I did not expect this) https://github.com/openstack/nova/commit/c0f773a616fb48bf699539c5ac18bd9c55a540c9
17:30:03 stephenfin melwitt:Oh, fun. Fancy suggesting a commit message I should use? :)
17:32:22 melwitt stephenfin: technically your commit message is right, so I think we can leave it. let me just add a comment and change my vote
17:35:30 stephenfin melwitt: Okay. I do need to repush that series to address gibi's comments on the base patch though so if I do need to edit the commit message, let me know in the next 2 mins :)
17:39:08 melwitt stephenfin: k, I think it's fine as-is
17:43:12 openstackgerrit Stephen Finucane proposed openstack/nova master: trivial: Remove unused 'cache_utils' APIs https://review.opendev.org/705652
17:43:13 openstackgerrit Stephen Finucane proposed openstack/nova master: trivial: Bump minimum version of websockify https://review.opendev.org/705654
17:43:13 openstackgerrit Stephen Finucane proposed openstack/nova master: trivial: Fetch 'Service' objects once when building AZs https://review.opendev.org/705653
17:43:14 openstackgerrit Stephen Finucane proposed openstack/nova master: trivial: Remove 'run_once' helper https://review.opendev.org/705656
17:43:14 openstackgerrit Stephen Finucane proposed openstack/nova master: trivial: Merge unnecessary 'NovaProxyRequestHandlerBase' separation https://review.opendev.org/705655
17:43:15 openstackgerrit Stephen Finucane proposed openstack/nova master: mypy: Add type annotations to 'nova.pci' https://review.opendev.org/676209
17:43:15 openstackgerrit Stephen Finucane proposed openstack/nova master: tox: Integrate mypy https://review.opendev.org/676208
17:43:16 openstackgerrit Stephen Finucane proposed openstack/nova master: WIP: mypy: Add type annotations to top-level modules https://review.opendev.org/705658
17:43:16 openstackgerrit Stephen Finucane proposed openstack/nova master: mypy: Add nova.cmd, nova.conf, nova.console https://review.opendev.org/705657
17:47:54 stephenfin I, however, am outta here
17:47:54 stephenfin melwitt: There's a super trivial follow-up to that too, btw, if you'd like to take a look https://review.opendev.org/#/c/705655/
17:48:37 efried sean-k-mooney (sean-k-mooney1): https://blueprints.launchpad.net/nova/+spec/config-tsc-freq looks like something you would be able to grok. I talked to umbSublime and he's interested in working the implementation. At a glance, it seems like it should be fairly straightforward, but I'd like your opinion if you've got a moment to look.
17:48:46 melwitt stephenfin: will do
17:49:16 sean-k-mooney ya i was ment to write up a spec/blueprint
17:49:21 sean-k-mooney or help them
17:50:52 sean-k-mooney efried: the propoasal was to basically to add 2 extra specs one for the tsc frequency and the other for enableing invtsc
17:51:56 sean-k-mooney also add a trait to land on host that support invtsc
17:52:17 efried sean-k-mooney: cool. This would be umbSublime's first contribution, so he'll need some mentoring...
17:53:37 sean-k-mooney looking at my browser history we created 1 an i think i was ment to help draft the update to the bluepirnt.
17:53:41 sean-k-mooney https://etherpad.openstack.org/p/invtsc
17:53:56 sean-k-mooney is umbSublime about
17:54:18 efried That etherpad looks like the same content as what's in the bp description.
17:54:21 sean-k-mooney my isp decied to force recet my router again and broke my internet
17:54:23 sean-k-mooney yes
17:54:37 sean-k-mooney so i obvioulsy never wrote the updated draft
17:54:40 efried from what I could glean, "we" decided at some point that the blueprint could be specless.
17:54:54 sean-k-mooney yes i brought it up in the nova meeting
17:55:15 sean-k-mooney and we said if we spell out the minor changes need then it could be specless
17:55:22 efried great
17:56:52 sean-k-mooney umbSublime: this sliped off my radar sorry.
17:57:41 umbSublime reporting in o/
17:58:04 sean-k-mooney umbSublime: if you are still up for working on the code cahgne ill try and draft some worth in the ether path to describe what is needed. if you are happy with tem we can update the blueprint and i can help you with reviewing your patch
17:58:57 umbSublime Sounds good to me! I don't think I'll have much time to work on this this week, but next week should be fine!
17:59:34 umbSublime sean-k-mooney, in which etherpad would that be ?
17:59:56 efried sean-k-mooney, umbSublime: we'll want something in approvable form by next week (spec freeze).
18:00:08 sean-k-mooney https://etherpad.openstack.org/p/invtsc

Earlier   Later