| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-05-22 | |||
| 21:02:56 | sean-k-mooney | i have 4 ip currently trying to brute force my root password. which will be hard since i dont allow password loggin as root | |
| 21:04:14 | mriedem | efried: on the virtual persistent memory spec https://review.opendev.org/#/c/601596/ and cyborg integration spec https://review.opendev.org/#/c/603955/ i'm waiting to see those settle down from the people that have been most involved, but is there light at the end of the tunnel at some point? | |
| 21:04:34 | efried | mriedem: I hope so. What do you mean? | |
| 21:04:54 | mriedem | every time i look at these there are multiple -1s | |
| 21:04:59 | mriedem | for like, months | |
| 21:05:14 | mriedem | i guess i'm saying feel free to ping me to review those when they settle down | |
| 21:05:23 | efried | mriedem: ah, cool, thanks. | |
| 21:05:24 | mriedem | but until them i'm not wanting to jump into reviewing them | |
| 21:05:30 | mriedem | *then | |
| 21:05:31 | efried | yeah, understood. | |
| 21:06:00 | efried | for my part, I'm waiting until you're done tearing the host/hypervisor_hostname spec apart before I re-engage :) | |
| 21:09:45 | sean-k-mooney | mriedem: efried i dont think there is much curn on the cyborg one at this point but sundar also has not updated it in two weeks so that is why most of the -1s are still there | |
| 21:10:21 | efried | Right, I think that one is pretty close, updates still trickling down from PTG discussions. | |
| 21:10:33 | mriedem | efried: this? https://review.opendev.org/#/c/660560/ | |
| 21:11:01 | efried | mriedem: no, https://review.opendev.org/#/c/660560/3 | |
| 21:11:11 | mriedem | i'm just adding it to the nova meeting agenda for a stuck review, i see boxiang is getting some back and forth | |
| 21:11:22 | mriedem | that's the same change :) | |
| 21:11:43 | mriedem | seems the debate is if we should validate the requested host/node in the api and fail fast or fail late in the scheduler with novalidhost | |
| 21:11:57 | efried | thought the whole point of the exercise was to fail early. | |
| 21:11:59 | mriedem | i think unless it's egregious we generally want to validate and fail fast in the api if possible | |
| 21:12:20 | mriedem | the whole point of the spec is to request a host and/or node and have the scheduler filters validate it | |
| 21:12:22 | mriedem | rather than force it through | |
| 21:12:31 | mriedem | which is what we have today (unless you're using the little known query hint) | |
| 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. | |