Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2022-06-27
10:11:01 gibi that way we can avoid the sentinel
10:11:31 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:36 sean-k-mooney perhaps or wrap the parmters in a class
10:12:22 sean-k-mooney i would think that az_provided would be more readable
10:13:07 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:27 sean-k-mooney that better expresses the intent
11:37:58 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:40:36 bauzas Uggla: sean-k-mooney: gibi: sorry folks, I was cooking and lunching with my kid
11:42:00 bauzas sean-k-mooney: not sure I understand why you unlike the sentinel pattern
11:42:13 bauzas sean-k-mooney: and why you ask for a new parameter
11:42:29 sean-k-mooney bauzas: its very very error prone
11:42:38 bauzas ?
11:42:49 sean-k-mooney you need to keep the context that the vraiable is a union of a sential and the az
11:42:55 sean-k-mooney and everywhere you use it test for both
11:43:15 sean-k-mooney its very easy to miss that
11:43:17 bauzas sean-k-mooney: not if you provide a doc
11:43:24 sean-k-mooney no one reads docs
11:43:31 bauzas a doc comment I mean
11:43:36 bauzas like I said
11:43:47 bauzas :param az: (optional) blah
11:43:52 sean-k-mooney yep i hate those
11:44:29 bauzas and so, you prefer to have pypi parameters...
11:44:33 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:53 sean-k-mooney well thos are at least testable by mypy so yes
11:44:59 sean-k-mooney but that is missign the point
11:45:26 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:30 sean-k-mooney you are adding extra context that you need to keep in mind
11:45:49 bauzas sean-k-mooney: but we have done that since I'm working in Nova
11:46:01 sean-k-mooney in very limits places
11:46:16 sean-k-mooney i really dont want o see us doing this more in the code
11:46:17 bauzas lemme look at Uggla's change
11:49:01 bauzas sean-k-mooney: in general within RPC methods
11:49:18 bauzas so, I briefly looked at Uggla's code
11:49:47 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:50:07 bauzas and in https://review.opendev.org/c/openstack/nova/+/831507/14..16/nova/compute/api.py#4467 Uggla added docstrings
11:51:33 bauzas but I understand your point
11:51:40 bauzas maybe we could use PEP661 then https://peps.python.org/pep-0661/
11:52:21 bauzas and using the sentinels module
11:52:39 bauzas lemme see if py3.8 supports it
11:53:09 bauzas mmm, not stdlib
11:53:46 bauzas oh my bad, PEP661 is a drafy
11:53:50 bauzas draft*
11:54:19 sean-k-mooney bauzas: its not the call site
11:54:36 sean-k-mooney its the use site within the function i find distasteful
11:54:58 bauzas you mean, the caller or the called method ?
11:55:07 sean-k-mooney the use in the called method
11:55:14 bauzas the if conditional ?
11:55:18 sean-k-mooney yes
11:55:21 bauzas hah
11:55:31 sean-k-mooney the fact you have to check the sential with an is check explictly
11:55:39 bauzas well, then we could have one single conditional that would set a value or not
11:55:55 sean-k-mooney right which is what i actully was askign for
11:56:09 bauzas would you then prefer something like
11:56:13 sean-k-mooney i didnt leave review feedback yet because i was in the midel of other thngs
11:56:48 sean-k-mooney bauzas: also https://peps.python.org/pep-0661/ is basically the same as cpp's std:optional
11:57:00 sean-k-mooney at least in inteded usage
11:57:05 sean-k-mooney so yes i would prefer that
11:57:36 sean-k-mooney that said i dont se how its used
11:58:13 bauzas sean-k-mooney: https://paste.opendev.org/show/bLmJ5uKGsufcYjXAIda3/
11:58:34 bauzas would you prefer this pattern ?
11:59:03 bauzas we only set an internal value if the parameter was provided
11:59:21 bauzas so we only check the sentinel value once at the top of the method
11:59:31 sean-k-mooney not quite
11:59:41 sean-k-mooney the problem with that is we can refernce undeined varbles
12:00:08 sean-k-mooney that actully a error in any path where new_az is not _sentinel
12:00:36 bauzas well, the reference to _sentinel is unique
12:00:49 bauzas you can check its id
12:01:21 bauzas that's why this pattern exists in Python to verify whether this parameter was called
12:01:33 sean-k-mooney https://paste.opendev.org/show/bqHsJAFVEGmRncJcERMd/
12:01:41 bauzas as, if the id of the value explicitly matches the unpassed
12:02:05 sean-k-mooney if _new_az or host: in your version is a runtime error
12:02:23 bauzas sean-k-mooney: true
12:02:30 bauzas I like your counterproposal
12:03:01 sean-k-mooney i just like have a name for the concept that we are modeling
12:03:11 sean-k-mooney and abstracting how it implemtned via the name
12:03:14 bauzas of course, "az_passed == new_az is not _sentinel" without the trailing double-dot :)
12:03:29 bauzas and with a single equal
12:03:36 bauzas but I got your idea
12:04:03 bauzas you create an always-set internal reference for knowing whether this field was set or not
12:04:15 bauzas this is a good pattern
12:04:23 bauzas and only at the top of the method
12:04:31 sean-k-mooney yep
12:04:51 sean-k-mooney you check once and define a varible at the top of the method with a meaningful name
12:05:02 sean-k-mooney and then use that later so you dont need to keep the context loaded
12:05:02 bauzas wfm
12:05:12 bauzas Uggla: ^
12:05:29 bauzas Uggla: tl;dr follow the pattern proposed by https://paste.opendev.org/show/bqHsJAFVEGmRncJcERMd/
12:05:51 bauzas but modify the first line by "az_passed = new_az is not _sentinel
12:27:52 Uggla bauzas, ok
12:44:22 gibi Uggla: fyi I'm in the middle of reviewing your patches so you will get feedback from me soon too
12:45:05 Uggla gibi, a lot of new ones ?
12:46:15 gibi Uggla: couple of nits in the code and a list of suggestions in the functional tests
12:46:45 Uggla gibi, ok I'm gonna wait for your comments.
13:00:28 gibi Uggla: posted my comments
13:00:39 Uggla gibi, ok thx
14:01:23 opendevreview Jan Hartkopf proposed openstack/nova master: add support for updating server's user_data https://review.opendev.org/c/openstack/nova/+/816157
14:03:03 opendevreview Jan Hartkopf proposed openstack/python-novaclient master: add support for microversion 2.91 https://review.opendev.org/c/openstack/python-novaclient/+/816158
16:29:41 opendevreview Jan Hartkopf proposed openstack/nova master: add support for updating server's user_data https://review.opendev.org/c/openstack/nova/+/816157
19:55:11 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.
#openstack-nova - 2022-06-28
04:58:08 manuvakery1 Hi .. if i take an instance snapshot after resizing to a higher flavor the min_disk property it set to the older flavor disk size. Is this a known issue in train?

Earlier   Later