Move Incus gateway package to go-cowsql

[masnax]

This moves Incus' current implementations for cluster management over to go-cowsql, to then be consumed by Incus and other projects that want to use the same pattern.

There are 4 interfaces that must be implemented externally:

• Cluster for the gateway's management of cluster member node info in the global cowsql database
• Node for the gateway's management of cached cluster member node info in the local database
• State for system non-database related state information
• Transactor for defining transaction behaviour on concurrent access, and managing connection timeouts.

[stgraber]

@masnax a few things I picked up:

• Maybe go through the incus package imports and see exactly what's being pulled in and whether it's realistic to try to undo the dependency on the Incus packages. As it stands, it's not too bad for Incus itself and Operations Center as they already have some cross-dependencies, but anyone else using go-cowsql will probably be rather unhappy to pull in Incus and all its dependencies in the process.
• IsCowsqlNode appears to either panic or return true
• Would be good to not have any "panic" codepaths in the new logic
• heartbeat.go:188-191, returning while under lock
• retry.go:55, code is unreachable as the loop will exit before
• HearbeatCancelFunc, typo
• db/cluster.go:72,85,87, comments name the wrong function or parameters
• info.go:16-17 has a duplicated comment
• transaction.Enable returns an unexported type
• Join never uses serverCert
• The notify logic doesn't seem to allow for NotifyAlive anymore?
• Looks like retries are broken? cluster/db/transaction/context.go:118-128 re-uses the dead Tx
• context.go:105-116, bad scoping, tx gets re-defined instead of replaced
• cluster/membership/membership.go:619-623, shadows hbMembers causing a nil map to be used
• membership.go:691-701, looks a bit odd, aren't we going to get demotions if exceptRoles is empty
• cluster/gateway/notify.go:87, global err gets re-defined from multiple goroutines
• membership.go:395, key gets written as world readable
• cluster/gateway/cluster.go:818, calls GetNodes from inside a transaction.Do callback

Sorry for not commenting in line but given it's a single huge commit, I ended up cloning locally and digging through things there.

I think it'd also be good to put golangci-lint in place as part of this change and run it with a pretty strict config (I'd base it off our crazy IncusOS one and then turn off the few things that make sense).

[masnax]

That should take care of most of them, just 2 to go (Incus package audit and retriable ForceTx)

Here's the full set of Incus dependencies:

	incusapi "github.com/lxc/incus/v7/shared/api"
	incusproc "github.com/lxc/incus/v7/shared/subprocess"
	incusrevert "github.com/lxc/incus/v7/shared/revert"
	incustcp "github.com/lxc/incus/v7/shared/tcp"
	incustls "github.com/lxc/incus/v7/shared/tls"
	incusutil "github.com/lxc/incus/v7/shared/util"

The only tough one here is shared/tls as CertInfo and KeyPairAndCA are somwhat hefty.

retry.go:55, code is unreachable as the loop will exit before

Not sure how you're getting that one, any error that returns true for isRetriableError will move past line 55? I just tested this with

package main

func main() {
	transaction.Retry(context.Background(), 10, func(ctx context.Context) error { return errors.New("database is locked") })
}

And it seems to get past that line just fine.

The notify logic doesn't seem to allow for NotifyAlive anymore?

This is the recent logic you added to Incus to make NotifyAlive less aggressive. Only change is it returns the whole error set and leaves it up to the caller what to do with the errors, given the policy. I did it this way because Incus' definition of a connection error seems tied to how it sets up the client.

Looks like retries are broken? cluster/db/transaction/context.go:118-128 re-uses the dead Tx
context.go:105-116, bad scoping, tx gets re-defined instead of replaced

It's on purpose that tx is only instantiated with force=true (immediately begin a transaction), Whereas BeginDBTX is not called with force=false at all ( to let the callback handle beginning the transaction)

The real callback wrapped in doFunc (lines 71-86) does not use the tx field at all if force=false, in fact it's not exposed as an argument to the callback func signature. I've removed the toplevel tx variable declaration to make this clear.

This follows OC's logic where the transaction is begun by the first DB query/exec call, which implicitly occurs in the doFunc body.

Though you did remind me that I forgot to add retries for the force case entirely. That should just require calling BeginDBTX inside the retry block and passing the resulting tx to doFunc.

[freeekanayaka]

Hello guys,

I didn't quite go through the details yet, and maybe I'm getting the wrong impression, but it feels that this change is somehow Incus (or Incus-ecosystem) specific. Please could you provide a bit more details about how you're going to consume this code? I suppose that you now have something other than Incus that will need it.

Perhaps it'd be cleaner to keep this code in a standalone project? That way go-cowsql would keep providing the low-level primitives, and this clustering code would be an opinionated implementation of higher-level functionalities. Just gut feeling, happy to discuss further.

