| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2024-02-19 | |||
| 13:23:27 | opendevreview | Takashi Kajinami proposed openstack/nova-specs master: libvirt: Stateless firmware support https://review.opendev.org/c/openstack/nova-specs/+/908297 | |
| 13:24:08 | opendevreview | Takashi Kajinami proposed openstack/nova-specs master: libvirt: AMD SEV-ES support https://review.opendev.org/c/openstack/nova-specs/+/907702 | |
| 14:37:50 | bauzas | dansmith: sean-k-mooney: when you have a minute, I'd like to discuss about the mdev live-mig series https://review.opendev.org/c/openstack/nova/+/904209/16 | |
| 14:38:18 | bauzas | (about the periodic call) | |
| 14:41:04 | sean-k-mooney | sure | |
| 14:41:32 | sean-k-mooney | we will be hitting meetings shortly but im free for a while | |
| 14:42:11 | bauzas | awaiting dan when he can | |
| 14:42:29 | bauzas | my concern is about the API we could use for calling the drivcer | |
| 14:43:07 | dansmith | bauzas: I'm just getting started in my morning so I'm doing lots of stuff, sorry | |
| 14:43:16 | bauzas | no worries then | |
| 14:43:36 | bauzas | I'll try to provide a WIP then | |
| 14:44:33 | sean-k-mooney | bauzas: just do this in update_aviable_resouces | |
| 14:45:02 | sean-k-mooney | thats already being called and its very closely related | |
| 14:45:14 | bauzas | sean-k-mooney: sure, but given we don't call the driver by it, I need to create a new driver method :( | |
| 14:45:21 | sean-k-mooney | we do | |
| 14:45:30 | sean-k-mooney | update_avlaiable_resouces calls the driver | |
| 14:45:41 | sean-k-mooney | thats why im suggesting it | |
| 14:46:08 | bauzas | https://github.com/openstack/nova/blob/master/nova/virt/libvirt/driver.py#L9582 | |
| 14:47:24 | sean-k-mooney | i belive that is called form https://github.com/openstack/nova/blob/master/nova/compute/manager.py#L10587 | |
| 14:48:23 | sean-k-mooney | yep here https://github.com/openstack/nova/blob/master/nova/compute/resource_tracker.py#L911 | |
| 14:48:33 | sean-k-mooney | the resouce tracker calls the driver | |
| 14:48:47 | sean-k-mooney | so you can just add it to get_available_resource | |
| 14:48:50 | sean-k-mooney | in the driver | |
| 14:49:07 | sean-k-mooney | which is what you linked too | |
| 14:50:12 | sean-k-mooney | the alternitive which i assume you wanted to dicuss | |
| 14:50:24 | sean-k-mooney | was adding a new virt driver api function | |
| 14:50:33 | sean-k-mooney | and then calling that directly | |
| 14:50:47 | sean-k-mooney | alternitivly you could decorate a method in the libvirt driver as a perodic | |
| 14:51:03 | sean-k-mooney | and have it run seperatly form those run by the compute manager | |
| 14:52:08 | sean-k-mooney | both are workable but the minimal change is just make get_avialable_resoucces call a new function to reconsile the dict. | |
| 14:53:25 | sean-k-mooney | bauzas: looking at the code i wonder if there is a better approch | |
| 14:53:52 | sean-k-mooney | bauzas: have you coniserd doing it in update_provider_tree | |
| 14:54:05 | sean-k-mooney | https://github.com/openstack/nova/blob/master/nova/virt/libvirt/driver.py#L8869C9-L8869C29 | |
| 14:54:25 | sean-k-mooney | that has the advantage of being passed in the allocations | |
| 14:54:50 | sean-k-mooney | so if you see there is not allocation against the provider but there is a reservation in the dict for an instance | |
| 14:55:03 | sean-k-mooney | that would be a stale reservation correct? | |
| 14:55:55 | sean-k-mooney | hum perhaps not | |
| 14:56:09 | sean-k-mooney | the allocation may be owned by the migration uuid | |
| 14:56:47 | sean-k-mooney | if you were to do it in _update_provider_tree i would do it in the existing _update_provider_tree_for_vgpu fucntion where you do reshapes | |
| 14:56:52 | bauzas | sean-k-mooney: sorry got a phonecall meanwhile | |
| 14:57:00 | sean-k-mooney | no worries | |
| 14:57:08 | sean-k-mooney | readback and we can chat later | |
| 14:58:09 | bauzas | sean-k-mooney: okay I see your point | |
| 14:58:49 | bauzas | so, we would need to inject the db to get the migrations in _update_provider_tree_for_vgpu() | |
| 14:59:04 | bauzas | sean-k-mooney: would you be okay ? | |
| 15:00:04 | bauzas | I mean calling objects.MigrationList.get_in_progress_and_error() in _update_provider_tree_for_vgpu() | |
| 15:01:49 | dansmith | I've lost track of what we're trying to solve here | |
| 15:02:14 | dansmith | I thought the periodic was to clean up reservations that are now stale for migrations that never happened? | |
| 15:02:40 | dansmith | in the driver memory-resiident dict | |
| 15:03:22 | sean-k-mooney | yes | |
| 15:03:32 | sean-k-mooney | so i think we can do that in _update_provider_tree_for_vgpu | |
| 15:03:34 | dansmith | so what injecting into the DB needs to happen? | |
| 15:03:50 | sean-k-mooney | nothing has to happen in the db | |
| 15:04:05 | dansmith | ack, okay, I'm confused by bauzas' statement then | |
| 15:04:08 | sean-k-mooney | dansmith: is the destionation allcotion held by the instance uuid or the migration uuid | |
| 15:04:18 | sean-k-mooney | i think the instance_uuid? | |
| 15:04:27 | dansmith | instance IIRC yeah | |
| 15:04:43 | sean-k-mooney | ack, assuming that is correct all _update_provider_tree_for_vgpu need to do | |
| 15:05:05 | sean-k-mooney | is loop over the in memory dict | |
| 15:05:12 | dansmith | maybe bauzas is talking about _querying_ for canceled migration objects to know what to clean from the dict? | |
| 15:05:15 | sean-k-mooney | and remove any key that does not have an allocation | |
| 15:05:17 | bauzas | dansmith: btw. I added a new check by https://review.opendev.org/c/openstack/nova/+/904209/16/nova/virt/libvirt/driver.py#1740 | |
| 15:05:41 | dansmith | sean-k-mooney: you can't do that I think because you might be racing with the start of the migration? | |
| 15:05:55 | bauzas | dansmith: my question was about where we could verify the dict | |
| 15:06:02 | dansmith | sean-k-mooney: or are you thinking the instance already has an allocation for that host, until it's canceled and we can go on the presence of that allocation? | |
| 15:06:08 | sean-k-mooney | we should have claimed the placment allcotion before calling migrate correct | |
| 15:06:19 | bauzas | and I just don't want to create a new virt driver API method just for this | |
| 15:06:21 | dansmith | right, okay, yeah that makes sense | |
| 15:06:45 | dansmith | I'm just wondering what bauzas meant by "so, we would need to inject the db to get the migrations in _update_provider_tree_for_vgpu() " | |
| 15:07:13 | bauzas | sean-k-mooney: I wouldn't check the allocations, I would just verify that all the dict keys have a current migration | |
| 15:07:28 | sean-k-mooney | well that required addtional db lookups | |
| 15:07:36 | sean-k-mooney | which can only be done in the compute manager or resouce tracker | |
| 15:07:38 | sean-k-mooney | not the driver | |
| 15:07:44 | dansmith | bauzas: but "has an allocation for gpu on this host" is good enough right? | |
| 15:07:44 | bauzas | dansmith: I'm saying that I want to get the current migrations | |
| 15:07:51 | sean-k-mooney | so that why im trying to use the allcotions instead | |
| 15:08:15 | dansmith | it also means the reservation mirrors the allocation in placement, which seems right | |
| 15:08:16 | bauzas | dansmith: no, because you could have an allocation for an instance while it's not because of a migration | |
| 15:08:36 | bauzas | if we want to check the dict, we need to look at the *migrations* | |
| 15:08:41 | dansmith | bauzas: okay, but that would be for an instance that migrated in successfully in which case it doesn't hurt right? | |
| 15:08:45 | bauzas | dansmith: that's what you asked me | |
| 15:09:00 | bauzas | dansmith: no, it would only be for current migrations | |
| 15:09:10 | bauzas | once the migration is done, then the dict key is removed | |
| 15:09:19 | bauzas | (or aborted) | |
| 15:09:24 | dansmith | bauzas: actually, what I asked is for it to not to get out of sync silently so that things stop working and a restart is the only solution :) | |
| 15:09:25 | bauzas | so we need to compare | |
| 15:09:43 | sean-k-mooney | bauzas: not what dansmith is saying is if we have an entry in the dict and the instance ahs an allcoation agaisnt the host and there is no migration | |
| 15:09:50 | dansmith | bauzas: if that happens, then we're fine, this is just for cleanup if the migration does *not* get rolled back successfully right? | |
| 15:09:59 | sean-k-mooney | then its can only happy because the migration succeeded | |
| 15:10:07 | dansmith | sean-k-mooney: well, not exactly | |
| 15:10:10 | sean-k-mooney | and we failed to remove it form the dict for some bug | |
| 15:10:23 | bauzas | dansmith: that's what I'm saying, we need to compare migrations with the dict | |
| 15:10:38 | dansmith | sean-k-mooney: what I'm saying is that if there's a reservation in the dict and there's an allocation, then it's either still in progress, has yet to be rolled back, or it succeeded in which case keeping the reservation in the dict doesn't prevent other stuff from working | |
| 15:10:38 | bauzas | if a dict key is not related to a current migration, then it's stale | |
| 15:11:01 | dansmith | once the instance is deleted or migrated away, healing based on the allocation would work itself out too | |
| 15:11:02 | sean-k-mooney | dansmith: ya i agree with that | |
| 15:11:20 | sean-k-mooney | and yes to the healing too | |
| 15:11:28 | sean-k-mooney | also bauzas if we ever use consumer_types | |
| 15:11:39 | dansmith | bauzas: a dict key for a completed migration where the instance is on our host is not incorrect, even if it is stale, and won't harm anything yeah? | |
| 15:11:40 | sean-k-mooney | you can actully tell if the allocation is a migration if you really want too | |