| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2021-02-12 | |||
| 10:58:14 | gibi | what we pass around is a string representation of an UUID . If some code would threat it as uuid.UUID and call .fields on it, the code will fail with AttributeError | |
| 10:58:43 | gibi | python still enforce types :) | |
| 10:59:48 | bauzas | that's the reason why we have code reviews and docstrings... but the ship sailed eitherway | |
| 11:00:31 | bauzas | I'm just a bit sad we hadn't discussed it in a PTG session | |
| 11:00:32 | gibi | and we still have code review and doc string in the future, mypy just help with that review by automating some part of it | |
| 11:00:44 | bauzas | before telling to use mypy | |
| 11:00:49 | gibi | bauzas: we discussed it in some point and was no consensus | |
| 11:01:02 | gibi | so as a nova project we don't tell you that you have to use mypy | |
| 11:01:11 | gibi | it is stephenfin who asks it | |
| 11:01:14 | bauzas | well, here I'm asked to do it :) | |
| 11:01:50 | gibi | can I forbid stephenfin to ask such thing? No I don't think so | |
| 11:02:12 | gibi | what I can say that it is not mandatory to add mypy type hint to your patch | |
| 11:02:22 | gibi | as we never decided to make it mandatory | |
| 11:02:23 | bauzas | do we also want to use type hints for private methods ? | |
| 11:03:59 | gibi | in my eyes the benefit of mypy (if any) applies equally on private and public too | |
| 11:04:09 | stephenfin | bauzas: You're misstating what I asked. I suggested adding type hints, but I was very clear that they weren't mandatory | |
| 11:04:28 | bauzas | anyway, I'm adding them | |
| 11:04:37 | stephenfin | Please don't say I'm forcing you into anything wrt mypy because it's not true | |
| 11:04:57 | bauzas | ack | |
| 11:05:35 | stephenfin | "Code itself is fine, but I do question the wisdom of throwing these into the utils modules and I don't like reinventing the wheel and doing something neutronclient appears to support. The -1 is for the latter. I'd also like to see type hints and have provided them for you, but I can't force than on you of course 😄" | |
| 11:05:45 | stephenfin | from https://review.opendev.org/c/openstack/nova/+/773976/5 | |
| 11:06:30 | bauzas | oki, then I'm trying to use the neutron client method | |
| 11:06:32 | stephenfin | And we did discuss this at a PTG and as gibi said, there was no consensus. We said we could add them if we wanted but they weren't mandatory | |
| 11:06:47 | bauzas | which is a good call | |
| 11:06:53 | stephenfin | given the value had yet to be proven | |
| 11:06:58 | stephenfin | I can find the notes if you'd like | |
| 11:07:17 | stephenfin | but tbc, it's also not true to say this wasn't discussed. It absolutely was | |
| 11:08:38 | stephenfin | wrt changing things from manual calls to using the neutronclient, this patch might be helpful https://review.opendev.org/c/openstack/nova/+/706295 | |
| 11:08:53 | stephenfin | as a blueprint, I mean | |
| 11:09:08 | stephenfin | though I suspect your changes won't need to be as big since it's not baked in yet :) | |
| 11:11:10 | bauzas | I already used the client method for getting subnets, so it's indeed quick | |
| 11:34:06 | stephenfin | gibi, bauzas: Could you take a look at this requirements patch today? https://review.opendev.org/c/openstack/nova/+/775142 | |
| 11:34:26 | bauzas | I can try | |
| 11:34:36 | stephenfin | We're trying to uncap a dependency (PrettyTable) and need to do that across multiple projects | |
| 11:36:48 | bauzas | gibi: stephenfin, others: I just spotted the fact that most of our prefilters don't raise exceptions but one | |
| 11:37:13 | bauzas | do we have kind of a consensus about a prefilter error behaviour ? | |
| 11:37:38 | bauzas | if so, I should catch the exceptions I raise in the subsequent modules | |
| 11:37:59 | stephenfin | hmm, I've no idea. Depends on what happens with those exceptions. Do we capture them or would it result in a HTTP 5xx? | |
| 11:38:02 | bauzas | or raising them nested into a RequestFilter exc | |
| 11:38:17 | bauzas | the caller is a prefiler, so that's async | |
| 11:38:22 | bauzas | prefilter* | |
| 11:38:48 | bauzas | the scheduling phase should set an ERROR state, that's it | |
| 11:38:58 | bauzas | exactly like a NoValidHosts | |
| 11:39:09 | bauzas | hence the questionj | |
| 11:39:33 | bauzas | I'm OK with nesting any exception within a RequestFilter exception so we make consistent behaviour | |
| 11:39:42 | gibi | bauzas: that sounds like a good behavior. Set the instance to ERROR and let the create instance action store the exception | |
| 11:40:04 | stephenfin | Ah yes, I see what you mean | |
| 11:40:06 | bauzas | ok then stephenfin's point about better exception handling is legit | |
| 11:40:19 | stephenfin | the other filters just log and return False | |
| 11:41:08 | bauzas | right but there is the require_tenant_aggregate() prefilter which does this too | |
| 11:41:15 | bauzas | hence my question | |
| 11:41:26 | bauzas | looks like we hadn't thought about this | |
| 11:41:35 | bauzas | not saying the other filters don't raise exceptions | |
| 11:41:57 | bauzas | their own calls could fail too, that's just they don't handle them straight | |
| 11:42:07 | gibi | explicit failure is better than simply skipping the prefilter behavior and move forward with the scheduliung | |
| 11:42:36 | stephenfin | Yeah, we don't seem to do anything with the return values | |
| 11:42:39 | stephenfin | outside of tests | |
| 11:42:59 | stephenfin | process_reqspec simply calls the filter - it doesn't do anything with the return value | |
| 11:43:14 | stephenfin | so raising does seem like a sensible thing to do, if it's something we can't recover from | |
| 11:43:28 | stephenfin | and we should probably do the same for the other filters | |
| 11:44:46 | bauzas | gibi: a filter generally can fail without blocking | |
| 11:45:04 | bauzas | we don't hard stop on a scheduler filter failure iirc | |
| 11:45:35 | bauzas | honestly, I don't know what to say | |
| 11:45:38 | gibi | hm, so if the NumaTopologyFilter borks on a request then we simply ignore the NUMA part of the request? | |
| 11:45:49 | bauzas | checking, unsure | |
| 11:47:04 | bauzas | gibi: there is a precedent : https://bugs.launchpad.net/nova/+bug/1473308 | |
| 11:47:06 | openstack | Launchpad bug 1473308 in OpenStack Compute (nova) "NUMATopologyFilter raise exception and not continue filter next node when there is no wanted pagesize in current filtered host" [Medium,Fix released] - Assigned to Chung Chih, Hung (lyanchih) | |
| 11:47:21 | bauzas | here, we decided to catch the exception and continue filtering | |
| 11:47:53 | bauzas | so the utils and network methods can fail and raise, but the prefiler has to handle those | |
| 11:48:02 | gibi | we assumed that if we fail to filter a host then that host is bad, but other host might be good | |
| 11:48:11 | bauzas | right | |
| 11:48:17 | gibi | but a prefilter is not per host | |
| 11:48:26 | bauzas | good call | |
| 11:48:34 | gibi | it is applied globally on the placement a_c query | |
| 11:48:35 | bauzas | I'm tore | |
| 11:48:39 | bauzas | torn* | |
| 11:55:39 | bauzas | stephenfin: the PrettyTable reqs bump is a bit frightening | |
| 11:55:50 | bauzas | we're crossing 2 major releases | |
| 11:56:02 | stephenfin | There's no bump | |
| 11:56:10 | stephenfin | It's still constrained by upper-constraints | |
| 11:56:30 | stephenfin | https://github.com/openstack/requirements/blob/master/upper-constraints.txt#L143 | |
| 11:56:55 | stephenfin | that will eventually be raised, but it'll just be like anything else being increased | |
| 11:57:34 | bauzas | does the community has a plan for bumping ? | |
| 11:57:46 | bauzas | hopefully they won't just raise the bar at the top | |
| 11:57:56 | bauzas | but rather incrementing it slowly | |
| 11:58:09 | stephenfin | not yet, though I have checked and the 2.0.0 API is identical to the 0.7.2 API | |
| 11:58:24 | stephenfin | The major version bump just indicates that Python 2 support was dropped | |
| 11:59:15 | stephenfin | I had to make one change in cliff to get things working with 2.0.0, and that was only because the previous version had a bug that we were inadvertently benefiting from :) | |
| 11:59:33 | bauzas | ack | |
| 11:59:38 | stephenfin | context https://review.opendev.org/c/openstack/cliff/+/774915 | |
| 12:31:32 | kashyap | stephenfin: Hey, a random question: what OS is your base Xeon server running? | |
| 12:31:49 | stephenfin | It's powered off atm, but typically Ubuntu 20.04 | |
| 12:31:53 | stephenfin | cos that's what the gate runs | |
| 12:32:20 | stephenfin | Fedora on my laptop | |
| 12:32:40 | kashyap | I see; that's still fine. Perhaps next week, I might ask to test something for me. Let me know when you powered it on :) | |
| 12:32:53 | stephenfin | ack | |
| 12:33:00 | kashyap | Is it reason to assume that you enable nested KVM by default on your kernel? | |
| 12:33:21 | stephenfin | yup | |
| 12:33:46 | openstack | bugzilla.redhat.com bug 1928103 in libvirt "libvirt insists on including extra CPU features that were not asked for: "vme,x2apic,hypervisor"" [Unspecified,New] - Assigned to libvirt-maint | |
| 12:33:46 | kashyap | Cool; thank you. (It's in relation to a libvirt upstream bug I just filed: https://bugzilla.redhat.com/show_bug.cgi?id=1928103) | |