| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-03-15 | |||
| 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 | |
| 14:14:09 | bauzas | dansmith: sorry I could be confused | |
| 14:16:03 | bauzas | dansmith: you mean I should amend the archive_deleted_rows subcommand to delete the spec records ? | |
| 14:16:12 | dansmith | yes | |
| 14:16:15 | bauzas | okay | |
| 14:16:43 | bauzas | lemme write that soon, ideally this week | |
| 14:16:56 | bauzas | mriedem: you said you gonna write a bug report for this ? | |
| 14:17:37 | dansmith | bauzas: I'd really appreciate quick follow-up on the az thing with concrete suggestions | |
| 14:17:40 | bauzas | dansmith: for https://review.openstack.org/#/c/439754/23/nova/api/metadata/base.py I don't think we need to change this if we consider looking up the host AZ, nope ? | |
| 14:18:03 | dansmith | bauzas: you mean what was being done before? | |
| 14:18:05 | mriedem | bauzas: i'm working on something related to it | |
| 14:18:11 | bauzas | dansmith: the logic in that method is that we use the instance.az field to match with the host AZ information | |
| 14:18:32 | bauzas | dansmith: yeah, I don't see why we need to make it cellsv2-aware | |
| 14:18:43 | bauzas | if we fix instance.az | |
| 14:18:46 | dansmith | bauzas: because metadata runs in the cell and can't access the api database to look at az stuff | |
| 14:19:32 | bauzas | orly? | |
| 14:19:45 | dansmith | yarly! | |
| 14:20:04 | bauzas | I thought it was on top | |
| 14:20:27 | dansmith | bauzas: no, and definitely not if we're just using it to build configdrive | |
| 14:20:32 | bauzas | I see | |
| 14:20:39 | bauzas | lemme reconsider the problem then | |
| 14:21:54 | bauzas | dansmith: what really blocks us is https://github.com/openstack/nova/blob/67af1eefc5c1cf8cde4b4becb933808850acec2d/nova/availability_zones.py#L190 | |
| 14:22:34 | bauzas | dansmith: but conceptually, we need to know where the instance is located, hence why we call the AggregateList object | |