Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-05-22
21:19:07 sean-k-mooney honestly i dont think there is any issue with it failing when it gets to the schduler either
21:19:17 mriedem no it's not doing that,
21:19:32 mriedem just commented on that in the api validatoin code, it won't work as written
21:20:21 sean-k-mooney https://review.opendev.org/#/c/645520/20 is the code change ?
21:20:28 mriedem surya already pointed out the same thing i just did
21:20:32 mriedem sean-k-mooney: yes
21:20:40 mriedem if you request a host, we can look up the host mapping
21:20:47 mriedem if you request only a node, then we have to iterate the cells looking for it
21:20:58 mriedem or let the scheduler do that
21:21:01 mriedem since it already is
21:21:11 sean-k-mooney i would let the schduler do it personally
21:21:56 sean-k-mooney is there a reason not to use in_tree in the placement request as well?
21:22:34 mriedem the scheduler code will use in_tree already
21:23:13 mriedem but it will do the same compute node lookup to find the node uuid
21:23:33 mriedem the RequestSpec.requested_destination currently only has the host and nodename, not the uuid
21:23:54 sean-k-mooney well it could look it up via placmenet i nthe hypervior_host name case
21:24:08 sean-k-mooney the compute node RP name is the hypervior host name i think
21:24:35 sean-k-mooney and in the host case its in the instance mappings in the api db
21:24:43 sean-k-mooney sorry host mappings
21:24:44 mriedem s/instance/host/
21:24:56 sean-k-mooney so we should not need to iterate over teh cell dbs
21:26:19 mriedem i'm not sure how much i want to rely on the rp name matching the hypervisor_hostname forever
21:26:24 mriedem or always being correct
21:26:52 mriedem especially since you can change the name https://developer.openstack.org/api-ref/placement/?expanded=update-resource-provider-detail#update-resource-provider
21:26:58 sean-k-mooney im just checking that it is infact set to that. but i think we need to for other services to be able to identigy the host
21:27:20 sean-k-mooney e.g. we need a way for neutron to be abel to create nested RPs under the compute node RP
21:27:21 mriedem yes we set the rp name to the compute node hypervisor_hostname in the RT
21:27:53 mriedem https://github.com/openstack/nova/blob/2e85453879533af0b4d0e1178797d26f026a9423/nova/compute/resource_tracker.py#L954
21:28:09 sean-k-mooney i think its effectivly part of the contract nova has with cyborg and neutron at this point
21:28:36 mriedem heh, well, it's mutable in the placement api
21:28:54 sean-k-mooney its technically mutable in our config file too
21:28:57 mriedem https://docs.openstack.org/osc-placement/latest/cli/index.html#resource-provider-set
21:29:14 sean-k-mooney well not that but the host paramater
21:29:14 mriedem how so? if you're talking about CONF.host, that's something else
21:29:22 sean-k-mooney ya i was
21:29:24 mriedem hypervisor_hostname comes from the driver
21:29:25 mriedem not the config
21:29:52 mriedem https://github.com/openstack/nova/blob/2e85453879533af0b4d0e1178797d26f026a9423/nova/virt/driver.py#L1622
21:30:07 sean-k-mooney yep you are right
21:32:11 mriedem anyway, i threw it in the stuck review section for the meeting https://wiki.openstack.org/wiki/Meetings/Nova#Agenda_for_next_meeting
21:32:24 mriedem enumerating options in the ML ahead of the meeting would probably be useful
22:02:40 aspiers efried: around?
22:02:52 aspiers just had another idea for how to provide patch_exists()
22:02:56 efried aspiers: yeaux
22:03:09 aspiers class patch_exists(contextlib.ContextDecorator)
22:03:36 aspiers then it can be used both as a @patch_exists decorator, and via: with patch_exists(...)
22:04:51 aspiers I think there's also a case to be made for providing patch_open in a similar way
22:05:22 aspiers currently there's tons of stuff which does:
22:05:26 aspiers mock_open = mock.mock_open()
22:05:36 aspiers with mock.patch.object(six.moves.builtins, 'open', new=mock_open):
22:05:43 aspiers ... do stuff
22:05:53 aspiers that could be replaced with
22:06:04 aspiers with patch_open():
22:06:06 aspiers ... do stuff
22:06:21 aspiers and it could support selective patching
22:06:38 efried and custom side effects
22:06:44 aspiers right
22:06:51 aspiers with patch_open() as mock_open:
22:06:56 aspiers mock_open.side_effect = ...
22:07:13 efried whereupon you might as well just say
22:07:13 efried with mock.patch.object(six.moves.builtins, 'open', new=mock.mock_open()) as mock_open:
22:07:39 efried but sure, patch_open would be a neat little helper.
22:08:09 aspiers hrm
22:08:12 aspiers in two minds about this
22:08:24 aspiers there's a danger of trying to reinvent a bunch of the mock API
22:08:41 aspiers with patch_open(side_effect=...) as mock_open:
22:08:43 aspiers etc.
22:09:02 aspiers the bit I'm really missing easy selective patching
22:09:25 aspiers as in "patch opening path X, but pass anything else through to builtins.open"
22:11:48 aspiers efried: I can't find any existing implementing of selective patching of open(), although I thought you said you had done that in the past. Maybe outside nova?
22:14:05 sean-k-mooney i think we have an example of this in os-vif
22:14:25 sean-k-mooney by selectivly patching you mean mock right
22:14:48 sean-k-mooney you do it like this https://github.com/openstack/os-vif/blob/6f08a3b4f8be77b2b5eae653c05db6e5770b562d/vif_plug_ovs/tests/unit/test_linux_net.py#L65
22:14:58 sean-k-mooney @mock.patch('six.moves.builtins.open')
22:15:17 sean-k-mooney or this @mock.patch.object(builtins, 'open')
22:17:49 sean-k-mooney aspiers: oh you were sugging addign a wrapper to make it simpler
22:18:19 sean-k-mooney ya that soudn like a good idea becuse tis a bit of a pain to get right
22:24:56 openstackgerrit Eric Fried proposed openstack/nova master: Remove nova.compute.*API() shims https://review.opendev.org/660527
22:26:19 aspiers sean-k-mooney: yeah exactly
22:26:20 efried aspiers: Sorry, hold on, lemme find
22:26:45 efried aspiers: I just reviewed one this morning in fact.
22:26:51 aspiers sean-k-mooney: except the approach you linked there doesn't work if the code path under test makes additional calls to open or exists
22:27:18 aspiers sean-k-mooney: sometimes that can happen (Especially in functional tests) and then you don't want to hardcode assumptions that those calls happen in a particular order
22:27:42 aspiers you just want to patch when it is called with a certain param, otherwise pass through transparently
22:28:52 sean-k-mooney aspiers: you can pass an iterable of callable as the return value and have it return differnet things but yes that is really just for mocking things in unit tests
22:29:32 efried aspiers: nova/tests/unit/test_versions.py:45
22:30:00 sean-k-mooney aspiers: in unit test you actully do generally want to encode the order. or rather we generall do in openstack
22:31:54 sean-k-mooney efried: that is not really generic either
22:32:19 aspiers efried: thanks
22:32:30 efried sean-k-mooney: that's the point. aspiers is wanting to write a thing to do that pattern, so it could be used in places like that in a more readable and generic way.
22:32:40 aspiers right
22:32:55 sean-k-mooney well in the unit test we never want it to call real_open
22:33:06 sean-k-mooney and in the functional test im not sure that is vaild either
22:33:36 aspiers in the functional test I'm writing, placement needs to read placement-policy.yaml from the testenv
22:33:50 aspiers there's no way I'm hardcoding that path into my test
22:34:03 aspiers nor any assumption about *when* it needs to read that file
22:34:17 efried mriedem: Do you have a take on whether nova should talk to cyborg with an admin auth or with the user's auth?
22:34:20 sean-k-mooney why does it need to read a file instead of embeding it in a sting
22:34:44 aspiers because it's not mocking or stubbing placement
22:34:47 efried sean-k-mooney: It was just an example. Point is that there are valid and viable reasons unit tests should invoke real open().

Earlier   Later