| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-03-30 | |||
| 21:39:30 | mriedem | *his | |
| 21:40:33 | imacdonn | mriedem fried_bunny dansmith: it can wait (from my perspective) ... I was mostly treating it as "low-hanging fruit" | |
| 21:42:36 | fried_bunny | imacdonn: Are you interested in writing some code for this? | |
| 21:43:53 | imacdonn | fried_bunny: I was interested in a simple patch to remove the check and associated unit-test... beyond that, probably not really (have bigger fish to fry) | |
| 21:44:12 | fried_bunny | imacdonn: The fix wouldn't be much more than that. | |
| 21:44:22 | imacdonn | heheh ... or bunnies to fry, as the case may be ;) | |
| 21:44:54 | fried_bunny | raise exception.PlacementNotConfigured() | |
| 21:44:54 | fried_bunny | except: | |
| 21:44:54 | fried_bunny | self.reportclient.get('/') | |
| 21:44:54 | fried_bunny | try: | |
| 21:44:54 | fried_bunny | imacdonn: Instead of just removing the check, replace it with something like: | |
| 21:46:28 | imacdonn | I can give that a go ... I actually want to see what happens if placement is not configured and there's no check first, though | |
| 21:47:39 | fried_bunny | coolcool. Feel free to add me (efried) to the review if you do decide to spin something up. | |
| 21:48:33 | imacdonn | fried_bunny mriedem http://paste.openstack.org/show/718043/ | |
| 21:48:55 | imacdonn | that's what I get if there's no [placement] section in my compute's nova.conf | |
| 21:49:21 | imacdonn | looks pretty obvious to me | |
| 21:49:56 | fried_bunny | imacdonn: ...and you removed that region_name check? | |
| 21:49:58 | imacdonn | although maybe with different scheduler config, it'd be less-so | |
| 21:50:06 | imacdonn | yeah, I commented out the check | |
| 21:52:08 | fried_bunny | raise... | |
| 21:52:08 | fried_bunny | if self.reportclient.get('/') is None: | |
| 21:52:08 | fried_bunny | imacdonn: I amend my suggested code change above. It should be more like: | |
| 21:53:05 | fried_bunny | Then you'll get that auth warning as well as the PlacementNotConfigured exception. | |
| 21:57:46 | imacdonn | hmm, doesn't seem to be working ... still experimenting | |
| 21:58:30 | fried_bunny | oh | |
| 21:59:12 | fried_bunny | imacdonn: You don't need auth to get the version document. So if placement is actually running and there's a service catalog entry for it, that will succeed. | |
| 21:59:32 | imacdonn | well, it's getting the MissingAuthPlugin, so it doesn't actually get to raise the PlacementNotConfigured | |
| 21:59:37 | fried_bunny | Right. | |
| 21:59:47 | fried_bunny | I assume you have the service running and there's an entry for it in the service catalog? | |
| 21:59:57 | fried_bunny | ...but no [placement] section in your conf. | |
| 22:00:00 | imacdonn | right | |
| 22:00:28 | fried_bunny | The version document doesn't require authentication, and we'll get the endpoint from the service catalog, so get('/') will actually work. And then you'll blow up later when you try to hit a real URI. | |
| 22:00:37 | fried_bunny | And I contend that's actually the behavior we want. | |
| 22:00:54 | fried_bunny | If you shut down your placement service and try again, I think you'll get the PlacementNotConfigured error. | |
| 22:01:45 | imacdonn | that's not really what this check was intended for, though | |
| 22:01:53 | imacdonn | (originally, at least) | |
| 22:02:48 | fried_bunny | well, it was to make sure you had "configured" placement. It didn't previously do anything to ensure you'd configured it *right*. Heck, it wasn't even checking a thing that you had to have, clearly. | |
| 22:03:27 | imacdonn | in my interpretation, it was to check that you had the compute service configured to use placement, not to check that placement is functional | |
| 22:04:09 | fried_bunny | okay, I can buy that interpretation. In which case maybe auth stuff not being empty is more appropriate to check for in here. | |
| 22:05:00 | fried_bunny | because correct me if I'm wrong, but you *can't* get by without e.g. [placement]auth_type ? | |
| 22:05:35 | imacdonn | I thought so, but Matt said something about different scheduler options, so I lost confidence for a bit | |
| 22:06:09 | fried_bunny | shit, if there's a scheduler option that doesn't use placement, then why are we enforcing that you have it set up when you're not using that scheduler option. | |
| 22:06:45 | imacdonn | "mriedem> but we do want the computes putting inventory information into placement so we can eventually migrate CachingScheduler users" | |
| 22:06:48 | fried_bunny | Whatever, this is probably a better thing to discuss on Monday or Tuesday when people who were around for the inception of this check are back from worshipping egg-laying rabbits. | |
| 22:07:26 | imacdonn | you mean chocolate-egg-laying rabbits, of course | |
| 22:12:11 | imacdonn | try: | |
| 22:12:12 | imacdonn | raise exception.PlacementNotConfigured() | |
| 22:12:12 | imacdonn | except keystone_exception.MissingAuthPlugin: | |
| 22:12:12 | imacdonn | 'some sort of exception here') | |
| 22:12:12 | imacdonn | log.error('placement is not working - I should raise ' | |
| 22:12:12 | imacdonn | if self.reportclient.get('/') is None: | |
| 22:12:31 | imacdonn | (maybe - "thinking out loud") | |
| 22:16:31 | fried_bunny | imacdonn: MissingAuthPlugin will never happen there. | |
| 22:16:45 | imacdonn | it does, if the placement config is missing | |
| 22:16:48 | fried_bunny | It gets swallowed by @safe_connect | |
| 22:16:59 | fried_bunny | No, you get a warning about it, but the exception doesn't get raised. | |
| 22:17:11 | imacdonn | I tried it | |
| 22:17:38 | imacdonn | I mean - I tried the code above, with no placement config, and it did what I expected | |
| 22:17:40 | fried_bunny | that pastebin you showed me had the warning right above an unrelated exception. Did you see something different another way? | |
| 22:18:30 | imacdonn | I think that unrelated warning was caused by something that the scheduler happened to do that time ... I don't usually see that warning on startup | |
| 22:19:43 | fried_bunny | The warning was coming from the report client trying to bootstrap the compute node inventory. | |
| 22:19:46 | fried_bunny | through placement | |
| 22:20:24 | fried_bunny | which hits @safe_connect, which catches MissingAuthPlugin and prints that warning.... and then does nothing. Like, implicitly returns None. Which is why you got that NoneType blah blah error. | |
| 22:20:35 | imacdonn | http://paste.openstack.org/show/718047/ | |
| 22:21:10 | imacdonn | that's with code pasted above, and missing config | |
| 22:22:36 | fried_bunny | imacdonn: Are you running on master? | |
| 22:22:46 | imacdonn | no, this is queens | |
| 22:23:20 | fried_bunny | if you curl the base placement URI, do you get the version document or a 401? | |
| 22:23:55 | imacdonn | {"versions": [{"min_version": "1.0", "max_version": "1.17", "id": "v1.0"}]} | |
| 22:24:44 | fried_bunny | oh - .get isn't wrapped by safe_connect. Let me see where that MissingAuthPlugin business is coming from. | |
| 22:25:10 | fried_bunny | though it would be easier for you to find out - by removing the try/except and seeing what .get raises all by itself. | |
| 22:25:30 | imacdonn | can do | |
| 22:25:57 | fried_bunny | is my guess. | |
| 22:25:57 | fried_bunny | ...something in ksa loading... | |
| 22:25:57 | fried_bunny | get_ksa_adapter | |
| 22:25:57 | fried_bunny | _create_client | |
| 22:26:22 | fried_bunny | load_auth_from_conf_options | |
| 22:26:27 | imacdonn | http://paste.openstack.org/show/718048/ | |
| 22:29:10 | fried_bunny | oh, interesting - we actually let you load up the ksa adapter; and it fails on the request. But still, that's weird; you shouldn't need auth to get the version document. | |
| 22:30:16 | fried_bunny | What happens when you shut down the placement service? | |
| 22:30:59 | imacdonn | ConnectFailure | |
| 22:31:39 | imacdonn | from ksa trying to do a GET request | |
| 22:32:30 | fried_bunny | mm | |
| 22:33:20 | fried_bunny | Well, I'm not happy that you're getting MissingAuthPlugin for /. But it's what you'll get for anything else you try, so that's not the end of the world. | |
| 22:34:07 | imacdonn | I guess that the client doesn't know that auth is not required to get the version | |
| 22:34:26 | fried_bunny | mordred will not be happy about that. Or maybe it's my fault. | |
| 22:35:40 | fried_bunny | anyway, it's sounding like to cover bases we may want to do something like .get('/resource_providers?name=bogus'), which *should* require auth, and catch both MissingAuthPlugin and ConnectFailure and convert those to PlacementNotConfigured. | |
| 22:37:23 | fried_bunny | imacdonn: If that's more than you want to take on, or if you want to write part of it and then hand it off, put something somewhere and flag me on it. | |
| 22:39:41 | imacdonn | fried_bunny: that doesn't seem too bad ... have to think through unit test implications too, though | |
| 22:40:19 | fried_bunny | I'd be fine just mocking .get. One case to raise MissingAuthPlugin, one to raise ConnectError. Done. | |
| 22:40:34 | fried_bunny | (and of course one to make it return successfully) | |
| 22:40:37 | imacdonn | yeah, that makes sense | |
| 22:40:39 | fried_bunny | (which is probably already covered elsewhere) | |
| 22:41:01 | imacdonn | I'll fiddle with that a bit ... and maybe we can discuss further with the others next week | |
| 22:41:37 | fried_bunny | Sounds great. | |
| 22:41:52 | imacdonn | thanks! :) | |
| 22:46:55 | imacdonn | just '/resource_providers' should work? That seems to require auth, and provides a meaningful result | |
| 22:53:55 | fried_bunny | imacdonn: If you had a lot of resource providers, it could be slowish. Adding the ?name=bogus should make it very quick (even if you have a provider named 'bogus') | |
| 22:54:32 | fried_bunny | I *think* that returns a result with an empty payload (as opposed to a 404). | |
| 22:54:41 | imacdonn | will try it | |