| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2022-06-22 | |||
| 13:43:44 | bauzas | no need to have different calls then | |
| 13:43:52 | gibi | I'm OK with the kwargs way too | |
| 13:43:53 | bauzas | one single call but with optional args | |
| 13:44:17 | bauzas | gibi: well, as I said, this is the pattern we have in Nova for a while now :) | |
| 13:44:29 | bauzas | ie. default a param to a sentinel value | |
| 13:44:35 | bauzas | and call the method with kwargs | |
| 13:44:52 | bauzas | Uggla: ^ | |
| 13:45:43 | gibi | I can argue about how well established this as a pattern based on I'm not being aware of it. But I guess it is irrelevant. :) | |
| 13:45:59 | gibi | as I agree with the actual code | |
| 13:47:12 | bauzas | https://github.com/openstack/nova/blob/master/nova/scheduler/rpcapi.py#L152 | |
| 13:47:19 | bauzas | and https://github.com/openstack/nova/blob/master/nova/scheduler/manager.py#L145 | |
| 13:47:40 | bauzas | gibi: the problem with importing a global var from another module is about any possible circular import | |
| 13:47:52 | bauzas | if we don't need to import, let's not do it | |
| 13:48:14 | gibi | sure | |
| 13:48:17 | bauzas | also, the caller has to have some knowledge about the method | |
| 13:48:29 | gibi | as I said I agree with the code :) | |
| 13:48:34 | bauzas | this is a bad behaviour | |
| 13:48:41 | bauzas | gibi: sure, I just explain why | |
| 13:48:56 | bauzas | mostly not for you | |
| 13:49:00 | bauzas | but also for Uggla :) | |
| 13:49:45 | bauzas | so, yeah, unnecessary module import and internal knowledge of the called method are the two things we want to avoid | |
| 13:50:11 | bauzas | and since optional params in python methods are something easy to do, let's use this pattern | |
| 13:53:14 | Uggla | hum I think I understand. | |
| 13:53:27 | gibi | I think we use None a lot more to signal information-not-provided than a sentinel value. We only use dedicated sentinel if None means something else than information-not-provided | |
| 13:53:47 | gibi | and in this case None means something else than information-not-provide4d | |
| 13:54:25 | bauzas | gibi: correct | |
| 13:54:44 | bauzas | in general, we set to None unless None is used for a specific flag | |
| 13:54:52 | gibi | yepp | |
| 13:54:56 | bauzas | and if so, we use the sentinel pattern | |
| 13:55:49 | bauzas | but again, we create a specific instance of an object that we use, and we try to not expose this instance elsewhere | |
| 13:56:13 | bauzas | by instance, I mean a stored value in memory | |
| 14:05:30 | gibi | purely hypotetically and purely from the code understandability perspective I'm not in favor of the optionalness of a parameter on an API. In python you have to look up the signature of the called function to know if a parameter is optional (i.e. has a default value in the signature) but as soon as you looked that up you see the default value (the sentinel) so that default value already leaked to the | |
| 14:05:36 | gibi | caller side | |
| 14:06:35 | gibi | I would make all the parameters non defaulted and document what value of what parameter means what :D | |
| 14:06:54 | gibi | that is a bit more explict than param=sentinel in a signature | |
| 14:07:16 | gibi | but this is way less important that make the unshelve patch land :D | |
| 14:07:24 | gibi | s/that/than/ | |
| 14:07:42 | bauzas | that's why I explained why the pattern and also why I said in my last comment that docstrings help | |
| 14:08:12 | bauzas | I'd rather see a docstring saying (optional) my param | |
| 14:08:43 | bauzas | rather than asking to import a specific instance of a global class object | |
| 14:10:46 | gibi | the function itself is a global on the class an you import that to be able to call it | |
| 14:11:19 | gibi | None is an interpreter global :) | |
| 14:11:33 | gibi | (we are getting philosophycal ) | |
| 14:12:33 | gibi | we are using enums from the fields module those are class level fields too | |
| 14:26:49 | Uggla | gibi, bauzas, last stuff, ok with FIELD_SENTINEL wording ? | |
| 14:29:48 | gibi | go with what bauzas asked for I won't block on it | |
| 14:30:29 | bauzas | Uggla: gibi: you can even name it privatly | |
| 14:30:36 | bauzas | like _unset_field | |
| 14:30:45 | gibi | sure | |
| 14:30:48 | bauzas | or _unsel_field_sentinel | |
| 14:31:01 | bauzas | I don't wanna nitpick on the naming :) | |
| 14:31:22 | Uggla | naming one of the hardest stuff in computing. | |
| 14:31:30 | bauzas | but yeah, actually, since this is an internal object, make it private | |
| 14:31:43 | bauzas | (by convention of course) | |
| 14:32:27 | bauzas | https://twitter.com/codinghorror/status/506010907021828096?lang=fr | |
| 14:32:31 | bauzas | :) | |
| 14:32:44 | bauzas | (I like this tweet :p ) | |
| 14:33:38 | bauzas | https://www.karlton.org/2017/12/naming-things-hard/ for the wider context | |
| 17:11:44 | opendevreview | Amit Uniyal proposed openstack/nova master: Adds validation for hw machine type in host caps https://review.opendev.org/c/openstack/nova/+/847126 | |
| 19:45:42 | opendevreview | Amit Uniyal proposed openstack/nova master: Adds validation for hw machine type in host caps https://review.opendev.org/c/openstack/nova/+/847126 | |
| 20:38:09 | ade_lee | sean-k-mooney, sean-k-mooney[m] slaweq stephenfin hey - any idea what might be causing failures here? https://8767bac9cdd8c58da256-ee4c5d809145a8a3246cc4d26d65fbe0.ssl.cf5.rackcdn.com/831844/10/experimental/tempest-centos9-stream-fips/45be779/testr_results.html | |
| 20:41:16 | ade_lee | oh -- maybe thats this -- https://bugs.launchpad.net/neutron/+bug/1979047 | |
| #openstack-nova - 2022-06-23 | |||
| 05:28:12 | frickler | ade_lee: yes, that's the same failure pattern that caused us to make c9s non-voting in devstack | |
| 08:27:14 | bauzas | gibi: others, fwiw, I'll be on PTO tomorrow | |
| 08:27:24 | gibi | bauzas: o/ ack | |
| 09:15:51 | opendevreview | Amit Uniyal proposed openstack/nova master: Adds validation for hw machine type in host caps https://review.opendev.org/c/openstack/nova/+/847126 | |
| 10:06:59 | sean-k-mooney | ade_lee: assuming this is related to fips and centos 9 stream ya interface attach is broken due to an upstream libvirt bug which has a fix internally but is not relased yet | |
| 11:06:03 | opendevreview | Merged openstack/nova master: Change TooOldComputeService upgrade check code to failure https://review.opendev.org/c/openstack/nova/+/845262 | |
| 14:08:12 | opendevreview | Amit Uniyal proposed openstack/nova master: Adds validation for hw machine type in host caps https://review.opendev.org/c/openstack/nova/+/847126 | |
| 14:31:20 | amorin | hello nova! | |
| 14:34:55 | amorin | I was looking for an idea on how to schedule instances on computes based on some metrics that are not cpu related | |
| 14:35:04 | amorin | like, the kernel version, the operating system version | |
| 14:40:35 | amorin | i was thinking that I could tweak the compute monitors (that collect metrics) | |
| 14:40:40 | amorin | but I dont know if this is the best idea? | |
| 14:45:05 | mnaser | amorin: maybe you could use traits out of bands? | |
| 14:45:21 | mnaser | like have something that ships the traits of those boxes to placement | |
| 14:45:23 | amorin | out of bands? | |
| 14:45:32 | amorin | like a custom traits | |
| 14:45:44 | mnaser | yeah i meant out of band as in you wont be in the nova code path to determine this | |
| 14:45:46 | amorin | yes, I did try that, but it seems not available in the nova weighter | |
| 14:45:59 | mnaser | ah so you need this info in the weigher and not in the filter stage | |
| 14:46:20 | amorin | yes, because I dont want to filter out the hosts | |
| 14:46:28 | amorin | I just want them to be lower in the priority | |
| 14:47:30 | mnaser | ah okay so my suggestion wont help then | |
| 14:47:42 | amorin | unfortunately :( but thanks! | |
| 14:47:44 | gibi | placement can only be used today to do filtering decision, there is no weighing support there | |
| 14:47:54 | gibi | so you need a now weigher code for that | |
| 14:48:01 | gibi | so you need nova weigher code for that | |
| 14:48:02 | amorin | yup, and doing placement call from weigher seems a bad idea | |
| 14:50:46 | gibi | I dont think we have code for that in nova yet, you can try to abuse the MetricsWeigher | |
| 14:50:53 | gibi | but it feels hackish | |
| 14:51:50 | amorin | thats exactly what I am going to do, but I just wanted to know if something else, less hacky would exist | |
| 14:52:41 | gibi | I'm not aware of any less hackish way now | |
| 14:55:13 | amorin | ok, thanks gibi ! | |
| 15:00:36 | bauzas | amorin: I was trying to reply to you about your question :) | |
| 15:01:02 | bauzas | yeah, you should use weights | |
| 15:01:10 | bauzas | placement doesn't support them | |
| 15:10:21 | amorin | bauzas yeah! thanks | |
| 15:10:32 | amorin | you can still answer, maybe that would be nice for other people :) | |
| 15:10:44 | bauzas | amorin: sorry, I didn't have time to reply | |