[stgraber]

Hey @freeekanayaka,

The goal is to provide an easy integration point for projects who want to use cowsql from Go. It's based on a stripped down version of the Incus implementation and we're planning on using that to add cowsql support to Operations Center and are also looking into using it for OpenFGA.

I think having it as a separate package within go-cowsql should be fine and will make it easier to discover and also give us a bit more motivation to actively test and update go-cowsql's go.mod ;)

But I don't want it to have any negative impact to anyone who doesn't want to use this higher level client, which is why we're looking at cleaning up the external dependencies in go.mod (which would also be a problem for something like OpenFGA, so having this be an external repo wouldn't help anyway).

[freeekanayaka]

So just to check if I'm understanding this correctly: we have to concerns to would be directly or indirectly addressed by this PR, 1) make high-level clustering logic re-usable beyond Incus 2) make go-cowsql more "active" so its go.mod will be updated more frequently and won't get stale over time?

I'm not entirely sure if 2) is what you meant in your reply, I'm a bit confused there.

[stgraber]

Yeah, 2) is effectively a consequence of 1).

If instead of landing the higher level fixes in Incus and potentially have folks have to sync that into other codebases, we had that logic in go-cowsql, then those same folks would come here to fix such issues, would have an interest in getting more CI tests and automatic dependency reviews and updates directly in go-cowsql rather than fanning this work across a variety of downstream projects.

The fact that the entire logic is in a separate cluster package within this codebase means effectively no impact to anyone who only cares about the low level logic, that is, so long as we don't need to introduce a long list of extra dependencies for this.

[freeekanayaka]

