| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-07-13 | |||
| 14:36:30 | dansmith | yeah | |
| 14:36:46 | melwitt | dansmith: thanks, I can update to the approach you suggested | |
| 14:36:51 | dansmith | I'm no opposed to what you have here, but it sounds like it's more the symptom than the cause | |
| 14:36:57 | dansmith | *not | |
| 14:39:02 | melwitt | I suppose the potential problem with the idea of sorting by deleted is if somehow an instance does not have a non-deleted service... but I'm not sure if/how that could happen. it could definitely happen before we started blocking service delete for compute services that are still hosting instances | |
| 14:39:47 | dansmith | well, that's why I'm saying the thing you're doing here is also not bad, | |
| 14:40:04 | dansmith | although we might should put the check in save() itself, | |
| 14:40:17 | dansmith | since any save() on a deleted service will fail, this is just one scenario | |
| 14:40:57 | dansmith | point being, if the join can return multiple services for the instance, potentially in insert-order, then getting the first one from the list if we _don't_ sort will likely be wrong | |
| 14:41:06 | dansmith | I almost feel like we've had a bug about that at some point actually | |
| 14:41:09 | melwitt | I considered that, but what should we do if trying to save a deleted service other than raise? | |
| 14:41:47 | dansmith | say what? :) | |
| 14:41:53 | dansmith | but you said save() will never work on deleted right? | |
| 14:42:04 | melwitt | oh, we don't get only the first one, we process all of them (there's a Instance.services field) and something else takes the first one (something in the API I guess?) | |
| 14:42:06 | dansmith | or I guess it will if you have a proper context | |
| 14:42:35 | melwitt | when you said "put the check in save() itself" I was asking what we would do after we checked self.deleted there | |
| 14:42:49 | melwitt | or did you mean the min version check, just skip it if deleted | |
| 14:43:33 | dansmith | I was going to say, if deleted, don't freak out about the version check failure in save9), | |
| 14:43:38 | dansmith | but that won't fix your problem during load | |
| 14:43:53 | melwitt | ah, ok | |
| 14:44:31 | melwitt | I'm realizing I was wrong when I said sorting would help because it processes all services that came out of the join. my bad | |
| 14:45:07 | dansmith | so why are we getting the deleted one | |
| 14:45:08 | dansmith | ? | |
| 14:45:28 | melwitt | because it has the same 'host' as a non-deleted one | |
| 14:45:53 | dansmith | right, which means we chose the wrong one yeah? | |
| 14:45:59 | melwitt | if the operator has ever deleted a service and replaced it with another with the same host name | |
| 14:46:24 | melwitt | the field on Instance is 'services' so we get all of them and something in the API must be picking one of them | |
| 14:46:43 | melwitt | so we get a _from_db_object call per service returned from the join | |
| 14:47:00 | mriedem | the lazy load on instance.services is generic right? | |
| 14:47:50 | dansmith | melwitt: okay so we process all of them and try to generate uuids for all the previously-deleted ones? | |
| 14:47:53 | melwitt | what do you mean by generic? | |
| 14:48:15 | mriedem | in that we don't do a targeted query to get services by instance uuid and filter out deleted ones | |
| 14:48:17 | melwitt | dansmith: yeah process all of them and try to generate uuids for all that do not have a uuid (including the previously deleted ones) | |
| 14:48:23 | mriedem | we call instance_get_by_uuid and join on the services table | |
| 14:48:39 | dansmith | I see | |
| 14:49:20 | melwitt | mriedem: oh, for the lazy load I'm not sure. the scenario of the bug is the API is adding the 'services' expected attr before getting instances for the list | |
| 14:49:27 | mriedem | and this is the backref | |
| 14:49:27 | mriedem | instance = orm.relationship( | |
| 14:49:27 | mriedem | "Instance", | |
| 14:49:27 | mriedem | backref='services', | |
| 14:49:27 | mriedem | primaryjoin='and_(Service.host == Instance.host,' | |
| 14:49:27 | mriedem | 'Service.binary == "nova-compute",' | |
| 14:49:28 | mriedem | 'Instance.deleted == 0)', | |
| 14:49:28 | mriedem | foreign_keys=host, | |
| 14:49:29 | mriedem | ) | |
| 14:49:40 | mriedem | umm, so why would we load deleted services? | |
| 14:49:53 | mriedem | oh so not a lazy-load ok | |
| 14:50:04 | melwitt | because that's not filtering on Service.deleted? | |
| 14:51:30 | mriedem | righ | |
| 14:51:30 | mriedem | query_prefix = query_prefix.options(joinedload(column)) | |
| 14:52:26 | melwitt | why would the lazy load filter out deleted services? | |
| 14:53:14 | mriedem | maybe it doesn't, i thought it would use the backref | |
| 14:54:58 | melwitt | the backref says Instance.deleted == 0, not Service.deleted, so I thought it wouldn't filter out deleted service? maybe I'm missing something else | |
| 14:55:16 | mriedem | oh right, | |
| 14:55:20 | mriedem | glossed right over that | |
| 14:55:25 | mriedem | anyway, ignore me now | |
| 15:18:03 | openstackgerrit | Chen proposed openstack/nova master: fix cellv2 delete_host https://review.openstack.org/582171 | |
| 15:24:39 | mriedem | tssurya: i'm reminded that we can get a HostMappingNotFound when deleting a compute service https://review.openstack.org/#/c/554920/12/nova/api/openstack/compute/services.py@243 - we were just talking about that this week (or late last week), but i can't remember if a bug was reported for it? | |
| 15:25:57 | mriedem | especially since we lookup the service to delete by id, not by host (so we don't check for a host mapping when doing the service lookup) | |
| 15:29:09 | mriedem | i just opened a bug anyway https://bugs.launchpad.net/nova/+bug/1781625 | |
| 15:29:10 | openstack | Launchpad bug 1781625 in OpenStack Compute (nova) "Deleting compute service can fail with 500 error if HostMappingNotFound" [High,Triaged] - Assigned to Matt Riedemann (mriedem) | |
| 15:39:11 | tssurya | mriedem: sorry was out, yeah I had opened one actually, https://bugs.launchpad.net/nova/+bug/1780727 | |
| 15:39:12 | openstack | Launchpad bug 1780727 in OpenStack Compute (nova) "Handle HostMappingNotFound when deleting a service" [Undecided,Confirmed] - Assigned to Surya Seetharaman (tssurya) | |
| 15:39:32 | mriedem | ack | |
| 15:39:35 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Heal allocations with incomplete consumer information https://review.openstack.org/574488 | |
| 15:39:36 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Refactor _heal_instances_in_cell https://review.openstack.org/577896 | |
| 15:39:37 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Use consumer generation in _heal_allocations_for_instance https://review.openstack.org/577905 | |
| 15:41:34 | mriedem | tssurya: do you plan on putting up a patch for that soon? if not, i can do that quickly today | |
| 15:41:49 | mriedem | we just backported the code that added the regression so i'd like to fix it soon | |
| 15:42:06 | tssurya | mriedem: yea please go ahead, my day is almost over | |
| 15:42:10 | mriedem | ok | |
| 15:42:17 | mriedem | in general, | |
| 15:42:20 | tssurya | sorry for not getting around to that | |
| 15:42:30 | mriedem | you don't need to assign bugs to yourself if you're not going to immediately work on them | |
| 15:42:36 | tssurya | ack | |
| 15:53:13 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Skip more rebuild tests for cells v1 job https://review.openstack.org/581717 | |
| 15:55:21 | dansmith | mriedem: how far up the stack on the numa set do you want to get before we pull the plug and let the stack of stuff on the bottom drain out? | |
| 15:58:45 | dansmith | I guess the scheduler one still has a -1 on it | |
| 16:00:16 | mriedem | the "scheduler" one is really a whole mess o stuff | |
| 16:00:29 | mriedem | including api changes and such that i'm saying should be split apart and the api change should land at the end of the series | |
| 16:00:45 | mriedem | right now he's got the api changes turned on but the libvirt driver changes are coming after that, which doesn't make sense | |
| 16:01:08 | dansmith | yeah, I saw he had rev'd that one and assumed it'd be ready now but looks like not | |
| 16:01:10 | mriedem | i'd be ok with draining the bottom half if we had agreement on the nullable question in the request spec | |
| 16:01:26 | dansmith | I was just thinking it might be good to clear that road before I go, but yeah | |
| 16:01:34 | mriedem | i think the 3 options i pointed out yesterday were: | |
| 16:02:09 | mriedem | 1. make RequestSpec.network_metadata nullable so that create_resource_requests can return None and we just blindly set it (which is what he'd be doing as coded today), | |
| 16:02:45 | mriedem | 2. keep reqspec.network_metadata non-nullable and then: (a) he has to return an 'empty' NetworkMetadata if no networking or (b) leave RequestSpec.network_metadata unset | |
| 16:02:54 | mriedem | i don't really like 2-a | |
| 16:03:22 | dansmith | yeah I mean 2b is what I was thinking | |
| 16:03:23 | mriedem | since you can't tell if the network metadata is 'empty' because the user requested no networking, or because the network just doesn't have physnet/tunneled | |
| 16:03:27 | dansmith | right | |
| 16:03:34 | mriedem | i'm cool with 2-b | |
| 16:03:40 | dansmith | or not 2-b? | |
| 16:03:52 | mriedem | i'll pretend that didn't happen | |
| 16:04:01 | dansmith | haha | |
| 16:04:17 | mriedem | if 2-b, we can drain the bottom half now and he'll just have to handle the 'if network_metadata is not None: reqspec.network_metadata = network_metadata" in the API | |
| 16:04:21 | mriedem | but that's easy enough, | |
| 16:04:30 | mriedem | and his hardware.py change are already unset-aware | |
| 16:04:31 | dansmith | kinda need him to agree to that though right? | |
| 16:04:42 | mriedem | stephenfin: ^ agree? | |