| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-10-22 | |||
| 14:14:57 | mdbooth | sean-k-mooney: That's the race I'm writing a test for. | |
| 14:15:11 | sean-k-mooney | yep this is the thing stephen was working on | |
| 14:15:18 | sean-k-mooney | did you see the message i left | |
| 14:15:37 | mdbooth | sean-k-mooney: Did you leave it since this morning? | |
| 14:15:43 | sean-k-mooney | no | |
| 14:15:57 | mdbooth | sean-k-mooney: Then yeah. I've already written the test, though ;) | |
| 14:16:10 | mdbooth | I've been badgering you to review it for a couple of days :P | |
| 14:17:03 | sean-k-mooney | yes i know :) and i will look at it now | |
| 14:17:07 | mdbooth | In general it's better, but not having functioning locks would be super weird. | |
| 14:17:22 | mdbooth | It would basically invalidate the test. | |
| 14:17:33 | mdbooth | sean-k-mooney: Not worth it right now. I'm basically rewriting it. | |
| 14:18:56 | sean-k-mooney | ok so you tried mocking _update_instance_after_spawn to do the api delete and now your rewriting it based on mriedem suggestion | |
| 14:19:16 | sean-k-mooney | havent read all the comments since then but is see there is discussion on the patch | |
| 14:19:38 | mdbooth | sean-k-mooney: My previous test worked fine. mriedem wants to deprecate IntegratedTestBase, thouogh, so requested a rewrite which doesn't use it. | |
| 14:19:52 | mriedem | i just left more comments | |
| 14:19:58 | sean-k-mooney | ah ok | |
| 14:20:01 | mriedem | you're making this unnecessarily libvirt-specific when the fix isn't | |
| 14:20:10 | mriedem | and relying on a refactor which is going to muddy backports | |
| 14:20:15 | mdbooth | mriedem: Yeah, my rewrite also ditches libvirt | |
| 14:20:17 | mriedem | iow you're making this harder than it needs to be imo | |
| 14:20:30 | mriedem | ok good | |
| 14:20:52 | sean-k-mooney | mriedem: so on that should i consume mdbooth refactor or avoid it for https://review.opendev.org/#/c/687957/3 | |
| 14:21:21 | sean-k-mooney | give i do want to backport it would you be ok with backporting https://review.opendev.org/#/c/689186/2 | |
| 14:21:30 | mriedem | it's common sense to avoid depending on as many things as possible if you intend on backporting a patch | |
| 14:21:54 | gmann | mriedem: you can merge the peers and subnode groups setting. it was merged in base job also. Initially it was separate job for single and multinode jobs but during rocky PTG we discussed to merge both and have a single base job doing same network setting via subnode var | |
| 14:22:06 | mriedem | sean-k-mooney: your fix is also not libvirt specific | |
| 14:22:16 | mriedem | so i'm not sure why you need to depend on the libvirt functional test base stuff | |
| 14:22:26 | mriedem | you should be able to write isolated functional tests for the rebuild + numa stuff | |
| 14:22:32 | mdbooth | sean-k-mooney: I've pulled the non-placement bits out of the test mriedem pointed to. So it's still a small refactor, but I wanted to ability to not pull in a ton of placement stuff I don't need. | |
| 14:22:44 | sean-k-mooney | that is a good point. i only depended on it because that is the only numa fucntional test we have | |
| 14:22:58 | sean-k-mooney | but you are right i could proably remove all the libvirt stuff | |
| 14:23:08 | sean-k-mooney | i should think about that | |
| 14:24:09 | sean-k-mooney | mdbooth: well we cant ignore placement on rebuild in general | |
| 14:24:22 | sean-k-mooney | but yes its not relevent to what you are fixing/testing | |
| 14:24:50 | mriedem | if a test needs to create a server then you are going to require the placement fixture somewhere in your functional test | |
| 14:24:56 | mdbooth | Yeah. The resulting placement stuff could be a mixin, tbh. | |
| 14:24:58 | mriedem | otherwise you can't get through the scheduler | |
| 14:25:04 | sean-k-mooney | its also not technicall directly relevent to what im testing but if the traits chagned on rebuilt that in principal would need to be validated with placement | |
| 14:25:26 | mdbooth | mriedem: Right, it's all the placement helper stuff which isn't needed in general. | |
| 14:25:30 | sean-k-mooney | mriedem: yes | |
| 14:25:39 | mdbooth | I'm still starting placement. | |
| 14:25:48 | mriedem | mdbooth: if you need locks in the test you just add REQUIRES_LOCKING = True right? | |
| 14:26:47 | mdbooth | mriedem: Possibly, but I also didn't expect this lock to be filesystem based. I'm going to investigate thoroughly before assuming it's all ok. | |
| 14:27:34 | sean-k-mooney | mdbooth: the syconise decorators are always filesystem based | |
| 14:27:57 | mdbooth | sean-k-mooney: I thought that was only for external=True | |
| 14:28:09 | mdbooth | re-reading | |
| 14:28:10 | sean-k-mooney | oh am maybe | |
| 14:29:11 | gibi | fyi tomorrow is a national holiday here in Hungary so I will be off tomorrow | |
| 14:29:39 | mriedem | dansmith: i've abandoned my attempt at fixing https://bugs.launchpad.net/nova/+bug/1815082 but you might be interested in that | |
| 14:29:39 | openstack | Launchpad bug 1815082 in OpenStack Compute (nova) rocky ""DBNonExistentTable: (sqlite3.OperationalError) no such table: services" when starting nova-metadata under uwsgi" [Medium,Triaged] | |
| 14:29:39 | sean-k-mooney | mdbooth: i guess it default to false https://github.com/openstack/oslo.concurrency/blob/master/oslo_concurrency/lockutils.py#L284 | |
| 14:30:01 | mriedem | dansmith: tl;dr trying to get the indirection api working in n-api-meta in certain cases if we don't have direct access to the db | |
| 14:30:30 | openstackgerrit | Ghanshyam Mann proposed openstack/nova master: Fix policy doc for host_status and extended servers attribute https://review.opendev.org/689833 | |
| 14:30:55 | mriedem | er i guess we use the indirection api for n-api-meta when running with evetlet but not wsgi | |
| 14:32:31 | dansmith | mriedem: hmm, what you have seems like it should work, although your comment about it hitting superconductor isn't right, unless nova-cpu.conf has api db creds | |
| 14:33:25 | dansmith | mriedem: I could pick up on that, but unless someone is beating down the door for it and/or going to test it prior to merge, I'd probably set it aside too | |
| 14:33:38 | efried | mriedem: I can do those backports, yeah. | |
| 14:33:41 | sean-k-mooney | mriedem: before i go refactor my own work you would prefer i use ProviderUsageBaseTestCase instaead of _IntegratedTestBase and make it non libvirt specific | |
| 14:33:59 | mriedem | jroll: i have given up on https://review.opendev.org/#/c/637217/ - someone else that cares about completing the ironic flavor data migration added in pike is going to have to take over | |
| 14:34:27 | mriedem | dansmith: i had a tempest patch that depended on the nova fix to test it | |
| 14:34:44 | mriedem | https://review.opendev.org/#/q/status:abandoned+topic:bug/1815082 | |
| 14:35:29 | mriedem | dansmith: looking at latest comments the issue was n-api-meta was starting before conductor which blew up | |
| 14:36:21 | mriedem | frickler: ^ i've abandoned those changes for your bug fyi | |
| 14:36:40 | mriedem | sean-k-mooney: yes don't use _IntegratedTestBase for new tests | |
| 14:36:54 | sean-k-mooney | ok thanks | |
| 14:37:15 | mriedem | and yes it'd be cool if things that aren't libvirt specific didn't build on libvirt specific functional test base classes | |
| 14:37:43 | sean-k-mooney | ya i think i can create a small numa file that is not in the libvirt folder to test this | |
| 14:38:06 | sean-k-mooney | although there are few non libvirt numa things that will likely be added to it | |
| 14:39:33 | jroll | mriedem: thanks for the heads up | |
| 14:41:22 | mriedem | sean-k-mooney: just my opinion, don't bend over backward based on something i'm not actively reviewing | |
| 14:46:15 | sean-k-mooney | mriedem: ok be even so i tend to give you opipion weight even if you are not activly reviewing it since others often agree | |
| 14:49:39 | mdbooth | Whoa. It appears from tracing in oslo.concurrency that something is allowing a threading.Semaphore to be taken twice | |
| 14:49:56 | mdbooth | I wonder... is threading.Semaphore re-entrant by default? | |
| 14:50:45 | cdent | those have to be some most mdboothy statements I've ever seen | |
| 14:50:54 | cdent | "hey, everything might be wrong!" | |
| 14:51:12 | cdent | which I, of course, love | |
| 14:51:25 | mdbooth | cdent: I usually hope to follow them up by being wrong, but I like to think them first :) | |
| 14:51:33 | mdbooth | And unfortunately I'm not always wrong. | |
| 14:53:40 | bnemec | It seems highly unlikely that a concurrency class would be non-reentrant. | |
| 14:53:57 | mdbooth | bnemec: s/non-// | |
| 14:54:17 | mdbooth | bnemec: I would *not* expect Semaphore to be re-entrant, and certainly in a regular environment it isn't: | |
| 14:54:35 | bnemec | Are you sure the tracing is in the right place? We spent a bunch of time tracking down a double-locking problem at one point that was caused by logging in the wrong place. | |
| 14:54:48 | mdbooth | >>> import threading >>> s = threading.Semaphore() | |
| 14:54:49 | mdbooth | >>> with s: | |
| 14:54:49 | mdbooth | ... with s: ... print("Hello world") | |
| 14:54:49 | mdbooth | ... | |
| 14:54:51 | mdbooth | Eurgh | |
| 14:54:52 | mdbooth | Sorry | |
| 14:55:02 | mdbooth | Anyway, takes same semaphore twice, hangs | |
| 14:55:40 | bnemec | Oh, well no that wouldn't work. But that's not what reentrancy is. | |
| 14:55:51 | mdbooth | bnemec: Wouldn't rule it out, but I don't think so. It's in a functional test, though, so my prime suspect is environmental. | |
| 14:55:53 | bnemec | Reentrancy has to do with concurrent execution, which you can't get in a single thread. | |
| 14:56:06 | mdbooth | bnemec: You might want to look that up ;) | |
| 14:56:17 | bnemec | I did. Did you? https://en.m.wikipedia.org/wiki/Reentrancy_%28computing%29 | |
| 14:56:25 | bnemec | "In computing, a computer program or subroutine is called reentrant if multiple invocations can safely run concurrently." | |
| 14:56:39 | mdbooth | https://docs.python.org/3/library/threading.html#semaphore-objects | |
| 14:57:07 | mdbooth | https://en.wikipedia.org/wiki/Reentrant_mutex | |
| 14:57:52 | bnemec | Ah, we're talking about different things. | |
| 14:59:35 | mdbooth | So, Semaphore doesn't appear to be mocked, and yet I see 2 enters followed by 2 exits | |
| 15:00:36 | openstackgerrit | Eric Fried proposed openstack/nova stable/train: Func: bug 1849165: mig race with _populate_assigned_resources https://review.opendev.org/690099 | |