| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-05-22 | |||
| 21:13:09 | mriedem | anyway, i've said on the code change to take it to the ML or meeting agenda to sort it out with more than 2 people invovled | |
| 21:14:19 | mriedem | if we could throw a generic RequestGroup with the in_tree attribute set in the request spec from the api then the redundancy in the scheduler would be avoided | |
| 21:14:21 | mriedem | but that gets complicated | |
| 21:14:48 | sean-k-mooney | i have not read that one but if we are specifying the host or hypervior host name i assume we would be passing intree to placement and failing in teh scudler if we get no allocation candiates or the filters eliminate the host? | |
| 21:14:54 | efried | Right, I thought we wanted the scheduler filter validation so we would know earlier whether the guest was viable for the host. Rather than "forcing" it to a host it couldn't work on so it would fail late (or worse, pretend to succeed but really be borked) | |
| 21:15:13 | mriedem | efried: that is true | |
| 21:15:33 | mriedem | the debate is if that is good enough or if we should also validate that the requested host/node exist in the API so we can return a 400 if they don't | |
| 21:15:42 | efried | dahhh | |
| 21:15:47 | mriedem | b/c if you don't validate in the api, and they don't exist, you'll get a 202 and then NoValidHost later | |
| 21:15:50 | mriedem | which everyone loves | |
| 21:16:03 | sean-k-mooney | efried: well the current way to force the host is via the AZ e.g. --availability-zone nova:my_host | |
| 21:16:20 | efried | oh, "in the API" meaning before we get to the scheduler | |
| 21:16:37 | efried | f, I don't see why not. How expensive is that check? | |
| 21:17:20 | sean-k-mooney | is it just checkign the api db to see if there is a host mapping for the requested host? | |
| 21:17:25 | sean-k-mooney | if so then cheap | |
| 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 | |