-
Notifications
You must be signed in to change notification settings - Fork 21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
cmd/icingadb: automatically upgrade SQL schema #770
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,8 +3,11 @@ package main | |
import ( | ||
"context" | ||
"fmt" | ||
"github.com/icinga/icinga-go-library/backoff" | ||
"github.com/icinga/icinga-go-library/database" | ||
"github.com/icinga/icinga-go-library/logging" | ||
"github.com/icinga/icinga-go-library/redis" | ||
"github.com/icinga/icinga-go-library/retry" | ||
"github.com/icinga/icinga-go-library/utils" | ||
"github.com/icinga/icingadb/internal" | ||
"github.com/icinga/icingadb/internal/command" | ||
|
@@ -15,6 +18,8 @@ import ( | |
v1 "github.com/icinga/icingadb/pkg/icingadb/v1" | ||
"github.com/icinga/icingadb/pkg/icingaredis" | ||
"github.com/icinga/icingadb/pkg/icingaredis/telemetry" | ||
"github.com/icinga/icingadb/schema/mysql" | ||
"github.com/icinga/icingadb/schema/pgsql" | ||
"github.com/okzk/sdnotify" | ||
"github.com/pkg/errors" | ||
"go.uber.org/zap" | ||
|
@@ -67,8 +72,38 @@ func run() int { | |
} | ||
} | ||
|
||
if err := icingadb.CheckSchema(context.Background(), db); err != nil { | ||
logger.Fatalf("%+v", err) | ||
{ | ||
err := retry.WithBackoff( | ||
context.Background(), | ||
func(ctx context.Context) (err error) { | ||
return database.AutoUpgradeSchema( | ||
context.Background(), db, cmd.Config.Database.Database, "icingadb_schema", "version", "timestamp", | ||
map[string]database.SchemaData{ | ||
database.MySQL: { | ||
Schema: []string{mysql.Schema}, | ||
Upgrades: []database.SchemaUpgrade{ | ||
{Version: "3", DDL: []string{mysql.Upgrade100}}, | ||
{Version: "4", DDL: []string{mysql.Upgrade111}}, | ||
{Version: "5", DDL: []string{mysql.Upgrade120}}, | ||
}, | ||
}, | ||
database.PostgreSQL: { | ||
Schema: []string{pgsql.Schema}, | ||
Upgrades: []database.SchemaUpgrade{ | ||
{Version: "2", DDL: []string{pgsql.Upgrade111}}, | ||
{Version: "3", DDL: []string{pgsql.Upgrade120}}, | ||
Comment on lines
+79
to
+94
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Do you consider the design fine? * One has to maintain such schema file lists. A non-raw usage requires the specific project to have a function which auto-calculates schema upgrade lists from the embedded tree of schema/. |
||
}, | ||
}, | ||
}, | ||
) | ||
}, | ||
retry.Retryable, | ||
backoff.NewExponentialWithJitter(128*time.Millisecond, time.Minute), | ||
db.GetDefaultRetrySettings(), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please correct me if I'm wrong, but this would force (multiple) schema upgrades to be performed in five minutes. If this takes more time, the user ends up in a very messy situation, potentially don't even knowing how they got there. There were already long running schema upgrades in the past, https://icinga.com/docs/icinga-db/latest/doc/04-Upgrading/#upgrading-to-icinga-db-v120. |
||
) | ||
if err != nil { | ||
logger.Fatalf("%+v", errors.Wrap(err, "can't upgrade database schema")) | ||
} | ||
} | ||
|
||
rc, err := cmd.Redis(logs.GetChildLogger("redis")) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Generally speaking, I am against embedding both the schema files and all schema upgrades in Icinga DB as they are being installed as part of the package. This only bloats up the binary. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, then let's don't ship the separate files. 👍 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
package mysql | ||
|
||
import _ "embed" | ||
|
||
//go:embed schema.sql | ||
var Schema string | ||
|
||
//go:embed upgrades/1.0.0.sql | ||
var Upgrade100 string | ||
|
||
//go:embed upgrades/1.1.1.sql | ||
var Upgrade111 string | ||
|
||
//go:embed upgrades/1.2.0.sql | ||
var Upgrade120 string |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
package pgsql | ||
|
||
import _ "embed" | ||
|
||
//go:embed schema.sql | ||
var Schema string | ||
|
||
//go:embed upgrades/1.1.1.sql | ||
var Upgrade111 string | ||
|
||
//go:embed upgrades/1.2.0.sql | ||
var Upgrade120 string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would love to see some logging here. The
AutoUpgradeSchema
function just starts upgrading the schema without any notice.From a user's point of view, Icinga DB just "blocked" and Icinga Web will hint that Icinga DB might have stopped. Thus, even when a user have consulted the logs and sees nothing, they will likely restart Icinga DB and crash the migration.