| bauzas | good morning Nova | 07:15 |
|---|---|---|
| gibi | o/ | 07:24 |
| bauzas | one day off and a lot of emails to triager | 07:35 |
| gibi | frickler: re https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1013410 we have an upstream bug for that https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1013410 and it is being fixed by https://review.opendev.org/c/openstack/nova/+/845922 | 08:18 |
| gibi | bah, I mean upstream bug https://bugs.launchpad.net/nova/+bug/1978817 | 08:18 |
| gibi | colby_: I think we saw similar issues before but I don't find the relevan bug report now I suggest to ping bauzas as he might have more context with vgpus/mdevs | 08:26 |
| bauzas | context ? | 08:26 |
| gibi | bauzas: 19:49 < colby_> Hello everyone. we are experienceing a strange but with vGPU. We are using Virtoria on Centos 8 Stream with A40 gpus in SRIOV setup. Everything works correctly except when we delete an instance. It does not seem to release the virtualfunction mdev device. I can manually release them by echoing to the remove method of the mdev. So its like nova is not doing that. | 08:27 |
| gibi | bauzas: that was from friday | 08:27 |
| frickler | gibi: ah, cool, thx for the pointer | 08:30 |
| gibi | frickler: I replied in the debian tracker too | 08:30 |
| bauzas | gibi: colby_: yes we don't delete the mdevs | 08:32 |
| bauzas | 4 years ago, I thought it was not needed | 08:33 |
| Uggla | Hello, bauzas, gibi, sean-k-mooney https://review.opendev.org/c/openstack/nova/+831507, I have fixed latest comments from bauzas and I have verified it works fine with devstack. Can you go ahead with a new review round ? | 09:30 |
| sean-k-mooney | gibi: we are tarcking that as a downstream bug | 09:31 |
| sean-k-mooney | im suggesting that we shoudl free the mdev when we delete the vm | 09:31 |
| sean-k-mooney | we could resue the mdev on the next vm creation but | 09:31 |
| sean-k-mooney | it think that will be problematic if we start using mdevs more dynamicaly in the future | 09:32 |
| sean-k-mooney | it would work for our current usecase but i would prefer to only allocate mdevs if they are in use | 09:32 |
| sean-k-mooney | Uggla: gerrit says no | 09:33 |
| sean-k-mooney | Uggla: specifically it give a 404 | 09:33 |
| sean-k-mooney | and a | 09:33 |
| sean-k-mooney | ¯\_(ツ)_/¯ | 09:33 |
| Uggla | https://review.opendev.org/c/openstack/nova-specs/+/833669 | 09:33 |
| sean-k-mooney | that works fine | 09:34 |
| * Uggla a bit tired today. Forget about the above links. | 09:34 | |
| Uggla | Here is the good one: https://review.opendev.org/c/openstack/nova/+/831507 | 09:35 |
| Uggla | sean-k-mooney, please look at the link ^ | 09:36 |
| sean-k-mooney | ah it was miising a / | 09:38 |
| sean-k-mooney | so looking a the spec you are just missing the parmater to pass to the grant api to resolve the locking issue | 09:39 |
| sean-k-mooney | for manila | 09:39 |
| sean-k-mooney | im not a huge fan of _unset_field_sentinel but i guess that works | 09:41 |
| Uggla | sean-k-mooney, regarding manila yes. I have reviewed the spec locally, I will push it ASAP. Before the 5th hopping it will be approved and merged. | 09:47 |
| Uggla | sean-k-mooney, fyi _unset_field_sentinel was proposed and agreed from gibi and bauzas, so I used that as an humble padawan. :) | 09:51 |
| gibi | sean-k-mooney: about the sentinel, do you see a viable alternative? | 09:51 |
| sean-k-mooney | right i just dont like how pervaisve the check is | 09:51 |
| sean-k-mooney | gibi: nothing substaitally differnt i proably would have made it a module level constant and uppercase it rather then a class variable | 09:52 |
| sean-k-mooney | and i might have done the test once and assigned it to a varible at the top of the function | 09:53 |
| sean-k-mooney | no_az = new_az == AZ_SENTINAL | 09:53 |
| gibi | sean-k-mooney: I have no hard opinion abou the location, could be on module level too. | 09:53 |
| sean-k-mooney | if not no_az and new_az: ... | 09:53 |
| sean-k-mooney | well its just a nit | 09:54 |
| sean-k-mooney | as i said it does not really change anything | 09:54 |
| gibi | on the test side, I agree, that could be factored out | 09:54 |
| sean-k-mooney | just stypictlly i like constnats to eb UPPERCASE_WITH_UNDERSCORES | 09:54 |
| sean-k-mooney | *stylisticly | 09:55 |
| sean-k-mooney | Uggla: so realisticaly you do not need to change anything it just not an established pattern in our code base to have sentenial like this so it looks odd because its different then our normal pattern | 09:56 |
| gibi | bauzas: ^^ look sean-k-mooney also feels it is not an established pattern :D | 09:57 |
| gibi | sean-k-mooney: context: we had a bit of back and forth with bauzas around the sentinel | 09:57 |
| sean-k-mooney | we dont currntly have the equivalent of std:optional in python/nova | 09:58 |
| sean-k-mooney | i would kind of perfer to have that class but i know other recoile at c++ conventions | 09:59 |
| gibi | bauzas pointed out that the sentinel + kwargs is widely used in the nova rpc apis | 09:59 |
| sean-k-mooney | we use none as the sentinal there no? | 09:59 |
| sean-k-mooney | i dont really like that the custom sentinal we use is not "truthy" so we can just use if directly | 10:00 |
| gibi | I think we use kwargs not to send a value even | 10:00 |
| sean-k-mooney | we use None however as the sentinal https://github.com/openstack/nova/blob/c53ec4e48884235566962bc934cbf292ad5b67b8/nova/compute/manager.py#L11017-L11019= | 10:02 |
| sean-k-mooney | so what Uggla is doing is very differnt | 10:03 |
| sean-k-mooney | the use of None as a sential for kwargs is idiomatic python | 10:04 |
| Uggla | sean-k-mooney, fyi bauzas provided this as a sentinel example: https://github.com/openstack/nova/blob/master/nova/scheduler/rpcapi.py#L152 and https://github.com/openstack/nova/blob/master/nova/scheduler/manager.py#L145 so I try to respect that pattern. | 10:04 |
| sean-k-mooney | i see | 10:05 |
| sean-k-mooney | i still dont like that | 10:05 |
| Uggla | :) | 10:05 |
| sean-k-mooney | its kind of like raw gotos | 10:05 |
| sean-k-mooney | we have if while ectra for a reason | 10:06 |
| gibi | we cannot use None in the current case as None has a different meaning than the missing param | 10:06 |
| gibi | as we want to be able to signal either unpin or no-change | 10:07 |
| sean-k-mooney | yes im aware | 10:07 |
| gibi | so we needed an extra value | 10:07 |
| sean-k-mooney | yep that is what std:optional does in c++ | 10:07 |
| sean-k-mooney | it give you a value that is outside the normal set | 10:08 |
| sean-k-mooney | same with ovo | 10:08 |
| sean-k-mooney | the field have an addtional unset value | 10:08 |
| gibi | yeah, I don't like the ovo way of missing fields, as that breaks the class invariant for me | 10:08 |
| sean-k-mooney | well i prefer it to this | 10:08 |
| sean-k-mooney | the ovo way at least work with in and is encapulated | 10:09 |
| gibi | I create an ovo class with predefined fields, and then I have to check at each access if the field exists, that seem against the fact that we predefine the fields | 10:10 |
| sean-k-mooney | i dont like that we have to rememeber that this is not just an az its a union of an az and a sentinal | 10:10 |
| gibi | anyhow I go back to review the unshelve patch | 10:10 |
| gibi | sean-k-mooney: so you would add an extra param to the call instead? like az_provided ? | 10:10 |
| gibi | that way we can avoid the sentinel | 10:11 |
| gibi | new_az can be None to unpin or a valid AZ name, and az_provided boolean can be set if no AZ field is provided in the REST request | 10:11 |
| sean-k-mooney | perhaps or wrap the parmters in a class | 10:11 |
| sean-k-mooney | i would think that az_provided would be more readable | 10:12 |
| sean-k-mooney | so ideally even if we had a sentinil i would like use to do that test once at the top of the function and defien a local az_provided varible | 10:13 |
| sean-k-mooney | that better expresses the intent | 10:13 |
| opendevreview | Amit Uniyal proposed openstack/nova master: Adds validation for hw machine type in host caps https://review.opendev.org/c/openstack/nova/+/847126 | 11:37 |
| bauzas | Uggla: sean-k-mooney: gibi: sorry folks, I was cooking and lunching with my kid | 11:40 |
| bauzas | sean-k-mooney: not sure I understand why you unlike the sentinel pattern | 11:42 |
| bauzas | sean-k-mooney: and why you ask for a new parameter | 11:42 |
| sean-k-mooney | bauzas: its very very error prone | 11:42 |
| bauzas | ? | 11:42 |
| sean-k-mooney | you need to keep the context that the vraiable is a union of a sential and the az | 11:42 |
| sean-k-mooney | and everywhere you use it test for both | 11:42 |
| sean-k-mooney | its very easy to miss that | 11:43 |
| bauzas | sean-k-mooney: not if you provide a doc | 11:43 |
| sean-k-mooney | no one reads docs | 11:43 |
| bauzas | a doc comment I mean | 11:43 |
| bauzas | like I said | 11:43 |
| bauzas | :param az: (optional) blah | 11:43 |
| sean-k-mooney | yep i hate those | 11:43 |
| bauzas | and so, you prefer to have pypi parameters... | 11:44 |
| sean-k-mooney | if we have a sential i would prefer use to do the test once at the top of the function and never refernce the centinal again | 11:44 |
| sean-k-mooney | well thos are at least testable by mypy so yes | 11:44 |
| sean-k-mooney | but that is missign the point | 11:44 |
| bauzas | sean-k-mooney: well, if we have two tests, one for verifying a sepific value for new_az and another one for verifying to not use the parameter, I wouldn't see why it could be a problem | 11:45 |
| sean-k-mooney | you are adding extra context that you need to keep in mind | 11:45 |
| bauzas | sean-k-mooney: but we have done that since I'm working in Nova | 11:45 |
| sean-k-mooney | in very limits places | 11:46 |
| sean-k-mooney | i really dont want o see us doing this more in the code | 11:46 |
| bauzas | lemme look at Uggla's change | 11:46 |
| bauzas | sean-k-mooney: in general within RPC methods | 11:49 |
| bauzas | so, I briefly looked at Uggla's code | 11:49 |
| bauzas | I don't see why people wouldn't understand this is an optional parameter when looking at the call https://review.opendev.org/c/openstack/nova/+/831507/14..16/nova/api/openstack/compute/shelve.py#106 | 11:49 |
| bauzas | and in https://review.opendev.org/c/openstack/nova/+/831507/14..16/nova/compute/api.py#4467 Uggla added docstrings | 11:50 |
| bauzas | but I understand your point | 11:51 |
| bauzas | maybe we could use PEP661 then https://peps.python.org/pep-0661/ | 11:51 |
| bauzas | and using the sentinels module | 11:52 |
| bauzas | lemme see if py3.8 supports it | 11:52 |
| bauzas | mmm, not stdlib | 11:53 |
| bauzas | oh my bad, PEP661 is a drafy | 11:53 |
| bauzas | draft* | 11:53 |
| * bauzas facepalms | 11:53 | |
| sean-k-mooney | bauzas: its not the call site | 11:54 |
| sean-k-mooney | its the use site within the function i find distasteful | 11:54 |
| bauzas | you mean, the caller or the called method ? | 11:54 |
| sean-k-mooney | the use in the called method | 11:55 |
| bauzas | the if conditional ? | 11:55 |
| sean-k-mooney | yes | 11:55 |
| bauzas | hah | 11:55 |
| sean-k-mooney | the fact you have to check the sential with an is check explictly | 11:55 |
| bauzas | well, then we could have one single conditional that would set a value or not | 11:55 |
| sean-k-mooney | right which is what i actully was askign for | 11:55 |
| bauzas | would you then prefer something like | 11:56 |
| sean-k-mooney | i didnt leave review feedback yet because i was in the midel of other thngs | 11:56 |
| sean-k-mooney | bauzas: also https://peps.python.org/pep-0661/ is basically the same as cpp's std:optional | 11:56 |
| sean-k-mooney | at least in inteded usage | 11:57 |
| sean-k-mooney | so yes i would prefer that | 11:57 |
| sean-k-mooney | that said i dont se how its used | 11:57 |
| bauzas | sean-k-mooney: https://paste.opendev.org/show/bLmJ5uKGsufcYjXAIda3/ | 11:58 |
| bauzas | would you prefer this pattern ? | 11:58 |
| bauzas | we only set an internal value if the parameter was provided | 11:59 |
| bauzas | so we only check the sentinel value once at the top of the method | 11:59 |
| sean-k-mooney | not quite | 11:59 |
| sean-k-mooney | the problem with that is we can refernce undeined varbles | 11:59 |
| sean-k-mooney | that actully a error in any path where new_az is not _sentinel | 12:00 |
| bauzas | well, the reference to _sentinel is unique | 12:00 |
| bauzas | you can check its id | 12:00 |
| bauzas | that's why this pattern exists in Python to verify whether this parameter was called | 12:01 |
| sean-k-mooney | https://paste.opendev.org/show/bqHsJAFVEGmRncJcERMd/ | 12:01 |
| bauzas | as, if the id of the value explicitly matches the unpassed | 12:01 |
| sean-k-mooney | if _new_az or host: in your version is a runtime error | 12:02 |
| bauzas | sean-k-mooney: true | 12:02 |
| bauzas | I like your counterproposal | 12:02 |
| sean-k-mooney | i just like have a name for the concept that we are modeling | 12:03 |
| sean-k-mooney | and abstracting how it implemtned via the name | 12:03 |
| bauzas | of course, "az_passed == new_az is not _sentinel" without the trailing double-dot :) | 12:03 |
| bauzas | and with a single equal | 12:03 |
| bauzas | but I got your idea | 12:03 |
| bauzas | you create an always-set internal reference for knowing whether this field was set or not | 12:04 |
| bauzas | this is a good pattern | 12:04 |
| bauzas | and only at the top of the method | 12:04 |
| sean-k-mooney | yep | 12:04 |
| sean-k-mooney | you check once and define a varible at the top of the method with a meaningful name | 12:04 |
| sean-k-mooney | and then use that later so you dont need to keep the context loaded | 12:05 |
| bauzas | wfm | 12:05 |
| bauzas | Uggla: ^ | 12:05 |
| bauzas | Uggla: tl;dr follow the pattern proposed by https://paste.opendev.org/show/bqHsJAFVEGmRncJcERMd/ | 12:05 |
| bauzas | but modify the first line by "az_passed = new_az is not _sentinel | 12:05 |
| Uggla | bauzas, ok | 12:27 |
| gibi | Uggla: fyi I'm in the middle of reviewing your patches so you will get feedback from me soon too | 12:44 |
| Uggla | gibi, a lot of new ones ? | 12:45 |
| gibi | Uggla: couple of nits in the code and a list of suggestions in the functional tests | 12:46 |
| Uggla | gibi, ok I'm gonna wait for your comments. | 12:46 |
| gibi | Uggla: posted my comments | 13:00 |
| Uggla | gibi, ok thx | 13:00 |
| opendevreview | Jan Hartkopf proposed openstack/nova master: add support for updating server's user_data https://review.opendev.org/c/openstack/nova/+/816157 | 14:01 |
| opendevreview | Jan Hartkopf proposed openstack/python-novaclient master: add support for microversion 2.91 https://review.opendev.org/c/openstack/python-novaclient/+/816158 | 14:03 |
| *** diablo_rojo__ is now known as diablo_rojo | 16:22 | |
| opendevreview | Jan Hartkopf proposed openstack/nova master: add support for updating server's user_data https://review.opendev.org/c/openstack/nova/+/816157 | 16:29 |
| *** diablo_rojo is now known as Guest3434 | 17:08 | |
| *** Guest3434 is now known as diablo_rojo | 18:03 | |
| *** diablo_rojo__ is now known as diablo_rojo | 19:25 | |
| colby__ | gibi: bauzas: sean-k-mooney: Thanks for the info. If the mdevs are not removed to be reused why would nova see them as being used then? Is that the bug mentioned? We have dynamic mdevs made from the sriov-manage on the pgpus that all work on initial spin up of instances, but once you delete the instances you can no longer use the vgpu device. Its still seen as busy. | 19:55 |
| *** dasm is now known as dasm|off | 22:16 | |
| *** hemna0 is now known as hemna | 23:38 | |
Generated by irclog2html.py 2.17.3 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!