Thursday, 2022-01-20

*** mtreinish_ is now known as mtreinish05:13
*** hemna2 is now known as hemna07:37
*** priteau_ is now known as priteau11:16
*** bhagyashris__ is now known as bhagyashris11:49
gibianyone with placement insights do you agree my conclusion in the bug https://storyboard.openstack.org/#!/story/2009795 ?11:53
sean-k-mooneygibi: ill read over it now11:58
gibisean-k-mooney: thanks11:59
gibiI've just added one more link to the end of the ticket supporting that it is doc bug12:01
sean-k-mooneyoh this is the resouce less traits issue12:01
sean-k-mooneywe talked about this before with regrads to neted resource providres and numa i think12:01
gibiyes, the nested magic spec class these traits as provider traits compared to resource traits that are tight to a specific resource class12:01
gibiyes the nested magic spec captures some of that discussion I believe12:02
sean-k-mooneywe only have one type of traits in plamcent12:03
sean-k-mooneywe dont make a distinciton in the api today12:03
gibiyes12:03
sean-k-mooneyaf far as i know12:03
gibithe distinction is just logical not implemented12:03
sean-k-mooneyright im assserting there is not distinciton and this is a bug not a docs bug12:04
sean-k-mooneybut ill re read teh nested magic spec now12:04
sean-k-mooneyim really not sure i agree with https://docs.openstack.org/placement/latest/specs/train/implemented/2005575-nested-magic-1.html#resource-versus-provider-traits12:07
sean-k-mooneyi do not belive we can assume all custom traits on a resouceless rp are provider traits12:07
sean-k-mooneygibi: so https://docs.openstack.org/placement/latest/specs/train/implemented/2005575-nested-magic-1.html#why-enforce-resourceless-same-subtree does not apply to https://storyboard.openstack.org/#!/story/2009795 since its not a request for a resoucless request group12:15
sean-k-mooneythe unnamed group is speical and unlike all other groups it does not require the resouce in it to come form the same RP12:16
sean-k-mooneyevery named request group does12:16
gibisean-k-mooney: so you are on the side that an required trait in an unnamed group can come from any RP of the tree?12:21
sean-k-mooneyyes stongly so12:21
gibithe code today only considers RPs that are providing resources to the request 12:21
sean-k-mooneywe do not use root_requires today12:21
gibiI'm reading https://docs.openstack.org/placement/latest/user/provider-tree.html12:22
gibiit seems to be relevant too12:22
sean-k-mooneyso if we moved all the resouces form the root rp to numa nodes but did not move the capablity traits they woudl stop working12:22
gibieither we need root_required or we need a resource on the root rp12:24
gibi* resource on the root rp that is requested12:24
sean-k-mooneyor the unnamed group is allowed to match it12:24
sean-k-mooneybut ya we would have to mvoe to root_reqiured which is an upgrade issue12:25
sean-k-mooneyas we would have to make 2 queires and combine them12:25
sean-k-mooneyto support rooling upgrades12:25
gibi"Traits can be requested explicitly in the GET /allocation_candidates operation with the required query parameter, but traits on resource providers never span other resource providers. If a trait is requested, one of the resource providers that appears in the allocation candidate should have the trait regardless of sharing or nested providers."12:26
gibihttps://docs.openstack.org/placement/latest/user/provider-tree.html#filtering-by-traits12:26
gibithis shows me that the intention is the one that I see in the code12:26
sean-k-mooney well the rp would appear in the allocagtion candiate in the provider tree summary even if it did not provide a resouce correct?12:27
gibi"A dictionary keyed by resource provider UUID included in the allocation_requests, of dictionaries of inventory/capacity information. "12:28
gibiso no, only the RPs that provides resources appear in the summary12:28
sean-k-mooneythe current approch impleie si can have a sharing resouceless resouce provider with traits and have them match the unnamed group12:28
gibithe "unused" child RPs are not12:28
sean-k-mooneywhich woudl be problematic since that is how we do isolated aggreates right12:29
sean-k-mooneyhttps://docs.openstack.org/nova/latest/reference/isolate-aggregates.html12:29
gibihm, that feature adds the trait on the root RP12:30
sean-k-mooneyactully no this i swhy we have to set the trait on every host in the aggreated which is a major ux issue12:30
sean-k-mooneyyes today it does12:30
sean-k-mooneybut you shoudl be able to create 1 rp12:30
sean-k-mooneyadd the trait to that12:31
sean-k-mooneywith misc_share_via aggreate12:31
gibiOK, I think except the API doc, the rest of the doc is self consistent (even if we don't like how they work)12:31
sean-k-mooneythen add other hosts ot it12:31
sean-k-mooneywell i think we can agree on not likeign the current behavior12:33
gibi:)12:33
sean-k-mooneyi dont think the behaivor shoudl change based on if the rp has a trait or not12:33
sean-k-mooneywhat happens is you use an older micorverion12:35
gibiI can be convinced to change the logic later / separately. I just hit this while implemented the any-trait logic and first I thought it is a code bug based on the API doc. But now with the other docs in the picture and the actual usage of this in the isolate-aggregate I think this is just a doc bug12:35
gibisean-k-mooney: do you have a particular microversion to check? I have test env so I can try12:35
sean-k-mooneyi think its more subtle then that12:35
sean-k-mooneyi think the doc captured the intent12:36
gibiwhich doc? the API or the provider-tree doc?12:36
sean-k-mooneyapi doc captures how i expected this to work the provider-tree doc captures what the code is doing12:37
sean-k-mooneyi find the current behavior hard to reasonable usefully12:37
sean-k-mooneyto me the idea that a trait in the unnamed group can match agaisnt any RP in the tree or in a sharing rp is a fundemental part fo the api regardelss of if resouces are requested form it12:38
sean-k-mooneygibi: with out it i dont know how to reason about trait request from images12:38
sean-k-mooneyor in the flavor to some degree but images are the most restict way a user can express traits12:39
sean-k-mooneythey can only form part of the unnamed group12:39
gibifrom the nested magic spec I feel that the current implemented logic supports resource traits, qualities related to certain resource classes (happen to be on the same RP) for the provider trait case we _shoudl_ use root_quired12:39
gibiprobably the traits from the image should be translated to root_required12:40
sean-k-mooneythat the thing i really dont like the intordcution fo provdier traits12:40
gibibut I guess we put them in the unnamed group12:40
sean-k-mooneyhow can we know if a CUSTOM_trait is a resouce trait or a provider tarit12:40
sean-k-mooneyit should not be based on if there is an inventoy on the RP12:40
gibiwith the current model we don't12:40
sean-k-mooneythat inventory may not be related to the trait12:40
gibisimply the current trait model cannot nicely describe both type of traits12:41
gibiand it seems the impl opt to consider a trait a resource trait by default12:41
gibiand provider triats are harder to model properl;y12:41
gibiwith the current model12:42
sean-k-mooneyi would put it more stongly right now there are only one type  fo traiats and provider/resouce traits shoudl not be discussed12:42
gibibut they exists logically12:42
gibijust not modelled12:42
sean-k-mooneygibi: to  me the grouping in that spec was lexical12:42
sean-k-mooneyand not part of the data model12:42
sean-k-mooneyso they dont exist12:42
sean-k-mooneythey were only grouping to think about idfferent type of trait but should have no baring on impelemention12:43
gibiwe use both types from nova12:43
gibi"types"12:43
sean-k-mooneynot without a way to catgories CUSTOM_traits12:43
gibiwe consider compute feature traits as provider traits12:43
sean-k-mooneywe do but i would not expect them to have diffeerent behaviors today12:43
sean-k-mooneyi do not12:44
gibitoday no different behavior, we carefully putting them to the root RP where we know that we will always consume resources from today12:44
gibiso our nova model works12:44
sean-k-mooneyyes because they are beign trated as resouce_traits today12:44
sean-k-mooneyby placement12:44
gibiyes12:45
sean-k-mooneyso i woudl reassert we are not using provider_traits or root_required today in nova12:45
gibinova (as the client of placement) needs to simulate the provider_trait logic, by putting provider traits on the root and alway allocation from the root12:46
sean-k-mooneyand i dont think we can until we have a way to model provider_traits and resouce_traits in a way that is indepent form the existnace of inventories12:46
sean-k-mooneyi dissagree12:46
sean-k-mooneywith out a way to model it in the data model we shoudl not try to use a half implemented feature12:46
gibithen which resource CUSTOM_SUPPORT_MULTIATTACH is connected?12:46
gibitoday there is no such resource12:47
gibiso it has to be a provider trait :)12:47
sean-k-mooneyno since that is not part of the api12:47
gibiit is part of the resource model of the nova 12:47
gibistored in placement12:48
sean-k-mooneyno since we dont need it12:48
sean-k-mooneyall our logic in nova works without the concpet of a provider trait12:48
sean-k-mooneygibi: if we want to have resouce traits and provider traits there is a simple solution12:49
sean-k-mooneywe need to move resouce tratis into the inventory12:49
gibi"simple"12:49
gibi:)12:49
gibinot even to inventory but connecting each trait to an instance of a resource class12:49
sean-k-mooneyyes if we want to have resouce traits and provider traits we need to store taits both on the resouce and proviers12:49
sean-k-mooneyi think the inventory is simpler to reason about12:50
sean-k-mooneysince it jsut works for CUSTOM_ traits12:50
sean-k-mooneyif they are applied to the inveotry then they apply to the resouce clas of that invetory12:50
sean-k-mooneyso i woudl propose inventory_traits and provider_traits as the speration instead12:51
gibia cpu flag trait should be connected to the VCPU or PCPU inventory but not a MEMORY_MB inventory12:51
sean-k-mooneytraits are opaque stings to placment12:51
sean-k-mooneyit does not know the releation ship12:51
sean-k-mooneyi dont think we as a client shoudl require placment too12:51
sean-k-mooneyunless we change what tratis are12:52
sean-k-mooneyand add a mapping tabel where we register every trait with the RC it applies too12:52
sean-k-mooneyand an api do that12:52
gibiyes, but if a trait only connects to the inventory but not a resource class instance then a VCPU providing a different set of flags than a PCPU cannot be modelled by the same set of cpu flag traits12:52
sean-k-mooneyit can12:52
sean-k-mooneysince they are two different inventoies12:53
gibion the same root RP providing both VCPU and PCU12:53
gibithen we are talking about "inventory" differently12:53
sean-k-mooneycorrect12:53
gibiI define inventory of an RP as a set of resource class instance12:53
gibiyou define an inventory of an RC instead :)12:53
sean-k-mooneyim say that the inventory need to change form Resouce class + count 12:53
sean-k-mooneyto resouce_class + count+ traits12:53
gibiI agree12:54
gibiI think we are in agreement on this12:54
gibijust using different meeting behind "inventory"12:54
sean-k-mooneyso we woudl be extending the invotry concpet with a new triats field12:54
gibiwe would extend the inventory of an RC concpet :D12:54
gibianyhow we agree here and we are pretty far from my original problem12:55
sean-k-mooneywell an invtory is a top level object in placment12:55
sean-k-mooneyhttps://github.com/openstack/placement/blob/master/placement/objects/inventory.py12:55
sean-k-mooneyso im saying we shoudl add a traits filed that is a list of traits12:55
gibiI will not change the current behavior of the trait filtering logic when I implement any-traits12:55
sean-k-mooneyack12:56
sean-k-mooneyi still think its a bug as currently defined12:56
sean-k-mooneybut we can change it fi we add inventory level traits12:56
gibiyeah12:56
gibiI think it is not a bug based on the original intent (the magic spec) it might be a bug from the current intent we have12:57
sean-k-mooneyi dont think the magic spec ful filles its intended uscases but it may be the intent of the autours12:57
gibistill as there is a disconnect between the API doc and the behavior I would change the doc to avoid a loss of a day of others like me reading only the aPI doc12:58
sean-k-mooneywe likely shoudl adress teh api doc issue yes to make it aling with how it work12:58
sean-k-mooneybut i also think we shoudl avoid using any of the feature intoduced in the magic spec12:58
sean-k-mooneyuntil we reconsider the data model12:59
gibiyou mean we should not use root_required from nova?12:59
sean-k-mooneycorrect12:59
gibiI'm fine with that limit12:59
gibiwe already use same_subtree tough12:59
sean-k-mooneywell12:59
gibiwhich was also defined by the magic spec12:59
sean-k-mooneyrather then limit i shoudl say be very very carful of depening on this if we want to change it going forward13:00
sean-k-mooneyim not sure the curent magic spec is compatblie with how i tought about numa in placment13:00
gibithat is a good question13:00
gibithe numa spec could be an input to rething the placement model13:01
sean-k-mooneyright now i dont think we are useing resouceless rps correct13:01
sean-k-mooneyi think that is the point we have to be careful13:01
sean-k-mooneyroot_required is proably ok13:01
sean-k-mooneyuntill we have resourceless rps the distinction does not matter13:02
sean-k-mooneyso that is the bit i would avoid for now13:02
gibiI agree13:02
gibiresourceless rp feels like an edge case13:02
sean-k-mooneydo we have any features that require it currently that are in flight13:03
sean-k-mooneyi do not think so but just wondering13:03
sean-k-mooneynic affinity/anti affintiy i think was on eof the usecase form the spec but we dont have that today13:04
sean-k-mooneyand my draft pci in placment spec did not use it13:04
gibiI dont know about any use case that needs it now13:04
sean-k-mooneyi was only going to model providres of PF ro VF but not the nic13:05
sean-k-mooneyok13:05
sean-k-mooneyso we can likely update the api doc without "breaking" exisitng usecases 13:05
gibiyeah13:05
sean-k-mooneyi think i need to breing up the ponit that we shoudl be spending more time workin on plamcnet internally13:06
sean-k-mooneyagain13:06
gibiyeah13:07
gibiI feel the pain touching placement internals now as I lost most of the context and honestly the core placement devs are not with us any more13:07
sean-k-mooneyplacement is decptive. it mostly just works and is well tested, but there are sharp edges in places where we dont use a feature yet13:08
sean-k-mooneythe bits we use work great13:08
gibi... and the sql in the implementation is way more complex than what I can easily grok13:09
*** dasm|off is now known as dasm13:09
opendevreviewImran Hussain proposed openstack/nova master: [nova/libvirt] Support for checking and enabling SMM when needed  https://review.opendev.org/c/openstack/nova/+/82549613:17
opendevreviewBalazs Gibizer proposed openstack/placement master: Clarify trait filtering in the API doc  https://review.opendev.org/c/openstack/placement/+/82550113:28
gibisean-k-mooney: ^^ this is the API doc update13:28
gibiand thanks for talking it through with me13:32
sean-k-mooneyack no worries13:38
sean-k-mooneyi did not expect the behavior in  the story so its good to know to not get bitten by it13:38
sean-k-mooneyi have an downstream ironic edgecase that might be related to this by the way although i think they just forgot to zero out cpu ram and disk requests in the flavor13:39
sean-k-mooneythey do not have a resouce class associated with the ironic node13:40
sean-k-mooneyso they likely have no inventoies on teh RP13:40
sean-k-mooneythey are seing the request to palcment elimiate teh placment rps for the ironic nodes13:41
sean-k-mooneyim waiting for the db dumps and flavor details to confirm but thats also likely a docs bug in our downstream docs13:42
sean-k-mooneywe dont have the changes required for moderen ironic where it does not report cpu ram and disk inveories in our downstream docs13:42
stephenfingibi: bauzas: Think you folks could look at two large-ish outstanding doc reviews I have? They're good stuff, IMO https://review.opendev.org/c/openstack/nova/+/814562 https://review.opendev.org/c/openstack/nova/+/81456314:00
bauzasstephenfin: ack, on a meeting14:03
sean-k-mooneystephenfin: ill add them to my list14:09
sean-k-mooneystephenfin: i was also looking at your unittests mock patch14:09
sean-k-mooneyit woudl be nice to land that this cycle14:09
gibistephenfin: added to my list14:09
*** bhagyashris is now known as bhagyashris|ruck15:17
* bauzas disappears for 30 mins15:19
opendevreviewMerged openstack/nova master: [doc] propose Review-Priority label for contribs  https://review.opendev.org/c/openstack/nova/+/81686117:05
opendevreviewmelanie witt proposed openstack/nova stable/ussuri: Ensure MAC addresses characters are in the same case  https://review.opendev.org/c/openstack/nova/+/81768917:08
melwittlyarwood: astupnik updated https://review.opendev.org/c/openstack/nova/+/776250 awhile back in response to your comments, if you could take another look. if you won't be able to get to it, lmk17:19
sean-k-mooneyjust an fyi. im more or less going to be afk until tommrrow. im going to minimis my irc and email and try and get some development envs (ooo and devstack) deployed so i likely wont see any pings until tomorrow18:08
*** ministry is now known as __ministry20:20
*** artom__ is now known as artom20:47
opendevreviewmelanie witt proposed openstack/nova master: Add stub unified limits driver  https://review.opendev.org/c/openstack/nova/+/71213721:58
opendevreviewmelanie witt proposed openstack/nova master: Assert quota related API behavior when noop  https://review.opendev.org/c/openstack/nova/+/71214021:58
opendevreviewmelanie witt proposed openstack/nova master: Make unified limits APIs return reserved of 0  https://review.opendev.org/c/openstack/nova/+/71214121:58
opendevreviewmelanie witt proposed openstack/nova master: DNM Run against unmerged oslo.limit changes  https://review.opendev.org/c/openstack/nova/+/81223621:58
opendevreviewmelanie witt proposed openstack/nova master: Add logic to enforce local api and db limits  https://review.opendev.org/c/openstack/nova/+/71213921:58
opendevreviewmelanie witt proposed openstack/nova master: Enforce api and db limits  https://review.opendev.org/c/openstack/nova/+/71214221:58
opendevreviewmelanie witt proposed openstack/nova master: Update quota_class APIs for db and api limits  https://review.opendev.org/c/openstack/nova/+/71214321:58
opendevreviewmelanie witt proposed openstack/nova master: Update limit APIs  https://review.opendev.org/c/openstack/nova/+/71270721:58
opendevreviewmelanie witt proposed openstack/nova master: Update quota sets APIs  https://review.opendev.org/c/openstack/nova/+/71274921:58
opendevreviewmelanie witt proposed openstack/nova master: Tell oslo.limit how to count nova resources  https://review.opendev.org/c/openstack/nova/+/71330121:58
opendevreviewmelanie witt proposed openstack/nova master: Enforce resource limits using oslo.limit  https://review.opendev.org/c/openstack/nova/+/61518021:58
opendevreviewmelanie witt proposed openstack/nova master: Add legacy limits and usage to placement unified limits  https://review.opendev.org/c/openstack/nova/+/71349821:58
opendevreviewmelanie witt proposed openstack/nova master: Update quota apis with keystone limits and usage  https://review.opendev.org/c/openstack/nova/+/71349921:58
opendevreviewmelanie witt proposed openstack/nova master: Add reno for unified limits  https://review.opendev.org/c/openstack/nova/+/71527121:58
opendevreviewmelanie witt proposed openstack/nova master: Enable unified limits in the nova-next job  https://review.opendev.org/c/openstack/nova/+/78996321:58
*** dasm is now known as dasm|off23:13

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