| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-09-06 | |||
| 20:41:43 | openstackgerrit | Mohammed Naser proposed openstack/nova master: Ensure instance mapping is updated in case of quota recheck fails https://review.openstack.org/501408 | |
| 20:42:03 | mriedem | although, | |
| 20:42:12 | mriedem | it was split into two parts due to melwitt's quota check change | |
| 20:42:38 | melwitt | I didn't change the 2-partness for the quota check | |
| 20:42:39 | mriedem | https://review.openstack.org/#/c/416521/ | |
| 20:43:07 | mriedem | melwitt: i mean the 2 loops over the tuple | |
| 20:43:28 | mriedem | that was a single for loop before | |
| 20:43:31 | melwitt | oh, I see (looking at the patch now) | |
| 20:43:47 | dansmith | hm why is that? | |
| 20:44:09 | mriedem | have to create them all to count | |
| 20:44:16 | mriedem | and then continue creating stuff | |
| 20:44:24 | melwitt | at the time I was thinking, just check all of the quota first before creating a bunch of other resources if it's just gonna possibly fail in the middle | |
| 20:44:30 | dansmith | why not do that at the end I mean | |
| 20:44:58 | dansmith | yeah, but, then we're kinda checking the quota without everything created, which is a little odd | |
| 20:44:58 | melwitt | we could. I was probably just thinking it would be less wasteful, why create BDMs and all of that if it's gonna fail the recheck anyway | |
| 20:45:13 | dansmith | I mean, I guess it's just checking instances | |
| 20:45:18 | melwitt | yeah | |
| 20:45:21 | mriedem | if we did just quota check at the end, then we also have to cleanup the bdms and tags | |
| 20:45:28 | mriedem | in addition to the stuff that _cleanup_build_artifacts is already removing | |
| 20:45:48 | dansmith | do we? | |
| 20:46:01 | dansmith | we delete the instance.. are those things cleaned up by compute normally or something? | |
| 20:46:12 | mriedem | it wouldn't be mapped to a host | |
| 20:46:15 | mriedem | so no compute involved | |
| 20:46:21 | dansmith | no, | |
| 20:46:35 | dansmith | this would be like a local delete | |
| 20:46:56 | dansmith | because we obviously need not call to compute in this case, so I'm asking in a normal local delete where compute is down, | |
| 20:47:04 | dansmith | do we have to clean up bdms and tags separately? | |
| 20:47:34 | mriedem | no, don't think so, those get removed via the db api | |
| 20:47:36 | mriedem | when you delete the instance | |
| 20:47:54 | dansmith | instance delete will delete BDMs | |
| 20:48:01 | mriedem | https://github.com/openstack/nova/blob/b79492f70257754f960eaf38ad6a3f56f647cb3d/nova/db/sqlalchemy/api.py#L1886 | |
| 20:48:05 | dansmith | and tags | |
| 20:48:05 | mriedem | https://github.com/openstack/nova/blob/b79492f70257754f960eaf38ad6a3f56f647cb3d/nova/db/sqlalchemy/api.py#L1894 | |
| 20:48:06 | dansmith | yeah | |
| 20:48:52 | dansmith | so when you delete the instance after the quota check those will get cleaned up if we have created them right? | |
| 20:49:45 | mriedem | well we don't delete the instance here, | |
| 20:49:54 | mriedem | we just set it to ERROR state | |
| 20:49:59 | mriedem | and then the user does the (local) delete | |
| 20:50:08 | dansmith | right | |
| 20:50:09 | mriedem | which should cleanup the bdms/tags | |
| 20:50:21 | openstackgerrit | Merged openstack/nova master: Fix broken link https://review.openstack.org/501391 | |
| 20:50:31 | mriedem | well, the bdms and tags wouldn't even exist in the cell db | |
| 20:50:37 | mriedem | if the quota check fails | |
| 20:50:47 | mriedem | oh but if we moved it to the end.. | |
| 20:51:03 | mriedem | yeah i think that would work | |
| 20:52:11 | dansmith | we can also do what mnaser wanted to do in the first place, which is just do this quick map fixup if we fail this quota check, | |
| 20:52:12 | melwitt | cells meeting in 8 minutes? | |
| 20:52:26 | dansmith | but then we will have an instance without those things in error state, and we'll be doing it from yet another place | |
| 20:52:41 | dansmith | I kinda want to keep it less spaghetti-like and do it in one place, personally | |
| 20:52:58 | dansmith | melwitt: I'm in an airport so we said this morning we'd cancel | |
| 20:53:08 | melwitt | oh, cool | |
| 20:53:11 | dansmith | if you have things to discuss we can, because I'm clearly connected, but, I didn't see the point | |
| 20:53:26 | melwitt | no, just forgot to check the log | |
| 20:53:36 | dansmith | ack | |
| 20:54:12 | mriedem | another alternative is just don't delete the build request in _cleanup_build_artifacts, or is that crazy? | |
| 20:54:24 | dansmith | no, I think we can't do that | |
| 20:54:36 | mriedem | the problem is that we haven't mapped the instance to a cell, and _cleanup_build_artifacts removes the build request, so the API literally can't find it anywhere | |
| 20:54:38 | mriedem | yeah? | |
| 20:54:44 | dansmith | or you mean in the failure case only? | |
| 20:54:47 | mriedem | yes | |
| 20:55:12 | mriedem | but then you'd have a build request in the api and an instance in the cell, unmapped | |
| 20:55:16 | dansmith | mriedem: we wouldn't know where to find the instance record to mark it as deleted when they deleted the buildreq, so we'd leave that undeleted but unfindable instance forever | |
| 20:55:18 | mriedem | and that instance in the cell db wouldn't get removed, | |
| 20:55:21 | mriedem | unless _cleanup_build_artifacts removes it | |
| 20:55:21 | dansmith | right | |
| 20:55:43 | mriedem | so, change _cleanup_build_artifacts to delete the instance from the cell and leave the build request for API requests? | |
| 20:56:10 | mriedem | unless we just overhaul this code to remove the 2 loops and do the quota check at the end | |
| 20:56:19 | melwitt | doing that would remove the instance fault message which explains the quota check failed | |
| 20:56:37 | mriedem | melwitt: can't the build request have a fault on it? | |
| 20:56:40 | dansmith | and we have the instance in the cell db, that's better than a buildreq, so we should use it | |
| 20:56:47 | melwitt | I didn't know it can | |
| 20:56:53 | dansmith | mriedem: I don't think we ever update that instance do we? | |
| 20:56:57 | dansmith | because it's fake right? | |
| 20:57:17 | mriedem | build request doesn't have a fault field, so yeah, can't do that | |
| 20:57:35 | melwitt | it has an instance field which would have a fault field I guess | |
| 20:57:47 | dansmith | no, I think the instance is fake and constructed on the fly | |
| 20:57:54 | melwitt | oh, okay | |
| 20:58:05 | mriedem | the BuildRequest.instance is persisted | |
| 20:58:10 | mriedem | it's just serialized json | |
| 20:59:29 | mriedem | but, that starts to get weird... | |
| 20:59:31 | dansmith | yeah, I guess so | |
| 20:59:36 | mriedem | managing state on an instance nested in a build requst | |
| 20:59:38 | dansmith | I'm not sure why that's better though | |
| 20:59:41 | dansmith | yeah | |
| 20:59:56 | dansmith | this makes a bunch of other regular code just be used for anything else you do to this dead instance | |
| 20:59:56 | mriedem | it's only better if we want to keep the 2 loops b/c we're afraid of changing them | |
| 21:00:15 | dansmith | I ain't skeered | |
| 21:00:20 | mriedem | dude, | |
| 21:00:28 | mriedem | i shit my pants everytime we touch nova these days | |
| 21:00:33 | dansmith | hah | |
| 21:01:03 | mnaser | (which is totally why i thought putting a tiny little call/fixup in the exception handling = less scary | |
| 21:01:05 | mnaser | :p | |
| 21:01:12 | melwitt | well, doesn't moving the quota check to the end and having only one loop address the issue fine since we know the BDMs and tags will get cleaned up at delete time? | |
| 21:01:20 | mriedem | melwitt: i think so | |
| 21:01:38 | melwitt | so the fake instance thing isn't the only way to get rid of the two loop thing | |
| 21:01:40 | dansmith | mnaser: yeah, but we already have lots of "clean this up in this snowflake way" calls | |
| 21:01:50 | mriedem | a test that creates an instance with a bdm and tag and asserts those are removed from the db would make me feel safer | |
| 21:01:54 | dansmith | melwitt: the fake instance is the way to keep the two loops he said | |
| 21:02:07 | dansmith | melwitt: fearing change | |
| 21:02:15 | mriedem | i fear the landmines | |