| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-04-11 | |||
| 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 | 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:48:38 | melwitt | yeah, okay. I'd also like to know | |
| 20:48:38 | dansmith | but today that's not the case | |
| 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 | 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: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: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 | openstack | Launchpad bug 1679750 in OpenStack Compute (nova) queens "Allocations are not cleaned up in placement for instance 'local delete' case" [Medium,Confirmed] | |
| 22:10:22 | mriedem | https://bugs.launchpad.net/nova/+bug/1679750 | |
| 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. | |
| 22:19:44 | mriedem | no it's not that | |
| 22:19:45 | efried | And sorry, I missed that we return False for any 4xx | |
| 22:20:21 | efried | mriedem: I think 404 should return True (or somehow otherwise be ignorable) and any other 4xx should be a failure in delete_resource_provider - agreed. | |
| 22:20:22 | mriedem | so i'm not actually calling this https://github.com/openstack/nova/blob/fe976dcc559d059589a9ccf953a28e855abf50fb/nova/scheduler/client/report.py#L1920 | |
| 22:20:49 | mriedem | because the list of instances is empty, because the compute service is down so when i delete the instance, the api removes it 'locally' but doesn't cleanup allocations | |
| 22:20:55 | openstack | bug 1679750 in OpenStack Compute (nova) queens "Allocations are not cleaned up in placement for instance 'local delete' case" [Medium,Confirmed] https://launchpad.net/bugs/1679750 | |
| 22:20:55 | mriedem | because of bug 1679750 | |
| 22:21:07 | mriedem | then https://github.com/openstack/nova/blob/fe976dcc559d059589a9ccf953a28e855abf50fb/nova/scheduler/client/report.py#L1922 fails with ResourceProviderInUse and we pass | |
| 22:21:41 | efried | noyce | |
| 22:22:11 | efried | mriedem: Me, I would rewrite a cascading RP deletion routine that doesn't rely on nova-isms. | |
| 22:22:30 | efried | You pass it a provider name/UUID and it goes after all related records, period. | |
| 22:22:42 | efried | I thought we had that somewhere already. | |
| 22:22:55 | mriedem | i just need to fix that bug | |
| 22:22:58 | mriedem | which has been around forever | |
| 22:23:30 | mriedem | i'm actually happy my test fails here, because the cascade silent fail means we're not doing something right | |
| 22:23:50 | efried | mriedem: Do you need help writing a real cascading delete in report client? | |
| 22:23:54 | mriedem | no | |
| 22:24:11 | mriedem | i need to fix that bug in the api local delete case to cleanup allocations in placement when the compute service is down | |
| 22:24:42 | mriedem | will be more clear when i post this code | |
| 22:24:44 | efried | Which you're going to need to do by getting placement to delete allocations based on the resource providers, not the instances. | |
| 22:24:53 | mriedem | no | |
| 22:24:56 | mriedem | we're deleting the instance | |
| 22:24:58 | mriedem | we know the consume | |