| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-03-15 | |||
| 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 | |
| 14:22:58 | dansmith | why are we doing that late instead of just making sure az gets set properly when the instance gets rescheduled or whatever? | |
| 14:23:17 | dansmith | we're taking this lookup hit every time we build metadata, but we should just make sure that we set the thing properly in the first place right? | |
| 14:25:40 | dansmith | like in schedule_and_build() and build_and_run(), we should be able to lookup and fix instance.az once so metadata never has to look again, right? | |
| 14:25:54 | dansmith | since we know the host it's going to land on each time | |
| 14:27:06 | bauzas | dansmith: well, it's something I discussed in the past | |
| 14:27:26 | bauzas | now that we have RequestSpec, I'm in favor of updating instance.az to the right value | |
| 14:27:47 | bauzas | semantically, we kept that unchanged in the past because we wanted to know what the user originally expressed | |
| 14:27:59 | bauzas | but now we have the Spec object that targets that | |
| 14:28:12 | dansmith | if the user expressed something, then we would be limiting the scheduling to that az anyway, no? | |
| 14:28:31 | bauzas | dansmith: yup, but we default that field for some unknown reason | |
| 14:28:38 | bauzas | we set it anyway | |
| 14:28:46 | dansmith | to nova if it was unset, right? | |
| 14:28:50 | bauzas | well, I actually know | |
| 14:29:12 | bauzas | yeah, if the user didn't ask for an AZ, we set it to a conf opt value | |