Monday, 2022-08-22

opendevreviewzhangqing proposed openstack/nova stable/train: remove redundant statement in SecurityGroupDefaultRulesController  https://review.opendev.org/c/openstack/nova/+/85390103:10
opendevreviewzhangqing proposed openstack/nova stable/train: remove redundant statement  https://review.opendev.org/c/openstack/nova/+/85390103:11
UgglaHello o/07:29
gibiUggla: welcome back07:37
UgglaHi gibi !07:38
gibifyi 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 week07:51
opendevreviewRajesh Tailor proposed openstack/nova master: Fix rescue volume-based instance  https://review.opendev.org/c/openstack/nova/+/85273708:06
opendevreviewJan Hartkopf proposed openstack/nova master: add support for updating server's user_data  https://review.opendev.org/c/openstack/nova/+/81615708:08
gibisean-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)08:52
sean-k-mooney ah right just getting started but ill pull your patch and see if i can reporduce locally and take a look09:10
gibiI need to refactor that piece of code and move it to the Instance ovo09:11
sean-k-mooneyi 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 objects09:13
sean-k-mooneysince we will now be creating these on both the neuton and non nueutron path09:13
gibisean-k-mooney: yes, that is a good point too09:14
gibisean-k-mooney: I will do that09:14
sean-k-mooneydo you recall what test failed?09:14
sean-k-mooneyi guess it will be in the zuul logs09:14
sean-k-mooneytest_cold_migrate_server_with_pci09:15
gibiyes that one09:17
gibiand 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 request09:18
gibiand I break that assumption09:18
sean-k-mooneywe have a function to figure that out09:18
sean-k-mooneywhich should be used instead09:19
sean-k-mooneythree is a source atribute09:19
sean-k-mooneyhttps://github.com/openstack/nova/blob/master/nova/objects/instance_pci_requests.py#L48-L5409:19
gibiexactly09:20
Ugglagibi, 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_cee0ed5709:22
sean-k-mooney i havent looked at it but ill check quickly09:23
* gibi clicks too09:23
sean-k-mooneyoh he wants to use the ciso no prefix09:23
sean-k-mooneyi personaly hate that09:23
gibiwe have examples with --no-* already in the client and the flag's doc clearly states that this means unpin so I'm OK09:26
sean-k-mooneywe do but we also have set and unset i belive09:27
sean-k-mooneyjust checkign that now09:27
sean-k-mooneyyes opnestack flavor set and unset09:27
gibibut that is not a flag but a subcommand09:27
sean-k-mooneyyes but i think openstack server unshleve --unset-az09:29
sean-k-mooneywould make sense09:29
gibiI have nothing against that either09:30
gibistephenfin: are you around?09:30
sean-k-mooneygibi: so printing the xml there are not hostdev elements which is why its getting None for elem09:40
gibiyes09:40
gibiI figured it out this morning09:40
sean-k-mooneyi find that very odd that adding the request id woudl have resulted in that09:40
gibinova uses PciDevice.request_id ==  None to signal flavor based PCI devs09:41
sean-k-mooneyso yes modifying get_instance_pci_devs09:41
sean-k-mooneyis likely the way to go09:41
gibiyes09:41
sean-k-mooneywell we do in some placees but not all09:41
gibiyes09:41
sean-k-mooneyoh its becasue we have the PciDevice object not the pci request objects here09:47
sean-k-mooneyi was going to just add  or device.source == objects.InstancePCIRequest.FLAVOR_ALIAS09:48
sean-k-mooneybut device is not an InstancePciREquest object09:48
sean-k-mooneywe have the pci request too09:49
gibiyes09:50
gibithere is somewhere a generic code that does PciDevice.request_id = InstancePCIRequest.request_id09:51
gibiwhich is I think correct09:51
sean-k-mooneythat basically waht im trying locally09:52
sean-k-mooneyim doing a set comprehention to get the flavor request ids and then checking if the current device is in that when request_id is none09:52
sean-k-mooneyhttps://paste.opendev.org/show/bqJUPAfRGibXn8mslgdU/09:54
gibiI added https://paste.opendev.org/show/b1nvksM7Fw7F4w5vnDKw/ to Instance ovo and replaced the get_instance_pci_devs calls with it and it seems to work09:54
sean-k-mooneythat seams to work09:54
sean-k-mooneyyou could do that but you can do it in the existing fucntion without changing the signiture or moving it09:55
gibiyeah but 1) I have the implicit not-providing-request-id meaning give me the flavor based CPI devs09:56
gibis/have/hate/09:56
gibiI want to make the query explicit by the caller09:56
sean-k-mooneyyep i get that09:56
gibi2) also request_id == 'all' is /o\09:56
sean-k-mooneythe duality because of the fact that this code predated neutron sriov i think09:57
sean-k-mooneyand source09:57
gibiyes, it is old code, it served wll but I retired it now :D09:57
sean-k-mooneysource is relitivly recent09:57
sean-k-mooneyyep so just runing the func test loocally my change seams to work09:58
sean-k-mooneyi suspect your will also09:58
sean-k-mooneyso you have too paths forward you coudl make my small change for not and then have a followup refactor patch to swap to your veriosion09:58
sean-k-mooneyor you could do that all in one go along with changing all callers to use the new version on the ovo09:59
gibiit is < 10 callers so I won't split this into two09:59
sean-k-mooneyok 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 that10:00
sean-k-mooneyim not driectly modifying that function for the vdpa code it just ends up calling it indreictly10:01
gibiyou vdpa series will have priority over my  change and I will rebase, no worries10:01
sean-k-mooneyim going to grab a drink but it sounds liek you ahve a way forward10:01
sean-k-mooneyim going to step away for 5 mins and ill be back. i like the simplicty of your new get_pci_devices function10:02
sean-k-mooneyalthough im not sure if the deepcopy is correct10:03
sean-k-mooneycan 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 function10:04
gibihm, you are right I need a shallow copy10:04
gibihm, not even that10:05
gibiI first tried a startegy that would modify the dev list 10:05
gibibut now it creates new lists10:05
gibiso no copy needed10:06
gibigood catch10:06
sean-k-mooneyi 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 honest10:13
sean-k-mooneyi.e. i was not sure if i missed it in mine or if it was extra in yours.10:14
sean-k-mooneyso you can just make it   devs = self.pci_devices or []10:14
gibiyes10:19
stephenfingibi: Sorry, I am around but I wasn't connected to IRC. What's up?10:22
gibistephenfin: it is about https://review.opendev.org/c/openstack/python-openstackclient/+/831902/comments/9b4913bd_cee0ed5710:23
stephenfinsean-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 OSC10:23
stephenfinWe shouldn't do nova-specific things, even if the terminology used by OSC isn't ideal10:23
stephenfinHopefully what I've said makes sense10:23
* stephenfin looks10:23
sean-k-mooneyit does but i dont think its sufficent10:24
stephenfinOh, one and the same :)10:24
sean-k-mooneyi really dont think using the no prfix shoudl be allowed10:24
sean-k-mooneythat siad i wont block on it10:24
sean-k-mooneybut i dont think we shoudl use it anywere in osc10:24
stephenfinIt's everywhere already though. That ship sailed so long ago that's it almost home again :)10:25
sean-k-mooneyi tought the gramer alone would make you recoile from its use :)10:25
stephenfinHeh, true. It's a very common pattern though10:26
stephenfinEven argparse supports it now10:26
stephenfinLook for BooleanOptionalAction on https://docs.python.org/3/library/argparse.html10:26
sean-k-mooneyits a cisco patteren that they took othres to court over in the past10:26
sean-k-mooneythey tried to trademark it i belive10:26
gibitrademark --no-*? lol, I want to be that lawyer :D10:27
stephenfinWe've gone off topic now, but how would we do inverse boolean options (flags) without that?10:27
sean-k-mooneygibi: they sued as a breach of there copyright when arista tried to use it https://www.networkcomputing.com/networking/cisco-arista-battle-over-cli10:28
sean-k-mooneystephenfin: but your githt it is off topic10:29
sean-k-mooneyi just dislike mimicic that style because of there previous actions and also i find it less clear10:30
sean-k-mooneystephenfin: i would not have it as a flag i woudl have =True|False personally10:32
sean-k-mooneyor use --revert --confrim e.g. two related terms10:33
sean-k-mooneyi generally perfer --*=True|false type CLIs10:34
sean-k-mooneytoo --* --no-* CLIs10:34
opendevreviewJan Hartkopf proposed openstack/python-novaclient master: add support for microversion 2.93  https://review.opendev.org/c/openstack/python-novaclient/+/81615810:41
opendevreviewRajesh Tailor proposed openstack/nova master: Fix rescue volume-based instance  https://review.opendev.org/c/openstack/nova/+/85273711:18
sean-k-mooneystephenfin: im not  -1ing since im more or less delegating the style desisson to you11:23
stephenfinack, thanks. And I'm just following the OSC guidelines11:23
sean-k-mooneyits 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 it11:24
sean-k-mooneystephenfin: 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 place11:40
sean-k-mooneyi was just thinking about the downstream escalation 11:40
sean-k-mooneywould a nova status command to find all instnace that have incorrect pci device adress in there neutron ports be in scope11:41
sean-k-mooneyi.e. nova-status validate instance-pci-slots11:41
sean-k-mooneyand have that print a list of instnace uuids11:42
sean-k-mooneyas an admin you would either need to manually fix them or just cold migrate them and let nova do it11:42
sean-k-mooneywe 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 blob11:44
sean-k-mooneythis would be entirely unrelated to upgrades however so not sure if nova-status is the right place11:44
sean-k-mooneybut it would tell you if there is "db currption" where the neutron db is out of sync with nova's pci allcoations11:44
sean-k-mooneythere 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 sesne11:45
sean-k-mooneyim not sure if ye have seen https://review.opendev.org/c/openstack/nova-specs/+/853837 that dansmith  started for hardening our hostname requirements11:47
sean-k-mooneywhen im thinking about the fallout of hostname changes i think some of the sideeffect are things we coudl detect and codify in nova-status checks11:48
sean-k-mooneyfor example checking that a vm with pci allcoations has them against the compute node its currently on11:49
sean-k-mooneyso if non upgrade checks are in scope of nova status we could add a set of validations to it for similar things going forward11:50
sean-k-mooneyif we wanted to have automated fixes i would sugggest using nova-manage11:50
sean-k-mooneybut detecting the issues i think would fall into nova-status11:50
sean-k-mooneyor perhaps the health check api if the detachtions are cheap and can be part of a periodic11:51
sean-k-mooneyfor 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:52
sean-k-mooneystephenfin: by the way im starting to use the versionadded directive should i drop the histroy table at the end with the planned/inprogress line items11:54
sean-k-mooneyits proably reduntant now11:54
gibisean-k-mooney: I would add that to nova-manage as it more simila to the audit command there11:57
sean-k-mooneyok so nova-manage validate <whatever> or nova-manage check <thing>11:58
gibiyes. I would keep the upgrade checks separate as they have a well defined scope11:58
sean-k-mooneyack11:58
sean-k-mooneyim 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
gibiI agree11:59
gibimy recent leaked migration allocation case could be added there too12:00
sean-k-mooneyyep and depending on how we write them perhaps they could be reused for healtch checks later12:01
sean-k-mooneyi feel like many of these checks are two heavy weight for that12:01
gibiwe need to measure the load they create12:01
sean-k-mooneybut there are some simple cases where we currently raise excptiont that could set a booleing flag12:02
gibiI can imageine that some of them are too heavy12:02
sean-k-mooneyyep for the reousce tracker brakages we already have a pattern in the code i created for catching expctins and using those to set flags12:02
sean-k-mooneywhich will be the simple way to detect two vms using the same cpus for examle12:03
sean-k-mooneywe can catch the qemu issue when two vms try to use the same pci device the same way12:03
sean-k-mooneywithout computeing it12:03
sean-k-mooneyhaving the ablity to ask nova for the conflciitng vms seperatly however will help operators resolve that12:04
sean-k-mooneyand thats kind of where i was going with the new commands12:04
gibimake sense12:04
sean-k-mooneyso healtch notices we failed to boot because fo conflciting pci devices. then operartor runs the command to deterim what vms are using the wrong devices12:05
sean-k-mooneythen they cold migrate them to fix it or manually fix the port in neutron12:05
opendevreviewRajesh Tailor proposed openstack/nova master: Fix rescue volume-based instance  https://review.opendev.org/c/openstack/nova/+/85273712:47
gibisean-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#L132713:41
opendevreviewribaudr proposed openstack/nova master: Alphabetizes objects  https://review.opendev.org/c/openstack/nova/+/85398613:43
sean-k-mooneyoh ok i guess that explains why we do it seperatly13:43
opendevreviewBalazs Gibizer proposed openstack/nova master: Generate request_id for Flavor based InstancePCIRequest  https://review.opendev.org/c/openstack/nova/+/85383513:52
gibisean-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_id13:52
opendevreviewsean mooney proposed openstack/nova master: Add source dev parsing for vdpa interfaces  https://review.opendev.org/c/openstack/nova/+/84101613:59
opendevreviewsean mooney proposed openstack/nova master: Fix suspend for non hostdev sriov ports  https://review.opendev.org/c/openstack/nova/+/84101713:59
opendevreviewsean mooney proposed openstack/nova master: Add VDPA support for suspend and livemigrate  https://review.opendev.org/c/openstack/nova/+/85370413:59
sean-k-mooneygibi: 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 time14:00
sean-k-mooneyby 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 too14:01
sean-k-mooneywe dont need that but it might be nice14:02
gibisean-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_name14:02
*** dasm|off is now known as dasm14:02
gibiI could set requester_id, but I don't use it14:02
sean-k-mooneyits just a nice to have14:02
gibialso requester_id pointing to flavor_id is problematic as extra_spec can change on a flavor14:02
sean-k-mooneywell they can but they should not but i take your point14:03
sean-k-mooneyit would just be nice if it was also always set14:03
sean-k-mooneybut it does not have to be14:03
gibianyhow until I don't know how requester_id would be used I don't know if a changing flavor extra spec could cause trouble14:04
sean-k-mooneywell we woudl use the embeded copy anyway14:04
sean-k-mooneyi was wondering if it woudl help for resize14:05
gibithat would work, but I cannot enforce that when a InstancePCIRequest.requester_id is compared to a flavorid then that flavorid is actually a stored copy or not14:05
sean-k-mooneyso we can corralate the pci request to the source or dest flavor14:05
sean-k-mooneyfor example today there si no way to differnceate between source and dest flavor when resizing to same host14:06
gibiI definitely have to look at the resize case with pci in placement as the move claim re-creates the InstancePCIRequest so new uuids will be generated there14:06
sean-k-mooneybut if the requester id was the flavor uuid in that case we could14:06
gibiyes, it sounds useful14:07
gibiI keep it in mind14:07
sean-k-mooneyack im trying not to over complicate the requirement for your mvp14:07
opendevreviewBalazs Gibizer proposed openstack/nova master: Generate request_id for Flavor based InstancePCIRequest  https://review.opendev.org/c/openstack/nova/+/85383514:08
gibisean-k-mooney: no worries :)14:08
sean-k-mooneystephenfin: gibi  when you have time the vdpa seriese i belive is not ready for review14:08
stephenfinnow?14:08
sean-k-mooneyim going to switch to some down stream stuff for a bit14:09
stephenfin(or not)14:09
sean-k-mooneystephenfin: yes just pushed it14:09
sean-k-mooneysorry now14:09
sean-k-mooney:)14:09
gibiI will review it right now, to fit it before the downstream call14:09
gibisean-k-mooney: there are some service version mistmatches in the last vdpa patch, I left a -1 there14:28
sean-k-mooneyya also i just ran those tests localy and 2 of them fail14:28
sean-k-mooneyi tought i did that but i guess not14:28
sean-k-mooneyill fix the issues today but likely after our downstream meetings14:29
sean-k-mooneythanks for taking a look14:29
gibiack14:30
JayFHey all again, still looking for reviews on this improvement to CI for Ironic-related patches https://review.opendev.org/c/openstack/nova/+/85352915:02
JayFand a couple of stable patches I'm trying to get through the gate; one is at victoria right now (  https://review.opendev.org/c/openstack/nova/+/821350 ) and the other is at ussuri ( https://review.opendev.org/c/openstack/nova/+/853540 )15:03
JayFAlso if there's anything I can do to more properly integrate my requests for reviews into whatever process you all use, I'm happy to take feedback.15:03
opendevreviewSofia Enriquez proposed openstack/nova master: WIP: Check NFS protocol  https://review.opendev.org/c/openstack/nova/+/85403016:05
*** tkajinam is now known as tkajinam|off16:35
opendevreviewJean-SĂ©bastien Bevilacqua proposed openstack/nova master: Add Lustre support to nova  https://review.opendev.org/c/openstack/nova/+/85378616:37
opendevreviewRajesh Tailor proposed openstack/nova master: Fix rescue volume-based instance  https://review.opendev.org/c/openstack/nova/+/85273716:49
opendevreviewsean mooney proposed openstack/nova master: Add VDPA support for suspend and livemigrate  https://review.opendev.org/c/openstack/nova/+/85370416:57
opendevreviewElod Illes proposed openstack/nova stable/victoria: Ignore plug_vifs on the ironic driver  https://review.opendev.org/c/openstack/nova/+/82135017:15
stephenfinsean-k-mooney: question on the last vDPA patch but good on the other two https://review.opendev.org/c/openstack/nova/+/85370417:16
sean-k-mooneygibi: stephenfin  what is the lifetime of this fixture https://github.com/openstack/nova/blob/master/nova/tests/functional/libvirt/test_pci_sriov_servers.py#L8317:17
sean-k-mooneywhen i inhirt form this class instead it not actully in applied in my live migration test17:18
sean-k-mooneydo i need to assgin that to a varible17:18
gibia single test case17:18
sean-k-mooneyto extend it to the full test17:18
gibido you call super() from your class setUp?17:18
sean-k-mooneyyep17:19
sean-k-mooneyif i do17:19
sean-k-mooneyfrom nova.tests.fixtures.libvirt import Domain as Dom17:19
sean-k-mooney        self.assertEqual(self._migrate_stub, Dom.migrateToURI3)17:19
sean-k-mooneythey are not the same17:19
sean-k-mooneystephenfin: looking now17:19
gibistrange. I would say push the patch and I will look at it but I will only do that tomorrow. I' mostly off for today17:20
sean-k-mooneygibi: the current one has the issue17:21
sean-k-mooneyand no worries17:21
sean-k-mooneyi might assign it to a var and see if that helps17:21
gibishould not make a difference but worth trying as it maybe tease out something else17:21
sean-k-mooneygibi: by the way stephenfin  question is basicaly somethign i need you to answer https://review.opendev.org/c/openstack/nova/+/853704/5/nova/objects/service.py#23617:22
sean-k-mooneygibi: it can wait till tomorrw but my current answer is becasue gibi said so17:22
gibistephenfin, sean-k-mooney: about the N-1 version list in service.py. I think the easiest way out is not to change that in this patch series17:23
gibiwe change that at every start of a cycle17:23
sean-k-mooneyack i can drop it17:24
stephenfinwfm17:24
gibibut to answer the question. When I introduced that logic I made it generic to support the case when somebody upgrade nova to beta version (or to m2 version)17:24
gibiso the recorded service version for a given release is the first version appeared in that release17:24
gibiso when we say Yoga supports Xena computes it means Yoga supports even the first (not just the last) service version of the Xena computes17:25
sean-k-mooneyhum ok i tought this was ment to be the latest version17:25
sean-k-mooneyim not sure we make that guarentee17:26
gibiI think talked about later that it does not really need to support m2 upgrades17:26
gibiso my genericness is not strictly needed17:26
gibibut it is not wrong per se17:26
sean-k-mooneyit does not need to be in this patch anyway17:26
gibibut if we change to latest, then I suggest to change it with some extra comments that describes that past values are not latest per release17:26
gibisean-k-mooney: yes, it is not needed in this patch17:26
sean-k-mooneycool enjoy your eventing17:27
gibibtw, I don't see any obvious problem with your live migration test, but I will look more tomorrow17:27
sean-k-mooneyim going to finsih soon too as i have a headache17:27
sean-k-mooneyya its weired17:27
sean-k-mooneyim going to try one or two thing and call it aday17:27
sean-k-mooneyim wondering is this a sideefect fo fixtures._fixtures.monkeypatch.MonkeyPatch vs fixtures._fixtures.mock17:29
opendevreviewMerged openstack/nova master: Fix a deprecation warning about threading.Thread  https://review.opendev.org/c/openstack/nova/+/85386919:07
opendevreviewRico Lin proposed openstack/nova master: Add locked_memory extra spec and image property  https://review.opendev.org/c/openstack/nova/+/77834720:35
opendevreviewRico Lin proposed openstack/nova master: libvirt: Add vIOMMU device to guest  https://review.opendev.org/c/openstack/nova/+/83064620:35
opendevreviewRico Lin proposed openstack/nova master: Add traits for viommu model  https://review.opendev.org/c/openstack/nova/+/84450720:35
*** dasm is now known as dasm|off21:51
opendevreviewBrett Milford proposed openstack/nova master: Handle "no RAM info was set" migration case  https://review.opendev.org/c/openstack/nova/+/85200223:02

Generated by irclog2html.py 2.17.3 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!