| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-07-03 | |||
| 17:13:47 | dansmith | fair enough | |
| 17:15:45 | mriedem | http://logs.openstack.org/69/556669/11/check/nova-multiattach/b3195af/logs/screen-n-api.txt.gz#_Jun_22_14_30_55_677141 | |
| 17:15:48 | mriedem | that's the failure | |
| 17:15:55 | mriedem | Unexpected exception in API method: ResourceProviderUpdateConflict: A conflict was encountered attempting to update resource provider b9342d94-3665-4da1-90c8-e48ab77e181e (generation 6): {"errors": [{"status": 409, "request_id": "req-fe8eea9d-ec9d-4128-9117-73a68e462394", "detail": "There was a conflict when trying to complete your request.\n\n Update conflict: Another thread concurrently updated the data. Please retry your u | |
| 17:15:55 | mriedem | e ", "title": "Conflict"}]} | |
| 17:16:20 | efried | just so | |
| 17:16:21 | mriedem | who wants to open the bug | |
| 17:16:28 | efried | Well, it's not a bug yet | |
| 17:16:31 | dansmith | so no retry around te mirror? | |
| 17:16:34 | mriedem | nope | |
| 17:16:43 | efried | it doesn't show up until we start gen checking aggs. | |
| 17:16:56 | mriedem | i read that as 'chicken eggs' | |
| 17:17:01 | mriedem | mmm | |
| 17:17:08 | efried | A rat snake has been eating our eggs in the coop. | |
| 17:17:21 | efried | I mean, the race is still there, we're just blowing away whatever the first one did with the second. | |
| 17:17:42 | efried | I hear you're supposed to put a light bulb in the nest box. They eat the bulb and... ouch. | |
| 17:17:44 | mriedem | add_host_to_aggregate does handle several other exceptions and log a warning and return | |
| 17:18:52 | efried | mm, we could add UpdateConflict in there, but that would just mask the problem (I had that objection to that try/except when this code went in iirc). | |
| 17:19:08 | mriedem | hmm, wtf | |
| 17:19:15 | mriedem | we don't pass a generatoin when updating the aggregates | |
| 17:19:22 | efried | mriedem: That patch does. | |
| 17:19:36 | mriedem | ah ok | |
| 17:19:52 | efried | Swhat I was saying earlier. This isn't causing failures except in that patch. (The ironic thing is different.) | |
| 17:20:11 | efried | (though related, and probably fixable by the same retry loop) | |
| 17:21:52 | dansmith | I'm not sure why being obsessive about the generation in reportclient would cause the api side to fail, | |
| 17:22:04 | dansmith | unless it's just that it generates a little more load and opportunity to collide with the api mirror | |
| 17:25:04 | efried | dansmith: Every time _update runs (so periodic and from instance_claim) it tries to sync placement. Part of that is hitting the method to push aggregates for the compute host, which is the same method the host agg sync runs. If _update thinks it needs to push *any* change associated with the compute node RP, and the host agg sync happens between when we retrieve the data and when we push it, we'll 409. | |
| 17:25:44 | efried | ...once we're checking gens on aggs, which is what this patch introduces. | |
| 17:25:54 | dansmith | okay so just a lot of bumping the agg generation yeah? | |
| 17:26:45 | efried | dansmith: update_from_provider_tree is smart enough to invalidate the cache entry for the provider that 409s, so the next time around it'll re-GET and have the new generation. But up to this point, we're not doing that retry until the next periodic hits. | |
| 17:27:10 | efried | So from the pov of periodic, we're fine, it heals itself. But if the 409 happens in the instance_claim path, it can cause a resched/fail. | |
| 17:27:27 | dansmith | okay but the failure that breaks a test is the agg mirroring in the api | |
| 17:27:35 | dansmith | which is presumably because it doesn't retry on 409 | |
| 17:27:39 | dansmith | what I'm wondering is, | |
| 17:27:40 | efried | right | |
| 17:27:53 | dansmith | why is the compute causing any update churn on the agg generation | |
| 17:27:57 | dansmith | ? | |
| 17:28:15 | efried | Don't we bump the RP gen whenever we allocate? | |
| 17:28:29 | efried | The agg doesn't have a generation. The RP does. | |
| 17:29:19 | dansmith | okay, so the 409 that the api is getting is that the RP it's trying to add to the agg has changed generations (and it doesn't retry)? | |
| 17:29:57 | efried | right | |
| 17:30:07 | dansmith | so what extra churn on the RP is the patch causing? | |
| 17:30:28 | efried | It's not causing extra churn. Before this patch, we weren't checking generations on agg updates - we were just blasting them in. | |
| 17:30:40 | efried | So we never got 409 - just whoever went second won. | |
| 17:30:50 | dansmith | right, but that's my point, | |
| 17:30:52 | efried | which is a bug | |
| 17:30:56 | efried | but it would be hidden | |
| 17:31:02 | dansmith | we're doing the same number of RP updates as before this patch? | |
| 17:31:03 | efried | checking gens exposes the bug. | |
| 17:31:05 | efried | yes | |
| 17:31:22 | dansmith | ...so what did it change that causes it to race and conflict with the api mirroring? | |
| 17:31:42 | efried | The change that caused the race was the introduction of host agg sync. | |
| 17:31:42 | dansmith | or are we potentially doing one more update if we got a 409 on the compute node and do a retry? | |
| 17:31:51 | efried | But the race stayed hidden because we weren't checking rp gen on agg updates. | |
| 17:31:55 | efried | Until this patch. | |
| 17:32:08 | efried | Nothing does a retry (yet) | |
| 17:32:12 | dansmith | no, that doesn't make sense | |
| 17:32:15 | dansmith | you checking aggs on the compute node doesn't make something else get a 409 | |
| 17:32:19 | efried | I'm suggesting that as the fix. | |
| 17:32:36 | efried | Oh, the generation checking is in the method that's common to both the compute side and the host agg sync. | |
| 17:32:52 | efried | sorry that wasn't clear | |
| 17:33:08 | dansmith | oh set_aggregates_for_provider() | |
| 17:33:09 | dansmith | ? | |
| 17:33:37 | efried | https://review.openstack.org/#/c/556669/11/nova/scheduler/client/report.py@1250 yeah | |
| 17:33:41 | dansmith | api is using that for the mirroring, which never would fail before because it didn't check, but now is failing because it does, in the patch | |
| 17:33:57 | dansmith | okay, I got it.. I thought you were saying lots of things other than this | |
| 17:34:14 | efried | :) | |
| 17:34:23 | efried | You were too late. I already killed your hit man. | |
| 17:34:36 | dansmith | I requested NFO | |
| 17:34:42 | dansmith | so that must have been jaypipes' hit man | |
| 17:34:42 | efried | while typing with the other hand | |
| 17:35:46 | efried | dansmith: So do you agree adding a retry on both sides of this thing would be a viable solution? | |
| 17:36:14 | dansmith | well, I agree that (a) mirroring should be checking generation and (b) that it should retry if generation conflicts, so .. yes? | |
| 17:36:25 | efried | dig | |
| 17:38:28 | efried | I'll retract what I said earlier about there not being a bug, mriedem. There is, we were just hiding it before. I'll open it and *try* to explain it. Then fix it via ^ | |
| 17:38:40 | dansmith | agree | |
| 17:39:21 | efried | Does the Big Lock in the rt mutex the _update periodic with the instance_claim path? | |
| 17:39:39 | efried | I guess it would have to. | |
| 17:41:04 | dansmith | that's the point of it I think | |
| 17:41:55 | efried | confirmed. | |
| 18:11:50 | openstackgerrit | Merged openstack/nova master: Merge server create schema for multiple create extension https://review.openstack.org/578338 | |
| 18:11:57 | openstackgerrit | Merged openstack/nova master: Merge server create schema for scheduler hint extension https://review.openstack.org/578343 | |
| 18:42:49 | karimull | Eric: I have one issue with a functional test case in nova on shared RP, can you help me understand where the issue is? here is the link | |
| 18:43:07 | karimull | Eric: http://paste.openstack.org/show/i9a6MRtJ1IUxF9xlI8to/ | |
| 18:45:49 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Add action initiator attribute to the instance payload https://review.openstack.org/536243 | |
| 18:52:11 | karimull | efried: oops! pasted with wrong handle | |
| 18:52:31 | efried | karimull: looking... | |
| 18:52:48 | karimull | efried:thank you.. | |
| 18:53:05 | efried | karimull: What patch are you working on this test for? | |
| 18:57:01 | karimull | efried:https://review.openstack.org/#/c/569498/9 | |
| 18:57:56 | efried | karimull: Is the rebuild succeeding | |
| 18:57:57 | efried | ? | |
| 18:58:21 | efried | karimull: This is trying to rebuild to the same host? | |
| 18:59:27 | openstackgerrit | Merged openstack/nova master: conf: Correct documentation for '[pci] passthrough_whitelist' https://review.openstack.org/552874 | |
| 18:59:33 | openstackgerrit | Merged openstack/nova master: [doc] enhance admin/configuration/api.rst https://review.openstack.org/566008 | |
| 18:59:40 | openstackgerrit | Merged openstack/nova master: Remove mox in sec group test and functional tests https://review.openstack.org/576751 | |
| 19:00:21 | karimull | Efried:server state is Active after rebuild, but the DISK_GB usage is not accounted? | |
| 19:00:40 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Time how long pre_live_migration() takes https://review.openstack.org/574481 | |
| 19:00:50 | efried | karimull: or it got the disk from the compute node. | |
| 19:00:56 | efried | karimull: Where is the flavor defined? | |