[arvados] created: 2.1.0-3172-g228028e52

git repository hosting git at public.arvados.org
Mon Dec 12 21:46:13 UTC 2022


        at  228028e523983a847239fd4a5530d3e54b27cfa5 (commit)


commit 228028e523983a847239fd4a5530d3e54b27cfa5
Author: Tom Clegg <tom at curii.com>
Date:   Mon Dec 12 16:46:03 2022 -0500

    19709: Test schema_migrations vs. db/migrate/*.rb.
    
    Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at curii.com>

diff --git a/lib/boot/rails_db.go b/lib/boot/rails_db.go
index 44ceb263c..ad9124d7d 100644
--- a/lib/boot/rails_db.go
+++ b/lib/boot/rails_db.go
@@ -6,6 +6,7 @@ package boot
 
 import (
 	"context"
+	"fmt"
 	"io/fs"
 	"os"
 	"path/filepath"
@@ -47,28 +48,10 @@ func (runner railsDatabase) Run(ctx context.Context, fail func(error), super *Su
 	// there are no new migrations, that would add ~2s to startup
 	// time / downtime during service restart.
 
-	todo := map[string]bool{}
-
-	// list versions in db/migrate/{version}_{name}.rb
-	fs.WalkDir(os.DirFS(appdir), "db/migrate", func(path string, d fs.DirEntry, err error) error {
-		fnm := d.Name()
-		if !strings.HasSuffix(fnm, ".rb") {
-			return nil
-		}
-		for i, c := range fnm {
-			if i > 0 && c == '_' {
-				todo[fnm[:i]] = true
-				break
-			}
-			if c < '0' || c > '9' {
-				// non-numeric character before the
-				// first '_' means this is not a
-				// migration
-				break
-			}
-		}
-		return nil
-	})
+	todo, err := migrationList(appdir)
+	if err != nil {
+		return err
+	}
 
 	// read schema_migrations table (list of migrations already
 	// applied) and remove those entries from todo
@@ -113,3 +96,35 @@ func (runner railsDatabase) Run(ctx context.Context, fail func(error), super *Su
 	defer dblock.RailsMigrations.Unlock()
 	return super.RunProgram(ctx, appdir, runOptions{env: railsEnv}, "bundle", "exec", "rake", "db:migrate")
 }
+
+func migrationList(dir string) (map[string]bool, error) {
+	todo := map[string]bool{}
+
+	// list versions in db/migrate/{version}_{name}.rb
+	err := fs.WalkDir(os.DirFS(dir), "db/migrate", func(path string, d fs.DirEntry, err error) error {
+		if d.IsDir() {
+			return nil
+		}
+		fnm := d.Name()
+		if !strings.HasSuffix(fnm, ".rb") {
+			return fmt.Errorf("unexpected file in db/migrate dir: %s", fnm)
+		}
+		for i, c := range fnm {
+			if i > 0 && c == '_' {
+				todo[fnm[:i]] = true
+				break
+			}
+			if c < '0' || c > '9' {
+				// non-numeric character before the
+				// first '_' means this is not a
+				// migration
+				return fmt.Errorf("unexpected file in db/migrate dir: %s", fnm)
+			}
+		}
+		return nil
+	})
+	if err != nil {
+		return nil, err
+	}
+	return todo, nil
+}
diff --git a/lib/boot/rails_db_test.go b/lib/boot/rails_db_test.go
new file mode 100644
index 000000000..ac78a3c9a
--- /dev/null
+++ b/lib/boot/rails_db_test.go
@@ -0,0 +1,47 @@
+// Copyright (C) The Arvados Authors. All rights reserved.
+//
+// SPDX-License-Identifier: AGPL-3.0
+
+package boot
+
+import (
+	"git.arvados.org/arvados.git/lib/config"
+	"git.arvados.org/arvados.git/sdk/go/arvadostest"
+	"git.arvados.org/arvados.git/sdk/go/ctxlog"
+	"gopkg.in/check.v1"
+)
+
+type railsDBSuite struct{}
+
+var _ = check.Suite(&railsDBSuite{})
+
+// Check services/api/db/migrate/*.rb match schema_migrations
+func (s *railsDBSuite) TestMigrationList(c *check.C) {
+	todo, err := migrationList("../../services/api")
+	c.Check(err, check.IsNil)
+	c.Check(todo["20220804133317"], check.Equals, true)
+
+	cfg, err := config.NewLoader(nil, ctxlog.TestLogger(c)).Load()
+	c.Assert(err, check.IsNil)
+	cluster, err := cfg.GetCluster("")
+	c.Assert(err, check.IsNil)
+	db := arvadostest.DB(c, cluster)
+	rows, err := db.Query(`SELECT version FROM schema_migrations`)
+	for rows.Next() {
+		var v string
+		err = rows.Scan(&v)
+		c.Assert(err, check.IsNil)
+		if !todo[v] {
+			c.Errorf("version is in schema_migrations but not services/api/db/migrate/: %q", v)
+		}
+		delete(todo, v)
+	}
+	err = rows.Close()
+	c.Assert(err, check.IsNil)
+
+	// In the test suite, the database should be fully migrated.
+	// So, if there's anything left in todo here, there is
+	// something wrong with our "db/migrate/*.rb ==
+	// schema_migrations" reasoning.
+	c.Check(todo, check.HasLen, 0)
+}

commit 31d3138660ee51eccaceb52f99730b6124755b93
Author: Tom Clegg <tom at curii.com>
Date:   Fri Dec 9 15:01:17 2022 -0500

    19709: Explain design in comments. Improve migration filename check.
    
    Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at curii.com>

diff --git a/lib/boot/rails_db.go b/lib/boot/rails_db.go
index 3f8151144..44ceb263c 100644
--- a/lib/boot/rails_db.go
+++ b/lib/boot/rails_db.go
@@ -21,6 +21,10 @@ func (runner railsDatabase) String() string {
 	return "railsDatabase"
 }
 
+// Run checks for and applies any pending Rails database migrations.
+//
+// If running a dev/test environment, and the database is empty, it
+// initializes the database.
 func (runner railsDatabase) Run(ctx context.Context, fail func(error), super *Supervisor) error {
 	err := super.wait(ctx, runPostgreSQL{}, installPassenger{src: "services/api"})
 	if err != nil {
@@ -35,11 +39,33 @@ func (runner railsDatabase) Run(ctx context.Context, fail func(error), super *Su
 		appdir = filepath.Join(super.SourcePath, "services/api")
 	}
 
-	// list versions in db/migrate/{version}_{name}.rb
+	// Check for pending migrations before running rake.
+	//
+	// In principle, we could use "rake db:migrate:status" or skip
+	// this check entirely and let "rake db:migrate" be a no-op
+	// most of the time.  However, in the most common case when
+	// there are no new migrations, that would add ~2s to startup
+	// time / downtime during service restart.
+
 	todo := map[string]bool{}
+
+	// list versions in db/migrate/{version}_{name}.rb
 	fs.WalkDir(os.DirFS(appdir), "db/migrate", func(path string, d fs.DirEntry, err error) error {
-		if cut := strings.Index(d.Name(), "_"); cut > 0 && strings.HasSuffix(d.Name(), ".rb") {
-			todo[d.Name()[:cut]] = true
+		fnm := d.Name()
+		if !strings.HasSuffix(fnm, ".rb") {
+			return nil
+		}
+		for i, c := range fnm {
+			if i > 0 && c == '_' {
+				todo[fnm[:i]] = true
+				break
+			}
+			if c < '0' || c > '9' {
+				// non-numeric character before the
+				// first '_' means this is not a
+				// migration
+				break
+			}
 		}
 		return nil
 	})

commit 41b9b83812826b77034fe13ea34047e194b1027f
Author: Tom Clegg <tom at curii.com>
Date:   Mon Nov 14 15:31:42 2022 -0500

    19709: Apply pending rails migrations at service start/restart.
    
    Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at curii.com>

diff --git a/lib/boot/rails_db.go b/lib/boot/rails_db.go
new file mode 100644
index 000000000..3f8151144
--- /dev/null
+++ b/lib/boot/rails_db.go
@@ -0,0 +1,89 @@
+// Copyright (C) The Arvados Authors. All rights reserved.
+//
+// SPDX-License-Identifier: AGPL-3.0
+
+package boot
+
+import (
+	"context"
+	"io/fs"
+	"os"
+	"path/filepath"
+	"strings"
+
+	"git.arvados.org/arvados.git/lib/controller/dblock"
+	"git.arvados.org/arvados.git/lib/ctrlctx"
+)
+
+type railsDatabase struct{}
+
+func (runner railsDatabase) String() string {
+	return "railsDatabase"
+}
+
+func (runner railsDatabase) Run(ctx context.Context, fail func(error), super *Supervisor) error {
+	err := super.wait(ctx, runPostgreSQL{}, installPassenger{src: "services/api"})
+	if err != nil {
+		return err
+	}
+
+	// determine path to installed rails app or source tree
+	var appdir string
+	if super.ClusterType == "production" {
+		appdir = "/var/lib/arvados/railsapi"
+	} else {
+		appdir = filepath.Join(super.SourcePath, "services/api")
+	}
+
+	// list versions in db/migrate/{version}_{name}.rb
+	todo := map[string]bool{}
+	fs.WalkDir(os.DirFS(appdir), "db/migrate", func(path string, d fs.DirEntry, err error) error {
+		if cut := strings.Index(d.Name(), "_"); cut > 0 && strings.HasSuffix(d.Name(), ".rb") {
+			todo[d.Name()[:cut]] = true
+		}
+		return nil
+	})
+
+	// read schema_migrations table (list of migrations already
+	// applied) and remove those entries from todo
+	dbconnector := ctrlctx.DBConnector{PostgreSQL: super.cluster.PostgreSQL}
+	defer dbconnector.Close()
+	db, err := dbconnector.GetDB(ctx)
+	if err != nil {
+		return err
+	}
+	rows, err := db.QueryContext(ctx, `SELECT version FROM schema_migrations`)
+	if err != nil {
+		if super.ClusterType == "production" {
+			return err
+		}
+		super.logger.WithError(err).Info("schema_migrations query failed, trying db:setup")
+		return super.RunProgram(ctx, "services/api", runOptions{env: railsEnv}, "bundle", "exec", "rake", "db:setup")
+	}
+	for rows.Next() {
+		var v string
+		err = rows.Scan(&v)
+		if err != nil {
+			return err
+		}
+		delete(todo, v)
+	}
+	err = rows.Close()
+	if err != nil {
+		return err
+	}
+
+	// if nothing remains in todo, all available migrations are
+	// done, so return without running any [relatively slow]
+	// ruby/rake commands
+	if len(todo) == 0 {
+		return nil
+	}
+
+	super.logger.Infof("%d migrations pending", len(todo))
+	if !dblock.RailsMigrations.Lock(ctx, dbconnector.GetDB) {
+		return context.Canceled
+	}
+	defer dblock.RailsMigrations.Unlock()
+	return super.RunProgram(ctx, appdir, runOptions{env: railsEnv}, "bundle", "exec", "rake", "db:migrate")
+}
diff --git a/lib/boot/seed.go b/lib/boot/seed.go
deleted file mode 100644
index b43d90720..000000000
--- a/lib/boot/seed.go
+++ /dev/null
@@ -1,31 +0,0 @@
-// Copyright (C) The Arvados Authors. All rights reserved.
-//
-// SPDX-License-Identifier: AGPL-3.0
-
-package boot
-
-import (
-	"context"
-)
-
-// Populate a blank database with arvados tables and seed rows.
-type seedDatabase struct{}
-
-func (seedDatabase) String() string {
-	return "seedDatabase"
-}
-
-func (seedDatabase) Run(ctx context.Context, fail func(error), super *Supervisor) error {
-	err := super.wait(ctx, runPostgreSQL{}, installPassenger{src: "services/api"})
-	if err != nil {
-		return err
-	}
-	if super.ClusterType == "production" {
-		return nil
-	}
-	err = super.RunProgram(ctx, "services/api", runOptions{env: railsEnv}, "bundle", "exec", "rake", "db:setup")
-	if err != nil {
-		return err
-	}
-	return nil
-}
diff --git a/lib/boot/supervisor.go b/lib/boot/supervisor.go
index ca88653fa..0f0600f18 100644
--- a/lib/boot/supervisor.go
+++ b/lib/boot/supervisor.go
@@ -356,20 +356,24 @@ func (super *Supervisor) runCluster() error {
 		createCertificates{},
 		runPostgreSQL{},
 		runNginx{},
-		runServiceCommand{name: "controller", svc: super.cluster.Services.Controller, depends: []supervisedTask{seedDatabase{}}},
+		railsDatabase{},
+		runServiceCommand{name: "controller", svc: super.cluster.Services.Controller, depends: []supervisedTask{railsDatabase{}}},
 		runServiceCommand{name: "git-httpd", svc: super.cluster.Services.GitHTTP},
 		runServiceCommand{name: "health", svc: super.cluster.Services.Health},
 		runServiceCommand{name: "keepproxy", svc: super.cluster.Services.Keepproxy, depends: []supervisedTask{runPassenger{src: "services/api"}}},
 		runServiceCommand{name: "keepstore", svc: super.cluster.Services.Keepstore},
 		runServiceCommand{name: "keep-web", svc: super.cluster.Services.WebDAV},
-		runServiceCommand{name: "ws", svc: super.cluster.Services.Websocket, depends: []supervisedTask{seedDatabase{}}},
+		runServiceCommand{name: "ws", svc: super.cluster.Services.Websocket, depends: []supervisedTask{railsDatabase{}}},
 		installPassenger{src: "services/api", varlibdir: "railsapi"},
-		runPassenger{src: "services/api", varlibdir: "railsapi", svc: super.cluster.Services.RailsAPI, depends: []supervisedTask{createCertificates{}, seedDatabase{}, installPassenger{src: "services/api", varlibdir: "railsapi"}}},
-		seedDatabase{},
+		runPassenger{src: "services/api", varlibdir: "railsapi", svc: super.cluster.Services.RailsAPI, depends: []supervisedTask{
+			createCertificates{},
+			installPassenger{src: "services/api", varlibdir: "railsapi"},
+			railsDatabase{},
+		}},
 	}
 	if !super.NoWorkbench1 {
 		tasks = append(tasks,
-			installPassenger{src: "apps/workbench", varlibdir: "workbench1", depends: []supervisedTask{seedDatabase{}}}, // dependency ensures workbench doesn't delay api install/startup
+			installPassenger{src: "apps/workbench", varlibdir: "workbench1", depends: []supervisedTask{railsDatabase{}}}, // dependency ensures workbench doesn't delay api install/startup
 			runPassenger{src: "apps/workbench", varlibdir: "workbench1", svc: super.cluster.Services.Workbench1, depends: []supervisedTask{installPassenger{src: "apps/workbench", varlibdir: "workbench1"}}},
 		)
 	}
diff --git a/lib/controller/dblock/dblock.go b/lib/controller/dblock/dblock.go
index ad2733abf..c59bcef0b 100644
--- a/lib/controller/dblock/dblock.go
+++ b/lib/controller/dblock/dblock.go
@@ -22,6 +22,7 @@ var (
 	KeepBalanceService = &DBLocker{key: 10003} // keep-balance service in periodic-sweep loop
 	KeepBalanceActive  = &DBLocker{key: 10004} // keep-balance sweep in progress (either -once=true or service loop)
 	Dispatch           = &DBLocker{key: 10005} // any dispatcher running
+	RailsMigrations    = &DBLocker{key: 10006}
 	retryDelay         = 5 * time.Second
 )
 
diff --git a/lib/ctrlctx/db.go b/lib/ctrlctx/db.go
index 2a05096ce..b711b3e65 100644
--- a/lib/ctrlctx/db.go
+++ b/lib/ctrlctx/db.go
@@ -168,8 +168,20 @@ func (dbc *DBConnector) GetDB(ctx context.Context) (*sqlx.DB, error) {
 	}
 	if err := db.Ping(); err != nil {
 		ctxlog.FromContext(ctx).WithError(err).Error("postgresql connect succeeded but ping failed")
+		db.Close()
 		return nil, errDBConnection
 	}
 	dbc.pgdb = db
 	return db, nil
 }
+
+func (dbc *DBConnector) Close() error {
+	dbc.mtx.Lock()
+	defer dbc.mtx.Unlock()
+	var err error
+	if dbc.pgdb != nil {
+		err = dbc.pgdb.Close()
+		dbc.pgdb = nil
+	}
+	return err
+}

-----------------------------------------------------------------------


hooks/post-receive
-- 




More information about the arvados-commits mailing list