| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-03-30 | |||
| 20:29:58 | fried_bunny | mriedem: Ditto https://review.openstack.org/#/c/533396/ | |
| 20:30:07 | fried_bunny | mriedem: I can help with that if you like. Or were you just grumbling? | |
| 20:30:52 | mriedem | i think i've got a way around it | |
| 20:39:12 | mriedem | ugh | |
| 20:39:13 | mriedem | fake_virtapi.return_value.wait_for_instance_event.side_effect = exc | |
| 20:39:13 | mriedem | with mock.patch.object(self.compute, 'virtapi') as fake_virtapi: | |
| 20:39:19 | mriedem | i must be blind | |
| 20:39:32 | mriedem | that is not hitting | |
| 20:42:39 | mriedem | or | |
| 20:42:40 | mriedem | wait_for_event.return_value.__enter__.return_value.side_effect = exc | |
| 20:42:40 | mriedem | 'wait_for_instance_event') as wait_for_event: | |
| 20:42:40 | mriedem | with mock.patch.object(self.compute.virtapi, | |
| 20:43:29 | fried_bunny | mriedem: Is self.compute.virtapi a method or a property? | |
| 20:43:42 | mriedem | it's an attribute | |
| 20:43:48 | fried_bunny | mriedem: Then take out .return_value | |
| 20:43:49 | mriedem | with a wait_for_instance_event method | |
| 20:44:14 | fried_bunny | (in the first paste) | |
| 20:46:39 | mriedem | i don't think the first one will work, | |
| 20:46:48 | mriedem | wait_for_instance_event is a context manager, | |
| 20:46:52 | mriedem | so need to mock the __enter__ | |
| 20:47:41 | fried_bunny | okay, but fake_virtapi.return_value will never hit, because it's not self.compute.virtapi().wait_for_instance_event - just self.compute.virtapi.wait_for_instance_event, right? | |
| 20:47:51 | fried_bunny | so that's a start... | |
| 20:48:16 | mriedem | right | |
| 20:48:46 | fried_bunny | mriedem: Is it FakeVirtAPI you're actually using here? | |
| 20:48:52 | mriedem | no | |
| 20:48:59 | fried_bunny | ComputeVirtAPI? | |
| 20:49:00 | mriedem | also, trying to model this after https://github.com/openstack/nova/blob/master/nova/tests/unit/compute/test_compute_api.py#L1749 | |
| 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 | |