Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2018-04-09
20:30:06 jmlowe_ how convenient
20:32:37 mriedem i wanted to avoid fixing the shelve / clear case in the same patch since that gets slightly more complicated, and not really needed to fix this bug
20:32:56 mriedem unless your production env is 1 compute host...
20:33:01 mriedem maybe it's a mainframe
20:53:42 openstackgerrit Jay Pipes proposed openstack/nova master: mirror nova host aggregate members to placement https://review.openstack.org/553597
21:12:39 tblakes dansmith: Could you please take a look at https://review.openstack.org/#/c/559158/ if you have a chance. It's gotten a +2 from Matt Riedemann but still needs to get a +1 on workflow. It's a cherry pick from master to stable/queens.
21:51:35 imacdonn dansmith: if you have a moment, could you evaluate https://review.openstack.org/#/c/558089/ , please?
22:00:16 openstackgerrit Eric Berglund proposed openstack/nova master: PowerVM: Add proc_units_factor conf option https://review.openstack.org/554688
22:04:15 dansmith imacdonn: mriedem: sorry, I just can't get on board with that
22:04:41 imacdonn dansmith: OK.... do you have another idea ?
22:05:37 dansmith imacdonn: I don't think it's something we need to do at all, TBH, but if we're going to, we should make it useful
22:05:47 imacdonn efried: heads-up
22:05:57 dansmith imacdonn: we're going to hork out the resource provider a couple seconds later, so do that early or something
22:05:59 dansmith but really,
22:06:19 dansmith we should be fine to just log the problem, explain the issue and whatever
22:06:26 imacdonn dansmith: my original idea was to do away with the check completely ... but mriedem was concerned about the case where the scheduler doesn't require placement, but we want to force it anyway, for reporting purposes
22:07:06 dansmith imacdonn: okay I'm not sure why the current config-based check doesn't support that scenario
22:07:51 imacdonn dansmith: the issue is that the current check uses an option that's not required .. so it barfs on a perfectly valid config
22:08:10 imacdonn dansmith: I suppose an alternative would be to check for auth_type instead of region_name
22:08:27 dansmith imacdonn: ah, right, well, then no check at startup like I was saying I guess
22:08:28 imacdonn dansmith: if auth_type is always going to be required .............
22:08:40 dansmith imacdonn: catch it when we go to fetch our RP record the first time, log the bejeezus out of it
22:09:02 efried IMO it would be nice to fail early with a nice message rather than let it fail "naturally" somewhere down the line.
22:09:08 imacdonn dansmith: I believe that there's a case where there would be no request for RP (depending on scheduler config)
22:09:28 efried right, and in ^ that case, "down the line" could be way later.
22:09:56 dansmith efried: we don't check all of config on startup, but also, isn't this a thing that could be fixed at runtime in the service catalog
22:09:56 dansmith ?
22:10:19 dansmith imacdonn: efried: no, the RP setup will happen just a few seconds after startup, when listing the nodes and running the resource tracker stuff
22:10:36 efried For this code path, you have to fix it by editing conf.
22:11:02 efried which I hear is going to be able to be picked up dynamically across the board soon anyway, so...
22:11:13 dansmith efried: it already can be
22:11:20 dansmith efried: if you mark these things as reloadable and they are,
22:11:22 dansmith then done deal
22:11:30 efried cool cool.
22:11:45 efried dansmith: So you're advocating removing the check entirely and letting it fail organically down the line
22:12:00 dansmith efried: if we can't do a legit check then yeah
22:12:07 efried Just so you're aware, @safe_connect has the potential to make that somewhat... unclean.
22:12:17 efried Wait, how is this check not legit?
22:12:24 dansmith "why the fsck are my computes requesting /foo/?bogus" would be high on my list
22:12:40 dansmith efried: I mean a legit check without spamming the logs in placement
22:12:59 dansmith a legit config-only check
22:13:05 efried one entry on startup is hardly spamming, but okay. auth_type it is, I guess.
22:13:41 dansmith I just restarted 1000 computes
22:13:55 imacdonn can we say (with reasonable confidence) that auth_type will always be a required option? TBH, I'm not sure why it doesn't default to 'password'
22:13:56 dansmith that's a lot of spam
22:14:15 efried This I cannot answer.
22:14:20 dansmith if I didn't, and I'm auditing logs, I find some /?bogus queries to placement and wonder who the fsck is on my network
22:14:29 dansmith and then realize it's ... mah compute nodes
22:14:34 dansmith I don't want to be on that customer call
22:15:06 efried imacdonn: In other places we go to load the auth from conf and error if it comes back None.
22:15:36 dansmith we call pre_start_hook before we even listen to RPC, and that calls update_available_resource, which will pull our RPs from placement
22:15:40 efried Which in this case might look a little bit like publicizing (bits of) SchedulerReportClient._create_client.
22:17:03 efried Nope, forget that.
22:18:06 efried So we could check for auth_type, and if not set, log a helpful warning, but not raise, and then let the failure happen organically down the line.
22:18:20 efried that's future-proof in the case where auth_type becomes not required.
22:18:27 dansmith so, we'd get the first failure before we even listen to RPC, run our first periodic, etc
22:24:58 imacdonn dansmith: so are you still advocating just removing the check entirely? I'd be cool with that, but mriedem may not be
22:25:19 dansmith imacdonn: if we can't do the check reliably based just what is in config, then yeah I think we should remove it
22:25:57 dansmith imacdonn: I also think that we're doing legit talking to placement so early in service startup, that we could mangle up some of that code to make it fail really hard if you really wanted to
22:26:10 dansmith but I also think that heavy logging of what we found when we reliably found it is completely legit
22:27:08 imacdonn dansmith: I guess my perception is that we want to fail hard, not just log something and continue anyway ..... perhaps because people only check logs if something is obviously not working (e.g. nova-compute service not coming up)
22:27:29 dansmith imacdonn: if compute can't report to placement, then things won't work
22:28:05 dansmith imacdonn: we do a similar thing because we're dependent on conductor.. we check at startup and log in a loop that we can't do anything
22:28:19 imacdonn dansmith: I've been led to believe that there's a scheduler config where it would work OK for the most part, but not report into placement
22:28:22 dansmith we have to do that late because we need RPC to be up before we can reliably know if conductor is up, just like this situation
22:28:43 dansmith imacdonn: if you're using cachingscheduler then you can avoid hitting placement in the scheduler
22:28:56 dansmith imacdonn: and you could argue that computes being misconfigured for placement would be "okay" in that situation
22:29:12 dansmith imacdonn: however, going forward, we *have* to migrate those people to having data in placement so we can get rid of that situation
22:29:19 imacdonn dansmith: right, I believe that that's the case that mriedem was concerned about
22:29:42 dansmith so that's a reason to hard fail, but again, if we can't reliably detect that except at runtime, then..we don't have much choice
22:30:11 dansmith imacdonn: what about this.. make the compute service self-disable if we fail to update our RPs in placement?
22:30:17 imacdonn I guess we can use auth_type ... It just feels a bit like making the same mistake a second time
22:30:20 dansmith we do that now for transient issues we find out about late, like libvirt stopping
22:30:27 dansmith we self-disable so that no new builds will come to us,
22:30:33 dansmith which is a big fat "I need help" to the operator
22:30:39 imacdonn interesting
22:30:45 dansmith and there's a reason field
22:30:57 dansmith self.disable(reason="you dun fscked up")
22:31:04 imacdonn heh
22:31:45 imacdonn I think I like that idea better
22:32:21 dansmith we also self-disable if too many builds fail for unknown reasons
22:32:39 imacdonn unit-testing for that could be "fun" ...
22:32:48 dansmith nah, 's easy
22:32:48 dansmith https://github.com/openstack/nova/blob/master/nova/compute/manager.py#L1701-L1709
22:34:17 dansmith imacdonn: https://review.openstack.org/#/c/463597/
22:34:22 dansmith lots of test examples in there
22:34:30 mriedem just rely on auth_type https://review.openstack.org/#/q/status:open+project:openstack/nova+branch:master+topic:bug/1761487
22:34:35 mriedem we are doing that for neutron and cinder too
22:34:59 dansmith if that's legit, then sure
22:36:32 mriedem let me also state, for the record, that i don't care too much about this and said so awhile back http://eavesdrop.openstack.org/irclogs/%23openstack-nova/%23openstack-nova.2018-03-30.log.html#t2018-03-30T21:39:06
22:37:02 mriedem i definitely didn't think about the compute restart storm
22:38:18 mriedem and now i must attend to 1st grade homework
22:38:42 imacdonn efried: any objection to using auth_type ?
22:39:07 efried imacdonn: No. If they ever make that non-optional, I suspect many unintended side effects will result.
22:39:15 efried s/non-//
22:39:54 imacdonn yeah, and it seems like it'd probably become non-optional for neutron at the same time
22:41:25 efried Oh, it would be a ksa thing, so it would become optional for the world
22:41:52 imacdonn right, that's what I meant
22:42:33 imacdonn so if we do the same for placement as we do for neutron, such a change would break both at the same type
22:43:06 efried more or less. more or less.

Earlier   Later