| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-08-08 | |||
| 21:34:48 | melwitt | yeah, I can't remember what it's for, something automated where a headless user is doing it | |
| 21:34:49 | mriedem | user creates server, | |
| 21:34:51 | mriedem | admin shelves it, | |
| 21:34:55 | mriedem | user unshelves, | |
| 21:34:59 | mriedem | kablammo | |
| 21:35:00 | dansmith | boom | |
| 21:35:01 | dansmith | heh yeah | |
| 21:35:03 | mriedem | b/c user doesn't have access to the admin snapshot | |
| 21:35:08 | dansmith | yar | |
| 21:35:12 | melwitt | that's fun | |
| 21:35:27 | melwitt | I guess the keystone idea would solve that issue | |
| 21:35:34 | mriedem | i'm pretty sure it's a relatively straight-forward fix, just haven't ever had the proper motivation to work on it | |
| 21:35:41 | mriedem | no no no | |
| 21:35:56 | mriedem | when we snapshot, we make the instance owner (the user) have member access to the image | |
| 21:36:00 | mriedem | glance has a thing for that | |
| 21:36:30 | mriedem | https://specs.openstack.org/openstack/glance-specs/specs/api/v1/requesting_image_memberships.html | |
| 21:36:55 | mriedem | https://specs.openstack.org/openstack/glance-specs/specs/api/v1/adding_a_member_to_an_image.html | |
| 21:36:58 | melwitt | neat | |
| 21:37:41 | mriedem | https://bugs.launchpad.net/nova/+bug/1675791 | |
| 21:37:41 | openstack | Launchpad bug 1675791 in OpenStack Compute (nova) "Instance created by demo user(non-admin), shelved by admin and unshelved by demo user --> ends up in error state " [Medium,Triaged] - Assigned to Damini Chopra (damini) | |
| 21:38:09 | openstackgerrit | Merged openstack/nova master: Add explicit functional-py36 tox target https://review.openstack.org/589825 | |
| 21:38:19 | openstackgerrit | Merged openstack/nova master: Add the guideline to write API reference https://review.openstack.org/569058 | |
| 21:39:55 | mriedem | speaking of changing stuff on unshelve https://review.openstack.org/#/c/559828/ | |
| 21:40:44 | melwitt | you've opened the shelve bug bin | |
| 21:42:06 | mriedem | time to create a shelve sig | |
| 21:42:10 | mriedem | / support group | |
| 21:42:18 | mriedem | for admins that have enabled that api | |
| 21:42:32 | melwitt | lol | |
| 21:53:45 | mriedem | melwitt: dansmith: a couple of thoughts in this ocata-only func test https://review.openstack.org/#/c/588416/ | |
| 21:53:57 | mriedem | if you wanted to make it less unit-y | |
| 21:55:21 | dansmith | mriedem: I definitely think this is a unit test, as I said | |
| 21:55:23 | mriedem | i guess you don't actually start conductor when using NoopConductorFixture | |
| 21:56:07 | melwitt | argh, I thought I had to write it this way to fit in with the pattern of regression tests | |
| 21:56:45 | melwitt | whichever one it needs to be, I always pick the wrong one :) | |
| 21:56:53 | openstackgerrit | Merged openstack/nova master: Remove Neutron MetaAPIProxy from cellsv2-layout https://review.openstack.org/588525 | |
| 21:58:15 | mriedem | i'm a tad confused on the stub in the 2nd test | |
| 21:58:59 | melwitt | in what way? | |
| 21:59:08 | mriedem | it says, | |
| 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 | :***( | |