Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2018-03-30
20:55:12 fried_bunny You could try
20:55:12 fried_bunny with mock.patch('nova.compute.manager.ComputeVirtAPI#wait_for_instance_event') as w4ie:
20:55:12 fried_bunny w4ie.side_effect = exc
20:55:22 fried_bunny s/#/./
20:55:41 mriedem as far as i can tell, what i'm doing is basically the same as https://github.com/openstack/nova/blob/master/nova/tests/unit/compute/test_compute_api.py#L1749
20:55:46 mriedem just a different context manager
20:56:15 fried_bunny ahh
20:56:28 mriedem but wait a sec, i might have something else screwed up here
20:56:48 fried_bunny I don't actually know whether side_effect does the right thing for yield. Though I guess it should...
21:03:18 mriedem got it
21:03:28 mriedem it wasn't even the mock, it was the thing i was using as the exc
21:03:35 mriedem i knew it would be dumb
21:03:55 fried_bunny mriedem: Good, because I just tried it out in mini form and it's all working like it's sposedta
21:18:23 imacdonn mriedem: around ?
21:24:32 mriedem depends
21:24:50 imacdonn mriedem: heh ... looking at https://review.openstack.org/#/c/554759/
21:25:07 imacdonn mriedem: I think that's the wrong fix ... I think the right fix is to remove the check entirely
21:26:04 mriedem ?
21:26:08 mriedem placement is required to start nova-compute
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.

Earlier   Later