| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2021-03-26 | |||
| 17:30:21 | sean-k-mooney | artom: we use the normal instance claim wich is created on the compute node | |
| 17:31:06 | openstackgerrit | Stephen Finucane proposed openstack/nova master: conf: Deprecate AggregateImagePropertiesIsolation opts https://review.opendev.org/c/openstack/nova/+/783396 | |
| 17:31:09 | stephenfin | sean-k-mooney: ^ | |
| 17:31:39 | sean-k-mooney | cool too bad that didnt happen 2 weeks ago | |
| 17:31:50 | sean-k-mooney | ill take a look shortly | |
| 17:32:58 | sean-k-mooney | artom: i dont know if you have looked at unshelve but we go to task state spawning https://github.com/openstack/nova/blob/master/nova/compute/manager.py#L6525 | |
| 17:33:07 | sean-k-mooney | then we do a normal instance claim | |
| 17:33:15 | sean-k-mooney | and call driver.spwan | |
| 17:33:17 | sean-k-mooney | https://github.com/openstack/nova/blob/master/nova/compute/manager.py#L6577-L6585 | |
| 17:33:44 | sean-k-mooney | artom: so we use https://github.com/openstack/nova/blob/68ad2c67d891935c66e8c0e5a368695c202a9f49/nova/compute/claims.py#L49 | |
| 17:33:54 | sean-k-mooney | for unshelve just like we do for a new boot | |
| 17:36:21 | sean-k-mooney | artom: by the way im not sure why you need https://review.opendev.org/c/openstack/nova/+/783385/1/nova/tests/fixtures.py | |
| 17:36:56 | sean-k-mooney | you are removing the copies that we make which im not sure is correct | |
| 17:37:18 | sean-k-mooney | other wise what you wote is logically the same | |
| 17:38:03 | artom | sean-k-mooney, that fixes the KeyError | |
| 17:38:06 | sean-k-mooney | i mean if it works cool i just dont know why it does | |
| 17:38:24 | sean-k-mooney | right but body['port'].get('binding:profile') or {} | |
| 17:38:27 | sean-k-mooney | shoudl do the same thing | |
| 17:38:48 | sean-k-mooney | as | |
| 17:38:50 | sean-k-mooney | if body['port'].get('binding:profile'): | |
| 17:38:52 | sean-k-mooney | update['profile'] = body['port']['binding:profile'] | |
| 17:39:19 | sean-k-mooney | well not quite it wont set {} | |
| 17:39:27 | sean-k-mooney | if there is no profile | |
| 17:39:34 | sean-k-mooney | but that hsoudl not clober things | |
| 17:39:59 | artom | sean-k-mooney, right, but it means that if we update with no binding:profile, we'll hit the 'or {}', and assign that | |
| 17:40:00 | sean-k-mooney | oh i see whats hapening | |
| 17:40:05 | artom | Which effectively clobbers | |
| 17:40:14 | sean-k-mooney | ya | |
| 17:40:21 | sean-k-mooney | that is not how the real api work | |
| 17:40:51 | sean-k-mooney | what the real api does is keep the value of the fiedl you dont touch | |
| 17:40:57 | sean-k-mooney | whcih your change will do | |
| 17:41:10 | sean-k-mooney | the or {} shoudl have been more like | |
| 17:41:27 | sean-k-mooney | or self._port_bindings[port_id][host]['profile] | |
| 17:41:42 | sean-k-mooney | ya ok this makes sense to me | |
| 17:46:41 | sean-k-mooney | artom: looks like nova.tests.functional.test_nova_manage.TestNovaManagePlacementHealPortAllocations.test_heal_multiple_port_allocations_rollback_fails | |
| 17:46:49 | sean-k-mooney | broke but oterwise this looks good | |
| 17:47:23 | sean-k-mooney | stephenfin: nic commit message :) | |
| 17:48:41 | sean-k-mooney | good point on the hw_ possiblity but ya they will still work | |
| 18:18:46 | stephenfin | sean-k-mooney: That's a good point RE: using the isolated aggregates filtering feature via placement instead | |
| 18:19:11 | stephenfin | I wonder if we should just just deprecate both of these filters instead? | |
| 18:23:25 | sean-k-mooney | i would be ok with that | |
| 18:23:29 | sean-k-mooney | ptg topic maybe | |
| 18:23:48 | stephenfin | good idea | |
| 18:24:22 | openstackgerrit | Stephen Finucane proposed openstack/nova master: conf: Deprecate AggregateImagePropertiesIsolation opts https://review.opendev.org/c/openstack/nova/+/783396 | |
| 18:24:35 | sean-k-mooney | the draw back is you might need to update your flavors/image with teh traits | |
| 18:24:53 | sean-k-mooney | but at least of new deployment i would not recommend turnning them on any more na just use that | |
| 18:24:53 | stephenfin | Yes, I'm not sure we could ever remove it | |
| 18:25:00 | artom | Remote school is... interesting. She's already finished everything for the week with mom | |
| 18:25:13 | sean-k-mooney | well we could but it would need a few cycles | |
| 18:25:21 | stephenfin | at least not without offering a mechanism to update stored image/flavor metadata for an existing instance | |
| 18:25:32 | stephenfin | but the code costs very little to maintain and could happily be ignored | |
| 18:25:36 | sean-k-mooney | artom: enjoy | |
| 18:25:39 | stephenfin | anyway, PTG topic | |
| 18:26:05 | sean-k-mooney | artom: its the same type of claime we do for normal spawn | |
| 18:26:11 | artom | So yeah, we do a claim, but _update_port_binding_for_instance() assumed that pci_mapping can only change if there's a migration | |
| 18:26:15 | sean-k-mooney | which include claiming the pci devices | |
| 18:26:30 | artom | pci_mapping = self._get_pci_mapping_for_migration(instance, migration) | |
| 18:26:33 | sean-k-mooney | artom: ya that should like what might be happening | |
| 18:26:34 | artom | Yeah, we claim them | |
| 18:26:44 | sean-k-mooney | ah cool you found the issue then | |
| 18:26:46 | artom | But way before that, we've already updated the port bindings | |
| 18:27:01 | artom | With the old pci_slot, since the instance isn't actually migrating | |
| 18:27:25 | artom | Or rather, we haven't actually done the update, since pci_slot doens't change | |
| 18:28:31 | sean-k-mooney | well we just need to update the port again | |
| 18:28:43 | sean-k-mooney | it would be nice not to do it twice | |
| 18:28:52 | sean-k-mooney | but that is the simiple backportable fix | |
| 18:29:07 | sean-k-mooney | as long as we update it before we generate the xml its fine | |
| 18:29:41 | sean-k-mooney | then eventually i would like to use the adress form the claim/db instead but that harder | |
| 18:31:07 | artom | IOW, how do we *set* the pci_slot in the first place | |
| 18:31:20 | artom | _update_port_binding_for_instance() will only update it from a migration | |
| 18:38:02 | artom | Aha _populate_neutron_binding_profile() | |
| 18:38:14 | sean-k-mooney | ya it happens in the port bidning right | |
| 18:38:58 | sean-k-mooney | artom: i think its this https://github.com/openstack/nova/blob/68ad2c67d891935c66e8c0e5a368695c202a9f49/nova/network/neutron.py#L1332-L1333 | |
| 18:40:29 | sean-k-mooney | we call that during live migration fore example https://github.com/openstack/nova/blob/68ad2c67d891935c66e8c0e5a368695c202a9f49/nova/conductor/tasks/live_migrate.py#L414 | |
| 18:43:36 | sean-k-mooney | artom: if you called self.network_api.bind_ports_to_host here https://github.com/openstack/nova/blob/master/nova/compute/manager.py#L6579 | |
| 18:43:50 | sean-k-mooney | right before spawn i think that woudl fix it | |
| 18:44:41 | sean-k-mooney | you would need to change | |
| 18:44:42 | sean-k-mooney | with self.rt.instance_claim(context, instance, node, allocations, | |
| 18:44:44 | sean-k-mooney | limits): | |
| 18:44:53 | sean-k-mooney | to with self.rt.instance_claim(context, instance, node, allocations, | |
| 18:44:55 | sean-k-mooney | limits) | |
| 18:44:57 | sean-k-mooney | as claim | |
| 18:45:16 | sean-k-mooney | and then you could use the claim to get the pci diceis i think | |
| 18:49:28 | sean-k-mooney | artom: the update would look like this https://opendev.org/openstack/nova/src/branch/master/nova/compute/manager.py#L10634-L10657 | |
| 18:49:57 | sean-k-mooney | hum actullyno | |
| 18:51:10 | sean-k-mooney | well no this is how we actully do it for live miggration https://opendev.org/openstack/nova/src/branch/master/nova/compute/manager.py#L10597-L10632 | |
| 19:01:33 | artom | I guess we'd have to distinguish between unshelve from just SHELVED and also SHELVED_OFFLOADED | |
| 19:01:38 | sean-k-mooney | artom: in the normal spawn workflow after we take the instance calim we call _build_resources | |
| 19:01:54 | sean-k-mooney | which does call that yes | |
| 19:02:10 | sean-k-mooney | well it all alocate_netork | |
| 19:02:47 | sean-k-mooney | artom: https://github.com/openstack/nova/blob/68ad2c67d891935c66e8c0e5a368695c202a9f49/nova/compute/manager.py#L2349 | |
| 19:02:58 | sean-k-mooney | that is what allocate the networks on spawn normally | |
| 19:03:07 | artom | Yeah, I've been looking at that... | |
| 19:03:08 | sean-k-mooney | but we dont do that on unshelve | |
| 19:03:23 | artom | Which makes sense... but only if the instance was only shelved | |
| 19:03:40 | sean-k-mooney | well we woudl be calling it form unshelve | |
| 19:03:54 | sean-k-mooney | here https://github.com/openstack/nova/blob/master/nova/compute/manager.py#L6579 | |
| 19:04:13 | sean-k-mooney | litrally after the instance_claim but just before spawn | |
| 19:06:49 | sean-k-mooney | this is where its update for other move ops right https://github.com/openstack/nova/blob/68ad2c67d891935c66e8c0e5a368695c202a9f49/nova/network/neutron.py#L3394-L3419 | |
| 19:07:04 | sean-k-mooney | but since thre is no migration that oviously does not work | |