| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-03-30 | |||
| 20:49:07 | mriedem | yeah it's ComputeVirtAPI in this case | |
| 20:49:58 | mriedem | i don't think using FakeVirtAPI will help, since that also defines wait_for_instance_event as a context manager | |
| 20:50:00 | mriedem | it's just a noop | |
| 20:50:25 | openstackgerrit | Arvind Nadendla proposed openstack/nova master: Update ImageMetaProp object to expose traits https://review.openstack.org/557795 | |
| 20:50:27 | fried_bunny | mriedem: No, was just going to suggest the possibility of using a mock.patch('....ComputeVirtAPI') instead of a mock.patch.object. | |
| 20:50:46 | fried_bunny | mriedem: But okay, I think I see the problem. | |
| 20:50:51 | fried_bunny | Try this: | |
| 20:51:26 | fried_bunny | wait_for_event.return_value.__enter__.side_effect = exc | |
| 20:51:26 | fried_bunny | with mock.patch.object(self.compute.virtapi, 'wait_for_instance_event') as wait_for_event: | |
| 20:51:41 | fried_bunny | you had a different mistake in each attempt. | |
| 20:51:52 | mriedem | i just tried that, | |
| 20:51:59 | mriedem | believe me, i've tried about every iteration of this | |
| 20:52:07 | mriedem | anyway, trying something else here | |
| 20:54:12 | fried_bunny | mriedem: I'm not sure that mocking wait_for_instance_event as if it were a context manager is going to do what you expect. | |
| 20:54:25 | fried_bunny | (well, clearly not) | |
| 20:54:40 | fried_bunny | because it's using the decorator instead of being a real context manager. | |
| 20:55:12 | fried_bunny | w4ie.side_effect = exc | |
| 20:55:12 | fried_bunny | with mock.patch('nova.compute.manager.ComputeVirtAPI#wait_for_instance_event') as w4ie: | |
| 20:55:12 | fried_bunny | You could try | |
| 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 | mriedem | since ocata | |
| 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 | imacdonn | mriedem: see Sylvain's comment on https://bugs.launchpad.net/nova/+bug/1751692 | |
| 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 | 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? | |