| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-04-11 | |||
| 20:41:23 | dansmith | well, maybe you can by instance uuid, but you can't delete them by compute node uuid in bulk | |
| 20:41:24 | dansmith | yeah | |
| 20:41:51 | openstackgerrit | Jackie Truong proposed openstack/nova master: Implement certificate_utils https://review.openstack.org/479949 | |
| 20:41:52 | openstackgerrit | Jackie Truong proposed openstack/nova master: Add trusted_image_certificates to REST API https://review.openstack.org/486204 | |
| 20:41:53 | openstackgerrit | Jackie Truong proposed openstack/nova master: Add certificate validation docs https://review.openstack.org/560158 | |
| 20:41:55 | mriedem | you'd have to take all instances that were running on that host, find their allocations, and then remove any against RPs that no longer exist | |
| 20:42:06 | melwitt | okay, I'm not getting the "same concern with today's counting method" part yet | |
| 20:42:19 | dansmith | so I guess the question is.. why do we allow you to delete the service today with instances on it? | |
| 20:42:30 | mriedem | yeah i'm trying to get there | |
| 20:42:31 | dansmith | because that would cause accounting trouble in the RT the way it was when that was added | |
| 20:42:34 | melwitt | if you delete instances before the service today, the allocations will be deleted when you delete the instances, which is fine | |
| 20:43:44 | dansmith | if you just delete the service, then we'll still count those instances for quota, yes, is that what you mean? | |
| 20:43:53 | mriedem | excluding placement, if you delete the service and later restart it, we'll create a new service, but the RT will still look up instances on that service via host and nodename yeah? | |
| 20:44:00 | efried | johnthetubaguy_: I think your -1 on https://review.openstack.org/#/c/553605/ is moot now, yes? The flag changes are being done in the previous patch, and what you were seeing in PS6 was a bad rebase. | |
| 20:44:06 | dansmith | but you've broken things now, like you can't find the cell that the instance's host is in anymore | |
| 20:44:07 | dansmith | so you might not be able to delete instances | |
| 20:44:08 | melwitt | I think you meant the opposite, deleting the service before the instances. with counting today, we count from the instances table so it works. but it would break if we were counting from placement | |
| 20:44:33 | efried | johnthetubaguy_: That delta is no longer there (that file is unchanged by this patch) | |
| 20:44:40 | dansmith | sure, what I mean is, other things will break if you delete the service before the instances I think | |
| 20:44:52 | melwitt | I see | |
| 20:44:55 | mriedem | quota counting uses instance mappings right? | |
| 20:44:59 | mriedem | to find the cell they are in | |
| 20:45:09 | melwitt | yes | |
| 20:45:14 | dansmith | I guess delete will as well, not host mapping | |
| 20:45:18 | mriedem | i'm not sure what other things will break for the instance if the host mapping is gone | |
| 20:45:28 | dansmith | alright maybe not | |
| 20:45:41 | dansmith | that said, | |
| 20:45:55 | dansmith | I don't think that we should not delete in placement if we're deleting our record of what those allocations are, | |
| 20:46:01 | dansmith | because then they're leaked | |
| 20:46:09 | dansmith | and my point being, | |
| 20:46:22 | dansmith | we should tie the deleting of those to deleting of our service (and thus our record of how to find them) | |
| 20:46:41 | dansmith | it seems weird to me to let people delete the service with instances on it, | |
| 20:46:52 | dansmith | but I guess it might not fail anything today because we use the hostname as they key | |
| 20:47:23 | mriedem | i'm totally fine with deleting the resource provider if we're going to delete the service and compute node | |
| 20:47:36 | dansmith | we kindof have to | |
| 20:47:45 | mriedem | i'm trying to parse if you're confirming what tssurya asked, "so we should allow a service delete only if doesn't have any living instances on that compute node ?" | |
| 20:47:47 | melwitt | yeah, I agree we can't be leaking the allocations. but so far I'm leaning to we should block service delete if there are instances. I'm concerned about losing allocation records for instances in that case | |
| 20:48:12 | mriedem | the easy thing to do here is not delete anything if there are instances on the node | |
| 20:48:17 | dansmith | melwitt: yeah, I'm saying I think that makes the most sense, I'd just like to figure out why that wasn't done way back when | |
| 20:48:33 | dansmith | maybe because it would re-heal and so meh, | |
| 20:48:38 | dansmith | but today that's not the case | |
| 20:48:38 | melwitt | yeah, okay. I'd also like to know | |
| 20:48:38 | mriedem | dansmith: right, because "if you delete the service and later restart it, we'll create a new service, but the RT will still look up instances on that service via host and nodename yeah?" | |
| 20:49:02 | dansmith | yes, I mean, | |
| 20:49:14 | tssurya | melwitt: umm another doubt on quota counting - we don't use inst_mappings right ? we just query through all the cells DBs instances tables ? -> https://github.com/openstack/nova/blob/master/nova/quota.py#L1325 | |
| 20:49:15 | mriedem | https://github.com/openstack/nova/blob/2c5da2212c3fa3e589c4af171486a2097fd8c54e/nova/compute/resource_tracker.py#L714 | |
| 20:49:38 | dansmith | deleting the service today which deletes the compute node will break all manner of stuff, if not deleting instances and quotas, because we'll get a new compute node uuid | |
| 20:50:09 | dansmith | but likely just placement-forward things where the uuid started to be a thing | |
| 20:50:25 | mriedem | yeah. if we block the service delete, you have options to not f*ck yourself, | |
| 20:50:29 | mriedem | like migrate the instance | |
| 20:50:30 | melwitt | tssurya: oh, yep, you're right. I forgot ... cause we have to be able to get all instances for a project/user combo and we can't know that from instance_mappings (because it lacks user_id). but we could have done a pruning by project_id based on instance_mappings | |
| 20:50:59 | tssurya | melwitt: which is what we plan to do once we have your spec in :D | |
| 20:51:05 | mriedem | this is sort of like the "should we allow AZ renames for aggregates with hosts that have instances on them" | |
| 20:51:23 | mriedem | knowing that if you do ^ you'll likely break some stuff | |
| 20:51:27 | dansmith | yeah | |
| 20:51:43 | melwitt | tssurya: heh, yeah :) | |
| 20:51:53 | mriedem | ok so is this considered a separate bug? | |
| 20:52:01 | mriedem | or do we roll it into the same fix? | |
| 20:52:02 | dansmith | it's a separate patch IMHO | |
| 20:52:09 | dansmith | which is why I was saying they're two different concerns | |
| 20:52:12 | mriedem | that's kind of what i was thinking, it would be easier that way anyway | |
| 20:52:28 | melwitt | +1 to separate patches | |
| 20:52:30 | mriedem | we can build on the same functional test base i started | |
| 20:53:00 | tssurya | sure | |
| 20:53:02 | dansmith | yeah | |
| 20:53:31 | mriedem | tssurya: you want me to open a separate bug? | |
| 20:53:58 | tssurya | mriedem: yea | |
| 20:54:04 | mriedem | also, it's 11pm there right? | |
| 20:54:15 | tssurya | I will put up a patch tomorrow | |
| 20:54:17 | mriedem | do you get to bill belmiro for overtime? | |
| 20:54:30 | tssurya | mriedem: yes its 11pm :) | |
| 20:54:33 | tssurya | haha no | |
| 20:55:04 | tssurya | this is me learning stuff from the veterans :P | |
| 21:22:40 | openstackgerrit | Merged openstack/nova master: Remove mox in tests/unit/test_utils.py https://review.openstack.org/557883 | |
| 21:52:10 | mriedem | cdent: edleafe: oh seers of http status code wisdom, hear my call, shall thou return a 400 or 409 for https://bugs.launchpad.net/nova/+bug/1763183 ? | |
| 21:52:10 | openstack | Launchpad bug 1763183 in OpenStack Compute (nova) "DELETE /os-services/{service_id} does not block for hosted instances" [High,Triaged] - Assigned to Matt Riedemann (mriedem) | |
| 21:52:41 | mriedem | i think 409 | |
| 21:53:03 | cdent | i feel like I should have some kind of a 409phone | |
| 21:53:26 | mriedem | oh and if 409, guess what, @wsgi.expected_errors((400, 404)) | |
| 21:53:30 | mriedem | we get to have a microversion debate | |
| 21:53:40 | mriedem | alex_xu: ^ | |
| 21:54:16 | mriedem | although, it's an admin api... | |
| 21:54:40 | edleafe | mriedem: 400 usually means that you can make the call succeed by fixing the parameters, etc | |
| 21:54:44 | cdent | mriedem: yes 409 | |
| 21:54:53 | cdent | because of what edleafe just said | |
| 21:55:06 | cdent | the server has to change to make things work, not the request | |
| 21:55:17 | cdent | but if the server does change, then things might work when you try later | |
| 21:55:47 | mriedem | yeah 409 is correct, but need to figure out if i can get by the existing expected errors decorator | |
| 21:55:57 | mriedem | but i think with the legal caveat in https://docs.openstack.org/nova/latest/contributor/microversions.html#when-a-microversion-is-not-needed 3rd bullet | |
| 21:55:58 | mriedem | we maybe can | |
| 22:08:47 | mriedem | oh my | |
| 22:08:53 | mriedem | this is the issue that keeps on giving | |
| 22:10:22 | mriedem | https://bugs.launchpad.net/nova/+bug/1679750 | |
| 22:10:22 | openstack | Launchpad bug 1679750 in OpenStack Compute (nova) queens "Allocations are not cleaned up in placement for instance 'local delete' case" [Medium,Confirmed] | |
| 22:17:46 | mriedem | efried: seems delete_resource_provider should fail if delete_allocation_for_instance returns False... | |
| 22:18:03 | efried | ... | |
| 22:19:11 | efried | mriedem: 404 means there were no allocations for that consumer. Which is fine in the context of delete_resource_provider. Am I misunderstanding something? | |
| 22:19:18 | mriedem | getting a 409 | |
| 22:19:20 | mriedem | and now i know why | |
| 22:19:37 | efried | 409 is gonna mean generation conflict. | |