| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-10-22 | |||
| 14:07:07 | sean-k-mooney | so i guess locking is not always needed | |
| 14:07:10 | mdbooth | Except if you didn't want it to work | |
| 14:07:19 | mriedem | see the note above the condition | |
| 14:08:15 | mdbooth | mriedem: Guessing that was written in the days before functional/ ? | |
| 14:08:17 | sean-k-mooney | oh i guess it would also cause issues because we use the same instance uuids | |
| 14:08:28 | sean-k-mooney | e.g. for parallel test executions on the same host | |
| 14:08:48 | mdbooth | Because non-test code uses locks, and we're not going to 'fix' that code to not use locks | |
| 14:08:55 | mdbooth | Just so we can execute it from a test | |
| 14:10:44 | mdbooth | mriedem: For reference, the locks which are being ignored are the locks in _do_build_and_run_instance, and terminate_instance in ComputeManager | |
| 14:11:03 | mriedem | mdbooth: it's not before functional tests existed | |
| 14:11:07 | mdbooth | My test assumes that the locks works as expected. It would be weird to expect otherwise. | |
| 14:12:02 | sean-k-mooney | mdbooth: most fucntional tests assume that there are not cuncuretn action taking place | |
| 14:12:06 | mdbooth | It's kinda hard to write a test for a delete race when locks have different semantics in tests. | |
| 14:12:18 | sean-k-mooney | since all the service runn in the same interperter instnace | |
| 14:12:30 | mdbooth | Right. You can still have locks, though. | |
| 14:12:37 | mriedem | i would prefer that we don't have tests that have to rely on locks | |
| 14:12:44 | sean-k-mooney | yes but they would not race with anything | |
| 14:13:00 | mdbooth | Well the locks are critical to the semantics of the code | |
| 14:13:33 | sean-k-mooney | right but in any code path we should not have a singel action reneter the same critical section multiple times right | |
| 14:13:35 | mdbooth | Specifically, in this case they prevent the delete from completing while it is still building, which is the actual behaviour. | |
| 14:13:48 | sean-k-mooney | so the lock would always be aqired and never block | |
| 14:14:06 | mdbooth | If we disable locks, I have to write a race test which assumes that an instance can be completely deleted while it is still being built. | |
| 14:14:12 | mdbooth | That's obviously possible, but it's wrong. | |
| 14:14:20 | mdbooth | i.e. possible to write. | |
| 14:14:36 | mdbooth | But I'd be testing something which can't happen on a real system. | |
| 14:14:37 | sean-k-mooney | mdbooth: well the instnace can at least be partly deleted while its building | |
| 14:14:46 | mdbooth | sean-k-mooney: Absolutely, yes. | |
| 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 | sean-k-mooney | mdbooth: i guess it default to false https://github.com/openstack/oslo.concurrency/blob/master/oslo_concurrency/lockutils.py#L284 | |
| 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 | mriedem | dansmith: i've abandoned my attempt at fixing https://bugs.launchpad.net/nova/+bug/1815082 but you might be interested in that | |
| 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? | |