diff --git a/cmd/api/schema.go b/cmd/api/schema.go new file mode 100644 index 00000000..492dc96b --- /dev/null +++ b/cmd/api/schema.go @@ -0,0 +1,117 @@ +package api + +import ( + "context" + "fmt" + "sort" + "strings" + + "github.com/go-admin-team/go-admin-core/v2/sdk" + "gorm.io/gorm" + + "go-admin/cmd/migrate/migration" + "go-admin/common/health" + commonmodels "go-admin/common/models" +) + +// schemaCheckName is what a failing schema reports itself as in /ready's body. +const schemaCheckName = "schema" + +// registerSchemaCheck adds the pending-migration check to readiness. +// +// Readiness rather than a refusal to start, and rather than a log line alone. +// The two probes answer different questions: liveness is "restart me", and a +// process whose database is on the wrong schema comes back to the same schema, +// so restarting is not the answer. Readiness is "send me requests", and with a +// schema the binary does not match the answer is no. +// +// Issue #919 is what the absence of this looked like: the process started, +// both probes passed, and the first sign of trouble was a login failing with a +// driver-level encoding error. Refusing to start would have been the wrong fix +// - a process that exits tells an operator less than one that runs and says +// why, and under an orchestrator it crash-loops - while a log line alone is +// not something an orchestrator can act on. +func registerSchemaCheck() { + health.Register(schemaCheckName, schemaCheck) +} + +// schemaCheck fails while any tenant database is behind the migrations this +// binary registers. +// +// Any one of them, rather than only the tenant being served: migrations are +// applied to every database in one run, so one database behind means that run +// did not finish. Serving the rest would let a half-applied deploy look like a +// partial success. +// +// Evaluated per request rather than decided at start-up, so that running +// migrate clears it without a restart. +func schemaCheck(ctx context.Context) error { + registered := migration.RegisteredVersions() + if len(registered) == 0 { + // Nothing registered means nothing can be pending, which is the honest + // answer for a tree with no migrations. It is also what a broken build + // would produce - the registry is filled by init() in packages the + // binary has to link - so cmd/api's dependency test asserts the real + // binary links them. + return nil + } + + behind := make([]string, 0, 2) + for name, db := range sdk.Runtime.GetAllDb() { + applied, err := appliedVersions(ctx, db) + if err != nil { + return fmt.Errorf("reading applied migrations for %q: %w", name, err) + } + if pending := pendingVersions(registered, applied); len(pending) > 0 { + behind = append(behind, fmt.Sprintf("%s is %d behind, first pending %s", + name, len(pending), pending[0])) + } + } + if len(behind) == 0 { + return nil + } + sort.Strings(behind) + return fmt.Errorf("%s; run `go-admin migrate -c ` and see `go-admin migrate status`", + strings.Join(behind, "; ")) +} + +// appliedVersions reads what sys_migration records for one database. +// +// A missing table is not an error: a database that has never been migrated has +// applied nothing, which is exactly what the caller needs to hear, and is the +// state a first deploy is in. +func appliedVersions(ctx context.Context, db *gorm.DB) (map[string]bool, error) { + if db == nil { + return nil, fmt.Errorf("no database") + } + db = db.WithContext(ctx) + if !db.Migrator().HasTable(&commonmodels.Migration{}) { + return map[string]bool{}, nil + } + var rows []commonmodels.Migration + if err := db.Select("version").Find(&rows).Error; err != nil { + return nil, err + } + out := make(map[string]bool, len(rows)) + for _, r := range rows { + out[r.Version] = true + } + return out, nil +} + +// pendingVersions returns the registered versions applied does not contain. +// +// Split out and taking both sides as arguments because the registry is +// process-wide and filled by init() in packages cmd/api does not import: a +// test in this package cannot arrange it, so the arranging part is the part +// that is not tested here. +func pendingVersions(registered []string, applied map[string]bool) []string { + out := make([]string, 0) + for _, v := range registered { + if !applied[v] { + out = append(out, v) + } + } + sort.Strings(out) + return out +} diff --git a/cmd/api/schema_test.go b/cmd/api/schema_test.go new file mode 100644 index 00000000..505e0cf5 --- /dev/null +++ b/cmd/api/schema_test.go @@ -0,0 +1,124 @@ +package api + +import ( + "context" + "os/exec" + "strings" + "testing" + + "github.com/glebarez/sqlite" + "gorm.io/gorm" + + commonmodels "go-admin/common/models" +) + +func TestPendingVersionsReportsOnlyWhatIsNotApplied(t *testing.T) { + registered := []string{"1000_a", "2000_b", "3000_c"} + applied := map[string]bool{"1000_a": true, "3000_c": true} + + got := pendingVersions(registered, applied) + if len(got) != 1 || got[0] != "2000_b" { + t.Errorf("pending = %v, want [2000_b]", got) + } +} + +func TestPendingVersionsIsEmptyWhenTheDatabaseIsCurrent(t *testing.T) { + registered := []string{"1000_a", "2000_b"} + applied := map[string]bool{"1000_a": true, "2000_b": true} + + if got := pendingVersions(registered, applied); len(got) != 0 { + t.Errorf("pending = %v, want none", got) + } +} + +// A row recorded that this binary no longer registers is not pending. It is +// the orphan `migrate status` already reports, and readiness has nothing to +// say about it: the schema is ahead, not behind, and requests will be served +// correctly. +func TestPendingVersionsIgnoresAppliedRowsNothingRegisters(t *testing.T) { + registered := []string{"1000_a"} + applied := map[string]bool{"1000_a": true, "9999_gone": true} + + if got := pendingVersions(registered, applied); len(got) != 0 { + t.Errorf("pending = %v, want none - an orphaned row is not a pending migration", got) + } +} + +func memoryDB(t *testing.T) *gorm.DB { + t.Helper() + db, err := gorm.Open(sqlite.Open("file::memory:?cache=shared"), &gorm.Config{}) + if err != nil { + t.Fatalf("open: %v", err) + } + t.Cleanup(func() { db.Migrator().DropTable(&commonmodels.Migration{}) }) + return db +} + +// A first deploy has no sys_migration table. That is "nothing applied", not an +// error: reporting it as one would make the check fail for a reason the +// operator cannot act on, on the one deployment where every migration really +// is pending. +func TestAppliedVersionsTreatsAMissingTableAsNothingApplied(t *testing.T) { + db := memoryDB(t) + db.Migrator().DropTable(&commonmodels.Migration{}) + + got, err := appliedVersions(context.Background(), db) + if err != nil { + t.Fatalf("appliedVersions: %v", err) + } + if len(got) != 0 { + t.Errorf("applied = %v, want empty", got) + } +} + +func TestAppliedVersionsReadsWhatTheTableHolds(t *testing.T) { + db := memoryDB(t) + if err := db.AutoMigrate(&commonmodels.Migration{}); err != nil { + t.Fatalf("automigrate: %v", err) + } + db.Create(&commonmodels.Migration{Version: "1000_a"}) + db.Create(&commonmodels.Migration{Version: "2000_b"}) + + got, err := appliedVersions(context.Background(), db) + if err != nil { + t.Fatalf("appliedVersions: %v", err) + } + if !got["1000_a"] || !got["2000_b"] || len(got) != 2 { + t.Errorf("applied = %v, want the two rows written", got) + } +} + +// The check is only worth anything if the registry it reads is populated in +// the binary that serves requests, and it is filled by init() in packages +// cmd/api does not import - cmd/migrate blank-imports them, and cmd wires both +// subcommands into one binary. +// +// This cannot be asserted from an ordinary test: importing the version package +// to look at the registry would put it in the test binary's dependency graph +// and pass whatever the real binary links. So ask the build instead. +// +// Without this, dropping those blank imports leaves a check that reports +// "nothing pending" for every database forever, and every test above still +// passes. +func TestTheServingBinaryLinksTheMigrationRegistry(t *testing.T) { + out, err := exec.Command("go", "list", "-deps", "go-admin").Output() + if err != nil { + t.Skipf("go list unavailable: %v", err) + } + deps := string(out) + + const versions = "go-admin/cmd/migrate/migration/version" + if !strings.Contains(deps, versions+"\n") { + t.Errorf("the main package does not link %s, so the schema check would "+ + "read an empty registry and report every database as current", versions) + } + + // Negative control: a package the binary genuinely must not link, so that a + // `deps` that somehow contained everything would fail here rather than pass + // the assertion above for the wrong reason. + const notLinked = "go-admin/tools/checksilent" + if strings.Contains(deps, notLinked+"\n") { + t.Errorf("%s is in the binary's dependency closure, so this test cannot "+ + "tell a real link from a query that matches anything", notLinked) + } +} diff --git a/cmd/api/server.go b/cmd/api/server.go index 7bc45c58..f7ebbd4e 100644 --- a/cmd/api/server.go +++ b/cmd/api/server.go @@ -82,6 +82,11 @@ func setup() { // can call the API, which is only true once the socket is accepting. sdk.Runtime.SetPhase(runtime.AfterListen, startCronJobs) + // Registered before the configuration is read, because it registers a + // callback rather than reading anything: the check runs per request and + // asks the databases that exist then. + registerSchemaCheck() + //1. 读取配置 bootstrap.SetupConfig( file.NewSource(file.WithPath(configYml)), diff --git a/cmd/migrate/migration/init.go b/cmd/migrate/migration/init.go index 669bec65..309d41c2 100644 --- a/cmd/migrate/migration/init.go +++ b/cmd/migrate/migration/init.go @@ -185,6 +185,28 @@ func (e *Migration) mergedEntries() map[string]versionEntry { return out } +// RegisteredVersions returns every migration version this binary registers, +// sorted, without touching a database. +// +// Status answers a richer question - what is registered, what is applied, and +// what is applied while nothing registers it - and needs a database to do it. +// This is the half that can be asked of the process alone, which is what a +// readiness check needs: the check holds the databases it is asking about, and +// reusing Status would mean calling SetDb from a request handler, writing this +// package's shared state from a request path. +func (e *Migration) RegisteredVersions() []string { + all := e.mergedEntries() + out := make([]string, 0, len(all)) + for k := range all { + out = append(out, k) + } + sort.Strings(out) + return out +} + +// RegisteredVersions reports what the process-wide registry holds. +func RegisteredVersions() []string { return Migrate.RegisteredVersions() } + // StatusEntry is one row of migrate status. type StatusEntry struct { AppCode string diff --git a/common/health/health.go b/common/health/health.go index 4d4095eb..b6311120 100644 --- a/common/health/health.go +++ b/common/health/health.go @@ -41,6 +41,7 @@ import ( "encoding/hex" "errors" "fmt" + "sync" "sync/atomic" "time" @@ -68,6 +69,55 @@ type Check struct { Err string `json:"error,omitempty"` } +// extra are checks a host registers that this package cannot make itself. +// +// The direction is why this exists. Whether the schema matches what the binary +// expects is answered by the migration registry, which lives under cmd/ - and +// common/ has never imported cmd/. Rather than start, the host registers the +// check from where both are already in scope. +var ( + extraMu sync.RWMutex + extra []namedCheck +) + +type namedCheck struct { + name string + fn func(context.Context) error +} + +// Register adds a check to what Ready asks. +// +// It panics on a duplicate name rather than replacing or appending: two checks +// under one name make the failing one impossible to identify from the response, +// and registering the same one twice is a wiring mistake worth hearing about at +// start-up rather than never. +func Register(name string, fn func(context.Context) error) { + if name == "" { + panic("health: a registered check needs a name") + } + if fn == nil { + panic("health: check " + name + " is nil") + } + extraMu.Lock() + defer extraMu.Unlock() + for _, c := range extra { + if c.name == name { + panic("health: check " + name + " is already registered") + } + } + extra = append(extra, namedCheck{name: name, fn: fn}) +} + +// registered returns the checks a host has added, copied so that Ready is not +// iterating the slice while another goroutine appends to it. +func registered() []namedCheck { + extraMu.RLock() + defer extraMu.RUnlock() + out := make([]namedCheck, len(extra)) + copy(out, extra) + return out +} + // Ready asks every dependency this process cannot serve a request without. // // The queue is deliberately absent. Nothing on AdapterQueue answers "are you @@ -75,10 +125,14 @@ type Check struct { // a queue that is down degrades logging rather than stopping requests - which // is a reason to alert, not a reason to leave the load balancer pool. func Ready(ctx context.Context) []Check { - return []Check{ + checks := []Check{ safely("database", func() error { return pingDB(ctx) }), safely("cache", probeCache), } + for _, c := range registered() { + checks = append(checks, safely(c.name, func() error { return c.fn(ctx) })) + } + return checks } // safely turns a panic into a failed check. diff --git a/common/health/register_test.go b/common/health/register_test.go new file mode 100644 index 00000000..9edb6e19 --- /dev/null +++ b/common/health/register_test.go @@ -0,0 +1,125 @@ +package health + +import ( + "context" + "errors" + "testing" +) + +// isolate empties the registered checks and puts them back, so one test in +// this package cannot decide what the next one sees. +func isolate(t *testing.T) { + t.Helper() + extraMu.Lock() + previous := extra + extra = nil + extraMu.Unlock() + t.Cleanup(func() { + extraMu.Lock() + extra = previous + extraMu.Unlock() + }) +} + +func findCheck(checks []Check, name string) (Check, bool) { + for _, c := range checks { + if c.Name == name { + return c, true + } + } + return Check{}, false +} + +// A registered check has to reach Ready's answer, or the host has wired +// something that never gets asked. +func TestARegisteredCheckIsAsked(t *testing.T) { + isolate(t) + Register("schema", func(context.Context) error { return errors.New("two behind") }) + + checks := Ready(context.Background()) + c, ok := findCheck(checks, "schema") + if !ok { + t.Fatal("Ready did not ask the registered check") + } + if c.OK { + t.Error("the check returned an error and was still reported OK") + } + if c.Err != "two behind" { + t.Errorf("Err = %q, want the check's own message", c.Err) + } + if Healthy(checks) { + t.Error("Healthy said yes while a registered check was failing") + } +} + +// The context Ready is given has to reach the check: it carries the probe's +// deadline, and a check that ignores it can hold the handler past it. +func TestTheRegisteredCheckIsGivenReadysContext(t *testing.T) { + isolate(t) + type key struct{} + Register("ctx", func(ctx context.Context) error { + if ctx.Value(key{}) != "carried" { + return errors.New("the check was handed a different context") + } + return nil + }) + + checks := Ready(context.WithValue(context.Background(), key{}, "carried")) + c, ok := findCheck(checks, "ctx") + if !ok { + t.Fatal("the registered check was not asked") + } + if !c.OK { + t.Errorf("check failed: %s", c.Err) + } +} + +// A check that panics must not take the process down through the probe, the +// same guarantee the built-in checks have. +func TestARegisteredCheckThatPanicsFailsRatherThanCrashes(t *testing.T) { + isolate(t) + Register("boom", func(context.Context) error { panic("registry unreachable") }) + + checks := Ready(context.Background()) + c, ok := findCheck(checks, "boom") + if !ok { + t.Fatal("the registered check was not asked") + } + if c.OK { + t.Error("a panicking check was reported OK") + } +} + +func TestRegisteringTheSameNameTwicePanics(t *testing.T) { + isolate(t) + Register("dup", func(context.Context) error { return nil }) + + defer func() { + if recover() == nil { + t.Error("registering a duplicate name did not panic; two checks under " + + "one name make the failing one impossible to identify") + } + }() + Register("dup", func(context.Context) error { return nil }) +} + +func TestRegisterRefusesAnEmptyNameOrNilCheck(t *testing.T) { + isolate(t) + for _, tc := range []struct { + name string + fn func(context.Context) error + why string + }{ + {"", func(context.Context) error { return nil }, "empty name"}, + {"nilfn", nil, "nil function"}, + } { + func() { + defer func() { + if recover() == nil { + t.Errorf("%s did not panic", tc.why) + } + }() + Register(tc.name, tc.fn) + }() + } +}