| bogdando[m] | hi, WDYT folks on resurrecting https://specs.openstack.org/openstack/cyborg-specs/specs/2023.1/approved/attribute-api-support.html and applying that design for phys_net of deployables instead of attributing particular semanthics to attach handles in generic drivers? context is https://review.opendev.org/c/openstack/cyborg-specs/+/1005089 | 08:53 |
|---|---|---|
| sean-k-mooney | we will likely reviit but not on the deployable | 08:54 |
| sean-k-mooney | the attibutes shoudl have been on the device instead | 08:54 |
| bogdando[m] | well, device is PF, and for smart nic, its more makes sense to attribute one or a group of its VFs (deployables) | 08:54 |
| sean-k-mooney | bogdando[m]: but to be clear we will asy want to have a config driven approch as a primary approch | 08:54 |
| sean-k-mooney | we may add some api driven suprpot later | 08:55 |
| sean-k-mooney | but its really not a good ux at scale | 08:55 |
| sean-k-mooney | device is not the pf | 08:55 |
| sean-k-mooney | tdevice is the assiable thing it can be the pf or vf | 08:55 |
| sean-k-mooney | a deployable is a pool of allcoatabel devices | 08:56 |
| sean-k-mooney | currently resouce classes for palcement are tracked via the atibutes api | 08:56 |
| sean-k-mooney | that shoudl be on the device | 08:56 |
| sean-k-mooney | becuase each deployable corresponds to a placment resouce provider | 08:56 |
| bogdando[m] | ack, it seems I missread "A device has a management interface, whose address is the control path identifier: for SR-IOV devices, this is usually the PCI Physical Function (PF)" - PF is indeed control managmenent interface of a device, not the device | 08:57 |
| sean-k-mooney | and we want to be able to have 1 placement resouce provier with inventoreis of diffent resouce class | 08:57 |
| bogdando[m] | yes, I like the idea that a deployable is a pool of VFs | 08:58 |
| sean-k-mooney | to be clear i dont think that hte atibute api as orgianly specified is the right shap and the impelation that was done didnt follow the spec and is even worse | 08:58 |
| bogdando[m] | but I am not sure about PFs VFs boundaries as you suggested | 08:58 |
| bogdando[m] | grouping devices or PFs seems a bit another layer of abstraction than VFs affinity | 08:59 |
| sean-k-mooney | placement is not deisign to have 100s of rps for a given host | 09:00 |
| sean-k-mooney | it casue sever performance probelms and a singel nic can have 100s of VFs | 09:00 |
| sean-k-mooney | we group VFs by pf in nova today | 09:00 |
| bogdando[m] | I see, that makes sense to not break the contracts established like each deployable is a placement rp | 09:01 |
| sean-k-mooney | there are 3 related thigns | 09:01 |
| sean-k-mooney | devices deployables and attachmet handels | 09:01 |
| sean-k-mooney | there is a 1:1 relathiship between devices adn attachment handels | 09:02 |
| sean-k-mooney | and a many:1 relasthip between device and deployable | 09:02 |
| sean-k-mooney | or attachmetn_handel and deployabel depending on how you look at it | 09:02 |
| bogdando[m] | hm, my understanding was 1:many for devices and deployables and 1:many for deployables and ah | 09:03 |
| sean-k-mooney | no | 09:03 |
| bogdando[m] | it is based on https://specs.openstack.org/openstack/cyborg-specs/specs/train/implemented/cyborg-nova-placement.html#background | 09:03 |
| bogdando[m] | "A Cyborg device has one or more components named deployables, each of which contains one or more accelerators" | 09:04 |
| bogdando[m] | so accelerators to handles is 1:1, right | 09:05 |
| bogdando[m] | everyting else seems to be 1:many :) | 09:05 |
| sean-k-mooney | so in the db they are not corratled at all directly | 09:06 |
| bogdando[m] | if there is some hidden knowledge based on placement desing, do we have a better place to read about it? | 09:06 |
| sean-k-mooney | not really and the curen implation is both incodsitend and not scalable | 09:07 |
| sean-k-mooney | you really need to look at the code and already know hwo placment was designe to be used | 09:07 |
| bogdando[m] | I see, that comlpicates design review on shaky grounds | 09:07 |
| sean-k-mooney | so the actuall relation ship is as follow | 09:08 |
| sean-k-mooney | device <> contoplathID is 1:1 contolpath id <> attachment handel is 1:1 | 09:08 |
| sean-k-mooney | attachment handel <> deployable is many to 1 | 09:09 |
| sean-k-mooney | the attachment handel is actullth theing that maps contoplath ides to deployables | 09:09 |
| bogdando[m] | would be nice to reflect that in dev docs | 09:09 |
| sean-k-mooney | and devices are assocated transitivly | 09:09 |
| sean-k-mooney | bogdando[m]: its on my todo list | 09:10 |
| bogdando[m] | ack, thanks for explaining! | 09:10 |
| sean-k-mooney | but you can see this by looking at the db schema today https://github.com/openstack/cyborg/blob/master/cyborg/db/sqlalchemy/models.py#L78-L210 | 09:10 |
| sean-k-mooney | the problem iwth the atibutes api is it shoudl have been again the device or optionaly the device or deployable | 09:11 |
| bogdando[m] | looking into code may confuse reader because old implementation paths not strictly following old design specs | 09:11 |
| sean-k-mooney | this is consitent with the spec | 09:11 |
| sean-k-mooney | but you need more context then is captured there | 09:11 |
| sean-k-mooney | or rather ti coudl have done with better diagrams to make it very clear | 09:12 |
| sean-k-mooney | https://specs.openstack.org/openstack/cyborg-specs/specs/train/implemented/cyborg-nova-placement.html#background does not show it clearly | 09:12 |
| bogdando[m] | that comment about code was mostly for "to be clear i dont think that hte atibute api as orgianly specified is the right shap and the impelation that was done didnt follow the spec and is even worse" | 09:12 |
| sean-k-mooney | ah well yes the implementation didnt follwo the spec | 09:13 |
| sean-k-mooney | the spec was reasonabel the implelatin less so | 09:13 |
| sean-k-mooney | i was effectivly planing to remvoe the top level /atibute api because that was neer approved and doesnt really work well | 09:14 |
| sean-k-mooney | https://specs.openstack.org/openstack/cyborg-specs/specs/2023.2/implemented/attribute-api-support.html#rest-api-impact | 09:15 |
| sean-k-mooney | the speci specificly said `/v2/deployable/{uuid}/attribute` | 09:15 |
| sean-k-mooney | we shoudl have atibute son both deployabels and devices in my opion | 09:15 |
| sean-k-mooney | but the impletion added /v2/attributes | 09:16 |
| sean-k-mooney | https://docs.openstack.org/api-ref/accelerator/#list-attributes | 09:16 |
| sean-k-mooney | the impltion also does not work properly and was never properly completed | 09:16 |
| bogdando[m] | attach handles to deployables relation that you explained seems strictly following the original design link I provided above, and matches 1 to many / many to one - depending on which goes first. The difference seems to be more for controlpaths, devices and accelearators | 09:17 |
| sean-k-mooney | my prefence is to add /v2/deployable/{uuid}/attribute and /v2/device/{uuid}/attribute and delete /attibute in a new microverison | 09:17 |
| sean-k-mooney | not quite i think that is still following the sepc but again i was involved in the spec design and over all nova/placment design in person | 09:19 |
| sean-k-mooney | so i have more context then may have been capatured | 09:19 |
| bogdando[m] | back to the subject, it seems we might want to postpone the spec which suggests using phys_net in attach handles - until we have new attributes design implemented? | 09:22 |
| bogdando[m] | to avoid double migration paths | 09:22 |
| sean-k-mooney | no | 09:24 |
| sean-k-mooney | the phsnet is already in the attachment handel | 09:24 |
| bogdando[m] | ah | 09:25 |
| sean-k-mooney | what that spec is doign is propsong we port the logic out of the intel only nic driver | 09:25 |
| sean-k-mooney | into the generic pci driver | 09:25 |
| sean-k-mooney | so it wokrs with any nic | 09:25 |
| bogdando[m] | I see. Well my concern remains - phys net likely an attribute and needs another implementation | 09:26 |
| sean-k-mooney | it may be at some point | 09:27 |
| sean-k-mooney | but it shoudl be set via config | 09:27 |
| sean-k-mooney | as the priamry way to configure it | 09:27 |
| sean-k-mooney | nova never uses the atibute api and never will | 09:27 |
| sean-k-mooney | it uses only the arq api and device profile apis | 09:28 |
| sean-k-mooney | the attachment handel is the summeraisation of the information required to attach a device to a workload (vm or container) | 09:28 |
| sean-k-mooney | bogdando[m]: you proably should read https://specs.openstack.org/openstack/nova-specs/specs/xena/implemented/sriov-smartnic-support.html | 09:29 |
| bogdando[m] | also it looks like having phys net in ah of legacy intel driver is OKish, while having a fresh started geneeric pci driver design clean | 09:30 |
| sean-k-mooney | when we implmented suprpot for cyborg managed neutron ports in nova in xena we added code in nova that checks teh phsynet | 09:30 |
| sean-k-mooney | bogdando[m]: the attachmetn handle is the corect place for it | 09:31 |
| sean-k-mooney | bogdando[m]: nova should never use the attibutes api ever | 09:31 |
| bogdando[m] | that is a nice reference to read, thank you. I understood that there is another established contract for nova to cyborg and neutron interaction | 09:32 |
| bogdando[m] | albeit do you think that adding network specific attributes, then for example gpu specific ones, and maybe later one more for nvmes or fpgas, still keeps that driver attach handle generic? | 09:33 |
| bogdando[m] | am not trying to make a revolution here though :) | 09:35 |
| sean-k-mooney | first of all yes, but secondly the only things that will be specifid via that interface are thigns that change how the device is assocated with the guest | 09:35 |
| sean-k-mooney | bogdando[m]: currently your propsoal are misaligned with the driction i woudl liek to take the proejct. im open ot dicussion but i do not want to have nova need to know more about cybrog then it already does in general | 09:36 |
| sean-k-mooney | nov ainteract with only 2 api, the device profile api to get the resouce request | 09:36 |
| sean-k-mooney | and the the acclerator request api to bind allcoated devices | 09:36 |
| bogdando[m] | I am ok with that just maybe renaming generic driver to something else then | 09:37 |
| sean-k-mooney | the attchmeth handel is part of the repocne of the arq bind | 09:37 |
| bogdando[m] | like universal driver | 09:37 |
| sean-k-mooney | the term generic is chooen specifly to be vendor agnistic | 09:37 |
| sean-k-mooney | we have a pci driver, and are addning an nvme dirver | 09:37 |
| sean-k-mooney | they arem named after the bus tehy manage | 09:38 |
| sean-k-mooney | i.e. the mdev drvier that we are creating | 09:38 |
| sean-k-mooney | my hope is to eventually remove most of the vendor specific drivers | 09:38 |
| bogdando[m] | yes, I also support that | 09:38 |
| sean-k-mooney | and only have the generic driver unless there is a stong reaodn ot keep a vendor driver | 09:38 |
| sean-k-mooney | so if we say wanted ot supprot USB passthough in the future we woudl add a usb driver that can work with any usb device instead of a logitech driver | 09:39 |
| sean-k-mooney | same fr cxl, block devices ectra. | 09:40 |
| sean-k-mooney | the reason nvme is not part fo the generic pci deriver is because its stateful and need specific cleaning | 09:40 |
| bogdando[m] | just as a thought experiment, what if attributes api to be added for accelerators api to which nova already interacts | 09:40 |
| sean-k-mooney | it already is kidn of there in a form more useful to nova | 09:41 |
| sean-k-mooney | bogdando[m]: the only thing that this is used for today is addign an removing traits | 09:41 |
| sean-k-mooney | that it | 09:41 |
| sean-k-mooney | the device profile already contians groups of resoucelass and trait requests | 09:42 |
| sean-k-mooney | the atibute api is not inteded as a way to do devifce configuration | 09:42 |
| sean-k-mooney | its a metadata store | 09:42 |
| sean-k-mooney | i do not want to extend it to do configuration if we do that it will be iva a diffent api | 09:43 |
| sean-k-mooney | bogdando[m]: form the api ref "A attribute represents the trait of a physical card like FPGA or GPU." | 09:43 |
| sean-k-mooney | its litally just a proxy api to add traits to the placement resouce provider | 09:43 |
| sean-k-mooney | its not needed today because you can do that directly via the placment api | 09:44 |
| sean-k-mooney | this is why i said i dont think its fit for porpus today and i woudl prefer to entirly replace it in the future | 09:45 |
| bogdando[m] | attributes api looks redundant, yes, although mentions non-placement/nova use cases | 09:47 |
| sean-k-mooney | it would acked as a geneitc metatdata store | 09:48 |
| sean-k-mooney | but https://bugs.launchpad.net/openstack-cyborg/+bug/2158996 prevents it form being used at all today | 09:48 |
| sean-k-mooney | the implemtion today requries you to know the internal database primary key which is never returend form teh api | 09:49 |
| bogdando[m] | "A device may have components, such as flash memory or BMC, which are not of relevance to Nova or Placement. Those components may have attributes" | 09:49 |
| sean-k-mooney | we do not have any usecase that require that currnetly | 09:50 |
| sean-k-mooney | to be clear if i was a core when this was propsoed i would have been -2 on the spec based on its curent content | 09:51 |
| bogdando[m] | actually that "not of Nova relevance" answers to my propoal already - phys_net is in relevance, hence not a fit for that api | 09:52 |
| sean-k-mooney | i can see usecase for device metadta or config apis in the future but that diffent | 09:53 |
| sean-k-mooney | melwitt: ok i finially finsihed reviewign both your spec and left comments inline | 16:27 |
| sean-k-mooney | directionally both are a good start but between teh 2 i left 60 ish comments so maybe wait till monday to read them :) | 16:28 |
| melwitt | sean-k-mooney: 60?? lol ok. thank you for reviewing :) | 16:57 |
| sean-k-mooney | melwitt: some of them may be duplicative | 17:06 |
| sean-k-mooney | i went seciton by seciton and later noteice that soem eiarer coment were paticly adressed | 17:06 |
| melwitt | ah nice I got some review from bogdando[m] too. thanks both | 17:06 |
| melwitt | ah k | 17:07 |
| sean-k-mooney | i started early today and it now 18:00 so im going to finsh nowish | 17:09 |
| sean-k-mooney | if you want to chat about any of the feed back on a call or somehting we can grab some time next week | 17:09 |
| melwitt | sounds good :) | 17:09 |
| sean-k-mooney | but we can continue to dicuss on gerrit too | 17:09 |
| opendevreview | Takashi Kajinami proposed openstack/cyborg-tempest-plugin master: Add stable/2026.2 job https://review.opendev.org/c/openstack/cyborg-tempest-plugin/+/1005315 | 17:16 |
Generated by irclog2html.py 4.1.0 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!