Skip to content
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.

Sign up for GitHub

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

allow force drop of metadata db #19

Closed
wants to merge 1 commit into from
Closed

allow force drop of metadata db #19

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 15 additions & 5 deletions main.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,19 +124,29 @@ resource "kubernetes_job" "db_init_job" {
container {
name = "init-db"
image = "postgres:${var.postgres_version}"

command = [
"/bin/sh",
"-c",
"psql $DATABASE_URL -c \"CREATE DATABASE ${each.key};\""
"-c"
]
args = [
<<-EOC
set -x
if $FORCE_RECREATE; then
# psql $DATABASE_URL -c 'DROP DATABASE IF EXISTS ${each.key} WITH (FORCE);'
fi
set -e
psql $DATABASE_URL -c 'CREATE DATABASE ${each.key};'
EOC
]

env {
name = "DATABASE_URL"
value = replace(each.value.metadata_backend_url, "/${basename(each.value.metadata_backend_url)}", "/postgres")
}
env {
name = "FORCE_RECREATE"
value = var.metadata_database_force_recreate
}
}

restart_policy = "OnFailure"
}
}
Expand Down
6 changes: 6 additions & 0 deletions variables.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,3 +104,9 @@ variable "postgres_version" {
type = string
default = "15"
}

variable "metadata_database_force_recreate" {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sounds a bit dangerous, as it will force recreate the database for all instances. Would it be better if we had it on the instances variable level?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've prepared a PR for that, let me know what you think:

#20

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bobbyiliev I was trying to get a that here:

This should be doable with a lifecycle replace triggered by with each.key on the db init, but it seems to crash.
We should not merge this until replacement is figured out.

force_recreate is perhaps misnamed. The goal is to recreate the database if and only if the db_init job needs to be re-run.

I think this is a weird one. I actually wouldn't expect all materialize CRDs to block on all init_db commands. They should only depend on their respective init_db resource. Additionally materialize resources must get fully recreated (not just re-rolled) if their db were to get dropped and recreated.

I do like that your pr makes this configurable per materialize instance. That is totally worth making its way into whatever solution we choose. I'm currently leaning towards randomizing the db_name and leaving the old one as cruft. I think it's got a relatively ignorable downside but is otherwise safer and more flexible.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes! This makes sense now! I don't have any strong opinions on which approach to take; main concerns that I would have for each would be:

  • With the lifecycle, maybe this will not be very obvious for users, and they might end up accidentally recreating a database
  • With the randomized name, users might end up with some orphan dbs without being able to easily pin point which instances they belong to

Copy link
Contributor Author

@jubrad jubrad Mar 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like your pr more than this one. I was trying to keep things simple, but this pr won't handle things like password changes very gracefully. We may also want to do a huddle sometime or a discussion item to the next team hangout. Graceful password rotation is a good item to discuss.

Getting together a list of the behaviors I think we want. I'm not sure we can have all of these with a simple init_db k8s job.

  1. If the database needs to get recreated we have to recreate the materialize instance
  2. If the materialize instance is recreated we need to recreate the database (although maybe not when we think about restore?)
  3. If there are changes like password rotation we can do a create if not exists, but we need to do a rollout
  4. If there are just job changes it should do a create if not exists and do nothing to the materialize instance
  5. If a fresh materialize instance is deployed it should fail if the DB already exists (maybe a dupe of 2?).

description = "Force recreation of the PG database on changes. This should only be turned on during development"
type = bool
default = false
}