| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2022-08-22 | |||
| 07:51:50 | gibi | fyi folks, we have "Non-client library freeze: August 25th, 2022 (R-6 week)" which is this week. So if you have anything depending on os-traits, os-resource-classes, os-vif, etc then those dependencies needs to land this week | |
| 08:06:14 | opendevreview | Rajesh Tailor proposed openstack/nova master: Fix rescue volume-based instance https://review.opendev.org/c/openstack/nova/+/852737 | |
| 08:08:33 | opendevreview | Jan Hartkopf proposed openstack/nova master: add support for updating server's user_data https://review.opendev.org/c/openstack/nova/+/816157 | |
| 08:52:19 | gibi | sean-k-mooney[m]: this is the code that causes the missing PCI device failure https://github.com/openstack/nova/blob/99dd3f75cd23a4ff419c20826f5abfcfed417889/nova/pci/manager.py#L485-L503 in https://review.opendev.org/c/openstack/nova/+/853835 (I needed fresh brains for it to find) | |
| 09:10:12 | sean-k-mooney | ah right just getting started but ill pull your patch and see if i can reporduce locally and take a look | |
| 09:11:40 | gibi | I need to refactor that piece of code and move it to the Instance ovo | |
| 09:13:21 | sean-k-mooney | i was thinking about this since we last spoke. is there any reason not to have the consturctor generate a uuid automitically when we constuct the pci request objects | |
| 09:13:41 | sean-k-mooney | since we will now be creating these on both the neuton and non nueutron path | |
| 09:14:03 | gibi | sean-k-mooney: yes, that is a good point too | |
| 09:14:07 | gibi | sean-k-mooney: I will do that | |
| 09:14:33 | sean-k-mooney | do you recall what test failed? | |
| 09:14:48 | sean-k-mooney | i guess it will be in the zuul logs | |
| 09:15:17 | sean-k-mooney | test_cold_migrate_server_with_pci | |
| 09:17:38 | gibi | yes that one | |
| 09:18:29 | gibi | and it fails as when the libvirt driver tries to get the PciDevice objects of the instance to generate the xml it gets [] as the above linked piece of code assumes request_id = None means flavor based PCI request | |
| 09:18:33 | gibi | and I break that assumption | |
| 09:18:54 | sean-k-mooney | we have a function to figure that out | |
| 09:19:03 | sean-k-mooney | which should be used instead | |
| 09:19:20 | sean-k-mooney | three is a source atribute | |
| 09:19:39 | sean-k-mooney | https://github.com/openstack/nova/blob/master/nova/objects/instance_pci_requests.py#L48-L54 | |
| 09:20:00 | gibi | exactly | |
| 09:22:52 | Uggla | gibi, sean-k-mooney any objection to rename the options as proposed by stephen here : https://review.opendev.org/c/openstack/python-openstackclient/+/831902/comments/9b4913bd_cee0ed57 | |
| 09:23:33 | sean-k-mooney | i havent looked at it but ill check quickly | |
| 09:23:54 | sean-k-mooney | oh he wants to use the ciso no prefix | |
| 09:23:59 | sean-k-mooney | i personaly hate that | |
| 09:26:44 | gibi | we have examples with --no-* already in the client and the flag's doc clearly states that this means unpin so I'm OK | |
| 09:27:03 | sean-k-mooney | we do but we also have set and unset i belive | |
| 09:27:05 | sean-k-mooney | just checkign that now | |
| 09:27:40 | sean-k-mooney | yes opnestack flavor set and unset | |
| 09:27:59 | gibi | but that is not a flag but a subcommand | |
| 09:29:10 | sean-k-mooney | yes but i think openstack server unshleve --unset-az | |
| 09:29:16 | sean-k-mooney | would make sense | |
| 09:30:15 | gibi | I have nothing against that either | |
| 09:30:27 | gibi | stephenfin: are you around? | |
| 09:40:08 | sean-k-mooney | gibi: so printing the xml there are not hostdev elements which is why its getting None for elem | |
| 09:40:15 | gibi | yes | |
| 09:40:21 | gibi | I figured it out this morning | |
| 09:40:36 | sean-k-mooney | i find that very odd that adding the request id woudl have resulted in that | |
| 09:41:04 | gibi | nova uses PciDevice.request_id == None to signal flavor based PCI devs | |
| 09:41:09 | sean-k-mooney | so yes modifying get_instance_pci_devs | |
| 09:41:19 | sean-k-mooney | is likely the way to go | |
| 09:41:21 | gibi | yes | |
| 09:41:28 | sean-k-mooney | well we do in some placees but not all | |
| 09:41:53 | gibi | yes | |
| 09:47:30 | sean-k-mooney | oh its becasue we have the PciDevice object not the pci request objects here | |
| 09:48:06 | sean-k-mooney | i was going to just add or device.source == objects.InstancePCIRequest.FLAVOR_ALIAS | |
| 09:48:22 | sean-k-mooney | but device is not an InstancePciREquest object | |
| 09:49:04 | sean-k-mooney | we have the pci request too | |
| 09:50:43 | gibi | yes | |
| 09:51:11 | gibi | there is somewhere a generic code that does PciDevice.request_id = InstancePCIRequest.request_id | |
| 09:51:18 | gibi | which is I think correct | |
| 09:52:16 | sean-k-mooney | that basically waht im trying locally | |
| 09:52:50 | sean-k-mooney | im doing a set comprehention to get the flavor request ids and then checking if the current device is in that when request_id is none | |
| 09:54:08 | sean-k-mooney | https://paste.opendev.org/show/bqJUPAfRGibXn8mslgdU/ | |
| 09:54:09 | gibi | I added https://paste.opendev.org/show/b1nvksM7Fw7F4w5vnDKw/ to Instance ovo and replaced the get_instance_pci_devs calls with it and it seems to work | |
| 09:54:12 | sean-k-mooney | that seams to work | |
| 09:55:30 | sean-k-mooney | you could do that but you can do it in the existing fucntion without changing the signiture or moving it | |
| 09:56:05 | gibi | yeah but 1) I have the implicit not-providing-request-id meaning give me the flavor based CPI devs | |
| 09:56:10 | gibi | s/have/hate/ | |
| 09:56:34 | gibi | I want to make the query explicit by the caller | |
| 09:56:49 | sean-k-mooney | yep i get that | |
| 09:56:56 | gibi | 2) also request_id == 'all' is /o\ | |
| 09:57:15 | sean-k-mooney | the duality because of the fact that this code predated neutron sriov i think | |
| 09:57:26 | sean-k-mooney | and source | |
| 09:57:31 | gibi | yes, it is old code, it served wll but I retired it now :D | |
| 09:57:32 | sean-k-mooney | source is relitivly recent | |
| 09:58:00 | sean-k-mooney | yep so just runing the func test loocally my change seams to work | |
| 09:58:09 | sean-k-mooney | i suspect your will also | |
| 09:58:38 | sean-k-mooney | so you have too paths forward you coudl make my small change for not and then have a followup refactor patch to swap to your veriosion | |
| 09:59:01 | sean-k-mooney | or you could do that all in one go along with changing all callers to use the new version on the ovo | |
| 09:59:45 | gibi | it is < 10 callers so I won't split this into two | |
| 10:00:29 | sean-k-mooney | ok this might conflcit with the vdpa seriese although it might not im hopefully goign to adress the last bits in that today and i can stop thinking about that | |
| 10:01:01 | sean-k-mooney | im not driectly modifying that function for the vdpa code it just ends up calling it indreictly | |
| 10:01:04 | gibi | you vdpa series will have priority over my change and I will rebase, no worries | |
| 10:01:50 | sean-k-mooney | im going to grab a drink but it sounds liek you ahve a way forward | |
| 10:02:53 | sean-k-mooney | im going to step away for 5 mins and ill be back. i like the simplicty of your new get_pci_devices function | |
| 10:03:18 | sean-k-mooney | although im not sure if the deepcopy is correct | |
| 10:04:14 | sean-k-mooney | can you add a comment as to why you are doing that. we may want to modify the existing devices and i woudl proably put the deep copy if needed on the caller rather then doing it in this function | |
| 10:04:48 | gibi | hm, you are right I need a shallow copy | |
| 10:05:13 | gibi | hm, not even that | |
| 10:05:51 | gibi | I first tried a startegy that would modify the dev list | |
| 10:05:58 | gibi | but now it creates new lists | |
| 10:06:01 | gibi | so no copy needed | |
| 10:06:03 | gibi | good catch | |
| 10:13:43 | sean-k-mooney | i was just comparing your version to mine and noticed you had an extra copy and was not sure why you were doing it if i was totaly honest | |
| 10:14:21 | sean-k-mooney | i.e. i was not sure if i missed it in mine or if it was extra in yours. | |
| 10:14:31 | sean-k-mooney | so you can just make it devs = self.pci_devices or [] | |
| 10:19:00 | gibi | yes | |
| 10:22:53 | stephenfin | gibi: Sorry, I am around but I wasn't connected to IRC. What's up? | |
| 10:23:12 | gibi | stephenfin: it is about https://review.opendev.org/c/openstack/python-openstackclient/+/831902/comments/9b4913bd_cee0ed57 | |
| 10:23:29 | stephenfin | sean-k-mooney: Replied to https://review.opendev.org/c/openstack/python-openstackclient/+/831902/ I realize it's no ideal, but as I've tried to stress in my response, consistency is key when it comes to OSC | |
| 10:23:44 | stephenfin | We shouldn't do nova-specific things, even if the terminology used by OSC isn't ideal | |
| 10:23:50 | stephenfin | Hopefully what I've said makes sense | |
| 10:24:02 | sean-k-mooney | it does but i dont think its sufficent | |
| 10:24:06 | stephenfin | Oh, one and the same :) | |
| 10:24:18 | sean-k-mooney | i really dont think using the no prfix shoudl be allowed | |
| 10:24:29 | sean-k-mooney | that siad i wont block on it | |
| 10:24:42 | sean-k-mooney | but i dont think we shoudl use it anywere in osc | |
| 10:25:10 | stephenfin | It's everywhere already though. That ship sailed so long ago that's it almost home again :) | |
| 10:25:55 | sean-k-mooney | i tought the gramer alone would make you recoile from its use :) | |