🎉 Migrate config persistence to database

[tuliren]

What

• This PR resolves Allow using a PG database for the config database #3605.

How

• Next time users upgrade their Airbyte version, the following will take place:
  • An airbyte_configs table will be created in the internal airbyte database (only if this table has not been created yet).
    • This will only happen in ServerApp, but not in SchedulerApp to prevent race condition.
  • Existing configs from file-based config persistence will be copied to this new table (only if this table is empty).
• With extra parameters, it is possible to specify an external database for the configs.

Recommended reading order

1. airbyte_configs_table.sql - table schema for the airbyte_configs table
2. ConfigPersistenceFactory.java - how configs are migrated from file-based persistence to the database persistence
3. DatabaseConfigPersistence.java

[ChristopheDuong]

This will throw warnings when running docker-compose up if the variables are not defined in the .env file. Some user got confused by those warnings in the past.

So, maybe it's worth setting default values as "" or explicitly set the same values as DATABASE_* variables in the .env instead?

Another option is to explicitly express this in the docker-compose:

Suggested change

	- CONFIG_DATABASE_USER=${CONFIG_DATABASE_USER}
	- CONFIG_DATABASE_PASSWORD=${CONFIG_DATABASE_PASSWORD}
	- CONFIG_DATABASE_URL=${CONFIG_DATABASE_URL}
	- CONFIG_DATABASE_USER=${CONFIG_DATABASE_USER:-$DATABASE_USER}
	- CONFIG_DATABASE_PASSWORD=${CONFIG_DATABASE_PASSWORD:-$DATABASE_PASSWORD}
	- CONFIG_DATABASE_URL=${CONFIG_DATABASE_URL:-$DATABASE_URL}

[tuliren]

This somehow sets CONFIG_DATABASE_USER to literally $DATABASE_USER, instead of the value of that variable. Default it to empty string works:

CONFIG_DATABASE_USER=${CONFIG_DATABASE_USER:-}

[jrhizor]

Yes! This has bitten us a few times...

[jrhizor]

TODO reminder bot

[tuliren]

I don't plan to address this TODO in this PR. Adding this enum will break the auto migration tests. I will look into it in the migration project.

[jrhizor]

I don't like the increased optionality around a custom config database here. Is there a reason we want to encourage separating this?

For users that were previously on the job database for configs, we don't want to handle migrating configs to the config database or having multiple versions of configs sitting around if we can help it.

It isn't clear to me what the benefit is, and it seems to just increase operational complexity.

[tuliren]

We do want to have the option to separate the two databases. But I don't think we want to encourage it, at least for self-deployed instances.

The benefits of separating the two databases are:

1. The jobs database and the configs database have different usage patterns. The former can grow much quicker than the latter (because we track job attempts there).
2. The configs database contain secrets. So people may want extra security procedure around it.

What about if I remove the comments in the .env file, and only mention it in the documentation? In this way, we are not encouraging people to do it.

[tuliren]

Removing them from the .env will result in warning messages about missing environment variables. So I added them back with comments:

# Airbyte Internal Config Database, default to reuse the Job Database
# Usually you do not need to change them
CONFIG_DATABASE_USER=${DATABASE_USER}
CONFIG_DATABASE_PASSWORD=${DATABASE_PASSWORD}
CONFIG_DATABASE_URL=${DATABASE_URL}

[ChristopheDuong]

I'm thinking it would be really nice to keep a working file-based config persistence and not necessarily remove it permanently...

If this file config persistence reads/writes from a docker volume that is mounted from a local folder of the filesystem (like what the LOCAL_ROOT does) rather than a named docker volume, then a user could configure a CI running as part of their git repository versioning their airbyte setup. This setup could include (tests) configurations and once the CI is successful, they could publish in the same workflow their final changes to a remote Airbyte Database config persistence that would affect their airbyte instance in production for example.

WDYT?

[jrhizor]

Just curious, what benefits do you see from working with a file-based config persistence vs just using import/export capabilities?

[tuliren]

I guess what Chris means is that if we keep the file config persistence code, users can still manipulate the configs using the code?

[ChristopheDuong]

For import/export capabilities, we'd have what is currently exposed as:

• getDbPersistenceWithYamlSeed: (standard files released with airbyte)
• getDbPersistenceWithFileSeed: (old named docker volumes)

In the future, I am guessing we could have something like these?

• getDbPersistenceWithYamlSeedFromGitRepo: clones a remote git repo where seeds are stored to re-init the DB from those
• getDbPersistenceWithYamlSeedFromArchive: same but from a local/remote .tar.gz archive file
• getDbPersistenceWithYamlSeedFromLocalFolder: same but from a local folder (no need to compress into an archive or push to a repo?)

Just curious, what benefits do you see from working with a file-based config persistence vs just using import/export capabilities?
if we keep the file config persistence code, users can still manipulate the configs using the code?

Yes.

For dev/test purposes, you can more easily edit/test your airbyte configurations locally by manually/programmatically editing the configs in parallel without having to restart airbyte all the time to re-init the database (re-deploy your changes)?

Plus the code is almost already written for that, so it could be worth keeping around
Of course, for production usage, it's better to go through the database

[tuliren]

Here is the plan for config persistence migration:
https://docs.google.com/document/d/1lBmzvL0Wc52amz0zAVaxtAn726rJJBfJTfB-diglfYw/edit#