Ok, then the concerns I have are 1) if we introduce additional dependencies needed by the cluster package, those would become go-cowsql level dependencies that all consumers of go-cowsql would pull in right? 2) this PR has barely any test coverage, so it would be hard to spot regressions solely based on go-cowsql tests (we'd presumably have to run Incus tests to validate any change to the cluster package?).

[freeekanayaka]

One thing that would be nice to have would be a test MVP (e.g. in a cluster/cluster_test.go file) that uses the cluster machinery and showcases how to use it and that has some reasonable level of validation/testing. FWIW there's no particular AI-related policy for go-cowsql, so I'd be ok if that part gets written using an agent if deemed more convenient.

[stgraber]

1. Yeah, that's why I'm having Max working on eliminating those now. A dependency on a very common external package would be fine, but pulling the entire Incus dependency chain, not so much.

2. Agreed, having a basic example of how to run this stuff and having that be exercised in Github Actions would be useful both as example/documentation for anyone interested AND to validate that this all actually works and give us some amount of confidence that things are okay before we merge them into this repo.

[freeekanayaka]

Ok thanks. Maybe we should also some communicate that the cluster module is still experimental and interfaces might change.

[freeekanayaka]

As @stgraber already alluded, I'd say pulling-in Incus as dependency should be avoided, and the number of new dependencies introduced by this PR should ideally be really minimal (afaict this is mostly wiring/networking code that should not need anything fancy).

Also, perhaps introducing some import-level versioning (e.g. github/cowsql/go-cowsql/v0) would be wise until we know that interfaces are stable and we won't need backward-compat breakages.

[stgraber]

@masnax some test failures. Also would be good to expand the test matrix to include:

• ubuntu 22.04, 24.04 and 26.04
• Go stable and oldstable
• amd64 and arm64

All of that should be perfectly fine and easy to do with the free Github Actions runners.

[masnax]

I was able to reproduce that last test failure on the main branch as well after running roles.sh in a loop for a while, so I don′t think it′s a regression.

[stgraber]

@masnax did some automated comparison of logic with Incus and found a few things.

1. The new logic has JSON tags set on the heartbeat structs. The Incus ones currently don't. This can lead to empty structs when an older Incus is receiving a newer struct, or vice-versa which may cause a hang/break during upgrade. There's also a comment saying the Updated isn't being sent on the wire, but now it appears to be included.

2. Still getting Transaction flagged as the retry logic not actually retrying. In cluster/db/transaction/context.go, the retry loop at line 135 reuses one transactionContainer across attempts, but Rollback at line 254 never clears tc.tx. The second attempt gets the rolled-back transaction back from getDBTX and fails with sql: transaction has already been committed or rolled back, which isn't retriable, so the loop exits and the original error is masked. Suggested fix is:

diff --git a/cluster/db/transaction/context.go b/cluster/db/transaction/context.go
index 9ed0026..669d27a 100644
--- a/cluster/db/transaction/context.go
+++ b/cluster/db/transaction/context.go
@@ -257,6 +257,10 @@ func (t *transactionContainer) Rollback() error {
      }

      err := t.tx.Rollback()
+
+     // Forget the finished transaction so a retry opens a fresh one.
+     t.tx = nil
+
      if !errors.Is(err, sql.ErrTxDone) {
              return err
      }

3. The ClusterExternal[any] assertions in Leave and Purge may never match a real implementation as Join[T] asserts ClusterExternal[T] (membership.go:322, 449) while Leave and Purge assert ClusterExternal[any] (lines 1123, 1257). Go interface satisfaction needs identical signatures, so an implementer with a concrete T silently fails the any assertion. The test uses Join[struct{}]. The result is that the "member still has instances or images" refusal on leave and ClearNode on purge are skipped without any error.

4. Incus has a full API check in rebalanceMemberRoles but the new logic only does a lightweight TLS check so we may end up promoting a server that's not currently "ready" (still starting up or shutting down). This may have been logic we fixed in Incus after you started your work on this one.

5. Incus had extra logic in Assign to handle the case where we may have an inconsistent state ("raft entry but no running cowsql node") and automatically recover from it. The new logic errors out instead. That may be fine but would want to be sure.

6. The NotifyAlive from earlier. The main logic itself is fine, but in Incus it's checked twice whereas now it's only checked once, causing an issue. Incus applies NotifyAlive twice. First at selection time, in internal/server/cluster/notify.go:537-551: members whose last heartbeat is older than the offline threshold are skipped. Second at execution time, at lines 580-588 of the Incus file. A peer that looked alive by heartbeat but could not be reached when connecting is logged and ignored under NotifyAlive, and the notifier returns nil. The new notifier at cluster/gateway/notify.go:136-161 appends every hook error to the slice and returns them all, with no policy check. That logic exists so if a server is dead but not past the offline threshold, NotifyAlive still succeeds in notifying the remaining servers. Else the user would need to wait for the offline threshold to hit or else would get an error.

Other smaller issues in bulk:

• Nil dereference risk in the heartbeat handler. gateway.go:394 and :411 use g.info.Name for time-skew warnings. g.info is nil when this member isn't a raft node, and it's read without g.lock. Incus used the daemon's server name.
• Offline threshold inconsistency. Heartbeat() passes the raw field at cluster.go:1036, :1043 and :1094, while NotifyHeartbeat and the example use the accessor with the default fallback. A consumer that never calls SetHeartbeatOfflineThreshold (the example doesn't) computes Online with a zero threshold on the leader.
• Rebalance exceptRoles semantics are inconsistent. The demotion loop at membership.go:668 requires a member to have all of the roles, while the candidate filter at line 715 and the hook use any. Fine with one role, wrong with more.
• Example GetRaftNode violates the found contract (example/node.go:161) by returning an error when missing, so the not-found fallback in nodeAddress never fires.
• QueryRowContext error path leaks a new *sql.DB per call at transaction.go:66. A package-level one would do.
• Dead code in NotifyHeartbeat at membership.go:549. Update always allocates Members.
• TransferLeadership lost its 10 second timeout and now relies on the caller's context. Incus's handover retry loop passes an unbounded one today and would hang on an unreachable cluster.

Issues that already exist in Incus and which we should fix in both:

• cowsqlNetworkDial takes g.lock.Lock() on the 426 path while DialFunc already holds RLock, which is a deadlock if that path is ever hit
• TriggerUpdate runs while holding g.lock, so a pre-update hook that sleeps blocks all dials

[masnax]

4. Incus has a full API check in rebalanceMemberRoles but the new logic only does a lightweight TLS check so we may end up promoting a server that's not currently "ready" (still starting up or shutting down). This may have been logic we fixed in Incus after you started your work on this one.

The function mentioned here is optional, it's there mainly for the example package and OC. Incus would continue to use its own role rebalance implementation.

5. Incus had extra logic in Assign to handle the case where we may have an inconsistent state ("raft entry but no running cowsql node") and automatically recover from it. The new logic errors out instead. That may be fine but would want to be sure.

This is the check that was fixed a few weeks ago, prior to that it would always either panic or return true, so I just dropped the false check entirely instead, given it's never been reached in practice.

6. The NotifyAlive from earlier. The main logic itself is fine, but in Incus it's checked twice whereas now it's only checked once, causing an issue. Incus applies NotifyAlive twice. First at selection time, in internal/server/cluster/notify.go:537-551: members whose last heartbeat is older than the offline threshold are skipped. Second at execution time, at lines 580-588 of the Incus file. A peer that looked alive by heartbeat but could not be reached when connecting is logged and ignored under NotifyAlive, and the notifier returns nil. The new notifier at cluster/gateway/notify.go:136-161 appends every hook error to the slice and returns them all, with no policy check. That logic exists so if a server is dead but not past the offline threshold, NotifyAlive still succeeds in notifying the remaining servers. Else the user would need to wait for the offline threshold to hit or else would get an error.

Line numbers are all wrong here (notify.go has about 100 lines, not 500). But in any case, this was addressed earlier: since go-cowsql isn't setting up the Incus dialer, it leaves it up to the caller to judge which errors are skippable based on the policy.

* Example GetRaftNode violates the found contract (example/node.go:161) by returning an error when missing, so the not-found fallback in nodeAddress never fires.

The not-found fallback checks the found boolean, not the error. I figured that would make more sense than hoping the implementation properly returns the right error type. I think this is comparing the new GetRaftNodes signature to the old nodeAddress implementation?.

* TransferLeadership lost its 10 second timeout and now relies on the caller's context. Incus's handover retry loop passes an unbounded one today and would hang on an unreachable cluster.

No change needed here, should be addressed in the Incus implementation by passing in a context with a 10s timeout.

TriggerUpdate runs while holding g.lock, so a pre-update hook that sleeps blocks all dials

For this one, I don't know if this code path has ever executed because it's based on the COWSQL version string which has been hardcoded at 1 for a long time.

[stgraber]

The not-found fallback checks the found boolean, not the error. I figured that would make more sense than hoping the implementation properly returns the right error type. I think this is comparing the new GetRaftNodes signature to the old nodeAddress implementation?.

The example implementation at example/node.go:161 returns an error instead of nil, false, nil, so the found branch can never fire with the example.

[stgraber]

Couple more nits that were found:

• Inverted guard in the legacy HEAD handler at gateway/cluster.go:284-294 and :319. It now returns 404 only when info is non-nil and not a voter. With nil info it falls through, and the later (info != nil && leader.ID != info.ID) is false, so it answers "I am leader". Unreachable today because g.server == nil already 404s earlier, but it should read info == nil || info.Role != db.RaftVoter.
• Unlocked read of g.networkCert. Releasing the read lock before cowsqlNetworkDial means the cert is read without the lock while NetworkUpdateCert writes under it. It would show under the race detector. The raftDial path already had this, so it is not new in kind.

[stgraber]

Other than that last point (not sure if actually an issue), this looks good to me.

[stgraber]

@freeekanayaka do you want to take a look before we hit merge?

Logic wise, this only affects the new package, changes outside of the new package are only there to satisfy the golangci-lint config, so shouldn't be any real functional changes in there.

We also now have a fair bit more tests being run which should help.

I think my plan from here on would be to merge this, do the Incus conversion, do the Operations Center implementation and once we're happy with all of those, no regression or anything, then we can tag a new release of go-cowsql. This is purely an addition so we don't need a new major or anything.

If after some time we somehow find that this is the only real way go-cowsql is consumed, we could do a major bump which would remove the app package, but I think we're a long way from that, if ever.

[freeekanayaka]

Thanks @stgraber I'll take a look

[freeekanayaka]

Hello, I gave a very quick look, istm that there are changes unrelated to the gateway package. Looks like they are mostly style-related changes accross the whole codebase. Please can we extract those changes into a separate PR, so they can be reviewed/merged separately? Thanks

[freeekanayaka]

My preference would be to have a PR of each independent type of change. E.g: 1) bumping CI dependencies (ubuntu 26.04) 2) style-related changes ("if err := foo(); bar() -> err := foo() \n if err { bar() }") etc. This way it's clear to assess the impact of each change and be sure they are unrelated.

[stgraber]

@masnax can you make a separate CI & static analysis PR which adds in the golangci-lint config and fixes the existing codebase, then adds the new Github workflow test matrix?

That will be trivial to review and merge, then we can have this PR effectively only touch the new Go package.

[stgraber]

@freeekanayaka want to take a look? It's all good on my end from a manual review and automated review. PR is now self-contained to a single additional Go package and only testify gets added to go.mod.

Next once merged will be to move Incus over to it, getting us a bunch more validation through Incus' extensive clustering testsuite. Then we can move on to do Operations Center and other projects where we want that logic.

[freeekanayaka]

@stgraber thanks, the example shows a working wiring and has tests and that's very useful. It seems there's quite a bit of boilerplate/wiring needed for consumers, but I understand the functionality is complex and this is needed for the moment to maintain enough flexibility. If it's enough for you guys I'm okay with merging, will do that now.

[stgraber]

@stgraber thanks, the example shows a working wiring and has tests and that's very useful. It seems there's quite a bit of boilerplate/wiring needed for consumers, but I understand the functionality is complex and this is needed for the moment to maintain enough flexibility. If it's enough for you guys I'm okay with merging, will do that now.

Thanks!

And yeah, for a very simple implementation, the app package still exists and is much lighter weight.