| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-11-13 | |||
| 22:11:04 | mriedem | but that's not a backportable change | |
| 22:11:18 | melwitt | yeah, if port bindings AND volume attach succeed then saying you're on the dest now is fine | |
| 22:11:18 | mriedem | and trying to rollback automatically from everything that could go wrong during finish_resize is likely to be pretty hairy | |
| 22:11:27 | melwitt | but if volumes fails, I don't see how dest could be ok | |
| 22:12:19 | melwitt | I was thinking of swapping the network setup and volume update only, in _finish_resize | |
| 22:13:44 | melwitt | so that if volume update fails, set the host/node back to the source like you said and bail. leave the port bindings alone | |
| 22:14:02 | mriedem | i don't think that gets you out of the woods, | |
| 22:14:31 | mriedem | because resize_instance on the source, before casting to finish_resize on the dest, deletes the old volume attachment with the source host connector and creates a new 'empty' volume attachment that gets updated on the dest with the dest host connector | |
| 22:14:37 | mriedem | see _terminate_volume_connections | |
| 22:14:44 | melwitt | O.o | |
| 22:14:52 | melwitt | dammit | |
| 22:15:10 | mriedem | so even if you go back to the source, the volume attachment has to be updated with the source host connector to re-connect the volumes on the source host | |
| 22:16:18 | melwitt | gah, this is over my head. I let them know they might have more to do to fix that VM | |
| 22:16:21 | mriedem | i want to say that at some point booth had a patch which would automatically call revert_resize (dest) from finish_resize (dest) if finish_resize failed | |
| 22:17:13 | mriedem | but i very much doubt that revert_resize is idempotent | |
| 22:17:18 | melwitt | that sounds familiar | |
| 22:17:51 | mriedem | i.e. i would not be surprised if just calling revert_resize from finish_resize fails in some weird way because it has some implicit preconditions that aren't setup when it's called to rollback | |
| 22:18:27 | melwitt | yeah | |
| 22:18:32 | mriedem | now if this all used task flow tasks with built in rollbacks...then we'd be cooking! | |
| 22:19:01 | mriedem | i kid, but that's why i used granular conductor tasks for the cross-cell stuff so we can rollback at certain points in the flow | |
| 22:20:01 | melwitt | task flow did come to mind | |
| 22:20:35 | mriedem | i was hoping efried had it as a keyword and he'd just appear | |
| 22:20:50 | efried | *poof* | |
| 22:21:06 | efried | you should ask johnsom, shiny new taskflow core. Been a while since I touched it. | |
| 22:21:31 | efried | though it's pretty simple when used simply. | |
| 22:22:06 | efried | and you're already doing most of the work, defining `execute` and `rollback` methods in your classes. Make those inherit from Task and do a little plumbing on the engine side, and presto. | |
| 22:22:21 | johnsom | Hi, happy to chat about taskflow, but I'm in a meeting right now. Ping me in 30 if you need something | |
| 22:22:44 | efried | jroll: There's generally a master switch to enable a feature like swtpm. I'm just defining that. I was starting to make it boolean, but it occurred to me that someone might care to say "enable vtpm {1.2 and/or 2.0}". Do you? | |
| 22:23:12 | jroll | efried: I'm not sure if we care | |
| 22:23:22 | jroll | that seems reasonable, though, 2.0 isn't backward compat | |
| 22:23:32 | jroll | (from an application perspective) | |
| 22:23:58 | efried | yeah, rn there's support for both or neither, conf opt notwithstanding. | |
| 22:24:12 | efried | so really it would just allow you to disable one or the other. | |
| 22:24:26 | melwitt | mriedem: how do we find the source host connector? is it in our db or somewhere else? | |
| 22:24:52 | efried | jroll: Easier for me if you don't care. Since you didn't immediately, I'll code it up bool, and we can discuss it in review if necessary. | |
| 22:24:54 | jroll | efried: in the 'both' case, how is it decided which version is presented to a vm? | |
| 22:25:01 | efried | jroll: flavor | |
| 22:25:07 | jroll | ah | |
| 22:25:13 | efried | you are required to ask for one version or the other | |
| 22:25:16 | jroll | yeah, +1 for discuss in review | |
| 22:25:20 | efried | because, as you say, they're not compat. | |
| 22:28:53 | mriedem | melwitt: the host connector is retrieved from the driver | |
| 22:29:27 | mriedem | cinder does stash the host connector in the volume attachment record when we update the attachment, but as noted in resize_instance we blow away the source host attachment and create a new empty attachment for the dest host to update | |
| 22:29:27 | melwitt | argh.. ok, I was just seeing that in the code | |
| 22:30:10 | mriedem | having said that, we (nova) do stash the last host connector used in the bdm.connection_info i think | |
| 22:30:14 | melwitt | :'( | |
| 22:30:26 | melwitt | cray cray | |
| 22:30:49 | mriedem | that might only be when using initialize_connection for legacy attachments though | |
| 22:32:14 | mriedem | i don't think we do that pack rat stash for new style volume attachments because cinder stores that information per volume attachment | |
| 22:32:39 | mriedem | whereas before with initialize_connection we worked with a single attachment record and duplicated it in our bdm.connection_info | |
| 22:33:04 | melwitt | looks like we're ok, this is using ceph and somehow it's connected to the correct host (source) | |
| 22:33:16 | mriedem | in this very specific scenario | |
| 22:33:18 | melwitt | and it's boot from volume | |
| 22:33:20 | melwitt | yeah | |
| 22:33:48 | mriedem | i think another idea that has come up before when talking about resize fail recovery is allowing the admin to reset the status on the server so they could then revert the resize | |
| 22:34:02 | mriedem | or allow the revert resize api to work with instances in ERROR status | |
| 22:34:31 | melwitt | yup. that is the first thing I told them to try and there was no way to get the api to allow it | |
| 22:34:51 | mriedem | the instance was in ERROR state in this case right? | |
| 22:35:11 | mriedem | so if the api was changed to allow reverting a resize on an instance in ERROR status that could be potentially one way | |
| 22:35:13 | melwitt | it was originally yeah | |
| 22:35:30 | mriedem | and we can detect that the instance was being resized because of the old_vm_state and old/new flavor stuff stashed on the instance that isn't cleaned up until confirm/revert | |
| 22:35:48 | mriedem | meaning, the api could puke if you tried reverting a resize on an ERROR instance that wasn't actually being resized | |
| 22:37:14 | mriedem | i want to say dansmith was in that discussion when it happened and wasn't crazy about the idea - maybe around the time of the proposal from booth to auto-revert on failed resize | |
| 22:37:25 | mriedem | or maybe dan preferred just reverting from ERROR instead, i can't remember | |
| 22:38:02 | melwitt | hmm ok | |
| 22:38:31 | mriedem | https://review.opendev.org/#/c/462521/ was booth's patch btw | |
| 22:40:29 | melwitt | ah ok | |
| 22:41:26 | melwitt | tbh I think reverting from ERROR would be nice as just something | |
| 22:42:38 | melwitt | it's painful knowing all the code is there to do the thing but you can't get to it because of a vm_state block and then have to do a bunch of gnarly manual stuff | |
| 22:46:29 | mriedem | yeah it's definitely a kick in the ass to fix up a failed resize because of volumes, networking, allocations in placement, and some day cyborg devices, | |
| 22:46:42 | mriedem | plus the quota stuff involved to even move the server back | |
| 22:47:00 | mriedem | like you said, the source host was full when they tried moving it back | |
| 22:47:21 | mriedem | i left a note in https://review.opendev.org/#/c/462521/12/releasenotes/notes/resize-auto-revert-6e1648828aba16b2.yaml@5 if it helps start a new discussion about changing the API | |
| 22:49:55 | melwitt | yeah thanks. and thanks for talking through this, I'm gonna refresh on what was going on in that review and then I was thinking start a ML thread about the API angle based on your comment | |
| 22:51:08 | mriedem | i believe the tl;dr on why that patch to auto-revert was not great was dan's comment in the reno | |
| 22:51:20 | mriedem | external tooling could get screwed up | |
| 22:51:38 | melwitt | given the behaviorial/api change called out on that patch, I'm not sure whether there's a decent way to roll back automatically. if there's not, the api change would be a big help imho | |
| 22:51:43 | mriedem | just allowing reverting an ERROR'ed resize from the api though is pretty straight-forward and opt-into-y | |
| 22:51:52 | melwitt | yeah | |
| 22:52:16 | openstackgerrit | Takashi NATSUME proposed openstack/nova master: Update keypairs in saving an instance object https://review.opendev.org/683043 | |
| 23:01:34 | dansmith | mriedem: yeah, not sure I remember revert-from-ERROR specifically, but definitely not in favor of auto rollback | |
| 23:09:31 | openstackgerrit | Takashi NATSUME proposed openstack/nova master: Update keypairs in saving an instance object https://review.opendev.org/683043 | |
| 23:13:47 | openstackgerrit | Takashi NATSUME proposed openstack/nova master: Update keypairs in saving an instance object https://review.opendev.org/683043 | |
| 23:22:31 | openstackgerrit | Merged openstack/nova master: cond: rename 'recreate' var to 'evacuate' https://review.opendev.org/692900 | |
| 23:27:53 | openstackgerrit | Merged openstack/nova master: Remove PlacementAPIConnectFailure handling from AggregateAPI https://review.opendev.org/660852 | |
| 23:30:10 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Do not reschedule on ExternalNetworkAttachForbidden https://review.opendev.org/694179 | |
| 23:34:42 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Do not reschedule on ExternalNetworkAttachForbidden https://review.opendev.org/694179 | |
| 23:36:38 | openstackgerrit | Eric Fried proposed openstack/nova master: WIP: Add emulated TPM support to Nova https://review.opendev.org/631363 | |
| 23:36:39 | openstackgerrit | Eric Fried proposed openstack/nova master: WIP: vTPM request_filter https://review.opendev.org/678325 | |
| 23:36:39 | openstackgerrit | Eric Fried proposed openstack/nova master: Add support for resize and cold migration of emulated TPM files https://review.opendev.org/639934 | |
| 23:43:48 | openstackgerrit | Merged openstack/nova master: Remove dead HostAPI.service_delete code https://review.opendev.org/693422 | |
| 23:43:55 | openstackgerrit | Merged openstack/nova master: Add support matrix for Delete (Abort) on-going live migration https://review.opendev.org/625781 | |
| 23:44:03 | openstackgerrit | Merged openstack/nova master: Implement update_provider_tree for mocked driver in test_resource_tracker https://review.opendev.org/693431 | |
| 23:45:42 | openstackgerrit | Matt Riedemann proposed openstack/nova master: api-ref: mark device response param as optional for list/show vol attachments https://review.opendev.org/690383 | |
| #openstack-nova - 2019-11-14 | |||
| 00:54:46 | openstackgerrit | Merged openstack/nova master: Drop compat for non-update_provider_tree code paths https://review.opendev.org/693435 | |
| 00:59:21 | openstackgerrit | Merged openstack/nova master: Delete _normalize_inventory_from_cn_obj https://review.opendev.org/693436 | |
| 00:59:27 | openstackgerrit | Merged openstack/nova master: Remove unused CannotMigrateWithTargetHost https://review.opendev.org/693936 | |
| 01:10:35 | openstackgerrit | Merged openstack/nova master: Restrict RequestSpec to cell when evacuating https://review.opendev.org/650429 | |
| 01:17:09 | openstackgerrit | Merged openstack/nova stable/rocky: doc: fix and clarify --block-device usage in user docs https://review.opendev.org/693554 | |
| 01:17:16 | openstackgerrit | Merged openstack/nova stable/rocky: Revert "openstack server create" to "nova boot" in nova docs https://review.opendev.org/693240 | |