| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2023-04-12 | |||
| 16:14:35 | bauzas | the NopClaims were there for saying 'meh' | |
| 16:14:53 | bauzas | if no compute record was existing then basically we were tracking nothing | |
| 16:14:56 | dansmith | right but they also cause us to skip all the other stuff like PCI and migration objects, etc | |
| 16:15:10 | dansmith | but we allow the instance to be sent there | |
| 16:15:40 | bauzas | if the scheduler isn't having a compute node record, then it shouldn't be able to send an instance to it | |
| 16:15:57 | bauzas | and I think this assumption is still valid | |
| 16:16:14 | dansmith | except for times where we didn't go through the scheduler, which I think back when this was done, was the case | |
| 16:16:26 | dansmith | but if you're right, why would we not raise an exception here instead of silently "meh" ? | |
| 16:16:42 | bauzas | good question | |
| 16:16:52 | dansmith | and also, | |
| 16:17:06 | dansmith | transient changes could result in the scheduler thinking a compute node was valid for a given host, send a node there, | |
| 16:17:09 | bauzas | the NopClaims even predates Mr Jay's work on RT | |
| 16:17:18 | bauzas | and even my presence on OpenStack since this decade :) | |
| 16:17:21 | dansmith | and the RT would literally create a migration record with a destination node name that does not match anything it knows about | |
| 16:18:39 | dansmith | the nopclaim stuff itself predates tons of stuff, yeah, but the usage of it in this case is much newer | |
| 16:18:46 | dansmith | here's my problem: | |
| 16:19:07 | bauzas | oh | |
| 16:19:10 | dansmith | in order to do the strict tying of instance->compute->service, we need migrations to have a strict tie from migration->compute | |
| 16:19:19 | dansmith | which means we need the compute id in the migration | |
| 16:19:21 | bauzas | I missed the fact we record a migration before we return the NopClaim | |
| 16:19:29 | dansmith | right | |
| 16:19:31 | bauzas | technically this isn't a noop | |
| 16:19:31 | dansmith | crazy right? | |
| 16:19:54 | bauzas | dansmith: I wonder something, lemme check | |
| 16:19:59 | dansmith | so my code moves that to below the disabled check and refuses to create a migration record for a destination compute node we don't know anything about (and thus don't have a node id) | |
| 16:20:11 | bauzas | my guess is that someone added the migration checks *after* the nopclaim implementation | |
| 16:20:21 | bauzas | without thinking about the implication | |
| 16:20:23 | dansmith | here, the nodename loose affiliation is us just lying about the destination, which is easy if the nodename is just a name, but the id requires stricter adherence | |
| 16:20:29 | bauzas | (gosh hope it isn't me :) ) | |
| 16:22:15 | dansmith | what "migration checks" ? | |
| 16:22:24 | dansmith | those lines I'm moving are the creation of the migration object | |
| 16:22:33 | dansmith | (or the assignment of the migration to a node) | |
| 16:23:31 | dansmith | like, creating the migration and then returning out of that early also means instance.migration_context isn't created | |
| 16:25:12 | bauzas | yeah, I guess we need to be smarter here | |
| 16:25:38 | bauzas | we probably piled a couple of modifications without really considering the impact | |
| 16:26:13 | dansmith | okay, that's what I'm hoping .. that we really never get here and that we're not either (a) depending on this behavior or (b) silently sometimes hitting this code path and losing resources or something | |
| 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 | |