| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2023-11-08 | |||
| 11:21:51 | sean-k-mooney | dvo-plv: like this https://github.com/openstack/nova/blob/master/nova/scheduler/utils.py#L288-L304 | |
| 11:22:13 | sean-k-mooney | dvo-plv: it has the same effect | |
| 11:22:37 | sean-k-mooney | dvo-plv: the only reason to prefer a prefiler over a translate function like that is if you want ot make it configurable | |
| 11:23:02 | bauzas | right | |
| 11:23:06 | sean-k-mooney | dvo-plv: in your case we dont need or want it to be configurable (its not in your patch) | |
| 11:23:08 | bauzas | you can opt-out | |
| 11:23:23 | sean-k-mooney | but we have mandaotry pre-filters too | |
| 11:23:28 | bauzas | (or opt-in, depending on the default value :) ) | |
| 11:23:29 | sean-k-mooney | so either approch works | |
| 11:23:56 | bauzas | actually | |
| 11:24:19 | bauzas | this sounds terrible for interop if we make those image properties be config-driven, right? | |
| 11:24:20 | sean-k-mooney | dvo-plv: i think we get slightly better logging in general with the prefilter but its basically the same | |
| 11:24:34 | sean-k-mooney | bauzas: it woudl be but its not | |
| 11:24:43 | sean-k-mooney | triat help with interup | |
| 11:24:57 | sean-k-mooney | but only if you have access to placement | |
| 11:25:01 | bauzas | well, if you disable the prefilter, then your property is useless, right? | |
| 11:25:16 | sean-k-mooney | bauzas: the prefilter cant be disabeld | |
| 11:25:36 | bauzas | in the current code, yeah | |
| 11:25:45 | bauzas | but in general, we have prefilters that are config-driven | |
| 11:26:02 | sean-k-mooney | yes but those are genrelaly not related to extra specs | |
| 11:26:18 | bauzas | sure, but that's again errorprone | |
| 11:26:27 | dvo-plv | okay, great that you are on the same page now and all left the friends :) So, I will resolve comments asap, than kyou again | |
| 11:26:48 | sean-k-mooney | bauzas: over time i would expect use to make some prefilters that you can currently opt out of mandatory | |
| 11:26:53 | bauzas | some day we may want to have some configurable prefilter that would do property translation | |
| 11:27:03 | sean-k-mooney | i recently did that for the az prefilter when i remvoed teh az filter | |
| 11:27:51 | sean-k-mooney | i could see use makign the require_image_type_support or transform_image_metadata required at some point | |
| 11:28:21 | sean-k-mooney | bauzas: actully looking at the code more are required then optional today | |
| 11:28:50 | bauzas | yeah fortunately | |
| 11:29:05 | sean-k-mooney | bauzas: we have for image properties already | |
| 11:29:23 | sean-k-mooney | i wrote a genric one to work with all image properties that have corresponding triats | |
| 11:29:36 | sean-k-mooney | we just need to extend that as it make sense | |
| 11:29:58 | sean-k-mooney | i didnt do that for flavors mainly because there are less extra spec that map to triat | |
| 11:30:11 | sean-k-mooney | but reflectign on that it might make sense too revisit that | |
| 11:30:35 | sean-k-mooney | there is likely some consoliation i coudl to with low amoutn of work | |
| 11:31:00 | sean-k-mooney | do you think that woudl be valuable. i just havent felt the effort was worth it previously | |
| 11:31:39 | bauzas | that's a very good question | |
| 11:31:55 | sean-k-mooney | prefilters/translaotr tend to be a single short funciton that is easy to test in isolation | |
| 11:32:13 | sean-k-mooney | combining them loose some of that simplicty depending on how you approch it | |
| 11:32:32 | bauzas | I'm still afraid by the imagination of our lovely operators and I want us to draw a solid yellow line in terms of support, since I don't wanna buy some trait forever | |
| 11:33:07 | bauzas | so, any forgery hiding traits sounds preferable | |
| 11:33:22 | sean-k-mooney | i think its fair to say that if you have very complex resouce and traits request in flavor that opens you up to some upgrade impacts that you woudl otehrwise not be exposed too | |
| 11:33:30 | bauzas | whether we should factorize the forgeries into one piece is debatable | |
| 11:33:50 | sean-k-mooney | perhasp something to consier in the future | |
| 11:34:00 | sean-k-mooney | but i dint think its required right now | |
| 11:34:06 | bauzas | yeah | |
| 11:34:54 | bauzas | also, have we ever tried to sanitize os-traits before ? | |
| 11:35:49 | bauzas | sounds now nearly impossible to me | |
| 11:36:13 | sean-k-mooney | we are not allowed to remove traits form os-traits | |
| 11:36:17 | sean-k-mooney | ever | |
| 11:36:24 | sean-k-mooney | placement does not support that today | |
| 11:36:29 | bauzas | I reached to that conclusion | |
| 11:36:55 | sean-k-mooney | it could be done with some mettadata but basically we need to resver the triat ids in the db | |
| 11:40:23 | bauzas | anyway, I need to deserve time for lunch and writing my own spec | |
| 11:56:33 | opendevreview | Merged openstack/nova-specs master: Add libvirt-dev-alias spec https://review.opendev.org/c/openstack/nova-specs/+/892806 | |
| 12:19:52 | opendevreview | sean mooney proposed openstack/nova master: add pyproject.toml to support pip 23.1 https://review.opendev.org/c/openstack/nova/+/899753 | |
| 12:22:06 | Continuity | hey all, I have submitted a PR (https://review.opendev.org/c/openstack/nova/+/900252) and I'm struggling to understand and fix the build errors. Mostly around openstack-tox-cover, openstack-tox-py38 and nova-tox-py310-with-sqlalchemy-2x. It seems to be a case of, works fine on my local machine :D | |
| 12:22:12 | Continuity | anyone have any pointers | |
| 12:28:46 | sean-k-mooney | Continuity: the pep8 failures look valid https://zuul.opendev.org/t/openstack/build/914ce175822a4ada8271b3d5ae9d6877/log/job-output.txt#4269-4282 | |
| 12:30:11 | sean-k-mooney | Continuity: we will also need a bug or specless blueprint to proceed with this feature | |
| 12:31:06 | sean-k-mooney | the unit test failrue also look valid https://storage.bhs.cloud.ovh.net/v1/AUTH_dcaab5e32b234d56b626f72581e3644c/zuul_opendev_logs_3b1/900252/2/check/openstack-tox-py38/3b1dc22/testr_results.html | |
| 12:31:42 | sean-k-mooney | requests.exceptions.MissingSchema: Invalid URL 'POST': No scheme supplied. Perhaps you meant https://POST? | |
| 12:33:19 | Continuity | sean-k-mooney: yeah I agree they look correct, thats the issue, when I run the tox commands locally they pass :S | |
| 12:33:22 | sean-k-mooney | Continuity: the fact you are gettign fewer error in 3.10 vs 3.8 is concerning | |
| 12:33:34 | Continuity | and yes i need to drop a BP in as well | |
| 12:33:41 | sean-k-mooney | as it imply your relaying on a behaivor in requests that is diffent between the two versions | |
| 12:34:07 | Continuity | not intentionally, but its possible. I will dig a litter deeper | |
| 12:35:23 | sean-k-mooney | in general in not sure how i feel about adding http basic auth to this feature | |
| 12:35:38 | sean-k-mooney | escially since we have not tempest testing of this at all | |
| 12:36:22 | sean-k-mooney | Continuity: we will have to see how others feel about that and if a spec is required and ci coverave beyond unit test. i woudl like to see at least some functionl coverate for this i think | |
| 12:36:38 | sean-k-mooney | but maybe even devstack/tempest testing | |
| 12:37:26 | Continuity | fair enough, basic auth felt like a good step between having to shoehorn keystoneauth into and endpoint that may already exist and nothing. | |
| 12:37:38 | sean-k-mooney | basically devstack woudl jsut have to stand up a fake webservice with http_auth configured that returns some static data to show the auth actully works | |
| 12:37:49 | Continuity | yeah i can deffo take a look at that | |
| 12:38:12 | Continuity | let me get a BP created at least then we can invite conversation.. | |
| 12:38:37 | sean-k-mooney | sure. im not agaisnt basic auth in general but i dont want to see us supproting more auth protocl in the future | |
| 12:38:44 | sean-k-mooney | i.e. oath or saml tokens | |
| 12:38:59 | sean-k-mooney | if we needed anything more advanced then you shoudl use keystone | |
| 12:39:13 | Continuity | yeah | |
| 12:39:15 | Continuity | i agree | |
| 12:39:37 | Continuity | it would get massively messy otherwise. | |
| 12:40:52 | sean-k-mooney | are you missign POST here https://review.opendev.org/c/openstack/nova/+/900252/3/nova/api/metadata/vendordata_dynamic.py#116 | |
| 12:41:24 | sean-k-mooney | oh no | |
| 12:41:32 | sean-k-mooney | your using session.post | |
| 12:42:04 | sean-k-mooney | is there a reason your not using res = self.session.request(url, 'POST', | |
| 12:42:40 | sean-k-mooney | ah sessionis a difent object | |
| 12:42:56 | sean-k-mooney | dependin on if you use _load_ks_session or _load_request_session | |
| 12:43:47 | sean-k-mooney | one is using a keystone auth client vs a requests client directly | |
| 12:48:14 | Continuity | yeah | |
| 12:48:19 | Continuity | might be the wrong approach ... | |
| 12:48:45 | Continuity | i know that that ks_sessions is built ontop of requests, but had issues picking it apart | |
| 12:48:53 | sean-k-mooney | it is yes | |
| 12:48:56 | Continuity | and was trying to not muddy the two | |
| 12:49:05 | sean-k-mooney | using request directly is fine | |
| 12:49:15 | sean-k-mooney | but can you try and use the same method assumign it exits | |
| 12:49:19 | sean-k-mooney | i commented on the patch | |
| 12:49:31 | sean-k-mooney | you are not mockign the funciton that you are calling in the unit test correctly | |
| 12:49:43 | sean-k-mooney | you are using session.post but mocking session.request | |
| 12:50:04 | sean-k-mooney | if we can just use session.request in both cases that woudl be preferable | |
| 12:50:15 | sean-k-mooney | or session.post in both cases | |
| 12:55:30 | Continuity | right, thanks | |