| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-12-08 | |||
| 12:51:02 | fried_rice | sean-k-mooney Don't they call that "Irish coffee"? ;P | |
| 12:51:19 | sean-k-mooney | cdent: but if that does not work you can try the irish solution of brandy/wisky infused coffe | |
| 12:52:56 | fried_rice | cdent Can you explain this delta? https://review.openstack.org/#/c/414230/2..6/nova/scheduler/client/report.py@180 In particular, why did we feel it necessary to check for both `openstack-request-id` and `x-openstack-request-id`? | |
| 12:54:53 | cdent | sean-k-mooney: that will be later in the day, while I think a weekly resource providers update supported by whisky might be entertaining, it might not be all that useful | |
| 12:55:23 | cdent | fried_rice: because there was a brief time when both was going to be supported (as in moving to not using the x- prefix) but then that didn’t happen | |
| 12:55:36 | fried_rice | Okay, so https://review.openstack.org/#/c/523606/ is kosher? | |
| 12:56:10 | sean-k-mooney | x-openstack-request-id is the http header we use for the osprofiler stuff right? | |
| 12:56:23 | fried_rice | oh, you're saying we still might move to the non-x version at some point, whereupon the above would have to be reverted? | |
| 12:56:42 | sean-k-mooney | i taught all non standard http headers were required to have an x prefix | |
| 12:56:45 | cdent | it is kosher, as I said on my +1 on ps3 | |
| 12:57:06 | cdent | sean-k-mooney: they got rid of that requirement somewhere in the last 3 years or so | |
| 12:57:42 | fried_rice | Oh, I didn't see your review. Cool. | |
| 12:57:42 | cdent | fried_rice: even if we did move to wanting the new style, we would have to continue producing the old style, because of compatibility between version | |
| 12:57:43 | sean-k-mooney | cdent: i guess to prevent peopel from updating all there code to support both when its standariesed | |
| 12:58:02 | cdent | it’s like how the microversion headers in nova always return and support both the old and new styles | |
| 12:58:06 | cdent | sean-k-mooney: yes | |
| 13:00:59 | sean-k-mooney | in anycase key.endswith('-request-id') was a little too broad before i like the v6 version more that said wont this rais an exception if neither form are found | |
| 13:01:16 | openstackgerrit | Takashi NATSUME proposed openstack/nova master: [placement] Add x-openstack-request-id in API ref https://review.openstack.org/523007 | |
| 13:02:04 | openstackgerrit | Takashi NATSUME proposed openstack/nova master: List/show all server migration types (1/2) https://review.openstack.org/430608 | |
| 13:04:06 | openstackgerrit | Takashi NATSUME proposed openstack/nova master: List/show all server migration types (2/2) https://review.openstack.org/459483 | |
| 13:05:43 | sean-k-mooney | actully never mind if .get does not find the key and no default is given it returns None instead of raising an exception | |
| 13:17:34 | openstackgerrit | Silvan Kaiser proposed openstack/nova master: Added mount fstype based validation of Quobyte mounts https://review.openstack.org/522245 | |
| 13:32:49 | giblet | fried_rice: regarding https://review.openstack.org/#/c/521605 Can we change the traits of an RP without bumping its generation? | |
| 13:33:21 | giblet | fried_rice: I wondering about having the default None in https://review.openstack.org/#/c/521605/5/nova/compute/provider_tree.py@134 is necessary | |
| 13:35:02 | fried_rice | giblet So | |
| 13:35:42 | fried_rice | IIUC, the generation is intended to be used to detect async updates when we push stuff back to placement. | |
| 13:36:00 | fried_rice | at the moment, I feel like it's not well defined how it's supposed to get used. | |
| 13:36:35 | fried_rice | I.e. who's responsible for bumping the generation, who's responsible for checking for inconsistencies, what the result of an inconsistency should be (exception? HTTP error? Retry? ...) | |
| 13:37:42 | giblet | fried_rice: I think the client should send in bumped generation and the server is responsible to reject update with outdated generation | |
| 13:37:58 | fried_rice | I think that's about the gist of it, yeah. | |
| 13:38:02 | leakypipes | fried_rice: the nova.objects.resource_provider._increment_provider_generation() function pretty well explains it. | |
| 13:39:49 | fried_rice | leakypipes So the server side is responsible for incrementing the generation. | |
| 13:40:04 | fried_rice | It should never be done by the client | |
| 13:40:45 | fried_rice | The client is supposed to take the generation from the server side, make whatever changes to the *other* fields, and push back the object with the *same* generation, which lets the server detect whether a concurrent update occurred. | |
| 13:41:11 | fried_rice | IOW, the only time we should be updating the generation in ProviderTree is when we've freshly retrieved the object from placement. | |
| 13:42:12 | giblet | leakypipes, fried_rice: thanks, now it make sense | |
| 13:43:01 | giblet | fried_rice: so the case when generation is None in https://review.openstack.org/#/c/521605/5/nova/compute/provider_tree.py@134 is when the client update the trait of the RP | |
| 13:43:43 | leakypipes | fried_rice: the client is responsible for refreshing its knowledge of the latest generation, yes. | |
| 13:44:13 | fried_rice | giblet Yeah, so the code path (in a subsequent patch in the series) is going to be a GET of the RP's traits. Uhm, I need to check whether that response includes the RP generation I suppose. | |
| 13:44:30 | leakypipes | fried_rice: it does not. | |
| 13:44:51 | fried_rice | Okay. Then we would have no way to know what to update it to. | |
| 13:45:05 | fried_rice | In fact, I'm not even sure we should have the generation arg there at all... | |
| 13:45:15 | fried_rice | Same for aggregates, I assume. | |
| 13:46:09 | leakypipes | fried_rice: changing aggregates doesn't increment the generation. | |
| 13:47:39 | leakypipes | fried_rice: because when you associate or disassociate an aggregate, you aren't changing the state of the resource provider's own attributes (like traits or inventories). there could be an argument made for incrementing the generation when aggs change on a rp, but I'm a bit skeptical about that | |
| 13:48:40 | fried_rice | leakypipes But generation would change if you update traits? | |
| 13:48:46 | giblet | leakypipes: the worst thing is that two parallel aggregate update on the same RP overwrites each others | |
| 13:50:09 | leakypipes | giblet: sure, understood. | |
| 13:50:14 | leakypipes | fried_rice: yes. | |
| 13:50:40 | leakypipes | fried_rice: for instance, if/when we support setting distances between aggregates (for affinity/anti-affinity), some attribute of the aggregate is being changed that could affect placement decisions and therefore the generation should be incremented. | |
| 13:51:11 | fried_rice | That makes sense. But in that case, the RP generation should really come back with the GET of traits/aggs, nah? | |
| 13:52:18 | fried_rice | leakypipes But then we run into the issue of: if I have my RP cached at generation 1, and I GET traits and they come back at generation 2, I really ought to re-GET the RP - and on and on until the generation agrees on all the GETs. | |
| 13:52:52 | leakypipes | fried_rice: https://github.com/openstack/nova/blob/master/nova/api/openstack/placement/handlers/trait.py#L215 | |
| 13:52:59 | leakypipes | fried_rice: the generation is return when listing traits. | |
| 13:53:07 | mriedem | so i guess zuul just totally crapped out last night huh | |
| 13:53:15 | leakypipes | mriedem: yup | |
| 13:53:29 | leakypipes | fried_rice: correctomundo. | |
| 13:53:51 | leakypipes | fried_rice: which is why your proposed (GET /resource_providers_and_all_the_subresources call makes sense to me. | |
| 13:55:05 | fried_rice | mriedem They restarted some stuff that may have fixed it around 4h ago, but backlog will take a while to clear. | |
| 13:55:21 | mriedem | yikes | |
| 13:55:30 | mriedem | we've had things sitting in the gate for 2 days now | |
| 13:55:31 | mriedem | but ok | |
| 13:56:24 | fried_rice | leakypipes Okay, I had gotten the impression that that guy wasn't going to get traction; the code in my current series does all of that via a dozen separate calls (but without the generation checking). | |
| 13:58:15 | leakypipes | fried_rice: ack. which is fine, frankly. it may seem like there's a potential for data corruption, but aside from the aggregates, there isn't. | |
| 13:58:42 | cdent | fried_rice: i think the lack of traction is not because it is a fundamentally bad idea, but because there’s insufficient sharing of info on what all that stuff’s doing (as I’ve noted in the rp updated pushed out just now) | |
| 14:00:58 | fried_rice | cdent sorry, noted where? | |
| 14:01:43 | cdent | fried_rice: in the nested providers section of http://lists.openstack.org/pipermail/openstack-dev/2017-December/125318.html | |
| 14:03:05 | leakypipes | fried_rice: cdent is referring to his weekly status email | |
| 14:03:10 | leakypipes | oh, jinx | |
| 14:03:54 | fried_rice | aha | |
| 14:04:55 | leakypipes | cdent, giblet, fried_rice: in any case, to just wrap this conversation up in a tidy little bow, the resource provider generation protects the resource provider's important attributes (inventories, traits, allocations, etc) against concurrent writes. It means that yes, you need to occasionally refresh that generation if you get a 409 Conflict about a concurrent write. | |
| 14:05:47 | cdent | leakypipes: we still haven’t done server side retry of allocations have we? reckon we should get that in sooner than later or does it matter? | |
| 14:06:50 | fried_rice | leakypipes At the moment the only thing that's actually doing RP updates (qua updates - not fresh creates) is inventory. And there's no concurrency issues yet because it's always 1:1 compute node to RP. | |
| 14:06:51 | leakypipes | but also keep in mind that that refresh/concurrent update on the provider generation is exceedingly rare in the ProviderTree (since the compute host is what houses the ProviderTree and all operations against it are currently done while holding a semaphore in the compute manager. The scheduler's claim_resources() block is where the contention is, but it's designed for quick retries when it receives a 409 Conflict for either a concurrent update | |
| 14:06:52 | leakypipes | or a capacity exceeded due to a concurrent claim | |
| 14:07:17 | leakypipes | fried_rice: claim_resources() in the scheduler is the only hot-spot currently. | |
| 14:07:23 | leakypipes | fried_rice: and that is by design. | |
| 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. | |