| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-12-08 | |||
| 14:07:50 | fried_rice | wait, claims don't update a RP generation, do they?? | |
| 14:07:54 | leakypipes | cdent: nope, only client-side (in the reportclient.claim_resources(*) | |
| 14:08:01 | leakypipes | fried_rice: they absolutely do. | |
| 14:08:21 | fried_rice | oh, dang, then we definitely need to do more than we're doing. | |
| 14:08:56 | leakypipes | fried_rice: https://github.com/openstack/nova/blob/master/nova/objects/resource_provider.py#L2098 | |
| 14:09:14 | cdent | leakypipes: Is that “no we shouldn’t bother” or “ack, I agree we aren’t” | |
| 14:09:46 | figleaf | mriedem: heh, you are already rechecking the alternate host series | |
| 14:10:01 | leakypipes | cdent: on the server-side retry thing? that was a "no, we just haven't gotten to that yet and I'm not too worried because we have client-side retries" | |
| 14:10:13 | leakypipes | cdent: i.e. the server-side would be a welcome optimization but isn't unsafe | |
| 14:10:15 | figleaf | mriedem: I had them open, and didn't see your update until I rechecked 'em | |
| 14:10:25 | cdent | ✔ | |
| 14:10:44 | mriedem | i have rechecked all things | |
| 14:10:57 | figleaf | mriedem: cool, thanks | |
| 14:11:22 | leakypipes | diga: whatup? :) | |
| 14:11:27 | diga | leakypipes: Hi | |
| 14:11:52 | leakypipes | diga: the thing ew've been discussing -- server-side retries of the claim_resources() logic -- is something I think you might be interested in contributing. | |
| 14:12:34 | leakypipes | diga: it all stems from the following TODO left in the code: | |
| 14:12:35 | leakypipes | https://github.com/openstack/nova/blob/master/nova/objects/resource_provider.py#L2148-L2152 | |
| 14:12:50 | diga | leakypipes: yep, this can be gr8 opportunity for me to pitch in :) | |
| 14:12:53 | diga | leakypipes: let me go through this | |
| 14:13:46 | diga | leakypipes: How can I test this first | |
| 14:13:54 | leakypipes | diga: currently, we are doing a retry of the "claim resources" transaction from the client side (i.e. in the scheduler). It would be an optimization to be able to retry the transaction from the placement server side (thus saving the round-trip HTTP call) | |
| 14:15:07 | fried_rice | diga The real life scenario would be to have two separate threads doing claims, and make sure they do them in a certain sequence. | |
| 14:15:41 | openstackgerrit | Surya Seetharaman proposed openstack/nova master: update_cell allows more than once cell to have the same db/transport url https://review.openstack.org/518556 | |
| 14:15:42 | fried_rice | diga For test purposes, you could simulate it by cloning the local RP data, doing a claim with one copy, and then doing a different claim with the second copy (which will have the old generation). | |
| 14:19:48 | cdent | sigh, I was wrong about deploy.deploy() being a public interface, it’s deploy.loadapp() that I was thinking of. I’ll fix things appropriately. | |
| 14:39:04 | mriedem | fried_rice: do we need to hold https://review.openstack.org/#/c/522112/ for https://review.openstack.org/#/c/508345/ or can yours be stacked on top? | |
| 14:40:35 | mriedem | diga left huh | |
| 14:40:42 | mriedem | was going to point them at how we deal with this client-side today https://github.com/openstack/nova/blob/master/nova/scheduler/client/report.py#L98 | |
| 14:45:18 | openstackgerrit | Chris Dent proposed openstack/nova master: [placement] annotate loadapp as public interface https://review.openstack.org/526691 | |
| 14:47:32 | diga | leakypipes: Hi | |
| 14:47:37 | diga | sorry got disconnected | |
| 14:48:57 | diga | leakypipes: I will try to test it as per your suggestion, if anything requires, will ping you | |
| 14:51:07 | openstackgerrit | Stephen Finucane proposed openstack/nova master: tests: fixes mock autospec usage https://review.openstack.org/447505 | |
| 14:52:01 | fried_rice | mriedem Either way will work. Mine isn't working yet, so do the other first. | |
| 14:52:07 | diga | cdent: I will ping you if need help | |
| 14:52:53 | mriedem | fried_rice: ok - did you see anything wrong with the way they are doing the ksa options in that one? | |
| 14:53:35 | fried_rice | mriedem No, looks fine. Mine will need a manual rebase to accomodate, of course. | |
| 14:56:16 | fried_rice | mriedem I left a +1. | |
| 14:56:25 | mriedem | thanks | |
| 15:02:48 | openstackgerrit | Jackie Truong proposed openstack/python-novaclient master: Microversion 2.57 - Add trusted_image_certificates https://review.openstack.org/500396 | |
| 15:19:01 | openstackgerrit | Jackie Truong proposed openstack/python-novaclient master: Microversion 2.57 - Add trusted_image_certificates https://review.openstack.org/500396 | |
| 15:22:21 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Deprecate file injection https://review.openstack.org/522027 | |
| 15:25:20 | openstackgerrit | Jackie Truong proposed openstack/python-novaclient master: Microversion 2.57 - Add trusted_image_certificates https://review.openstack.org/500396 | |
| 15:30:14 | melwitt | mriedem: I utilitized the quotas follow up patch https://review.openstack.org/#/c/524234 | |
| 15:31:32 | mriedem | ok, currently re-reviewing superdan's live migration allocations thing | |
| 15:34:00 | melwitt | sahid: would you be able to take a look at this review where there's a bug with disk device addresses being changed during a live migration? https://review.openstack.org/#/c/518022 | |
| 15:35:15 | melwitt | it looks like it's a regression caused by a different bug fix from the past https://review.openstack.org/#/c/459741 | |
| 15:36:24 | openstackgerrit | Chris Dent proposed openstack/nova master: [placement] add name to resource provider create error https://review.openstack.org/526710 | |
| 15:36:55 | sahid | melwitt: i think i did that this morning | |
| 15:37:09 | sahid | is there something not clear you want to discuss? | |
| 15:37:15 | melwitt | mdbooth: it would be good to have your review on this change for fixing the cache value with regard to O_DIRECT support during an image conversion https://review.openstack.org/#/c/523554 | |
| 15:38:03 | mdbooth | melwitt: I vaguely recall having looked at something similar, if not this | |
| 15:38:09 | melwitt | sahid: oh geez, sorry. I didn't see that you reviewed it this morning. I had it on my todo to ask you the past few days and just did it blindly. thanks | |
| 15:43:04 | openstackgerrit | Merged openstack/nova master: Avoid stashed connector lookup for new style detach https://review.openstack.org/526182 | |
| 15:43:46 | melwitt | mdbooth: most of the patch is moving the supports_direct_io check to nova/utils.py so that it could be used by both virt/images and libvirt/driver. but the main part of the fix is in here https://review.openstack.org/#/c/523554/6/nova/virt/images.py | |
| 15:47:06 | mriedem | superdan: done | |
| 15:47:19 | mriedem | however, it's an easy one | |
| 15:47:30 | superdan | received | |
| 15:50:34 | leakypipes | mriedem: I'm trying to square your code comment here: https://github.com/openstack/nova/blob/master/nova/compute/api.py#L4591-L4595 with the following in the functional test case setup here: https://github.com/openstack/nova/blob/master/nova/tests/functional/compute/test_host_api.py#L76-L79 | |
| 15:51:12 | leakypipes | mriedem: the reason is I have some PoC code I'm working on locally that would allow host aggregates to be associated to compute nodes (via UUID) in addition to compute *services* by service hostname | |
| 15:51:19 | cdent | superdan: not sure if you saw my earlier ping on this potentially cells related bug that needs some evaluation: https://bugs.launchpad.net/nova/+bug/1736101 | |
| 15:51:20 | openstack | Launchpad bug 1736101 in OpenStack Compute (nova) "nova placement resource_providers DBDuplicateEntry when name repeat" [Undecided,New] | |
| 15:51:51 | superdan | I did not | |
| 15:51:53 | mriedem | leakypipes: https://github.com/openstack/nova/blob/master/nova/tests/functional/compute/test_host_api.py#L78 is just test data, the hostmapping.host wouldn't be a uuid | |
| 15:52:01 | mriedem | leakypipes: at least, not a uuid that matches the computenode.uuid | |
| 15:52:31 | leakypipes | mriedem: sorry, I'm not following you... | |
| 15:52:55 | leakypipes | mriedem: you're creating a HostMapping with the host attribute of the mapping set to the compute node's UUID. | |
| 15:53:02 | superdan | cdent: so, two hosts with the same name is not a valid thing, but it sounds like they only have that situation because they're moving between cells? | |
| 15:53:07 | leakypipes | mriedem: which is what the code comment in HostsAPI says we're not doing? | |
| 15:53:16 | mriedem | leakypipes: https://github.com/openstack/nova/blob/master/nova/compute/api.py#L4591-L4595 is just saying, we're looking up the compute node (in a cell) by uuid, (microversion >=2.53), if we had the uuid stored in the host mapping (api db), then we wouldn't have to iterate the cells querying each db for a compute node with that uuid | |
| 15:53:26 | mriedem | we could just get the host mapping from the api db which would tell us which cell that compute node is in | |
| 15:53:37 | cdent | superdan: I struggled to parse it and wasn’t entirely clear on the naming constraints | |
| 15:53:55 | mriedem | leakypipes: https://github.com/openstack/nova/blob/master/nova/tests/functional/compute/test_host_api.py#L78 - the hostmapping.host field could be "turd.ferguson" | |
| 15:53:56 | superdan | cdent: the conflict is in the placement db, right? | |
| 15:54:07 | cdent | yes, rp names are supposed to be unique | |
| 15:54:17 | superdan | cdent: the rp name comes from what though? | |
| 15:54:18 | leakypipes | mriedem: ok. so are we adding HostMapping records for compute nodes now or are we not doing that? | |
| 15:54:19 | mriedem | leakypipes: the test is just using something random, which in this case is the uuid that matches the compute node in the cell - confusing the test i guess | |
| 15:54:30 | leakypipes | mriedem: note: compute nodes, not service hosts. | |
| 15:54:31 | mriedem | leakypipes: we are adding host mappings for service hosts | |
| 15:54:32 | mriedem | not compute nodes | |
| 15:54:53 | leakypipes | mriedem: ok, that's what I needed to know, thanks man | |
| 15:54:53 | cdent | superdan: that’s what I’m not clear on. some portion of the host? If it’s not the fqdn, then that’s a clear potential for problems | |
| 15:55:11 | leakypipes | mriedem: I'm writing func tests for this code I'm proposing, thus the question... | |
| 15:55:14 | mriedem | leakypipes: this is what creates the host mappings http://git.openstack.org/cgit/openstack/nova/tree/nova/objects/host_mapping.py#n192 | |
| 15:55:31 | superdan | cdent: I wonder if it is something like the hostname, and that moving between cells has caused the name to be the same but the uuid change? | |
| 15:55:35 | mriedem | http://git.openstack.org/cgit/openstack/nova/tree/nova/objects/host_mapping.py#n177 | |
| 15:55:40 | leakypipes | mriedem: I copied test_hosts_api.py and saw that after reading the comment in api.py about us not storing compute node records in host mappings table and was like... uh? | |
| 15:55:41 | mriedem | host_mapping = HostMapping( ctxt, host=compute.host, cell_mapping=cm) host_mapping.create() | |
| 15:56:26 | mriedem | leakypipes: the api comment says "We wouldn't have to loop over cells if we stored the ComputeNode.uuid in the HostMapping" - specifically the uuid, but yeah i can see how that's confusing now | |
| 15:56:33 | mriedem | because even if we did, it'd be wrong | |
| 15:56:37 | mriedem | we should probably remove that comment | |
| 15:57:07 | leakypipes | mriedem: right. I'm hacking around the host mapping stuff, though, and calling ComputeNode.get_all() on all cells via the context.scatter_gather_skip_cell0() function, since I figured it would not be possible to look up compute node information in the host_mappings table. | |
| 15:57:19 | cdent | superdan: I haven’t had much of a chance to think about it, I was just going through placement tagged bugs and that one was making a dance between invalid and new and thought I better try to make sure it wasn’t lost if it is a real issue | |
| 15:57:22 | leakypipes | s/get_all()/get_by_uuid() | |
| 15:57:44 | mriedem | leakypipes: yeah correct - only the compute_nodes.host field is in there, and if you're ironic that doesn't help you | |
| 15:57:47 | mriedem | because 1:M | |
| 15:58:09 | superdan | cdent: well, I imagine it's a real issue, but I'm not sure exactly what to tell them.. I guess you could provide them the curl commands to grab a token and delete the old provider record to confirm that it heals up properly afterwards right? | |
| 15:58:17 | leakypipes | mriedem: FYI, this is to support PUT /os-aggregates/{agg_id} and passing a compute node UUID instead of a service hostname. | |