| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2021-03-26 | |||
| 15:53:54 | sean-k-mooney | it hink it woudl be ok | |
| 15:53:55 | artom | stephenfin, that infamous comment of your for the cold migration functional test mocking is getting around | |
| 15:54:08 | artom | 11 | |
| 15:54:08 | artom | [artom@zoe nova]$ ag dumb nova/tests/functional/libvirt/ | wc -l | |
| 15:54:21 | stephenfin | the dog | |
| 15:54:22 | sean-k-mooney | artom: which one? | |
| 15:54:46 | sean-k-mooney | im surprised we have less use fo dumb in our func test :) | |
| 15:55:08 | sean-k-mooney | there are many silly things | |
| 15:55:08 | artom | # TODO(stephenfin): The mock of 'migrate_disk_and_power_off' should probably be less...dumb | |
| 15:55:24 | sean-k-mooney | hehe | |
| 15:55:33 | stephenfin | I did try to fix that one time | |
| 15:55:36 | stephenfin | and quickly gave up | |
| 15:55:37 | sean-k-mooney | i dont think i have actully seen that beofre or at least read it | |
| 15:55:43 | sean-k-mooney | but im sure it could be | |
| 15:55:43 | stephenfin | too much involved | |
| 16:03:17 | openstackgerrit | Merged openstack/os-vif master: Add Python3 xena unit tests https://review.opendev.org/c/openstack/os-vif/+/781891 | |
| 16:20:51 | openstackgerrit | Artom Lifshitz proposed openstack/nova master: WIP: Test SRIOV port move operations with PCI conflicts https://review.opendev.org/c/openstack/nova/+/783084 | |
| 16:45:35 | openstackgerrit | Artom Lifshitz proposed openstack/nova master: WIP: Neutron fixture: don't clobber profile and vif_details if empty https://review.opendev.org/c/openstack/nova/+/783385 | |
| 16:45:35 | openstackgerrit | Artom Lifshitz proposed openstack/nova master: WIP: Test SRIOV port move operations with PCI conflicts https://review.opendev.org/c/openstack/nova/+/783084 | |
| 16:45:52 | artom | sean-k-mooney, ^^ there :) Good time to pause for lunch | |
| 17:25:01 | sean-k-mooney | artom: yep that is a good approch | |
| 17:25:46 | sean-k-mooney | the inner move_operation fucntion true me for a sec but it makes sense | |
| 17:26:04 | artom | sean-k-mooney, yeah, it's not great, but I wanted to test cold migration too | |
| 17:26:08 | sean-k-mooney | this is where i woudl like to use named lambda but those are not allowed | |
| 17:26:09 | artom | That one works, thankfully | |
| 17:26:54 | sean-k-mooney | as far as i am aware this cannot happen for live mifgation since we dont use the claims in the same way | |
| 17:27:09 | sean-k-mooney | so ya | |
| 17:27:14 | sean-k-mooney | just cold migratoien and shelve | |
| 17:27:22 | sean-k-mooney | although evacualte might also be affected | |
| 17:27:58 | artom | In fact for unshelve we don't have any claims | |
| 17:28:08 | artom | I guess when it was written it was supposed to never change hosts? | |
| 17:28:12 | sean-k-mooney | well we do | |
| 17:28:23 | sean-k-mooney | we have pci claims in the db | |
| 17:28:27 | artom | I dunno, but the way I read it, we just plop the instance on the compute, no questions asked | |
| 17:28:30 | sean-k-mooney | in the pci tracker | |
| 17:28:47 | sean-k-mooney | we do not use move claims | |
| 17:28:57 | sean-k-mooney | because its not actully a move operations | |
| 17:29:13 | sean-k-mooney | unshelve basically uses spawn | |
| 17:29:48 | sean-k-mooney | the vm never really exist in two compute hosts at the same time liek with migrrate and live migrate or evacuate | |
| 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 | |