[JUJU-1944] DB-backed lease store implementation

[manadart]

Implementing leases in Dqlite requires a DB-backed implementation of the lease.Store indirection.

Here we add such an implementation.

This constitutes the core functionality, which will be subject to review and iteration upon threading it into the lease manager worker.

QA steps

No functional changes yet, but changes can be verified by running tests:

• Run make dqlite-deps-pull.
• Create a file in the project root, test.list, with the following contents:

github.com/juju/juju/database
github.com/juju/juju/worker/lease

• Run TEST_PACKAGE_LIST=test.list make test and ensure tests pass.

Documentation changes

None.

Bug reference

N/A

[jack-w-shaw]

Getting build failures with:

$ make install
Generating facade schema...
go: downloading github.com/mattn/go-sqlite3 v1.14.14
# github.com/juju/juju/core/lease
core/lease/storedb.go:358:24: undefined: sqlite3.Error
core/lease/storedb.go:360:48: undefined: sqlite3.ErrConstraintUnique
make: *** [rebuild-schema] Error 2

But confusingly the unit test are succeeding

[jameinel]

Mostly just going through it and raising some points for conversation.

[jameinel]

Is this because it always was only 0 or 1 (and we should turn it into a *Key instead)?
IIRC I did implement the listing with N>=2 when doing it elsewhere (and you could do that for the DB by doing an IN query)

I guess is starts getting weird if you have multiple model uuids/namespaces, etc.
You could probably do a few IN queries and then filter at the end. We'd have to see if we need it. (we could also obviously create a composite key with all 3 and just search based on that.)

[jameinel]

I did check that we have an index (and even a unique index on lease_type_id, model_uuid, name).
Do we have any checking to see that the index itself is actually being used in this case? (given vagaries around t.id vs t.type, etc, I can see that a query optimizer may or may not actually get the index right)

[jameinel]

interesting that sqlite uses datetime, and pgsql seems to express this differently:
https://www.postgresqltutorial.com/postgresql-date-functions/postgresql-now/ (now()::timestamp).

I'm surprised that you have to express it differently by backend.

[jameinel]

note that sqlite does support floating point seconds according to the spec. so rather than doing int64(Ceil), we could just do "+%.3f seconds"

[jameinel]

I'm curious about this request response design. If we do trigger cancel(), shouldn't we still wait on <-err to see that the request has actually been stopped and the goroutine returned?

[manadart]

I've made this change.

[jameinel]

So you made a chance to call cancel() if we got an error, but I don't see a change that you wait for the cancel to trigger in the goroutine and then wait for the error to come back again.

[manadart]

I was always calling cancel, but we wait after cancellation everywhere with:

case <-stop:
    cancel()
    return errors.Trace(<-errCh)

[jameinel]

It would be an interesting exercise to think about what these complicated joins look like with sqlair. Since it is primarily input parameters, it should be relatively straightforward, I think. Though you do end up with a "uuid" that doesn't exist on your Key, and an entity that is just a string. So unless we craft a custom type for input, there isn't a simple named thing for all the parameters.

[manadart]

There is a UUID extension for SQLite, though I don't know the feasibility of compiling it in when building Dqlite.

Then it would be easy then to trigger if-null creation upon insert.

I would like to see us using UUID on our domain objects though - it would be great for traceability to pass them with requests into the API and have objects uniquely identified in the wider system whether-or-not they have made to the the point of persistence.

[jameinel]

And for this particular delete/join the UUID is looked up with a subselect anyway, so type, model_uuid, name, are all part of the key, but the "entity_id" is not. Did we have something which was just ".str" as a way to pass a simple value? I know we supported "Map.name"

but something like:

query := sqlair.Query(`
DELETE FROM lease_pin
WHERE uuid = (
  SELECT p.uuid
  FROM lease_pin p
    JOIN lease l on l.uuid = p.lease_uuid
    JOIN lease_type t ON l.lease_type_id = t.id
  WHERE t.type = $Key.Namespace
  AND l.model_uuid = $Key.model_uuid
  AND l.name = $Key.name
  AND p.entity_id = Map.entity_id
)`[1:]

err := query.Exec(lease, sqlair.M{"entity_id": entity})

Does that look about right to you for where sqlair would be going?

[SimonRichardson]

FYI:

If a separate thread makes changes on the same database connection while sqlite3_changes() is running then the value returned is unpredictable and not meaningful.

https://www.sqlite.org/c3ref/changes.html

[manadart]

This might not necessarily be the mechanism for populating RowsAffected. LXD appear to use it in a few places.

[SimonRichardson]

This should be obsolete when we start passing context.Context through the API server stack.

[SimonRichardson]

defer cancel() under the ctx, cancel := context.WithCancel(context.Background())

[manadart]

We don't really need to, as there is no path where it isn't called.

[SimonRichardson]

I'm unsure if this will work, I'll investigate, but if you have a leadership change do we keep the prepared statement for all followers?

[manadart]

We do, and it seems to work OK. LXD looks like they prepare up-front too.