| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-07-13 | |||
| 13:59:34 | mriedem | yup 3am rambling | |
| 13:59:36 | melwitt | efried, leakypipes, stephenfin: reminder that non-client library release freeze (os-traits and os-vif) is next thurs July 19 so if there's anything you want to make the release, get them in by the end of next thurs | |
| 14:00:06 | mriedem | yikun: ^ you should start working on the advanced policy changes to novaclient | |
| 14:03:11 | melwitt | novaclient release freeze is same as r-3 July 26 | |
| 14:29:52 | mriedem | dansmith: thanks; i think i'll clean this change up and then rebase the api change on top with the service version check i'm talking about, then Kevin_Zheng can flesh out the rest | |
| 14:30:04 | melwitt | dansmith: I wanted to get your thoughts on this approach for a bug fix before you are out next week https://review.openstack.org/582408 | |
| 14:30:04 | dansmith | ack | |
| 14:32:07 | mriedem | melwitt: btw, i've had a functional test up for one of those forever https://review.openstack.org/#/c/562041/ | |
| 14:32:39 | melwitt | mriedem: oh, sorry, I didn't know | |
| 14:32:40 | dansmith | melwitt: we probably don't want to filter on deleted for the join, but I would think we should sort by deleted and pick the first service in the list, which would be the non-deleted one, if there is one | |
| 14:33:07 | dansmith | would that solve the bug without this patch? | |
| 14:33:40 | melwitt | dansmith: I think it would because it would also avoid generating a UUID for a deleted service | |
| 14:34:02 | dansmith | so that might be a better approach | |
| 14:34:09 | melwitt | Service.save()ing a deleted service will always raise ServiceNotFound, for example | |
| 14:35:09 | dansmith | because of _check_minimum_version() yeah? | |
| 14:35:27 | melwitt | no, ServiceNotFound will raise if version checks out | |
| 14:35:40 | melwitt | if version is too old, we raise ServiceTooOld first before we get to the db.service_update call | |
| 14:36:15 | dansmith | aight, well, anyway | |
| 14:36:24 | melwitt | to be clear, _check_minimum_version raises ServiceTooOld if the deleted service has too old a version, db.service_update raises ServiceNotFound for the deleted service | |
| 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 | |