-
Notifications
You must be signed in to change notification settings - Fork 707
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
1521 delete tables on upgrade #1532
1521 delete tables on upgrade #1532
Conversation
@@ -98,10 +98,10 @@ Create name for the apprepository-controller based on the fullname | |||
{{- end -}} | |||
|
|||
{{/* | |||
Create name for the apprepository bootstrap job |
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'm assuming this is an old job which no longer exists, as I couldn't see the name being used anywhere.
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.
LGTM
cmd/asset-syncer/postgresql_utils.go
Outdated
// InvalidateCache for postgresql deletes and re-writes the schema | ||
func (m *postgresAssetManager) InvalidateCache() error { | ||
tables := strings.Join([]string{dbutils.RepositoryTable, dbutils.ChartTable, dbutils.ChartFilesTable}, ",") | ||
_, err := m.DB.Exec(fmt.Sprintf("DROP TABLE IF EXISTS %s CASCADE", tables)) |
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.
is the CASCADE
necessary?
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.
Not here it's not, thanks. I'd originally only deleted the one table and relied on the cascades, but I had to change that as the files table doesn't have a FK in this PR. I've removed the CASCADE and will re-add it when the FK's are in the next PR (I think)>
return err | ||
} | ||
|
||
return m.initTables() |
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.
this is not really necessary since it will be executed with syncing but I am okay if you want to leave it to leave the tables ready
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.
Yeah, the reason for having it here is so that it is sync'd with the correct code (the new, rather than from the still running older pods). We will probably need a transaction here to avoid race conditions with the existing pods too.
* Record the namespace in the assetsvc when syncing chart data. * Update tests * Add db test setup and start using for pgutils. Review feedback. * Use postgres db tests based on ENV var. * 1521 delete tables on upgrade (#1532) * Add pre-upgrade hook to invalidate cache. * Enable hook delete policy. * Remove currently unnecessary CASCADE
Ref: #1521
Following on from #1526, this PR adds a pre-upgrade hook which invalidates the database cache of charts. When postgres is being used, this drops and recreates the tables. For mongodb, it's currently a no-op. I've not added functionality to check for a schema change at the moment, so this will rebuild the cache on every upgrade.
IRL test:
Initial deploy dev environment and verify schema is the original (ie. without namespace field in repos table):
Upgrade with the new sync image (which has the new schema):
See preupgrade job deployed successfully (before I added the hook-delete-policy):
Re-check the table and now see it's updated.
Verified cache is being repopulated as app repo starts.