| opendevreview | OpenStack Proposal Bot proposed openstack/nova master: Imported Translations from Zanata https://review.opendev.org/c/openstack/nova/+/846876 | 03:52 |
|---|---|---|
| opendevreview | Amit Uniyal proposed openstack/nova master: Added validation for hw machine type in host capabilities https://review.opendev.org/c/openstack/nova/+/847126 | 06:09 |
| *** vishalmanchanda_ is now known as vishalmanchanda | 06:12 | |
| *** rpioso_ is now known as rpioso | 06:12 | |
| *** carloss_ is now known as carloss | 06:12 | |
| *** arne_wiebalck_ is now known as arne_wiebalck | 06:19 | |
| opendevreview | Amit Uniyal proposed openstack/nova master: Added validation for hw machine type in host capabilities https://review.opendev.org/c/openstack/nova/+/847126 | 06:20 |
| bauzas | good morning Nova | 07:03 |
| gibi | bauzas: o/ | 07:06 |
| bauzas | gibi: thanks for having written the email about the PCI series | 07:06 |
| bauzas | gibi: today is review day for me | 07:07 |
| gibi | bauzas: I felt I needed to give some guide to encourage reviewers | 07:07 |
| bauzas | :) | 07:10 |
| bauzas | I guess we will be only a few of them | 07:10 |
| gibi | bauzas: I have to respin the series due to a doc issue | 07:10 |
| gibi | just I want to finish the downstream backport review first | 07:10 |
| bauzas | gibi: ok, no worries, I'll start with Uggla series | 07:10 |
| bauzas | after a coffee... | 07:11 |
| gibi | sure | 07:11 |
| bauzas | Uggla: voted on https://review.opendev.org/c/openstack/nova/+/831507/14 | 08:37 |
| bauzas | Uggla: tl;dr: yeah IMHO you could squash your patch with gibi's one but I'd also want you to modify how you use the sentinel | 08:38 |
| Uggla | bauzas, ok I'm going to have a look and propose an updated version ASAP. | 08:40 |
| bauzas | Uggla: thanks | 08:40 |
| bauzas | with gibi's patch and just the thing I ask you, I don't think this is massive change | 08:40 |
| bauzas | Uggla: one thing I also wrote, please bear with logs | 08:40 |
| bauzas | Uggla: the point is, logs are very well appreciated, but large public clouds tend to disagree with us adding more logs :) | 08:41 |
| bauzas | in particular when you can get information from other services, like scheduler | 08:42 |
| Uggla | bauzas, oh ok. | 08:42 |
| bauzas | no worries, this is something not largely known | 08:42 |
| bauzas | but yeah, logs rotation are costly for them | 08:42 |
| bauzas | DEBUG level is a bit different tho | 08:43 |
| bauzas | but with INFO, we need to be cautious | 08:43 |
| bauzas | I'll then switch to your Manila spec | 08:43 |
| bauzas | the sooner we have it, the earlier we could merge the os-traits change | 08:44 |
| Uggla | bauzas, anyway thx for the review | 08:46 |
| bauzas | np pb, apologies this was that late | 08:47 |
| bauzas | Uggla: I briefly reviewed your REST change, nothing controversial appeared | 08:47 |
| bauzas | but I didn't vote | 08:47 |
| Uggla | bauzas, regarding the manila one, Goutham sent comments yesterday. | 08:48 |
| bauzas | nice | 08:49 |
| Uggla | bauzas, remaining blocking point is the "extra spec / trait" if we want to have this explicit or not. I would rather explicit one. But we need a consensus here. | 08:51 |
| Uggla | I would like to discuss this point with bauzas, gibi, sean-k-mooney. | 08:51 |
| gibi | I'm around | 08:52 |
| Uggla | gibi, o/ | 08:53 |
| gibi | o/ | 08:54 |
| bauzas | Uggla: gibi: give me 30 mins so I can recharge the context by reviewing the spec :) | 08:58 |
| gibi | bauzas, Uggla: do we want to wait for sean-k-mooney too? | 08:58 |
| bauzas | gibi: probably, again I need to review the spec | 08:59 |
| gibi | sure go ahead an review | 08:59 |
| Uggla | gibi, if possible yes. | 08:59 |
| bauzas | gibi: Uggla: we could do some session by 14:30 our time if needed | 09:00 |
| opendevreview | Balazs Gibizer proposed openstack/nova master: Extend device_spec with resource_class and traits https://review.opendev.org/c/openstack/nova/+/846218 | 09:10 |
| opendevreview | Balazs Gibizer proposed openstack/nova master: Reject PCI dependent device config https://review.opendev.org/c/openstack/nova/+/846435 | 09:10 |
| opendevreview | Balazs Gibizer proposed openstack/nova master: Reject mixed VF rc and trait config https://review.opendev.org/c/openstack/nova/+/846436 | 09:10 |
| opendevreview | Balazs Gibizer proposed openstack/nova master: Ignore PCI devs with physical_network tag https://review.opendev.org/c/openstack/nova/+/846219 | 09:10 |
| opendevreview | Balazs Gibizer proposed openstack/nova master: Reject devname based device_spec config https://review.opendev.org/c/openstack/nova/+/846466 | 09:10 |
| opendevreview | Balazs Gibizer proposed openstack/nova master: Support [pci]device_spec reconfiguration https://review.opendev.org/c/openstack/nova/+/846470 | 09:10 |
| opendevreview | Balazs Gibizer proposed openstack/nova master: Stop if tracking is disable after it was enabled before https://review.opendev.org/c/openstack/nova/+/847009 | 09:10 |
| bauzas | gibi: sean-k-mooney: Uggla yeah, so we have two concerns for the Manila spec | 09:21 |
| bauzas | 1/ about flavor vs. image | 09:21 |
| bauzas | 2/ about how to lock a share | 09:21 |
| bauzas | if you want, we can discuss this by 1230UTC, ie. 14:30 for me and 13:30 for sean | 09:21 |
| * bauzas needs to go off for getting my daughter | 09:22 | |
| Uggla | bauzas, I think 1 is more explicit / implicit trait requirement. | 09:22 |
| Uggla | bauzas, anyway ok for 14:30. | 09:22 |
| gibi | yepp it is about asking the user to know about all technical prereqs for attaching a manila share | 09:23 |
| sean-k-mooney[m] | for manila shars you should not explictly need to specify any traits | 09:33 |
| sean-k-mooney[m] | you can optionally do so but it should not be a requiement | 09:33 |
| sean-k-mooney[m] | we want this feature to work with existing flavors and images | 09:33 |
| sean-k-mooney[m] | so documenting that you enither need file backed memory or hugepages | 09:34 |
| sean-k-mooney[m] | and having the api check for the same should be sufficent | 09:34 |
| sean-k-mooney[m] | operators can use traits for the former if they want to enforce that or they can use host aggreates or az to model which host has file backed memory | 09:35 |
| sean-k-mooney[m] | and just tell there users to select that az | 09:35 |
| sean-k-mooney[m] | so i dont think we need to modify the flavor/image for this feature | 09:35 |
| sean-k-mooney[m] | bauzas: gibi https://review.opendev.org/c/openstack/nova/+/847001 now has a functional test and i made the other changes we discussed yesterday too. | 09:38 |
| gibi | sean-k-mooney[m]: it is OK to me not asking for any specific from the user in image/falvor, but checking the prereqs | 09:40 |
| gibi | sean-k-mooney[m]: I will check https://review.opendev.org/c/openstack/nova/+/847001 shortly | 09:40 |
| Uggla | sean-k-mooney[m], maybe I'm missing something. How can you be sure the instance will be started on a host with the proper requirement without flavor/extra spec ? | 09:41 |
| sean-k-mooney[m] | the curret spec if i recall say we will check in the api if the instance has hugepages or is on a host with file backed memory adn reject the request if that is not the case | 09:41 |
| Uggla | yes | 09:42 |
| sean-k-mooney[m] | if the falvor request hugepages it will be on a valid host or if the user select an az that is mapped to file backed memroy | 09:42 |
| sean-k-mooney[m] | so we dont need anythign explict in the flavor for file backed mory | 09:43 |
| sean-k-mooney[m] | we just need to document that requirement | 09:43 |
| sean-k-mooney[m] | they can also add the trait if they like | 09:43 |
| sean-k-mooney[m] | but they dont need too | 09:43 |
| sean-k-mooney[m] | its the same for vhost-user networking with ovs-dpdk | 09:44 |
| sean-k-mooney[m] | we just document that you need file backed memory or hugepages | 09:44 |
| sean-k-mooney[m] | we dont require you to use traits but you could use a cutom trait for that if you wanted too | 09:44 |
| sean-k-mooney[m] | most operators are not going to want to resize all there workload to be able to start using this feature so its imporant that we dont force that when its not required. | 09:45 |
| sean-k-mooney[m] | does that make sense? | 09:45 |
| sean-k-mooney[m] | by the way are we accpeting translations in tree again? | 09:49 |
| sean-k-mooney[m] | i just noticed https://review.opendev.org/c/openstack/nova/+/846876 | 09:50 |
| sean-k-mooney[m] | but i would kind of prefer to not have those in tree | 09:51 |
| Uggla | hum I think so. I'm just wonder how to check for trait at the Rest API level. (atm looking at the code for an example) | 09:52 |
| sean-k-mooney[m] | you can just call placment to do a traits list on the host | 09:52 |
| bauzas | sean-k-mooney: yes | 09:53 |
| bauzas | +2/+Wd for me | 09:53 |
| sean-k-mooney[m] | hum i guess we do get thos octaionally https://github.com/openstack/nova/commits/master/nova/locale | 09:53 |
| gibi | Uggla: nova.scheduler.client.report.SchedulerReportClient.get_provider_traits | 09:54 |
| bauzas | sean-k-mooney: we only had a very few translations patches since 2020 | 09:55 |
| bauzas | https://docs.openstack.org/i18n/latest/reviewing-translation-import.html#reviewing | 09:55 |
| sean-k-mooney[m] | yep thats why i tought we had stop doing this | 09:56 |
| sean-k-mooney[m] | its fine we can proably merge it but if we are not doing a review of the content ectra it would be nicer if we just had this in a sperate repo that the i18n team could mange | 09:57 |
| sean-k-mooney[m] | i guess if its that infrequent its fine | 09:58 |
| sean-k-mooney[m] | if we start getting a lot of patches this way we should look at a different solution then we have currently. | 09:58 |
| bauzas | sean-k-mooney: the problem is with the i18n team | 09:58 |
| bauzas | sean-k-mooney: no, we won't have a lot of patches | 09:59 |
| bauzas | sean-k-mooney: even if they have a lot of i18n contributors, we will only have one change | 09:59 |
| bauzas | as it's an automatic import from Zanata | 09:59 |
| sean-k-mooney[m] | i dont like haveing a seperate review workflow for stuff or not being able to review the change and just merging it | 09:59 |
| sean-k-mooney[m] | so if that the workflwo we are going to have it think it would be better to have it entirly out of tree | 10:00 |
| bauzas | everything is done by https://translate.openstack.org/?dswid=2287 | 10:00 |
| sean-k-mooney[m] | as a stevador plugin for example that is loaded by nova | 10:00 |
| bauzas | sean-k-mooney: we had this workflow since 2013 IIRC | 10:00 |
| bauzas | and it was working | 10:00 |
| bauzas | we only need one single nova-core review | 10:00 |
| sean-k-mooney[m] | sure it works i just dont think its the right way to do things | 10:01 |
| bauzas | https://translate.openstack.org/project/view/nova?dswid=-7176 | 10:01 |
| bauzas | sean-k-mooney: if you have concerns, then you should be discussing with the i18n team, not here | 10:02 |
| Uggla | gibi, thx, I think it is ok. | 10:02 |
| sean-k-mooney[m] | why | 10:02 |
| sean-k-mooney[m] | i can but this is also a project desicion | 10:02 |
| sean-k-mooney[m] | and discussion | 10:02 |
| sean-k-mooney[m] | im wondering if we still want to ship tanslations as a project in tree and if we should change that going forward and remove them form the souce tree | 10:03 |
| bauzas | sean-k-mooney: because it's a TC question https://docs.openstack.org/project-team-guide/i18n.html | 10:04 |
| bauzas | and again, this is simple and it works | 10:04 |
| bauzas | anyway, I merged it | 10:04 |
| bauzas | I could even write some translations from French, if that helps :) | 10:05 |
| bauzas | Only 43% of Nova is translated in French :p | 10:05 |
| sean-k-mooney[m] | if you want too :P | 10:07 |
| gibi | we need more French! | 10:16 |
| opendevreview | Merged openstack/nova master: Imported Translations from Zanata https://review.opendev.org/c/openstack/nova/+/846876 | 10:19 |
| mnasiadka | Hello - is there a way to specify Cinder volume-type for boot from volume in flavour extra-specs? | 11:00 |
| gibi | mnasiadka: you have to pre-create the volume in cinder with the desired volume-type and then pass the volume to nova to boot from | 11:35 |
| mnasiadka | gibi: oh ok, thanks | 12:04 |
| bauzas | gibi: Uggla: sean-k-mooney : can we punt our session by 30 mins ? I have something at the moment | 12:21 |
| sean-k-mooney | i wont be able to do it sorry | 12:22 |
| bauzas | ie. 15:00 for CEST and 14:00 for BSR | 12:22 |
| sean-k-mooney | im in a meeting till the top of the hour | 12:22 |
| bauzas | BSTR* | 12:22 |
| bauzas | graaah | 12:22 |
| bauzas | BST* | 12:22 |
| sean-k-mooney | and then i have a dental apointmnt in an hour | 12:22 |
| sean-k-mooney | so it would have to be later today or you and gibi can proceed with Uggla without me | 12:22 |
| bauzas | ok, then let's discuss about Uggla's spec later then | 12:22 |
| gibi | later then | 12:22 |
| gibi | but as far as I see sean-k-mooney stated his oppinion above and I agreed | 12:23 |
| Uggla | bauzas, sean-k-mooney bauzas, I better understand sean-k-mooney proposal that sounds good. | 12:24 |
| bauzas | kk | 12:25 |
| sean-k-mooney | i just pushed my comments on the sepc too | 12:25 |
| * bauzas needs to go off for 20 mins | 12:25 | |
| *** mattia is now known as Guest2939 | 12:48 | |
| *** Guest2939 is now known as mattia | 12:51 | |
| *** mattia is now known as blmt | 12:52 | |
| Uggla | bauzas, gibi if you can have a look at my latest comments on https://review.opendev.org/c/openstack/nova/+/831507. On 2 points your opinions diverge and I can make both of you pleased at the same time. So I need a consensus between both of you to implement the solution you agree on. | 13:09 |
| bauzas | Uggla: sure, will look | 13: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 | 13:24 |
| gibi | Uggla: responded, fine with bauzas' suggestions | 13:26 |
| bauzas | gibi: Uggla: replied too | 13:31 |
| bauzas | Uggla: we already this kind of sentinel for example in https://github.com/openstack/nova/blob/master/nova/scheduler/manager.py#L114 | 13:33 |
| bauzas | that's how we flag an unset param | 13:34 |
| bauzas | this is a simple pattern | 13:34 |
| gibi | I think the special case here is that the caller needs differentiate too if some AZ value is needed to pass forward or the param should not be provided | 13:38 |
| bauzas | gibi: not sure I understand | 13:39 |
| bauzas | gibi: either the AZ is passed or not | 13:40 |
| bauzas | but the value can be None | 13:40 |
| bauzas | right? | 13:40 |
| gibi | by making the new_az optional the caller either needs to pass a value (including None) or not pass the parameter at all | 13:40 |
| gibi | that will be either a conditional on the caller | 13:40 |
| bauzas | https://specs.openstack.org/openstack/nova-specs/specs/zed/approved/unshelve-to-host.html#proposed-change | 13:41 |
| gibi | or the caller could pass the sentinel and avoif the conditional | 13:41 |
| bauzas | ah I see your concern | 13:41 |
| bauzas | well, in general we do this kind of conditional | 13:41 |
| gibi | I'm fine with that extra conditional on the caller side | 13:41 |
| bauzas | with kwargs | 13:42 |
| bauzas | like kwargs = {} | 13:42 |
| gibi | also I'm fine passing the sentinel and avoid the conditional too | 13:42 |
| bauzas | api.unshelve(ctxt, inst, host, **kwargs) | 13:42 |
| gibi | ahh, OK, you can do that too | 13:42 |
| bauzas | and if az is set, then kwargs['new_az'] = this | 13:42 |
| bauzas | that's the general pattern we have for sentinels | 13:43 |
| bauzas | no need to have different calls then | 13:43 |
| gibi | I'm OK with the kwargs way too | 13:43 |
| bauzas | one single call but with optional args | 13:43 |
| bauzas | gibi: well, as I said, this is the pattern we have in Nova for a while now :) | 13:44 |
| bauzas | ie. default a param to a sentinel value | 13:44 |
| bauzas | and call the method with kwargs | 13:44 |
| bauzas | Uggla: ^ | 13:44 |
| 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 |
| gibi | as I agree with the actual code | 13:45 |
| bauzas | https://github.com/openstack/nova/blob/master/nova/scheduler/rpcapi.py#L152 | 13:47 |
| bauzas | and https://github.com/openstack/nova/blob/master/nova/scheduler/manager.py#L145 | 13:47 |
| *** dasm|off is now known as dasm | 13:47 | |
| bauzas | gibi: the problem with importing a global var from another module is about any possible circular import | 13:47 |
| bauzas | if we don't need to import, let's not do it | 13:47 |
| gibi | sure | 13:48 |
| bauzas | also, the caller has to have some knowledge about the method | 13:48 |
| gibi | as I said I agree with the code :) | 13:48 |
| bauzas | this is a bad behaviour | 13:48 |
| bauzas | gibi: sure, I just explain why | 13:48 |
| bauzas | mostly not for you | 13:48 |
| bauzas | but also for Uggla :) | 13:49 |
| bauzas | so, yeah, unnecessary module import and internal knowledge of the called method are the two things we want to avoid | 13:49 |
| bauzas | and since optional params in python methods are something easy to do, let's use this pattern | 13:50 |
| Uggla | hum I think I understand. | 13:53 |
| 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 |
| gibi | and in this case None means something else than information-not-provide4d | 13:53 |
| bauzas | gibi: correct | 13:54 |
| bauzas | in general, we set to None unless None is used for a specific flag | 13:54 |
| gibi | yepp | 13:54 |
| bauzas | and if so, we use the sentinel pattern | 13:54 |
| bauzas | but again, we create a specific instance of an object that we use, and we try to not expose this instance elsewhere | 13:55 |
| bauzas | by instance, I mean a stored value in memory | 13:56 |
| * bauzas doesn't want to be pedantic, rather explaining why we have this pattern and why we don't pull this reference from the caller | 13:56 | |
| * Uggla not so clear with the **kwargs reason, but I think it will be clearer when I'll do it. | 14:01 | |
| 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 |
| gibi | caller side | 14:05 |
| gibi | I would make all the parameters non defaulted and document what value of what parameter means what :D | 14:06 |
| gibi | that is a bit more explict than param=sentinel in a signature | 14:06 |
| gibi | but this is way less important that make the unshelve patch land :D | 14:07 |
| gibi | s/that/than/ | 14:07 |
| bauzas | that's why I explained why the pattern and also why I said in my last comment that docstrings help | 14:07 |
| bauzas | I'd rather see a docstring saying (optional) my param | 14:08 |
| bauzas | rather than asking to import a specific instance of a global class object | 14:08 |
| gibi | the function itself is a global on the class an you import that to be able to call it | 14:10 |
| gibi | None is an interpreter global :) | 14:11 |
| gibi | (we are getting philosophycal ) | 14:11 |
| gibi | we are using enums from the fields module those are class level fields too | 14:12 |
| Uggla | gibi, bauzas, last stuff, ok with FIELD_SENTINEL wording ? | 14:26 |
| gibi | go with what bauzas asked for I won't block on it | 14:29 |
| bauzas | Uggla: gibi: you can even name it privatly | 14:30 |
| bauzas | like _unset_field | 14:30 |
| gibi | sure | 14:30 |
| bauzas | or _unsel_field_sentinel | 14:30 |
| bauzas | I don't wanna nitpick on the naming :) | 14:31 |
| Uggla | naming one of the hardest stuff in computing. | 14:31 |
| bauzas | but yeah, actually, since this is an internal object, make it private | 14:31 |
| bauzas | (by convention of course) | 14:31 |
| bauzas | https://twitter.com/codinghorror/status/506010907021828096?lang=fr | 14:32 |
| bauzas | :) | 14:32 |
| bauzas | (I like this tweet :p ) | 14:32 |
| bauzas | https://www.karlton.org/2017/12/naming-things-hard/ for the wider context | 14:33 |
| *** blmt is now known as mattia | 15:19 | |
| *** mattia is now known as blmt | 15:20 | |
| opendevreview | Amit Uniyal proposed openstack/nova master: Adds validation for hw machine type in host caps https://review.opendev.org/c/openstack/nova/+/847126 | 17:11 |
| 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 |
| 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:38 |
| ade_lee | oh -- maybe thats this -- https://bugs.launchpad.net/neutron/+bug/1979047 | 20:41 |
| *** dasm is now known as dasm|off | 23:03 | |
| *** hemna6 is now known as hemna | 23:37 | |
Generated by irclog2html.py 2.17.3 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!