| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-03-30 | |||
| 21:26:11 | imacdonn | mriedem: see Sylvain's comment on https://bugs.launchpad.net/nova/+bug/1751692 | |
| 21:26:11 | openstack | Launchpad bug 1751692 in OpenStack Compute (nova) "os_region_name an unnecessary required option for placement " [Low,Triaged] - Assigned to Digambar (digambarpatil15) | |
| 21:26:11 | mriedem | since ocata | |
| 21:26:51 | imacdonn | Isn't the recommendation to only specify region_name if you need to override what keystone offers ? | |
| 21:27:07 | mriedem | i don't think region_name gets a default | |
| 21:27:22 | imacdonn | and it doesn't *need* a default | |
| 21:27:28 | mriedem | https://docs.openstack.org/nova/latest/configuration/config.html#placement.os_region_name | |
| 21:28:24 | imacdonn | the check was originally put there for people upgrading from newton to ocata, to catch the case where they didn't notice that placement must be configured (where it was not required before) | |
| 21:29:24 | imacdonn | but we're way past that now .. if you don't have placement configured, it'll be very obvious | |
| 21:29:40 | mriedem | well, | |
| 21:29:52 | mriedem | if you're using the FilterScheduler yeah, but not if you're using the CachingScheduler which doesn't use placement | |
| 21:30:12 | mriedem | but we do want the computes putting inventory information into placement so we can eventually migrate CachingScheduler users | |
| 21:30:35 | mriedem | idk, maybe it's ok to remove at this point, | |
| 21:30:45 | mriedem | we don't fail to start nova-compute if you're using neutron but dont have [neutron] creds configured | |
| 21:30:47 | mriedem | but that's required also | |
| 21:30:55 | imacdonn | I forget how it failed for me when I had no [placement] config, but it was pretty obvious | |
| 21:31:00 | openstackgerrit | Merged openstack/nova master: remove unnecessary short cut in placement https://review.openstack.org/553122 | |
| 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 | 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: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: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: Teardown networking when rolling back live migration even if shared disk https://review.openstack.org/555481 | |
| 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:20 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Avoid unnecessary port update during live migration https://review.openstack.org/556332 | |
| 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:21 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Add "bind_ports_to_host" neutron API method https://review.openstack.org/523604 | |
| 21:38:21 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Add VIFMigrateData object for live migration https://review.openstack.org/515423 | |
| 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 "delete_port_binding" network API method https://review.openstack.org/552170 | |
| 21:38:23 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Add "activate_port_binding" neutron API method https://review.openstack.org/555947 | |
| 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:24 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Implement migrate_instance_start method for neutron https://review.openstack.org/556334 | |
| 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:25 | openstackgerrit | Matt Riedemann proposed openstack/nova master: WIP: Port binding based on events during live migration https://review.openstack.org/434870 | |
| 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: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: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 | imacdonn: Instead of just removing the check, replace it with something like: | |
| 21:44:54 | fried_bunny | try: | |
| 21:44:54 | fried_bunny | self.reportclient.get('/') | |
| 21:44:54 | fried_bunny | except: | |
| 21:44:54 | fried_bunny | raise exception.PlacementNotConfigured() | |
| 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 | imacdonn: I amend my suggested code change above. It should be more like: | |
| 21:52:08 | fried_bunny | if self.reportclient.get('/') is None: | |
| 21:52:08 | fried_bunny | raise... | |
| 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 | if self.reportclient.get('/') is None: | |
| 22:12:12 | imacdonn | log.error('placement is not working - I should raise ' | |
| 22:12:12 | imacdonn | 'some sort of exception here') | |
| 22:12:12 | imacdonn | except keystone_exception.MissingAuthPlugin: | |
| 22:12:12 | imacdonn | raise exception.PlacementNotConfigured() | |
| 22:12:31 | imacdonn | (maybe - "thinking out loud") | |