Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2018-07-03
16:29:43 mriedem wouldn't we just implement obj_load_attr (lazy-load) for Flavor.disabled?
16:30:14 dansmith we could, but that also seems obscure to me,
16:30:28 dansmith because the flavor could be coming from the instance, or from the actual db
16:30:40 dansmith and we wouldn't want the disabled default to affect the latter
16:30:54 dansmith it seems like we should patch it up if/when we're loading from extra and notice that field is gone
16:30:54 mriedem so then add default=False to 'disabled': fields.BooleanField(), ?
16:31:29 dansmith that's basically the same as the lazy-load, although slightly less icky, and one of the things I suggested in the original patch
16:32:22 mriedem ok so maybe handle in _flavor_from_db
16:32:25 mriedem Instance._flavor_from_db
16:32:31 mriedem if 'disabled' not in flavor_info
16:32:39 mriedem flavor_info['disabled'] = False
16:32:41 dansmith exactly
16:32:45 mriedem ok i'll push that up
16:32:46 gibi mriedem, dansmith I got it
16:32:48 dansmith because there we know what we're dealing with
16:33:00 dansmith and a comment about old instance flavors will be quite relevant
16:33:47 dansmith _load_flavor will get it implicitly if we're actually lazy-loading instance.flavor too, so that is good
16:34:35 zzzeek nova peeps, dansmith / mriedem , what happens when someone in Newton sets "workers=0" in nova.conf, does that mean default to number of CPUs ?
16:35:00 gibi mriedem, dansmith: thanks for the help
16:36:49 mriedem dansmith: gibi: pre-tests https://gist.github.com/mriedem/3f1560a7e9697ade633ab11b6aab8c62
16:37:01 mriedem zzzeek: yes
16:37:05 zzzeek mriedem: thanks!
16:37:10 dansmith mriedem: sure
16:37:54 gibi mriedem: LGTM
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 e ", "title": "Conflict"}]}
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: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.

Earlier   Later