| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-03-15 | |||
| 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 | |
| 14:29:27 | bauzas | if the user asked for an AZ, then I agree with your statement | |
| 14:29:39 | dansmith | so metadata right now reports the actual az it landed on correct? | |
| 14:30:50 | dansmith | it looks to me like we use instance.az in several other places | |
| 14:31:20 | bauzas | dansmith: for the metadata, yes | |
| 14:31:27 | dansmith | no, for other things too | |
| 14:31:36 | dansmith | nova/network/neutronv2/api.py: zone = 'compute:%s' % instance.availability_zone | |
| 14:31:38 | bauzas | dansmith: for other places, AFAICT, I think we return that too | |
| 14:31:42 | dansmith | nova/virt/block_device.py: return instance.availability_zone | |
| 14:32:03 | dansmith | is having instance.az being incorrect not affecting those places as well? | |
| 14:32:11 | bauzas | yup | |
| 14:32:13 | dansmith | like, shouldn't making it be the actual az the instance is on be better? | |
| 14:32:37 | bauzas | yeah, as I said, I agree, we should update instance.az to the right value | |
| 14:32:56 | bauzas | and use spec_obj.availability_zone for knowing the original request | |
| 14:33:14 | dansmith | okay | |
| 14:33:18 | dansmith | grim | |
| 14:33:20 | dansmith | oops | |
| 14:33:50 | dane-fichter | mriedem: could you unblock this change, please? https://review.openstack.org/#/c/351232/ | |
| 14:34:09 | jaypipes | mriedem: so, is there anything on the ironic virt driver patch you'd like me to change? | |
| 14:35:01 | openstackgerrit | Rafael Folco proposed openstack/nova master: DELETE all inventory for a resource provider https://review.openstack.org/416669 | |
| 14:35:01 | openstackgerrit | Rafael Folco proposed openstack/nova master: [placement] Raising http codes on old microversion https://review.openstack.org/444497 | |
| 14:35:04 | mriedem | dane-fichter: done, left a comment | |
| 14:35:08 | mriedem | jaypipes: let me check your replies | |
| 14:37:14 | dansmith | bauzas: do I really need elevated context here/ https://github.com/openstack/nova/blob/67af1eefc5c1cf8cde4b4becb933808850acec2d/nova/availability_zones.py#L190 | |
| 14:38:09 | jaypipes | mriedem: I can add a code comment about raising an exception if resource class is None in Queens, if you'd like. | |
| 14:38:46 | mriedem | jaypipes: let me look first, might be just follow up stuff | |
| 14:38:53 | mriedem | doing a few things at once | |
| 14:40:27 | bauzas | dansmith: I left a proposal in https://review.openstack.org/#/c/439754/23 by saying we could amend instance.az once we schedule a target | |
| 14:40:43 | bauzas | every time we touch instance.host actually | |
| 14:41:03 | dansmith | bauzas: this is what I'm testing right now: http://pastebin.com/raw/iWBb9wt5 | |
| 14:41:31 | dansmith | bauzas: for the old and new build operations | |
| 14:41:44 | bauzas | dansmith: for the elevated context, I think it's a legacy from the past | |
| 14:41:54 | dansmith | bauzas: okay I thought so too just checking | |
| 14:41:59 | bauzas | dansmith: now we use objects, we don't really need elevated | |
| 14:44:53 | bauzas | dansmith: hmmpppfff, instance.host isn't set on the conductor after we schedule | |
| 14:45:04 | bauzas | I thought it was the case but no | |
| 14:45:15 | bauzas | dansmith: we just use that for calling the right compute service | |
| 14:45:27 | dansmith | right, I'll change | |
| 14:45:30 | bauzas | dansmith: AFAICS, instance.host is only set when the instance is spawning | |