Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2022-08-22
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 stephenfin We've gone off topic now, but how would we do inverse boolean options (flags) without that?
10:27:38 gibi trademark --no-*? lol, I want to be that lawyer :D
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
11:23:09 sean-k-mooney stephenfin: im not -1ing since im more or less delegating the style desisson to you
11:23:42 stephenfin ack, thanks. And I'm just following the OSC guidelines
11:24:30 sean-k-mooney its one of those case of i dont like it but also dont plan to work on it to fix it so lets be consitent even if it think its not the best way to do it
11:40:28 sean-k-mooney stephenfin: gibi question regarding scope of nova status. can we add checks to that that are not for upgrades or would nova manage be a better place
11:40:49 sean-k-mooney i was just thinking about the downstream escalation
11:41:16 sean-k-mooney would a nova status command to find all instnace that have incorrect pci device adress in there neutron ports be in scope
11:41:54 sean-k-mooney i.e. nova-status validate instance-pci-slots
11:42:22 sean-k-mooney and have that print a list of instnace uuids
11:42:40 sean-k-mooney as an admin you would either need to manually fix them or just cold migrate them and let nova do it
11:44:05 sean-k-mooney we have the binding profile in the network_info_cache so we can actully do the check form the nova db if we do it in python to parse the json blob
11:44:23 sean-k-mooney this would be entirely unrelated to upgrades however so not sure if nova-status is the right place
11:44:52 sean-k-mooney but it would tell you if there is "db currption" where the neutron db is out of sync with nova's pci allcoations
11:45:33 sean-k-mooney there are some other one off validations like this that might be worth adding in the future just wondiging if addign a validate sub command for these types of checks makes sesne
11:47:58 sean-k-mooney im not sure if ye have seen https://review.opendev.org/c/openstack/nova-specs/+/853837 that dansmith started for hardening our hostname requirements
11:48:38 sean-k-mooney when im thinking about the fallout of hostname changes i think some of the sideeffect are things we coudl detect and codify in nova-status checks
11:49:19 sean-k-mooney for example checking that a vm with pci allcoations has them against the compute node its currently on
11:50:01 sean-k-mooney so if non upgrade checks are in scope of nova status we could add a set of validations to it for similar things going forward
11:50:37 sean-k-mooney if we wanted to have automated fixes i would sugggest using nova-manage
11:50:55 sean-k-mooney but detecting the issues i think would fall into nova-status
11:51:19 sean-k-mooney or perhaps the health check api if the detachtions are cheap and can be part of a periodic
11:52:10 sean-k-mooney for example asseting that the pci adresses in the network info cache are a subset of the pci claims and cyborg devices(if there are any)
11:54:14 sean-k-mooney stephenfin: by the way im starting to use the versionadded directive should i drop the histroy table at the end with the planned/inprogress line items
11:54:39 sean-k-mooney its proably reduntant now

Earlier   Later