Skip to content

fix(schemer): fail table migration when the peer schema cannot be read - #2144

Open
theluckystrike wants to merge 1 commit into
Altinity:0.28.0from
theluckystrike:fix/schemer-peer-schema-error
Open

theluckystrike wants to merge 1 commit into
Altinity:0.28.0from
theluckystrike:fix/schemer-peer-schema-error

Conversation

@theluckystrike

Copy link
Copy Markdown

Fixes #2129.

When the operator adds tables to a replica, it reads the CREATE statements from the other hosts of the cluster. If that read failed on every peer, the failure looked like an empty schema. queryUnzipColumns returned nil when QueryAny failed, debugCreateSQLs logged the error and dropped it, and createTablesSQLs ignored the error of both getters. So HostCreateTables ran no DDL and returned nil, migrateTables logged "Tables added successfully" and put the host in status.hostsWithTablesCreated. After that shouldMigrateTables skips the host, so the rebuilt replica stays without tables. The issue hits this when all peers answer Code 516 because the users.d ConfigMap with the new operator pod IP has not reached them yet.

This change passes the error up instead. queryUnzipColumns returns the QueryAny error, debugCreateSQLs returns the error it was given, the replicated and distributed getters stop at the first failed query, and HostCreateTables returns "unable to read schema from peers of host ..." without running any DDL. migrateTables already wraps a HostCreateTables error in ErrCRUDDeferred and does not record the host, so the migration runs again on a later reconcile.

Behaviour stays the same in the other cases. QueryAny still uses the first peer that answers, so the error only appears when no peer could be read. A done context still returns nil, as before. New installations skip the migration through IsInNewCluster. HostSyncTables and the drop replica path call QueryUnzip2Columns and already ignore its error, so they behave as before.

Two tests were added in pkg/model/chi/schemer/schemer_test.go. TestQueryUnzipReportsRejectedPeer points QueryUnzip2Columns and QueryUnzipAndApplyUUIDs at a local httptest server that answers HTTP 403 with a Code 516 body and expects an error. TestHostCreateTablesFailsWhenPeersCannotBeRead builds a CHI with 1 shard and 2 replicas whose namespaceDomainPattern ends in .invalid, so the peer cannot be reached, and expects HostCreateTables to return an error. On 0.28.0 (5d6c2fe) both fail with "An error is expected but got nil". With this change both pass.

I ran go test -vet=off -count=1 ./... on 0.28.0 and on this branch with Go 1.27.3 on macOS. This branch has 40 packages ok and 2337 passing tests, and 0.28.0 has 39 and 2335. The difference is the two new tests in a package that had no tests before. Both runs show the same 325 failing test lines, all in pkg/model/chbackup, because the job scripts need bash 4.4 and macOS ships bash 3.2. I used -vet=off because with Go 1.27.3 the vet step of go test stops pkg/controller/chi, pkg/controller/chk and pkg/controller/common/statefulset on non-constant format strings in files this PR does not touch. The same happens on 0.28.0. With vet off those packages pass. go vet ./pkg/model/chi/schemer/ ./pkg/model/clickhouse/ is clean and gofmt reports nothing for the changed files.

Not run. The e2e suite, a real ClickHouse server and a Kubernetes cluster. I did not reproduce the PVC deletion from the issue on a live cluster.

Template checklist. The PR is one commit, it targets 0.28.0 and the commit is signed off.

queryUnzipColumns returned nil when QueryAny failed on every peer,
debugCreateSQLs dropped the error and createTablesSQLs ignored it, so
HostCreateTables ran no DDL and returned nil. migrateTables then recorded
the host in hostsWithTablesCreated and never migrated it again.

Propagate the error up to HostCreateTables so migrateTables defers the
host and retries the migration.

Fixes Altinity#2129

Signed-off-by: theluckystrike <51033404+theluckystrike@users.noreply.github.com>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant