More sqlite fixes

[vrothberg]

Does this PR introduce a user-facing change?

None

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vrothberg

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [vrothberg]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[vrothberg]

@edsantiago, feel free to test them in your PR. I am optimistic.

[vrothberg]

@cevich is the Build for fedora-37 CI_DESIRED_DATABASE:sqlite required? It's a runtime feature, so one build job should in theory be enough.

[edsantiago]

is the Build for fedora-37 CI_DESIRED_DATABASE:sqlite required?

It's an annoying side effect of the way YAML does macro expansion. I don't think there's any sane way to get rid of it (without introducing horrible duplication), but maybe @cevich can think of a way.

[vrothberg]

Error: beginning refresh transaction: database is locked

Still there 😱

[edsantiago]

Yep, in my PR also.

[edsantiago]

Consistently (three out of three runs) failing in int f37 rootless. Consistently NOT failing int root nor sys rootless. What is it about int rootless that is triggering this?

[vrothberg]

Consistently (three out of three runs) failing in int f37 rootless. Consistently NOT failing int root nor sys rootless. What is it about int rootless that is triggering this?

Very curious but I've no idea.

[edsantiago]

Also... there's something else broken in int sqlite tests. They are still running, 40 minutes and counting. Will post final timing results when the jobs finished, but right now it looks very bad.

[vrothberg]

I dropped the first commit which actually always caused the DB locked error when running rootless.

[vrothberg]

Finger's crossed. Thinking about it, we haven't seen this issue without the shared cache which I dropped now again.

[vrothberg]

@edsantiago this looks better on my end.

[edsantiago]

YAY! None of the evil strings appear anywhere in the downloaded CI logs. LGTM but let's wait for my PR to finish CI.

[vrothberg]

YAY! None of the evil strings appear anywhere in the downloaded CI logs. LGTM but let's wait for my PR to finish CI.

\o/ it seems the cache option indeed is evil.

[edsantiago]

Sigh. I'm sorry. f37 rootless, two "UNIQUE constraint" failures. I've double-checked that my PR is correctly rebased on top of 17889, but please feel free to check again.

[vrothberg]

Sigh. I'm sorry. f37 rootless, two "UNIQUE constraint" failures. I've double-checked that my PR is correctly rebased on top of 17889, but please feel free to check again.

🤯

Thanks, Ed. I'll continue digging tomorrow.

[vrothberg]

There's still a race it seems.

[edsantiago]

So, from my pov: even if this doesn't fix the UNIQUE bug, I'm OK with this merging. It seems to cause no harm, and I trust your investigation about shared-cache. I don't know enough sqlite to be the one to merge it, though. Maybe @mheon can look at it tomorrow. Thanks, everyone, for your persistence on this.

[edsantiago]

Timing results (redacted, to only show difference between boltdb and sqlite):

type	distro	user	DB	local	remote	container
int	fedora-37	root		30:39	33:14	30:17
int	fedora-37	root	sqlite	28:50	33:38
int	fedora-37	rootless		28:55
int	fedora-37	rootless	sqlite	27:23

[vrothberg]

So, from my pov: even if this doesn't fix the UNIQUE bug, I'm OK with this merging. It seems to cause no harm, and I trust your investigation about shared-cache. I don't know enough sqlite to be the one to merge it, though. Maybe @mheon can look at it tomorrow. Thanks, everyone, for your persistence on this.

I am also OK with merging the PR as is. Maybe @mheon will find the needle in the haystack and get validation code healthy.

[edsantiago]

OK, this is weird. Two more UNIQUE constraint errors ... but in the same task (f37 rootless) and, Twilight Zone music, the same two subtests, and almost the same delta-T (10s vs 12s) between the failures. I'm tempted to conclude that there's something weird happening when those two tests run simultaneously. But what?? All tests use a different root and runroot. Could there be some other shared directory when sqlite is involved? And only rootless??

[edsantiago]

Another CI run, here's f36 and f37, both rootless, both in "restart running container" test.

[EDITED TO ADD: yes, f36. I modified my PR to just hammer on sqlite, almost all jobs, including f36 and debian]

[mheon]

This LGTM. I know what the other issue is (TOCTOU in the validation code, two Podman processes can race to create the row) but it's a bit annoying to fix to it will have to wait until tomorrow afternoon.

[edsantiago]

Here's a really, really nice one. f37 rootless, no surprise, but what's sweet is that it's a triple-failure in the restart test (single-failure is more common). Not only that, but there's a triple-failure in the stop podman.service test. I thought I had fixed that by adding --runroot to the podman invocation, but nope. (TL;DR the stop podman.service failure is a brand-new one, happening only in sqlite tests. I've never seen it until this week).

I am over 90% convinced that there is something happening with concurrent sqlite processes, some shared file that should not be shared.

[edsantiago]

@mheon thanks for checking in. I'll assume you have what you need, and I'll stop my repeat testing. Good night!

[vrothberg]

@baude @mheon @edsantiago merge me please

[edsantiago]

My comment from yesterday still stands:

So, from my pov: even if this doesn't fix the UNIQUE bug, I'm OK with this merging. It seems to cause no harm, and I trust your investigation about shared-cache. I don't know enough sqlite to be the one to merge it, though. Maybe @mheon can look at it tomorrow. Thanks, everyone, for your persistence on this.

[baude]

/lgtm