| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2021-08-17 | |||
| 12:15:17 | sean-k-mooney | i assume the except Exception: is to catch the libvirt error that is raised when that happens | |
| 12:16:41 | gibi_ | aarents: oops sorry I jumped to the wrong driver.live_migration_abort call. | |
| 12:16:45 | aarents | this will work only with network,RPC,DB issues not for libvirt issue | |
| 12:17:11 | sean-k-mooney | aarents: so for those cases im not sure we want to abort the migration | |
| 12:17:24 | sean-k-mooney | aarents: unless you want to abort all other operation when that happens | |
| 12:17:28 | aarents | sean-k-mooney: or it may work if there is only one flap from libvirt | |
| 12:17:33 | sean-k-mooney | spawns, deletes ectra | |
| 12:20:00 | sean-k-mooney | its a larger chagne but to me what feels like a more robost change would be to suspend the green thread if the connection is closed and resume it when we reconnect and only try to send the rpc call then | |
| 12:20:55 | sean-k-mooney | realistically if the rpc bus is down there is notight on the compute we can do to update the db state | |
| 12:24:18 | aarents | sean-k-mooney: honestly, the change is just ensuring to kill job regardless if state in can or cannot update in DB, we loss some instances due to that as explain in bug | |
| 12:24:19 | gibi_ | hm, so assuming we have the RPC down. the patch aborts the libvirt job. then raises the exception as today. That exception expected to update the instance and migration states which will not happen while the RPC is down. the nova compute RPC call to update the DB will time out eventually I guess. | |
| 12:24:55 | aarents | gibi_: yes | |
| 12:24:56 | gibi_ | so the nova DB will still see the migration as runnig | |
| 12:25:02 | gibi_ | but the compute already aborted it | |
| 12:25:19 | gibi_ | does the conductor time out the migration eventually too? | |
| 12:25:20 | sean-k-mooney | aarents: have you tested this with post-copy enabled | |
| 12:25:49 | sean-k-mooney | gibi_: i think the timeout happened at the comptue level | |
| 12:25:54 | sean-k-mooney | not the conductor | |
| 12:26:18 | gibi_ | sean-k-mooney: so there is no cleanup triggered be the conductor for this aborted migration | |
| 12:26:26 | gibi_ | s/be/by/ | |
| 12:26:36 | sean-k-mooney | im not sure | |
| 12:26:42 | aarents | gibi_: yes there will be inconsitency that need operator intervention, but vm will be safe because still referenced in source host & running on source host | |
| 12:26:55 | gibi_ | aarents: I see. that was the missing piece | |
| 12:27:04 | sean-k-mooney | aarents: again i dont know if that is always correct | |
| 12:27:17 | gibi_ | aarents: so this change does not try to fix an DB inconsistency but try to save the VM | |
| 12:27:18 | sean-k-mooney | you have ignored my post-copy question | |
| 12:28:00 | aarents | sean-k-mooney: good question | |
| 12:28:24 | gibi_ | sean-k-mooney: so you suggest that the save move would be to abort the non post-copy migrations and let the post-copy migrations run forward | |
| 12:28:24 | aarents | gibi_: exactly, I was not clear | |
| 12:28:33 | gibi_ | /save/safe/ | |
| 12:28:37 | sean-k-mooney | gibi_: yes | |
| 12:28:47 | sean-k-mooney | but only afte we are in the post copy phase | |
| 12:28:53 | gibi_ | sean-k-mooney: I guess we document that post-copy means no way back | |
| 12:29:14 | sean-k-mooney | well we can abort until we enter the post copy phase | |
| 12:29:27 | aarents | sean-k-mooneyI don't have so much experiance about post copy in operation | |
| 12:29:33 | sean-k-mooney | but when we hit post copy suspend we call post_live_migration | |
| 12:29:44 | sean-k-mooney | and update the host and prot bindings | |
| 12:31:05 | sean-k-mooney | aarents: we dont have access to the last known state of the instance at thsi point do we | |
| 12:31:09 | sean-k-mooney | form a libvirt perspecitiv | |
| 12:31:41 | sean-k-mooney | assuming not then i would make the abort condtional on "not postcopy_enabled" | |
| 12:31:57 | sean-k-mooney | to be on the safe side | |
| 12:33:04 | sean-k-mooney | gibi_: looking at https://github.com/openstack/nova/blob/master/nova/conductor/tasks/live_migrate.py i dont see anything that looks like cleanup logic once a migration has started | |
| 12:33:11 | aarents | sean-k-mooney: no we don't have access to the instance state | |
| 12:34:21 | sean-k-mooney | we handel messigng time ectra form check_can_live_migrate_destination and other cases but there seams to be no overall timeout enforced by the conductor | |
| 12:34:49 | sean-k-mooney | which kind of makes sense since the live migration timout option are virt diriver specific | |
| 12:35:16 | sean-k-mooney | as is whereter we abort or force complete when the timeout expires | |
| 12:35:19 | gibi_ | sean-k-mooney: ack, I got it now that aarents' goal is to save the VM running state instead of avoiding the DB inconsistency in case of RPC/DB issue | |
| 12:36:35 | gibi_ | sean-k-mooney, aarents: I'm fine with the patch with addition of the "not postcopy_enabled" condition as sean-k-mooney suggests | |
| 12:37:13 | sean-k-mooney | gibi_: i think i would be oke with it with that added also | |
| 12:37:19 | gibi_ | cool | |
| 12:37:37 | aarents | gibi_: sean-k-mooney yep this "not postcopy_enabled" condition make sense | |
| 12:38:30 | aarents | I will add that, thanks | |
| 12:39:19 | lyarwood | https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainAbortJob FWIW | |
| 12:39:26 | lyarwood | In case the job is a migration in a post-copy mode, virDomainAbortJob will report an error (see virDomainMigrateStartPostCopy for more details). | |
| 12:40:20 | lyarwood | https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainMigrateStartPostCopy has some more context | |
| 12:40:29 | lyarwood | On the other hand once the guest is running on the destination host, the migration can no longer be rolled back because none of the hosts has complete state. If this happens, libvirt will leave the domain paused on both hosts with VIR_DOMAIN_PAUSED_POSTCOPY_FAILED reason. It's up to the upper layer to decide what to do in such case. Because of this, libvirt will refuse to cancel post-copy migration via virDomainAbortJob. | |
| 12:41:38 | lyarwood | so tbh I don't think we need to check anything | |
| 12:42:42 | lyarwood | oh wait I missed that live_migration_abort is raising the error back, sigh | |
| 12:45:25 | sean-k-mooney | ya although we are catching and ignoring that with a log in aarents patch | |
| 12:45:44 | sean-k-mooney | i guess we could rely on that behavior but i would prefer to have a comment to that effect honestly | |
| 12:45:49 | sean-k-mooney | jsut to not forget that | |
| 12:46:13 | sean-k-mooney | as the next time i see the abbort ill get suspicios about post copy again. | |
| 12:49:01 | gibi_ | aarents: are you seeing this ^^ :) | |
| 12:50:17 | aarents | gibi_: Yes so I will add a comment that say that abort may not work in case of post copy ? | |
| 12:50:51 | gibi_ | aarents: I guess you need to catch the error returned from abort and ignore it | |
| 12:52:03 | aarents | So I drop the warning | |
| 12:52:25 | aarents | ? | |
| 12:53:22 | gibi_ | aarents: sorry, so you already catching the error from abort, that is OK | |
| 12:53:27 | gibi_ | keep the warning too | |
| 12:53:32 | aarents | And is there a concensus about sean-k-mooney suggestion to change except Exception with except libvirt.libvirtError: ? | |
| 12:53:32 | gibi_ | just add a note as sean-k-mooney requested | |
| 12:53:38 | aarents | gibi_: ok | |
| 12:53:55 | gibi_ | yepp go with libvirtError | |
| 12:55:00 | aarents | ok cool | |
| 13:05:41 | opendevreview | Stephen Finucane proposed openstack/nova master: db: Final cleanups https://review.opendev.org/c/openstack/nova/+/800484 | |
| 13:05:41 | opendevreview | Stephen Finucane proposed openstack/nova master: docs: Add documentation on database migrations https://review.opendev.org/c/openstack/nova/+/800078 | |
| 13:05:42 | opendevreview | Stephen Finucane proposed openstack/nova master: WIP tests: Enable SQLAlchemy 2.0 deprecation warnings https://review.opendev.org/c/openstack/nova/+/804709 | |
| 13:05:42 | opendevreview | Stephen Finucane proposed openstack/nova master: tests: Enable SADeprecationWarning warnings https://review.opendev.org/c/openstack/nova/+/804708 | |
| 13:05:47 | stephenfin | gibi_: ^ | |
| 13:07:02 | opendevreview | Lee Yarwood proposed openstack/nova master: api: Introduce microversion 2.89 adding attachment_id to responses https://review.opendev.org/c/openstack/nova/+/804275 | |
| 13:09:15 | gibi | stephenfin: ack | |
| 13:10:52 | gibi | stephenfin: thanks, now I'm +2 all the way | |
| 13:53:17 | opendevreview | Stephen Finucane proposed openstack/nova master: tests: Enable SQLAlchemy 2.0 deprecation warnings https://review.opendev.org/c/openstack/nova/+/804709 | |
| 13:53:18 | opendevreview | Stephen Finucane proposed openstack/nova master: Replace use of Engine.scalar(), Engine.execute() https://review.opendev.org/c/openstack/nova/+/804878 | |
| 13:55:29 | opendevreview | Elod Illes proposed openstack/nova stable/rocky: [stable-only] Fix lower-constraints job https://review.opendev.org/c/openstack/nova/+/769910 | |
| 14:00:34 | opendevreview | Alexandre arents proposed openstack/nova master: libvirt: Abort live-migration job when monitoring fails https://review.opendev.org/c/openstack/nova/+/764435 | |
| 14:28:08 | opendevreview | Elod Illes proposed openstack/nova stable/rocky: [stable-only] Fix lower-constraints job https://review.opendev.org/c/openstack/nova/+/769910 | |
| 14:50:29 | gibi | melwitt: hi! I left feedback in https://review.opendev.org/c/openstack/nova/+/713301 the most concerning for me is the dependency on an oslo.limit patch as non client libraries are going to feature freeze this week | |
| 15:49:10 | ganso | melwitt, gibi hi! if you have a few minutes could please take a look at this 1-liner fix https://review.opendev.org/c/openstack/nova/+/804303 ? Thanks in advance | |
| 15:50:32 | gibi | ganso: looking | |
| 15:52:40 | spatel | sean-k-mooney hey! i am upgrading minor version of victoria and during upgrade at this step i hit this issue - https://paste.opendev.org/show/808150/ | |
| 15:54:48 | gibi | FYI, nova meeting starts in 5 minutes here in the channel | |
| 15:56:13 | gibi | ganso: does the 1 vcpu + multiqueue case works for other than vif_type=tap? | |
| 15:57:33 | ganso | gibi that code path is only reached when vif_type=tap. If using openvswitch, it wasn't impacted by the previous patch (that introduced the regression), neither this. I am not sure if the same problem happens with ovs, but since the original problem didn't, I believe this one also doesn't | |
| 15:59:48 | gibi | ganso: ok, let me try with ovs | |
| 15:59:55 | sean-k-mooney | vif_type=tap is not currently used with ovs | |
| 16:00:03 | sean-k-mooney | it was added tempoery and then removed | |
| 16:00:18 | sean-k-mooney | i belive the only thing that uses vif_type=tap today is calico | |
| 16:00:28 | gibi | sean-k-mooney: yepp the bug mentions calico | |
| 16:00:35 | gibi | sean-k-mooney: https://bugs.launchpad.net/nova/+bug/1939604 | |