| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2023-04-12 | |||
| 16:26:15 | bauzas | the fact is, say the virt driver returns nothing suddently | |
| 16:26:29 | bauzas | we need to handle this case correctly | |
| 16:26:33 | dansmith | so my change there means we no longer update/create the migration if we're going to bail out early | |
| 16:26:45 | dansmith | but we might want to follow up with something to actually abort | |
| 16:26:54 | bauzas | yes | |
| 16:27:09 | dansmith | so I thought this might be for ironic for some reason | |
| 16:27:17 | bauzas | the fact is, we were allowing a migration to succeed even if we weren't tracking its resources | |
| 16:27:29 | dansmith | because they override their disabled check to mean more than just a missing compute | |
| 16:27:41 | dansmith | if not, I can update my comment there | |
| 16:27:56 | bauzas | do you feel brave enough to untangle this spaghetti code ? | |
| 16:28:03 | dansmith | I've been holding off writing tests for this change in case I was missing something but it sounds like I'm good to proceed | |
| 16:28:22 | bauzas | yeah and honestly I'm a bit afraid of the potential impacts we may create | |
| 16:28:30 | dansmith | all I have the appetite for at the moment is doing this change to it, because I've still got work to do on the rest of the series | |
| 16:28:54 | dansmith | potential impact of what? not creating the migration object or making this an error instead of a silent Nop ? | |
| 16:29:56 | bauzas | the potential impact of having a migration succeed as a noop without having a migration record set | |
| 16:30:26 | dansmith | okay I'm confused I thought you were sure we couldn't be actually running this | |
| 16:30:44 | dansmith | however, if we are, surely it's better to have it fail, find out how we're getting there, and fix it right? | |
| 16:30:47 | dansmith | because like I said, | |
| 16:30:54 | dansmith | if we hit this code, we're not even creating instance.migration_context | |
| 16:31:13 | dansmith | or the pci claims | |
| 16:31:18 | bauzas | there are two preconditions in order to have this disabled property returning True | |
| 16:31:22 | dansmith | and we won't have the numa stuff that goes along with it | |
| 16:31:57 | bauzas | the first precondition, which is to have the variable to be set internally, seems enough trivial to me, as this being a race condition hack at startup | |
| 16:32:29 | bauzas | I'm a bit concerned by the second precondition, which is that the virt driver returns yay or nay | |
| 16:32:36 | dansmith | yeah, and "disabled" is a very strange word to use for the first case | |
| 16:32:56 | dansmith | bauzas: for ironic it seems to be "if this node exists at all" which hopefully should never happen for transient reasons | |
| 16:33:40 | dansmith | for libvirt it would potentially be a rename issue I think, | |
| 16:33:52 | dansmith | which is a case where we might have been renamed and then accept a migration for a node we're not actually supposed to be hosting | |
| 16:33:59 | dansmith | which is another good reason to make this strict | |
| 16:38:17 | dansmith | like, if I send a message to a compute node and say "yo dawg, I'm looking to migrate an instance into your does-not-exist node", the compute node will happily create a migration object for that node, and punt on the rest of everything and silently say "roger that" | |
| 16:38:52 | dansmith | if I set CONF.host to something that matches another compute by accident, I'll do that for nodes they legit own | |
| 16:40:05 | bauzas | for libvirt, yup | |
| 16:40:14 | dansmith | right, I mean for libvirt | |
| 16:40:21 | bauzas | agreed, this is a weird exception handling | |
| 16:41:13 | bauzas | like, you are sent to a compute service where the virt driver attached to it reports that it doesn't know the compute node target you may want to use | |
| 16:41:27 | bauzas | this ^ is for all drivers | |
| 16:42:31 | dansmith | right | |
| 16:42:41 | dansmith | and it's crazy to just silently accept | |
| 16:43:06 | dansmith | since we don't even create the migration context in the current form, I'm sure no migrations could be working in this case | |
| 16:44:23 | bauzas | I also don't see any good reason for doing this, even with the FakeDriver | |
| 16:44:32 | dansmith | ack | |
| 16:44:57 | dansmith | I'll update the comments there to reflect this and proceed unless you have any other checking you want to do | |
| 16:46:28 | bauzas | dansmith: given all we discussed, my only open concern is, shouldn't we rather hardstop the migration call here? | |
| 16:46:39 | bauzas | instead of returning a noop ? | |
| 16:46:52 | dansmith | bauzas: we should, but I just don't want to tangle that up with my effort to get these objects linked in the database | |
| 16:47:11 | dansmith | because like I said, migration_context below is already skipped, and no migration can work in that case either, AFAIK | |
| 16:47:29 | dansmith | (instance.migration_context, even though the migration object is created at the top) | |
| 16:47:55 | bauzas | dansmith: that's the only concern I have, as said | |
| 16:48:05 | bauzas | if we don't return an exception, then the migration will be accepted | |
| 16:48:12 | dansmith | if we weren't creating the migration object at the top before we check for the node, I wouldn't even have had to ask about this, and it's not really related to the rest of the work | |
| 16:48:21 | dansmith | bauzas: right but it already is | |
| 16:48:42 | bauzas | eventually the instance status could be something like 'resize_confirm' | |
| 16:48:58 | bauzas | without having any migration records attached to it | |
| 16:49:08 | dansmith | bauzas: my point is that the create_migration() will proceed even with a missing node, so it's not like that is stopping us from returning the nopclaim | |
| 16:49:18 | bauzas | and revert resize wouldn't work, since no migration record exists | |
| 16:49:39 | dansmith | right but we can't even get that far without instance.migration_context right? | |
| 16:50:25 | bauzas | I have to admit this is out of my knowledge | |
| 16:51:05 | dansmith | actually, I think the conductor will fail earlier than that even, | |
| 16:53:00 | dansmith | okay you know what.. maybe we're missing more history here | |
| 16:53:19 | dansmith | I think maybe this _create_migration() is from before we had conductor-directed migrations? | |
| 16:53:33 | bauzas | honestly, tomorrow I'll setup a 2-node devstack and try doing crazypants migrations | |
| 16:53:37 | dansmith | so maybe we always pre-create the migration in the conductor (at least for resize) now? | |
| 16:53:55 | bauzas | oh, this is surely existing from 9 years | |
| 16:54:14 | dansmith | so maybe we should actually fail here if migration is None and not fall back to creating the migration | |
| 16:54:19 | dansmith | (need to check the other migration types though) | |
| 16:54:27 | bauzas | from what I recall, this was refactored by ndipanov when he wrote the NUMA migrations | |
| 16:54:47 | bauzas | which were something like Liberty IIRC | |
| 16:55:00 | bauzas | or even Kilo | |
| 16:55:24 | bauzas | way before we had the conductor engines managing the migration records (in Newton or Ocata IIRC) | |
| 16:55:42 | bauzas | so yeah you may be true | |
| 16:56:10 | bauzas | maybe all those codepaths aren't walked anyway, if we fail earlier | |
| 16:56:21 | bauzas | or maybe the conductor fails after | |
| 16:56:22 | dansmith | I'm now realizing there is more in the conductor that needs to be node-id-focused | |
| 16:56:35 | dansmith | yeah my concern is live migration | |
| 16:56:40 | dansmith | and evac | |
| 16:57:01 | dansmith | cross-cell definitely happens in the superconductor because it mirrors the source cell migration | |
| 16:57:41 | bauzas | I think we still rely on conductor-based operations for allocations | |
| 16:57:48 | bauzas | for those two move ops | |
| 16:57:54 | dansmith | either way, the node id part is handled on the destination node, so it actually doesn't change anything | |
| 16:58:11 | dansmith | we'll call to the compute and it will stamp the nodename we gave it on the migration even if it doesn't know that node | |
| 16:58:30 | dansmith | the update half of that | |
| 16:59:42 | dansmith | okay you're probably EOD at this point right? | |
| 16:59:54 | dansmith | I have to run to something else for a bit, but maybe think on it a bit | |
| 17:00:17 | dansmith | I guess I could also put a DNM patch to raise there and make sure at least we don't see it in any of our multinode CI tests | |
| 17:01:16 | bauzas | dansmith: tbh, I was wanting to cycle again on your spec | |
| 17:01:27 | bauzas | I started it before the meeting | |
| 17:01:31 | dansmith | yes, that would also be appreciated | |
| 17:01:35 | bauzas | and then you puzzled me :) | |
| 17:01:40 | dansmith | I updated it for some of this migration stuff last week | |
| 17:01:50 | bauzas | so before I call, I'll get it a round | |
| 17:02:21 | dansmith | ack | |
| 17:06:49 | bauzas | and done | |
| 17:06:59 | bauzas | this was quick, I was at the migration object change | |
| 17:09:03 | bauzas | Uggla: you're next in the review list :) | |
| 17:09:12 | bauzas | but this will be done tomorrow | |
| 17:09:16 | bauzas | see ya folks | |
| 17:26:32 | opendevreview | Dan Smith proposed openstack/nova master: Add compute_id columns to instances, migrations https://review.opendev.org/c/openstack/nova/+/879499 | |
| 17:26:32 | opendevreview | Dan Smith proposed openstack/nova master: Populate ComputeNode.service_id https://review.opendev.org/c/openstack/nova/+/879904 | |
| 17:26:33 | opendevreview | Dan Smith proposed openstack/nova master: Add compute_id to Instance object https://review.opendev.org/c/openstack/nova/+/879500 | |
| 17:26:33 | opendevreview | Dan Smith proposed openstack/nova master: Add dest_compute_id to Migration object https://review.opendev.org/c/openstack/nova/+/879682 | |