Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2020-02-05
15:54:36 sean-k-mooney mriosfer: that is the flavor one
15:54:44 sean-k-mooney yes
15:56:22 Sundar sean-k-mooney: I am here if you have any comments or questions about your evaluation of Cyborg patches.
15:56:57 sean-k-mooney Sundar: when i tested them yesterday it failed in the cyborg api to send the arq binding notification to nova
15:59:26 Sundar sean-k-mooney: Could you point me to the cyborg logs?
16:02:00 sean-k-mooney Sundar: http://paste.openstack.org/show/789142/
16:05:34 Sundar sean-k-mooney: Looks like you are pulling in an old version of Cyborg patches. NovaAPIConnectFailure exception has been replaced with InvalidAPIResponse exception: https://review.opendev.org/#/c/698846/6/cyborg/common/nova_client.py
16:06:48 mriosfer sean: dxdiag should show the param of 128MB for vRAM?
16:07:16 mriosfer https://gyazo.com/1ad894cd16911a7d6f3a9083fbd65fad
16:09:43 sean-k-mooney i think so yes
16:10:02 sean-k-mooney but i have not tested that
16:32:04 mriosfer humm im not sure if machine is getting the 128MB vram
16:32:20 mriosfer sockets and threats now looks better
16:32:55 efried dansmith: Left a review on https://review.opendev.org/#/c/631243/
16:32:55 efried TL;DR: the structural comments from PS43 still need to be addressed.
16:32:55 efried But dansmith (and gibi) it would be nice if you could scan through my analysis and see if you agree, or if I'm making a big deal out of nothing.
16:33:33 efried Basically I'm saying the steps of processing the device profiles should follow the steps of processing bandwidth requests.
16:34:04 dansmith efried: I don't have context on the bandwidth stuff to make that comparison, but will read
16:34:30 efried dansmith: I seeded the code with comments in the appropriate places, hopefully it's easy enough to follow.
16:34:46 dansmith ack
16:37:24 dansmith efried: I think we've told him specifically to follow the network_info and block_device_info patterns everywhere, which I think his code does
16:37:35 dansmith efried: i.e. make these look like our other attachable things, which are ports and volumes
16:38:03 efried I don't think what I'm suggesting deviates from that, does it?
16:38:24 dansmith seems like it, but I'm still reading
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: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:56 openstackgerrit Lee Yarwood proposed openstack/nova master: compute: Extract _get_bdm_image_metadata into nova.utils https://review.opendev.org/705212
16:40:56 openstackgerrit Lee Yarwood proposed openstack/nova master: WIP libvirt: Support boot from volume instance rescue https://review.opendev.org/701431
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 What he's got will work fine afaict and does seem simpler at first glance.
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 so it would be nice if the reader could follow that logic similarly for both kinds of resource.
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 dansmith so can I follow up after his set and make PRR set after from_components() like he is doing here?
16:50:55 efried But you're right, we could easily accept what's here and refactor later.
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 So your refactor would be like the caller doing
16:58:55 efried reqspec = RequestSpec(foo=bar, ...)
16:58:55 efried for all foo/bar that are static, and then
16:58:55 efried reqspec.fold_in_other_shit(filter_properties, maybe_others, ...)
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

Earlier   Later