| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-03-15 | |||
| 13:54:43 | bauzas | mriedem: https://review.openstack.org/#/c/391060/ | |
| 13:54:45 | bauzas | dansmith: ^ | |
| 13:54:51 | mriedem | ah | |
| 13:55:12 | bauzas | it requires a rebase | |
| 13:55:47 | jaypipes | stvnoyes1: gotcha. would you expect instead to have an "unsupported operation on stopped server" error message return instead of no error? | |
| 13:56:51 | mriedem | bauzas: left my concern inline, but that potentially cripples a restored soft deleted instance, | |
| 13:57:04 | bauzas | mriedem: cool, thanks | |
| 13:57:11 | mriedem | i'm not sure how we deal with that, since we don't want the compute doing a call back up to the API to delete the request spec when the instance is hard deleted | |
| 13:57:25 | bauzas | mriedem: the problem is that we never agreed on soft-deleting the Spec with alaski | |
| 13:57:40 | dansmith | he means soft delete of the instance | |
| 13:57:45 | dansmith | (right?) | |
| 13:57:45 | bauzas | yeah | |
| 13:57:49 | bauzas | I understand that | |
| 13:57:49 | dansmith | not soft delete of the instance db record | |
| 13:57:55 | bauzas | sure | |
| 13:58:12 | bauzas | but if we trigger the Spec hard deletion on an instance soft-delete | |
| 13:58:20 | stvnoyes1 | jaypipes: agree, it would be better to get some kind of error. since a server can stop at any time, doing a check in the API code would help a lot but it wouldn't cover all the cases. | |
| 13:58:26 | bauzas | then, how could we restore it if we restore the instance ? | |
| 13:58:34 | dansmith | you don't restore it, | |
| 13:58:49 | dansmith | you have to defer deletion of it until the soft-deleted instance is purged somehow | |
| 13:58:54 | dansmith | although that happens on the compute node | |
| 13:59:04 | bauzas | yeah, the trigger is then totally different | |
| 13:59:33 | bauzas | we could keep the spec record until the instance is hard-deleted | |
| 13:59:41 | bauzas | but I need to see where we trigger that | |
| 13:59:45 | dansmith | right, so you have to just not delete it in that case, and either leak it (as we do now for everything) or at least delete if you hard-delete | |
| 14:00:05 | bauzas | dansmith: you said we hard-delete on the compute side ? | |
| 14:00:14 | bauzas | a periodic I guess ? | |
| 14:00:21 | dansmith | compute is what does the purge yeah, in a periodic | |
| 14:00:30 | mriedem | dansmith: right | |
| 14:00:37 | mriedem | well, SOFT_DELETED in the API sense | |
| 14:00:39 | mriedem | not the DB sense | |
| 14:00:40 | bauzas | okay, that would ease my branch :) | |
| 14:01:11 | bauzas | IIRC, we already merged the .delete() method I added in Newton | |
| 14:01:23 | bauzas | the caller of that .delete() never merged in time in Newton | |
| 14:01:24 | mriedem | which delete() method? | |
| 14:01:28 | bauzas | the Spec one | |
| 14:01:36 | mriedem | yes RequestSpec.destroy() exists, | |
| 14:01:38 | mriedem | but nothing calls it | |
| 14:01:45 | bauzas | exactly | |
| 14:01:53 | bauzas | because I wrote the destroy() | |
| 14:01:58 | bauzas | it was in the same series | |
| 14:01:59 | mriedem | so i think we're left with leaking request specs for soft-deleted instances for now i guess | |
| 14:02:04 | bauzas | yup | |
| 14:02:11 | bauzas | lemme clarify | |
| 14:02:26 | bauzas | in Newton, I discovered that (because I missed that when I reviewed alaski's persistence series) | |
| 14:02:29 | mriedem | we could provide a nova-manage command to purge those if you wanted... | |
| 14:02:37 | bauzas | so I wrote a series | |
| 14:02:45 | bauzas | .destroy() as a bottom patch | |
| 14:02:49 | bauzas | the bottom merged | |
| 14:02:51 | dansmith | mriedem: we have InstanceMappings to purge in that case as well | |
| 14:02:57 | bauzas | but not the top one | |
| 14:03:13 | mriedem | dansmith: nova-manage api_db purge_zombies | |
| 14:03:13 | bauzas | and in Ocata, I had that in my plate, but the placement things diverted me | |
| 14:03:28 | bauzas | so, we're now in Pike and it's still in my plate :) | |
| 14:03:36 | mriedem | eat faster | |
| 14:03:42 | dansmith | mriedem: aye | |
| 14:04:17 | bauzas | mriedem: dansmith: I wrote a db online migrations that was creating the spec records for old instances, could we envisage some hack for purging ? | |
| 14:04:40 | bauzas | ie. we trigger deletes on the periodic task | |
| 14:04:54 | bauzas | but we could also write a db online migration script that would do that | |
| 14:05:19 | dansmith | we shouldn't abuse online migrations for periodic cleanup | |
| 14:05:21 | dansmith | IMHO | |
| 14:05:32 | bauzas | okay | |
| 14:05:34 | bauzas | fine by me | |
| 14:05:40 | bauzas | I'll revisit that change | |
| 14:06:04 | bauzas | if we have a periodic, do we still need a purge command ? | |
| 14:06:15 | bauzas | dansmith: mriedem^ | |
| 14:06:20 | dansmith | we can't have a periodic, IMHO | |
| 14:06:39 | dansmith | having all the conductors purging all the time would be bad I think | |
| 14:06:46 | dansmith | apis don't run periodics | |
| 14:06:49 | mriedem | yeah that ^ | |
| 14:06:50 | dansmith | and the compute node can't do it | |
| 14:06:55 | mriedem | computes can't talk to api | |
| 14:07:24 | mriedem | so yeah, it's on-demand purge for now it sounds like | |
| 14:07:35 | mriedem | the scheduler has periodics... | |
| 14:07:41 | mriedem | and this is related to scheduling stuff | |
| 14:07:50 | dansmith | it's too heavy for scheduler, IMHO, | |
| 14:07:57 | dansmith | requires hitting cell and api databases, | |
| 14:08:02 | mriedem | true | |
| 14:08:09 | mriedem | just like discover_hosts :) | |
| 14:08:21 | dansmith | and scheduler is supposed to be single-threaded | |
| 14:09:02 | dansmith | yeah, but that is _actually_ scheduling-related and also getting to be nearly zero time :) | |
| 14:09:14 | dansmith | and disabled by default and not required for normal operation | |
| 14:09:30 | bauzas | okay I see | |
| 14:09:52 | bauzas | dansmith: so you propose nova-manage api_db purge_zombies ? fine by me, lemme write it quickly | |
| 14:09:55 | dansmith | and also, remember the suggestion of potentially making the scheduler just a pass-through library call | |
| 14:10:07 | bauzas | yeah I see your point | |
| 14:10:17 | bauzas | and we can't use compute periodic because of the upcall | |
| 14:10:20 | dansmith | it'd be nice if we could just roll this into archive_rows or something | |
| 14:10:27 | dansmith | archiving rows in the api db is just deleting them | |
| 14:10:52 | bauzas | dansmith: well, it would require some change in .destroy() but nothing really hard :) | |
| 14:11:16 | mriedem | we wouldn't use reqspec.destroy() if it were in the archive method | |
| 14:11:18 | bauzas | FWIW, unrelated I have a concern with https://review.openstack.org/#/c/439754/23 | |
| 14:11:25 | bauzas | dansmith: ^ | |
| 14:11:26 | mriedem | it'd be straight sql | |
| 14:11:34 | mriedem | well, orm | |
| 14:11:55 | bauzas | mriedem: dansmith: well, do you think it could be turnable by opt ? | |
| 14:12:09 | bauzas | or by command | |
| 14:12:16 | bauzas | like "purge --archive" | |
| 14:12:42 | dansmith | bauzas: I know that's a concern, but unless you have a suggestion going forward I don't see what the options are | |
| 14:13:04 | dansmith | bauzas: just make archive_deleted_rows do it.. no need to overthink it | |