| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-08-08 | |||
| 21:59:09 | mriedem | "# Stub out the API to make the instance destroy raise InstanceNotFound, | |
| 21:59:09 | mriedem | # simulating if conductor already deleted it." | |
| 21:59:28 | mriedem | conductor only deletes the instance if conductor gets build request not found when it goes to delete the build request, right? | |
| 21:59:56 | mriedem | which only happens if _attempt_delete_of_buildrequest returns True right? | |
| 21:59:57 | melwitt | yes. I glossed over that from writing up all the other comments. O tjoml | |
| 22:00:05 | melwitt | *,I think | |
| 22:00:13 | melwitt | yes | |
| 22:00:20 | mriedem | so... | |
| 22:00:25 | mriedem | aren't the first and 2nd tests really the same thing? | |
| 22:00:40 | mriedem | or | |
| 22:00:45 | mriedem | i guess one is saying, | |
| 22:00:53 | melwitt | no because the first test will commit the reservations and the second one won't and will fall into the exception handling and rollback and refresh | |
| 22:00:54 | mriedem | the 2nd is saying, | |
| 22:01:15 | mriedem | between the time _lookup_instance returns the instance, another request deletes the build request, so conductor deletes the instance and then request 1 fails on instance.destroy()? | |
| 22:01:21 | melwitt | the first test never goes into the except InstanceNotFound block | |
| 22:01:43 | melwitt | yes | |
| 22:02:06 | mriedem | i'm not following "no because the first test will commit the reservations and the second one won't and will fall into the exception handling and rollback and refresh" | |
| 22:02:27 | mriedem | but both _lookup_instance and instance.destroy() are in the same try/except InstanceNotFound in _delete_while_booting | |
| 22:02:30 | melwitt | I just mean the first test will not cover the except InstanceNotFound block | |
| 22:02:38 | mriedem | so the point is, each test is covering two ways we could raise in there | |
| 22:02:41 | melwitt | _lookup_instance does not raise InstanceNotFound | |
| 22:02:52 | melwitt | it returns (None, None) if it catches InstanceNotFound | |
| 22:03:12 | mriedem | oh right | |
| 22:03:30 | mriedem | ok | |
| 22:04:37 | melwitt | so first case is _lookup_instance returns (None, None) because it found no instance. second case is instance was found but then instance.destroy() raises because conductor deleted it out from under us after we deleted the build request but before we got to instance.destroy() | |
| 22:05:19 | melwitt | *we deleted the build request, found the instance, and then it was gone in that small window when we got to destroy | |
| 22:06:09 | mriedem | ok | |
| 22:06:33 | mriedem | well, up to you on if you or dansmith want my comments addressed, if you don't care, then i don't really care much either and i'll +W | |
| 22:07:07 | mriedem | also, | |
| 22:07:33 | mriedem | you're sure you're not hitting this? https://github.com/openstack/nova/blob/stable/ocata/nova/compute/api.py#L1729 | |
| 22:07:33 | dansmith | I don't like the test on multiple fronts, | |
| 22:07:38 | mriedem | b/c i see you're not using the AllServicesCurrent fixture | |
| 22:07:51 | dansmith | but it seems like the fix is worth getting in, nits on the test aside | |
| 22:07:54 | melwitt | I'd rather not have to convert them to unit tests at this point but if you want me to change it, I will | |
| 22:08:29 | mriedem | no i'm not talking about full on re-writing them as unit tests | |
| 22:08:39 | mriedem | i just meant with stuff like the AllServicesCurrent fixture and NoopConductorFixture | |
| 22:09:06 | mriedem | but we must not be hitting https://github.com/openstack/nova/blob/stable/ocata/nova/compute/api.py#L1729 because your change to the tests in the next patch are ok | |
| 22:09:25 | melwitt | yeah ... seems like it would return early because of the service version but the test does pass after the fix, which means it's getting to the quota calls somehow | |
| 22:09:45 | dansmith | because there are no services? | |
| 22:09:53 | mriedem | the api service is running | |
| 22:09:54 | dansmith | I don't see why we want noopconductor over just not starting it | |
| 22:10:29 | dansmith | but it's all just forced because this should be a unit test where it doesn't matter anyway :) | |
| 22:10:49 | melwitt | SORRY GUYS for my blasphemous functional test | |
| 22:10:50 | mriedem | i'm just surprised that fixture isn't needed is all i'm saying | |
| 22:11:03 | mriedem | like, why do we even need it in the first place in other func tests is my point | |
| 22:11:31 | mriedem | thou shalt be smoten with review nit-pickery | |
| 22:11:34 | melwitt | we have definitely needed it before. I'm not sure what's going on here | |
| 22:11:46 | melwitt | re: ServicesCurrent | |
| 22:12:11 | dansmith | if you have no services you get back the current I think | |
| 22:12:18 | dansmith | we used to return zero, | |
| 22:12:23 | melwitt | the api service doesn't count as a service? | |
| 22:12:39 | melwitt | os_apicompute | |
| 22:13:01 | mriedem | idk, might have something to do with wsgi-intercept usage in OSAPIFixture now | |
| 22:13:03 | dansmith | you're not starting an api service for real here | |
| 22:13:05 | dansmith | right | |
| 22:13:14 | melwitt | O.o | |
| 22:13:19 | dansmith | but anyway if this was A UNIT TEST | |
| 22:13:28 | mriedem | we used to | |
| 22:13:29 | mriedem | self.osapi = service.WSGIService("osapi_compute") | |
| 22:13:29 | mriedem | self.osapi.start() | |
| 22:13:29 | mriedem | self.addCleanup(self.osapi.stop) | |
| 22:13:32 | melwitt | :***( | |
| 22:13:37 | mriedem | which is probably why AllServicesCurrent was added before | |
| 22:15:12 | mriedem | +W | |
| 22:15:32 | mriedem | moving onto soul crusher #2 | |
| 22:27:51 | mriedem | bazinga https://review.openstack.org/#/c/582413/8 | |
| 22:28:04 | melwitt | argh | |
| 22:29:05 | melwitt | ok that's not so bad | |
| 22:29:28 | mriedem | do you want/need to hold the functional test patch below it out to add checks for core and ram usage? | |
| 22:29:57 | mriedem | or just slap those into this one | |
| 22:30:41 | melwitt | I'm fine with adding them to the second one | |
| 22:50:07 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Docs: Add guide to migrate instance with snapshot https://review.openstack.org/584442 | |
| 23:00:18 | mriedem | heh https://review.openstack.org/#/c/382853/ | |
| 23:00:25 | mriedem | i knew i'd seen this recently | |
| 23:00:26 | mriedem | https://github.com/starlingx-staging/stx-nova/commit/71acfeae0d1c59fdc77704527d763bd85a276f9a#diff-77f9348ab09642ba46409b6828af4af0R3245 | |
| 23:00:32 | mriedem | oct of 2016 | |
| 23:04:48 | openstackgerrit | melanie witt proposed openstack/nova stable/ocata: [stable only] Handle quota usage during create/delete races https://review.openstack.org/582413 | |
| 23:11:45 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Update nova network info when doing rebuild for evacuate operation https://review.openstack.org/382853 | |
| 23:12:57 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Update nova network info when doing rebuild for evacuate operation https://review.openstack.org/382853 | |
| 23:15:24 | mriedem | wow so that evacuate fix was approved in march of 2017, | |
| 23:15:27 | mriedem | but apparently needed a rebase | |
| 23:15:34 | mriedem | and then no one ever mentioned needing re-approval | |
| 23:17:00 | melwitt | yikes | |
| 23:18:01 | openstackgerrit | Matt Riedemann proposed openstack/nova stable/pike: Fix instance evacuation with PCI devices https://review.openstack.org/590059 | |
| 23:29:22 | melwitt | I'm going to propose stable releases tomorrow. I meant to do it last week but didn't get to it | |
| 23:33:38 | openstackgerrit | Matt Riedemann proposed openstack/nova stable/queens: Update nova network info when doing rebuild for evacuate operation https://review.openstack.org/590062 | |
| 23:34:41 | openstackgerrit | Merged openstack/nova master: get provider IDs once when building summaries https://review.openstack.org/589945 | |
| 23:57:15 | openstackgerrit | Merged openstack/nova master: libvirt: Reduce calls to qemu-img during update_available_resource https://review.openstack.org/589513 | |
| 23:57:22 | openstackgerrit | Merged openstack/nova master: Add tempest-slow job to run the tempest slow tests https://review.openstack.org/567697 | |
| 23:57:32 | openstackgerrit | Merged openstack/nova master: Fix bad links for admin-guide https://review.openstack.org/580259 | |
| 23:57:41 | openstackgerrit | Merged openstack/nova master: [placement] Avoid rp.get_by_uuid in allocation_candidates https://review.openstack.org/589941 | |
| 23:57:49 | openstackgerrit | Merged openstack/nova master: api-ref: fix min_version for parent_provider_uuid in responses https://review.openstack.org/579577 | |
| 23:57:57 | openstackgerrit | Merged openstack/nova master: doc: mark the max microversion for rocky https://review.openstack.org/589598 | |
| #openstack-nova - 2018-08-09 | |||
| 00:24:58 | openstackgerrit | Merged openstack/nova master: Add functional test for forced live migration rollback allocs https://review.openstack.org/586636 | |
| 01:36:59 | openstackgerrit | Merged openstack/nova stable/ocata: [stable only] Add functional regression test for bug 1783613 https://review.openstack.org/588416 | |
| 01:36:59 | openstack | bug 1783613 in OpenStack Compute (nova) ocata "[ocata only] quota usage not decremented during boot/delete race" [Undecided,In progress] https://launchpad.net/bugs/1783613 - Assigned to melanie witt (melwitt) | |
| 02:08:17 | openstackgerrit | Chen proposed openstack/nova stable/queens: Fix bad links for admin-guide https://review.openstack.org/590068 | |
| 02:13:36 | openstackgerrit | Matt Riedemann proposed openstack/nova stable/pike: Update nova network info when doing rebuild for evacuate operation https://review.openstack.org/590070 | |
| 02:16:02 | openstackgerrit | Chen proposed openstack/nova stable/queens: Fix bad links for admin-guide https://review.openstack.org/590068 | |
| 02:16:37 | openstackgerrit | Chen proposed openstack/nova stable/pike: Fix bad links for admin-guide https://review.openstack.org/590072 | |