| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-09-01 | |||
| 16:00:10 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Modernize set_vm_state_and_notify https://review.openstack.org/499799 | |
| 16:00:10 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Refactor out claim_resources_on_destination into a utility https://review.openstack.org/499718 | |
| 16:00:18 | stephenfin | sdague: If you get a chance, would appreciate a sanity check on https://bugs.launchpad.net/nova/+bug/1704458 | |
| 16:00:19 | openstack | Launchpad bug 1704458 in OpenStack Compute (nova) "The use_ipv6 flag not only influences nova networking" [High,Invalid] - Assigned to Stephen Finucane (stephenfinucane) | |
| 16:07:32 | mriedem | stephenfin: regarding that dhcp_domain thing for dns, i think it makes sense that if you're using neutron, you pull it from the neutron network resource as it appears your series is doing | |
| 16:07:48 | mriedem | stephenfin: i just have to get through all of your unrelated changes to get to the meat of the fix first... : | |
| 16:07:49 | mriedem | :) | |
| 16:09:12 | stephenfin | mriedem: Right, but the question is whether we should actually be using FQDN at all, rather than from neutron instead. sean-k-mooney seemed to suggest it was a bad idea, but my network/sysadmin foo is not good enough to say why not :) | |
| 16:09:41 | stephenfin | Also, I like refactoring. Sorry :D I can pull those out | |
| 16:10:11 | mriedem | run swift young stephen | |
| 16:10:18 | mriedem | don't look back.... | |
| 16:18:37 | openstackgerrit | Matt Riedemann proposed openstack/nova master: doc: fix online_data_migrations option in upgrades doc https://review.openstack.org/500124 | |
| 16:34:37 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Set error state after failed evacuation https://review.openstack.org/499237 | |
| 16:35:50 | mriedem | we're going to want to backport this fix ^ back through newton | |
| 16:35:58 | mriedem | so it'd be nice to start that, it's pretty trivial | |
| 16:36:32 | mriedem | prevents the source compute from deleting local instance if the evacuation fails in conductor to find a dest host | |
| 16:39:00 | dansmith | mriedem: so just thinking about this quickly, | |
| 16:39:31 | dansmith | mriedem: are you thinking that if we fail to schedule a new place and the source comes back up that there's value in having it still there? | |
| 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 | openstackgerrit | Merged openstack/nova master: add online_data_migrations to nova docs https://review.openstack.org/493442 | |
| 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: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 | dansmith | we'll create the new allocation during the rebuild | |
| 16:54:18 | mriedem | meh? | |
| 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 | dansmith | it would from that novalidhost | |
| 16:55:08 | mriedem | and conductor doesn't set it to ERROR state if it fails to find a host | |
| 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 | 'task_state': None}, ex, request_spec) | |
| 16:55:30 | mriedem | {'vm_state': instance.vm_state, | |
| 16:55:30 | mriedem | 'rebuild_server', | |
| 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... | |