Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2020-02-05
16:38:24 dansmith seems like it, but I'm still reading
16:40:55 openstackgerrit Lee Yarwood proposed openstack/nova master: api: Introduce microverion 2.82 allowing boot from volume rescue https://review.opendev.org/701430
16:40:55 openstackgerrit Lee Yarwood proposed openstack/nova master: compute: Report COMPUTE_RESCUE_BFV and check during rescue https://review.opendev.org/701429
16:40:56 openstackgerrit Lee Yarwood proposed openstack/nova master: WIP libvirt: Support boot from volume instance rescue https://review.opendev.org/701431
16:40:56 openstackgerrit Lee Yarwood proposed openstack/nova master: compute: Extract _get_bdm_image_metadata into nova.utils https://review.opendev.org/705212
16:41:00 dansmith I'm also pretty sure we specifically told him to not use the legacy reqspec.from_components stuff
16:41:06 dansmith let me see if I can find that
16:44:26 efried so it would be nice if the reader could follow that logic similarly for both kinds of resource.
16:44:26 efried My concern is that it's logically very similar to how we're processing port bandwidth requests (pull stuff from flavor and $api, create granular request groups, put them in a special place in the request spec),
16:44:26 efried What he's got will work fine afaict and does seem simpler at first glance.
16:44:37 dansmith https://review.opendev.org/#/c/631243/30/nova/objects/request_spec.py
16:45:32 dansmith granted what he was doing was a lot more than what I _think_ you want him using from_components() for
16:46:15 openstackgerrit Stephen Finucane proposed openstack/nova master: objects: Add MigrationTypeField https://review.opendev.org/706013
16:46:19 efried dansmith: okay, yeah, I agree we shouldn't be doing the api callout from there. In what I'm suggesting, the device_profile_request_groups come into from_components ready-made, just like the port_resource_requests in the preceding chunk.
16:46:42 dansmith efried: shouldn't port_resource_requests just be resource_requests though?
16:46:53 efried yeah, that would be another great way to do it.
16:47:11 dansmith I'd much prefer that than just adding a new parameter of the same type of thing for each high level thing we add
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.

Earlier   Later