| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-09-01 | |||
| 16:40:03 | dansmith | because the way this was designed, once an evacuation starts we're *going* to delete it from the source compute to avoid a race between rebuilding it elsewhere and the compute coming back up and deciding to keep it or not | |
| 16:40:42 | mriedem | melwitt: commented on https://review.openstack.org/#/c/407346/ - cinder would also need changes if we do something like this in nova | |
| 16:42:25 | mriedem | dansmith: hmm | |
| 16:42:28 | mriedem | that's a good point | |
| 16:42:51 | dansmith | knowing the folks that use this a lot from a script, | |
| 16:42:58 | melwitt | mriedem: cool thanks. looks like it's not as straightforward was I initially thought | |
| 16:43:00 | dansmith | the last thing they want is vague behavior from this | |
| 16:43:14 | mriedem | i wish that would have been a comment in the _delete_evacuated_instances code because i spent a good chunk of time yesterday trying to figure out why we include 'accepted' in the filter | |
| 16:43:42 | dansmith | mriedem: was it in my robustify spec? I think the logic was sussed out there | |
| 16:43:44 | dansmith | anyway, | |
| 16:43:52 | mriedem | i didn't read the spec | |
| 16:44:16 | dansmith | I would think that what we want is to make sure you can call rebuild (or at least reset-state) on the instance and have it rebuilt somewhere else | |
| 16:44:55 | dansmith | because I think we've nulled out the instance.host here at this point right? meaning it won't be a simple restart on the source host | |
| 16:45:05 | openstackgerrit | Merged openstack/nova master: Functional test for regression bug #1713783 https://review.openstack.org/500057 | |
| 16:45:07 | openstack | bug 1713783 in OpenStack Compute (nova) "After failed evacuation the recovered source compute tries to delete the instance" [High,In progress] https://launchpad.net/bugs/1713783 - Assigned to Matt Riedemann (mriedem) | |
| 16:45:26 | mriedem | dansmith: i don't think the instance.host is nulled out | |
| 16:45:30 | mriedem | which is part of the problem | |
| 16:45:39 | mriedem | reading the docstring for _destroy_evacuated_instances, it says, | |
| 16:45:48 | mriedem | "Check that the instances reported by the driver are still associated with this host. If they are not, destroy them" | |
| 16:45:48 | openstackgerrit | Merged openstack/nova master: add online_data_migrations to nova docs https://review.openstack.org/493442 | |
| 16:45:57 | mriedem | but it doesn't actually compare the instances that it finds against self.host | |
| 16:46:13 | dansmith | mriedem: right, it used to, which is why it would delete everything if your hostname changed | |
| 16:46:14 | mriedem | it just says, oh you had an evacuation from this host, we assume it was cool, delete local | |
| 16:47:23 | mriedem | it also says, "with the exception of instances which are in the MIGRATING, RESIZE_MIGRATING, RESIZE_MIGRATED, RESIZE_FINISH task state or RESIZED vm state" | |
| 16:47:24 | dansmith | mriedem: so it might not be nulled out, but if we're in the middle of it starting on the new destination host, we don't know, so if we don't delete it, we'll end up with it in two places | |
| 16:47:29 | mriedem | but i don't see that filtering happening anywhere | |
| 16:48:20 | dansmith | mriedem: yeah that's not from me, that's from dd6fb1246ff2789bd78b772b45e1fcac21eda67a | |
| 16:48:29 | dansmith | which looks like it had a filter in it which is now gone | |
| 16:48:41 | dansmith | that makes no sense to me though, | |
| 16:48:47 | dansmith | since that code already only looks at evacuations | |
| 16:49:48 | mriedem | https://review.openstack.org/#/c/101803/ predates the robustify that used the migration record for filtering | |
| 16:50:12 | dansmith | ah yep | |
| 16:50:20 | mriedem | so we just need to cleanup the comments in here a bit, | |
| 16:50:22 | dansmith | so maybe my bad for not removing that then | |
| 16:50:27 | mriedem | so if we don't take this change, | |
| 16:50:41 | mriedem | and the compute is up and running, need to think about what the RT is going to do | |
| 16:51:23 | mriedem | so we'd remove the allocation on the source node when it starts up | |
| 16:51:29 | dansmith | which is correct | |
| 16:51:38 | mriedem | right b/c the guest was deleted, | |
| 16:51:45 | mriedem | the instance isn't actually gone, and it points at the source host still | |
| 16:51:51 | dansmith | and it looks to me like we would be able to run rebuild on it | |
| 16:52:25 | dansmith | you can run rebuild from error state and it seems like it'll just pick up the evacuation and try again, | |
| 16:52:30 | dansmith | but not if we error it out like this patch does | |
| 16:52:55 | mriedem | the update_available_resource task is going to query for instances on this host/node, and pull this in since it's not deleted | |
| 16:53:02 | mriedem | pass that to _update_usage_from_instances | |
| 16:53:27 | mriedem | which calls _update_usage_from_instance with has_ocata_computes=True | |
| 16:53:30 | mriedem | *False | |
| 16:53:38 | mriedem | assuming you're upgraded | |
| 16:54:03 | mriedem | and it won't recreate the allocation in placement for the source node, which again is correct since we don't have the guest anymore | |
| 16:54:16 | mriedem | if you have ocata computes, it would, but... | |
| 16:54:18 | mriedem | meh? | |
| 16:54:18 | dansmith | we'll create the new allocation during the rebuild | |
| 16:54:56 | dansmith | and when build finishes it'll replace the doubled (due to migration) allocation with the single one for the new host | |
| 16:54:59 | mriedem | the instance wouldn't necessarily be in ERROR state, | |
| 16:55:08 | mriedem | and conductor doesn't set it to ERROR state if it fails to find a host | |
| 16:55:08 | dansmith | it would from that novalidhost | |
| 16:55:12 | mriedem | nope | |
| 16:55:17 | mriedem | it just resets the task_state to None | |
| 16:55:18 | dansmith | it does _set_vm_state_and_notify() | |
| 16:55:29 | mriedem | self._set_vm_state_and_notify(context, instance.uuid, | |
| 16:55:30 | mriedem | 'rebuild_server', | |
| 16:55:30 | mriedem | {'vm_state': instance.vm_state, | |
| 16:55:30 | mriedem | 'task_state': None}, ex, request_spec) | |
| 16:55:34 | mriedem | i've been all up in this code for a week | |
| 16:55:35 | dansmith | oh, does that not set it to error? | |
| 16:55:38 | mriedem | nope | |
| 16:55:40 | dansmith | I see | |
| 16:55:46 | dansmith | well a reset state will put it in error | |
| 16:55:52 | mriedem | if you tell it to :) | |
| 16:56:00 | mriedem | reset-state takes the state you want it in | |
| 16:56:01 | mriedem | i htink | |
| 16:56:04 | dansmith | doesn't reset state default to error? | |
| 16:56:11 | dansmith | no, it's just error or --active as an option I think | |
| 16:56:34 | mriedem | oh i don't know what the CLI does | |
| 16:56:37 | mriedem | but the API doesn't default | |
| 16:56:40 | mriedem | "The state of the server to be set, active or error are valid." | |
| 16:56:55 | dansmith | well, sure | |
| 16:57:10 | mriedem | yeah default on the CLI is 'error' | |
| 16:57:38 | dansmith | I guess we could make it go to error state in here as a change if you think that's better | |
| 16:57:51 | dansmith | I think that won't affect rebuilds because we don't schedule if we do a rebuild, but do for evac | |
| 16:58:02 | mriedem | right we bypass the scheduler for rebuild | |
| 16:59:05 | mriedem | so yeah we could set the instance to ERROR state in conductor... | |
| 16:59:17 | mriedem | if it's an evac | |
| 16:59:20 | mriedem | *failed evac | |
| 17:01:31 | mriedem | well, now i'm not sure what to do | |
| 17:03:15 | dansmith | have a beer? | |
| 17:05:40 | mriedem | i left a summary in https://review.openstack.org/#/c/499237/ | |
| 17:05:52 | mriedem | feel free to r'ar or whatever | |
| 17:05:56 | mriedem | i'm going to make lunch | |
| 17:58:37 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Update docs for _destroy_evacuated_instance https://review.openstack.org/500144 | |
| 18:03:28 | openstackgerrit | Sean Dague proposed openstack/nova master: DNM: test cells v1/nova-net without screen https://review.openstack.org/500151 | |
| 18:03:42 | sdague | mriedem: doesn't seem like cellsv1 job is even in experimental queue for devstack? | |
| 18:04:26 | mriedem | it should be | |
| 18:04:45 | mriedem | used to be anyway | |
| 18:05:26 | mriedem | but yeah i don't see it in there now | |
| 18:06:01 | mriedem | only in experimental for tempest and nova from what i see | |
| 18:09:16 | mriedem | sdague: are you going to add that or should i? | |
| 18:10:16 | openstackgerrit | Matt Riedemann proposed openstack/nova master: doc: link to placement api-ref and history docs from main index https://review.openstack.org/498977 | |
| 18:10:16 | openstackgerrit | Matt Riedemann proposed openstack/nova master: doc: link to versioned notification samples from main index https://review.openstack.org/500081 | |
| 18:26:12 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Remove dest node allocation if evacuate MoveClaim fails https://review.openstack.org/499878 | |