Should we delete the old config volume? Or do something with it? Even if we don't do it now, what's the path for getting rid of it?

Ideally yes. But since we cannot force users to upgrade, it seems that we must always keep it so that users previously on an old Airbyte version can still move the data out of it.

Should we just entirely get rid of the FileBasedConfigPersistence? Again, doesn't need to be now, but since we are forcing everything to use the postgres one right now, it seems like we may want to permanently deprecate it?

Yes. We can do this in a future version.

Some of the the logic in the ConfigPersistenceBuilder is essentially code to migrate from the file database to the postgres database. How do we need think about keeping that code? For example, we may deprecate the file based database completely. When can we remove the code that does this migration stuff? Is it never? Maybe it is the sort of thing where we need to force a user to migrate in steps. Like if you were on version 0.14 and you want to go to version 99.4.0 First you need to migrate from 0.14 => 0.30.0. Turn everything on and off and then you migrate from 0.30.0 => 99.4.0. That way after version 0.30.0 we can safely drop that code that that moves data from file datastore to the postgres one. I just want to make sure we've thought about this enough that we don't box ourselves in. Probably nothing we need to do differently now.

Not sure if this kind of operation overhead is acceptable for users. Alternatively we can move the migration code out of the ConfigPersistenceBuilder and move it to a script. The script will be there forever. But in this way, we can avoid forcing people to upgrade to an intermediate version.

Are there any edge cases we need to worry about? Specifically, I'm wondering if a user is not using the postgres database for the jobs database that we provide will they run into problems? Definitely not the end of the world if this is just not going to work for this case, but it would be good to know about it ahead of time in case it comes up.

This is a good point that people may already be using non-postgres for the jobs database. I should make the airbyte_configs script general enough for other databases (i.e. drop the default UUID function for the config_id column).

I am pretty worried about unknown edge cases. Still thinking about it.

[tuliren]

If we dump any table that exists in the database, the migration will break here, because each extra table will result in its own ResourceId for airbyte_job, which does not match any input stream from the migration. This concern was previously raised by @ChristopheDuong in #3980 (comment).

@subodh1810, is there any concern to change the dump method here to exportDatabase? In the future, the database is no longer just for job tables. The config table may exist there as well.

[tuliren]

The Acceptance Tests (Kube) job seems very suspicious.

In a previous run it failed because the default workspace definition could not be found in the config persistence, which seems to be a db config persistence issue.

2021-07-19 05:50:17 INFO i.a.c.p.DatabaseConfigPersistence(lambda$loadData$3):101 - ***workspace_app_root=/workspace/server/logs*** - Loading data to config database...
2021-07-19 05:50:17 INFO i.a.c.p.DatabaseConfigPersistence(lambda$loadData$3):114 - ***workspace_app_root=/workspace/server/logs*** - Config database data loading completed
2021-07-19 05:50:17 ERROR i.a.s.ServerApp(setCustomerIdIfNotSet):197 - ***workspace_app_root=/workspace/server/logs*** - Could not find workspace with id: 5ae6b09b-fdec-41af-aaf7-7d94cfc33ef6
io.airbyte.config.persistence.ConfigNotFoundException: config type: STANDARD_WORKSPACE id: 5ae6b09b-fdec-41af-aaf7-7d94cfc33ef6
	at io.airbyte.config.persistence.DatabaseConfigPersistence.getConfig(DatabaseConfigPersistence.java:129) ~[io.airbyte.airbyte-config-persistence-0.27.3-alpha.jar:?]
	at io.airbyte.config.persistence.ValidatingConfigPersistence.getConfig(ValidatingConfigPersistence.java:55) ~[io.airbyte.airbyte-config-persistence-0.27.3-alpha.jar:?]
	at io.airbyte.config.persistence.ConfigRepository.getStandardWorkspace(ConfigRepository.java:55) ~[io.airbyte.airbyte-config-persistence-0.27.3-alpha.jar:?]
	at io.airbyte.server.ServerApp.setCustomerIdIfNotSet(ServerApp.java:185) [io.airbyte-airbyte-server-0.27.3-alpha.jar:?]
	at io.airbyte.server.ServerApp.runServer(ServerApp.java:218) [io.airbyte-airbyte-server-0.27.3-alpha.jar:?]
	at io.airbyte.server.ServerApp.main(ServerApp.java:264) [io.airbyte-airbyte-server-0.27.3-alpha.jar:?]

So I added logging about what data is loaded into the db config persistence upon server startup, 449cb30, which did not change any code logic but a logging.

+    LOGGER.info("Inserting {} record {}", configType, configId);

And in the next run, the kube acceptance test succeeded. The logs also showed that all initial seed data was indeed injected into the db config persistence.

2021-07-19 06:38:50 INFO i.a.c.p.DatabaseConfigPersistence(lambda$loadData$3):101 - ***workspace_app_root=/workspace/server/logs*** - Loading data to config database...
2021-07-19 06:38:50 INFO i.a.c.p.DatabaseConfigPersistence(insertConfigRecord):205 - ***workspace_app_root=/workspace/server/logs*** - Inserting STANDARD_WORKSPACE record 5ae6b09b-fdec-41af-aaf7-7d94cfc33ef6

This is the last blocker to merge this PR. I will think about what could be the root cause.