| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-03-30 | |||
| 18:19:29 | fried_bunny | claudiub|2: If https://review.openstack.org/557975 doesn't fix us, we'll use that. | |
| 18:19:45 | claudiub|2 | gotcha. :) | |
| 18:20:10 | claudiub|2 | mriedem: yeah, usually when there's a fire somewhere. :D | |
| 18:20:43 | claudiub|2 | because you know, a lot of fire leads to getting fired. ha. | |
| 18:21:02 | mriedem | it is that time of the quarter | |
| 18:35:45 | stephenfin | melwitt: RE: bug 1750672, I already fixed that with https://review.openstack.org/#/c/556894/2 | |
| 18:35:45 | openstack | bug 1750672 in OpenStack Compute (nova) "failure to generate Nova's doc in Python 3.6" [Medium,Confirmed] https://launchpad.net/bugs/1750672 | |
| 18:37:05 | openstackgerrit | Merged openstack/nova stable/pike: Return 400 when compute host is not found https://review.openstack.org/550707 | |
| 18:57:28 | fried_bunny | melwitt: On vacation, yah? | |
| 18:58:12 | mriedem | yes, mandatory pto | |
| 19:03:21 | fried_bunny | leakypipes: I answered here https://review.openstack.org/#/c/533821/ -- in your estimation was there something more she was expecting to see in the tests? | |
| 19:11:43 | openstackgerrit | Merged openstack/nova stable/pike: Always deallocate networking before reschedule if using Neutron https://review.openstack.org/555907 | |
| 19:20:10 | fried_bunny | leakypipes: Likewise https://review.openstack.org/#/c/520246/ | |
| 19:27:28 | openstackgerrit | Takashi NATSUME proposed openstack/nova master: Remove unnecessary code encoding specification https://review.openstack.org/557903 | |
| 19:53:36 | leakypipes | fried_bunny: yeah, I'm just not sure :( | |
| 20:05:48 | fried_bunny | mriedem: If you were waiting for my nod on https://review.openstack.org/#/c/553122/ it's done. | |
| 20:06:12 | mriedem | ack | |
| 20:13:12 | openstackgerrit | Arvind Nadendla proposed openstack/nova master: Update ImageMetaProp object to expose traits https://review.openstack.org/557795 | |
| 20:16:57 | mriedem | gah, trying to mock context managers, my old nemesis | |
| 20:20:09 | openstackgerrit | Arvind Nadendla proposed openstack/nova master: Update ImageMetaProp object to expose traits https://review.openstack.org/557795 | |
| 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 | with mock.patch.object(self.compute, 'virtapi') as fake_virtapi: | |
| 20:39:13 | mriedem | fake_virtapi.return_value.wait_for_instance_event.side_effect = exc | |
| 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 | with mock.patch.object(self.compute.virtapi, | |
| 20:42:40 | mriedem | 'wait_for_instance_event') as wait_for_event: | |
| 20:42:40 | mriedem | wait_for_event.return_value.__enter__.return_value.side_effect = exc | |
| 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 | with mock.patch.object(self.compute.virtapi, 'wait_for_instance_event') as wait_for_event: | |
| 20:51:26 | fried_bunny | wait_for_event.return_value.__enter__.side_effect = exc | |
| 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 | 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 | |