Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2023-04-12
16:03:44 dansmith which means, AFAICT, we would have created the migration object on the target machine, claimed no resources, and agreed to accept the new incoming instance
16:03:55 dansmith even though it's for a node that we don't have or know anything about?
16:04:28 bauzas I'm just trying to understand the intent
16:04:37 dansmith also note this from mr. pipes: https://github.com/openstack/nova/blob/master/nova/compute/resource_tracker.py#L154
16:04:51 dansmith in the instance claim, which we'll also do weird stuff for new instances with no such node
16:05:08 dansmith the earlier part of the comment says it shouldn't happen
16:05:24 dansmith so, like, I'm confused and feel like this is probably some very old cruft
16:05:43 dansmith like, surely we can't have these things happen in a placement world and not get everything messed up, right?
16:06:12 bauzas yeah, found the commit https://github.com/openstack/nova/commit/1c967593fbb0ab8b9dc8b0b509e388591d32f537
16:07:10 bauzas ok, so now I think I remember where it comes
16:07:25 bauzas the virt drivers are responsible for creating the RTs
16:08:06 bauzas at compute startup, we were originally setting one RT per compute node resources dict that was returned by the virt driver
16:08:21 bauzas and then mr jay factorized that
16:08:27 bauzas by having one single RT instance
16:08:30 dansmith ...right
16:09:09 bauzas so the disabled property was originally coming from the fact there could be a race where the RT was instanciated but the virt resources not fully set up
16:10:41 dansmith that may be the case for startup, but we should never accept an instance boot or migration if we can't account for the resources right?
16:10:54 bauzas https://github.com/openstack/nova/commit/1c967593fbb0ab8b9dc8b0b509e388591d32f537#diff-ed9525d7ae319fd575249ed72daf634d27182e08fea7a4f740cb3164233612b7L435
16:12:00 bauzas https://github.com/openstack/nova/commit/1c967593fbb0ab8b9dc8b0b509e388591d32f537#diff-ed9525d7ae319fd575249ed72daf634d27182e08fea7a4f740cb3164233612b7L412
16:12:00 bauzas well, that whole thing predates placement
16:12:07 dansmith right
16:12:14 dansmith (those links do not link to a hunk for me, btw)
16:12:21 bauzas ah shit
16:12:28 bauzas so, the fact is, before placement,
16:12:38 bauzas we were relying on the ComputeNode records for scheduling
16:13:09 bauzas if there were no ComputeNode records, then the HostStates from the scheduler weren't generated
16:13:23 bauzas and no instances were able to schedule into them
16:13:47 dansmith your point being that we could (should) never get into that case where we do the NopClaims because of that?
16:14:05 dansmith I think that's probably also not true because of forced migrations
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 dansmith crazy right?
16:19:31 bauzas technically this isn't a noop
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

Earlier   Later