Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2018-07-03
16:49:39 efried sambetts: Sorry, it's been a day. Can you tell me where/how to run this thing?
16:50:16 efried If I push a nova patch, do I have to push something in ironic with a Depends-On? Looks like an experimental job, needs special incantation?
16:51:30 efried sambetts: Can I mod the commit message on https://review.openstack.org/#/c/514312/ to add the Depends-On?
16:52:26 openstackgerrit Jay Pipes proposed openstack/nova master: move lookup of provider from _new_allocations() https://review.openstack.org/579920
16:52:27 openstackgerrit Jay Pipes proposed openstack/nova master: placement: delete auto-created consumers on fail https://review.openstack.org/579921
16:55:05 openstackgerrit Eric Fried proposed openstack/nova master: WIP: Delete orphan nodes before updating resources https://review.openstack.org/579922
16:57:14 sambetts efried: you can mod the commit message to add the Depends-On, then you need to leave a "cisco-experimental" comment to trigger our experimental job queue which has some other fixes for our ironic job
16:57:30 efried sambetts: Roger wilco, it's on the way...
16:57:39 sambetts efried: thanks so much for looking into this!
16:57:43 efried sambetts: the experimental fix is ---^ btw
16:58:25 sambetts efried: oh nice thats quite an eligant fix
16:59:15 efried sambetts: I think it's probably going to work, but I don't think that's the end of it. We're seeing similar races in other places as well (though not as consistently), so I think I really need to implement that retry loop I mentioned earlier.
17:01:15 sambetts yeah I feel like the whole compute claiming thing (particularly with ironic) is really racey, I still think we have the race to do with a claim being released on an ironic node that then goes into cleaning, doesn't get removed from the resource tracker before a new claim can be made on it
17:01:39 sambetts but thats another story
17:01:53 dansmith sambetts: there's no compute claiming anymore
17:03:38 sambetts might be using the wrong words, still a race between the scheduler and the resource tracker
17:03:39 efried dansmith: TL;DR we're running into races now because rt._update is run both during periodic and from instance_claim. Both of them hit update_[from_]provider_tree asynchronously, and freak each other out.
17:03:45 efried Yeah, what sambetts said.
17:04:15 dansmith there should be no changes coming from resource tracker
17:04:35 efried The periodic is designed to be self-healing, but if the race makes the claim side fail, that's bad.
17:04:41 dansmith or do you mean a race between an allocation and inventory changes/
17:05:24 efried dansmith: Yeah, or aggregate changes, which are happening a relatively lot now that we're mirroring host aggs to placement.
17:05:46 openstackgerrit Matt Riedemann proposed openstack/nova master: Default embedded instance.flavor.disabled attribute https://review.openstack.org/579925
17:06:03 mriedem dansmith: gibi: ^
17:06:13 dansmith well, I guess I see that as a major improvement over the races we used to have between scheduling and claiming (on the compute)
17:06:29 dansmith but yeah, "compute claiming" is the wrong term since that's not what happens now
17:06:31 efried dansmith: What we're seeing in tempest envs is aggs being added/removed as hosts are brought up & down in concurrent tests.
17:06:46 dansmith hosts aren't brought up and down in tempest
17:06:50 efried dansmith: I've been thinking the real solution will be a retry loop around this chunk: https://github.com/openstack/nova/blob/master/nova/compute/resource_tracker.py#L883-L914
17:06:53 dansmith but tempest may be creating/destroying aggregates
17:07:51 efried dansmith: Here's an example: http://logs.openstack.org/69/556669/11/check/nova-multiattach/b3195af/job-output.txt.gz#_2018-06-22_14_50_02_095506
17:08:23 efried Adding agg generation checking to the report client exposes it pretty badly.
17:08:41 dansmith that message means you didn't remove all the hosts from the agg before trying to delete the agg
17:08:55 dansmith that shouldn't have anything to do with inventory or instances being booted
17:09:15 dansmith perhaps means we failed to remove a host because a call to placement (for mirroring) failed or something like that
17:09:18 efried Actually, that message doesn't make sense to me.
17:10:07 efried can't remove X from {X, ...} because {X, ...} isn't empty? Well of course it isn't - it contains X. Or am I misinterpreting that? Is the first aggregate in that sentence the placement aggregate?
17:10:42 dansmith it's saying "can't delete aggregate "foo" because "foo" still contains some host "bar""
17:12:19 sambetts|afk efried: thanks again for looking into that stuff, I'll check the results in the morning, night all o/
17:12:30 efried o/
17:12:41 efried dansmith: That's not how it's worded, perhaps we could stand to fix thta.
17:13:03 dansmith I'm not sure why it's confusing.. it makes sense to me
17:13:12 dansmith you did a delete of aggregate 2
17:13:18 dansmith can't remove aggregate 2 because it's not empty
17:13:45 dansmith oh, I see the confusing part "cannot remove host"
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.

Earlier   Later