| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-03-30 | |||
| 21:31:07 | imacdonn | yeah | |
| 21:31:32 | imacdonn | I think it was a safety check that maybe made sense at the time, but it's not needed now | |
| 21:32:00 | imacdonn | ... and, if such a check really is needed, it should check some config option that's actually required | |
| 21:32:01 | mriedem | maybe you can convince fried_bunny to update https://review.openstack.org/#/c/557086/ to do what you want | |
| 21:32:13 | imacdonn | region_name is not required to be set, other than by this check | |
| 21:33:34 | fried_bunny | I was wondering about that. | |
| 21:33:51 | imacdonn | that does look related .... I think if we remove the check, that test would be removed with it | |
| 21:34:19 | fried_bunny | agree with that. | |
| 21:34:34 | fried_bunny | But yeah, I was wondering why we were checking for region_name, since I didn't think it was required. | |
| 21:36:19 | openstack | bug 1751692 in OpenStack Compute (nova) "os_region_name an unnecessary required option for placement " [Low,Triaged] https://launchpad.net/bugs/1751692 - Assigned to Digambar (digambarpatil15) | |
| 21:36:19 | imacdonn | yeah, so... we could either update your change, or abandon it and do a new one ... either way, it should "Closes-Bug: 1751692" | |
| 21:36:45 | fried_bunny | imacdonn: Any idea what *would* be a required opt in the [placement] section? | |
| 21:37:33 | mriedem | watch out | |
| 21:38:01 | fried_bunny | mriedem: Would it be the worst thing for us to change that check to actually go grab the version document from placement as a (better) way of assuring that it's configured? | |
| 21:38:19 | openstackgerrit | Matt Riedemann proposed openstack/nova master: DRY up test_rollback_live_migration_set_migration_status https://review.openstack.org/555489 | |
| 21:38:19 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Teardown networking when rolling back live migration even if shared disk https://review.openstack.org/555481 | |
| 21:38:20 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Add check if neutron "binding-extended" extension is available https://review.openstack.org/523548 | |
| 21:38:20 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Avoid unnecessary port update during live migration https://review.openstack.org/556332 | |
| 21:38:21 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Add VIFMigrateData object for live migration https://review.openstack.org/515423 | |
| 21:38:21 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Add "bind_ports_to_host" neutron API method https://review.openstack.org/523604 | |
| 21:38:22 | openstackgerrit | Matt Riedemann proposed openstack/nova master: WIP: libvirt: use dest host vif migrate details for live migration https://review.openstack.org/551370 | |
| 21:38:23 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Add "activate_port_binding" neutron API method https://review.openstack.org/555947 | |
| 21:38:23 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Add "delete_port_binding" network API method https://review.openstack.org/552170 | |
| 21:38:24 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Implement migrate_instance_start method for neutron https://review.openstack.org/556334 | |
| 21:38:24 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Delete port bindings in setup_networks_on_host if teardown=True https://review.openstack.org/556333 | |
| 21:38:25 | openstackgerrit | Matt Riedemann proposed openstack/nova master: WIP: Port binding based on events during live migration https://review.openstack.org/434870 | |
| 21:38:25 | openstackgerrit | Matt Riedemann proposed openstack/nova master: WIP: compute: use port binding extended API during live migration https://review.openstack.org/551371 | |
| 21:38:26 | openstackgerrit | Matt Riedemann proposed openstack/nova master: WIP: Wait for network-vif-plugged before starting live migration https://review.openstack.org/558001 | |
| 21:38:26 | openstackgerrit | Matt Riedemann proposed openstack/nova master: conductor: use port binding extended API in during live migrate https://review.openstack.org/522537 | |
| 21:39:06 | mriedem | fried_bunny: imacdonn: idk, definitely not something i care to think about when i'm about 10 minutes from a 4 day break | |
| 21:39:26 | mriedem | i'm sure dansmith would like to make this is first thing monday morning | |
| 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"}]} | |