Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2022-08-22
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
11:57:17 gibi sean-k-mooney: I would add that to nova-manage as it more simila to the audit command there
11:58:02 sean-k-mooney ok so nova-manage validate <whatever> or nova-manage check <thing>
11:58:30 gibi yes. I would keep the upgrade checks separate as they have a well defined scope
11:58:42 sean-k-mooney ack
11:59:35 sean-k-mooney im not nessisarly plannign on adding them myslef in the near term. but long term i think we want to codify some of those check rahter then providing a set of sql quriese to custoemr to run
11:59:57 gibi I agree
12:00:12 gibi my recent leaked migration allocation case could be added there too
12:01:03 sean-k-mooney yep and depending on how we write them perhaps they could be reused for healtch checks later
12:01:26 sean-k-mooney i feel like many of these checks are two heavy weight for that
12:01:56 gibi we need to measure the load they create
12:02:04 sean-k-mooney but there are some simple cases where we currently raise excptiont that could set a booleing flag
12:02:06 gibi I can imageine that some of them are too heavy
12:02:48 sean-k-mooney yep for the reousce tracker brakages we already have a pattern in the code i created for catching expctins and using those to set flags
12:03:16 sean-k-mooney which will be the simple way to detect two vms using the same cpus for examle
12:03:34 sean-k-mooney we can catch the qemu issue when two vms try to use the same pci device the same way
12:03:39 sean-k-mooney without computeing it
12:04:14 sean-k-mooney having the ablity to ask nova for the conflciitng vms seperatly however will help operators resolve that
12:04:36 sean-k-mooney and thats kind of where i was going with the new commands
12:04:53 gibi make sense
12:05:22 sean-k-mooney so healtch notices we failed to boot because fo conflciting pci devices. then operartor runs the command to deterim what vms are using the wrong devices
12:05:42 sean-k-mooney then they cold migrate them to fix it or manually fix the port in neutron
12:47:55 opendevreview Rajesh Tailor proposed openstack/nova master: Fix rescue volume-based instance https://review.opendev.org/c/openstack/nova/+/852737
13:41:59 gibi sean-k-mooney: btw, I cannot set request_id during __init__ of InstancePCIRequest as that is forbiden for ovos :/ https://github.com/openstack/nova/blob/3af84811c8b181a49195f640c9c971d16d6d3477/nova/tests/unit/objects/test_objects.py#L1327
13:43:22 opendevreview ribaudr proposed openstack/nova master: Alphabetizes objects https://review.opendev.org/c/openstack/nova/+/853986
13:43:27 sean-k-mooney oh ok i guess that explains why we do it seperatly
13:52:25 opendevreview Balazs Gibizer proposed openstack/nova master: Generate request_id for Flavor based InstancePCIRequest https://review.opendev.org/c/openstack/nova/+/853835
13:52:53 gibi sean-k-mooney: ^^ I will move this into the PCI in placement series so we don't need to merge it before we really need the request_id
13:59:08 opendevreview sean mooney proposed openstack/nova master: Fix suspend for non hostdev sriov ports https://review.opendev.org/c/openstack/nova/+/841017
13:59:08 opendevreview sean mooney proposed openstack/nova master: Add source dev parsing for vdpa interfaces https://review.opendev.org/c/openstack/nova/+/841016
13:59:09 opendevreview sean mooney proposed openstack/nova master: Add VDPA support for suspend and livemigrate https://review.opendev.org/c/openstack/nova/+/853704
14:00:28 sean-k-mooney gibi: ack, will this need an offline data migration to populate that on old records or are you jsut going to set it on load from the db when not set and heal it over time
14:01:54 sean-k-mooney by the way i dont know if you want to do this or not but you could set the requester_id=flavor.uuid in the flavor case if you wanted too
14:02:01 sean-k-mooney we dont need that but it might be nice
14:02:03 gibi sean-k-mooney: I think we don't need a data migration. The request_id == None asumption is now removed but those old request and old pci devices that exists already still work as InstancePCIRequest.source works via alias_name
14:02:33 gibi I could set requester_id, but I don't use it
14:02:54 sean-k-mooney its just a nice to have

Earlier   Later