| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2022-08-22 | |||
| 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 :) | |
| 10:26:20 | stephenfin | Heh, true. It's a very common pattern though | |
| 10:26:26 | stephenfin | Even argparse supports it now | |
| 10:26:33 | stephenfin | Look for BooleanOptionalAction on https://docs.python.org/3/library/argparse.html | |
| 10:26:41 | sean-k-mooney | its a cisco patteren that they took othres to court over in the past | |
| 10:26:58 | sean-k-mooney | they tried to trademark it i belive | |
| 10:27:38 | gibi | trademark --no-*? lol, I want to be that lawyer :D | |
| 10:27:38 | stephenfin | We've gone off topic now, but how would we do inverse boolean options (flags) without that? | |
| 10:28:52 | sean-k-mooney | gibi: they sued as a breach of there copyright when arista tried to use it https://www.networkcomputing.com/networking/cisco-arista-battle-over-cli | |
| 10:29:46 | sean-k-mooney | stephenfin: but your githt it is off topic | |
| 10:30:29 | sean-k-mooney | i just dislike mimicic that style because of there previous actions and also i find it less clear | |
| 10:32:59 | sean-k-mooney | stephenfin: i would not have it as a flag i woudl have =True|False personally | |
| 10:33:33 | sean-k-mooney | or use --revert --confrim e.g. two related terms | |
| 10:34:26 | sean-k-mooney | i generally perfer --*=True|false type CLIs | |
| 10:34:46 | sean-k-mooney | too --* --no-* CLIs | |
| 10:41:38 | opendevreview | Jan Hartkopf proposed openstack/python-novaclient master: add support for microversion 2.93 https://review.opendev.org/c/openstack/python-novaclient/+/816158 | |
| 11:18:23 | opendevreview | Rajesh Tailor proposed openstack/nova master: Fix rescue volume-based instance https://review.opendev.org/c/openstack/nova/+/852737 | |