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.

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

versatile-data-kit: postgresql embedded #1273

Merged
merged 13 commits into from
Nov 11, 2022
Merged

Conversation

ivakoleva
Copy link
Contributor

@ivakoleva ivakoleva commented Oct 31, 2022

versatile-data-kit: postgresql embedded

For embedded DB for control-service metadata storage, we need to support the Bitnami-available chart of PostgreSQL.

Added postgresql values.yaml configurations. In case cockroachdb.enabled is true, CockroachDB embedded is still being prioritized for usages. Also, CockroachDB remains the default embedded database.

Testing Done: verified postgresql.enabled with helm template --debug, also postgresql artifacts with helm install.

Signed-off-by: ivakoleva [email protected]

For embedded DB for control-service metadata storage, we need to use
the Bitnami-available chart of PostgreSQL.

Added postgresql `values.yaml` configurations. In case
`postgresql.enabled` is `true`, PostgreSQL embedded is being prioritized
for usages. It is also the default embedded database.

Testing Done: ci/cd

Signed-off-by: ivakoleva <[email protected]>
ivakoleva and others added 2 commits October 31, 2022 18:04
For embedded DB for control-service metadata storage, we need to use
the Bitnami-available chart of PostgreSQL.

Added postgresql `values.yaml` configurations. In case
`postgresql.enabled` is `true`, PostgreSQL embedded is being prioritized
for usages. It is also the default embedded database.

Testing Done: ci/cd

Signed-off-by: ivakoleva <[email protected]>
@ivakoleva ivakoleva marked this pull request as ready for review October 31, 2022 16:55
For embedded DB for control-service metadata storage, we need to use
the Bitnami-available chart of PostgreSQL.

Added postgresql `values.yaml` configurations. In case
`postgresql.enabled` is `true`, PostgreSQL embedded is being prioritized
for usages. It is also the default embedded database.

Testing Done: verified with helm template --debug, also postgresql
artifacts with helm install

Signed-off-by: ivakoleva <[email protected]>
@antoniivanov
Copy link
Collaborator

Changing the default database would be a breaking change. Any user upgrading would be broken.

But also why do we need to add it in the helm chart? If users want they are of course free to setup Postgres and connect to it. But not sure why do we need special support for that?

@ivakoleva
Copy link
Contributor Author

Changing the default database would be a breaking change. Any user upgrading would be broken.

But also why do we need to add it in the helm chart? If users want they are of course free to setup Postgres and connect to it. But not sure why do we need special support for that?

(1) Bitnami packages are compliant and secure, so the change improves security of the default recommended setup we provide
(2) Bitnami repository has been introduced in the earliest initial chart commit by design
(3) publishing VDK to Bitnami is something initiated a year ago yet to be completed -> introducing (default) Bitnami embedded dependency is on the right track in that direction

It is breaking change, yet I don't think as a result-anything will be actually broken, I think it will be sufficient to release as any other major change by bumping the version, and think about data migration in case someone bumps issue with upgrading and migrating

I was thinking to create a separate PR for the default setup switching to Postgres, however didn't split it so I can test and verify easier.

@antoniivanov
Copy link
Collaborator

Changing the default database would be a breaking change. Any user upgrading would be broken.
But also why do we need to add it in the helm chart? If users want they are of course free to setup Postgres and connect to it. But not sure why do we need special support for that?

(1) Bitnami packages are compliant and secure, so the change improves security of the default recommended setup we provide (2) Bitnami repository has been introduced in the earliest initial chart commit by design (3) publishing VDK to Bitnami is something initiated a year ago yet to be completed -> introducing (default) Bitnami embedded dependency is on the right track in that direction

It is breaking change, yet I don't think as a result-anything will be actually broken, I think it will be sufficient to release as any other major change by bumping the version, and think about data migration in case someone bumps issue with upgrading and migrating

I was thinking to create a separate PR for the default setup switching to Postgres, however didn't split it so I can test and verify easier.

CockroachDB is widely used and I am not convinced it's less secure.

It's possible that moving to a bitnami versioned chart is the right direction ( the alternative is to push Cockroachdb in bitnami.)

But changing the default DB we recommend users to use and all our automated tests test against is a rather major decision and requires more in-depth research and pros/cons analysis. I am not sure we should abandon testing against Cockroach db altogether as all the users we know about, use CockroachDB.

As there's currently no business need (for now we have paused any work on Bitnami integration), I see no need to try that now.

@ivakoleva
Copy link
Contributor Author

CockroachDB is widely used and I am not convinced it's less secure.

It's possible that moving to a bitnami versioned chart is the right direction ( the alternative is to push Cockroachdb in bitnami.)

But changing the default DB we recommend users to use and all our automated tests test against is a rather major decision and requires more in-depth research and pros/cons analysis. I am not sure we should abandon testing against Cockroach db altogether as all the users we know about, use CockroachDB.

As there's currently no business need (for now we have paused any work on Bitnami integration), I see no need to try that now.

Bitnami's charts are compliant, oppositely to CockroachDB custom chart. Bitnami's repository has been added since the initial versatile-data-kit commit by design, therefore fundamental.

Do you have any concern about the transition?

I think the business need had already manifested by the PR creation in the OSS repo; I don't mind to add PostgreSQL support and add the default switch into a separate PR. In fact, I shared earlier, I merged into one to facilitate testing.

@ivakoleva ivakoleva requested review from antoniivanov and removed request for antoniivanov November 8, 2022 08:34
@antoniivanov
Copy link
Collaborator

CockroachDB is widely used and I am not convinced it's less secure.
It's possible that moving to a bitnami versioned chart is the right direction ( the alternative is to push Cockroachdb in bitnami.)
But changing the default DB we recommend users to use and all our automated tests test against is a rather major decision and requires more in-depth research and pros/cons analysis. I am not sure we should abandon testing against Cockroach db altogether as all the users we know about, use CockroachDB.
As there's currently no business need (for now we have paused any work on Bitnami integration), I see no need to try that now.

Bitnami's charts are compliant, oppositely to CockroachDB custom chart. Bitnami's repository has been added since the initial versatile-data-kit commit by design, therefore fundamental.

Do you have any concern about the transition?

I think the business need had already manifested by the PR creation in the OSS repo; I don't mind to add PostgreSQL support and add the default switch into a separate PR. In fact, I shared earlier, I merged into one to facilitate testing.

I don't mind merging the postgres support.

But changing the default DB from cockroachDB to Postgres is something I am concerned about.
I don't think there's a pressing need. But it's possible I am not seeing something (it won't be my first time).
If you feel so, make a meeting. I'd like Miro to be included as he is Control Service Guru currently.

For embedded DB for control-service metadata storage, we need to
support the Bitnami-available chart of PostgreSQL.

Added postgresql `values.yaml` configurations. In case
`cockroachdb.enabled` is `true`, CockroachDB embedded is still
being prioritized for usages. Also, CockroachDB remains the default
embedded database.

Testing Done: verified `postgresql.enabled` with helm template --debug,
also postgresql artifacts with helm install.

Signed-off-by: ivakoleva <[email protected]>
@ivakoleva
Copy link
Contributor Author

Please use conversation mode, it's easier to have context and reply.

I don't mind merging the postgres support.

But changing the default DB from cockroachDB to Postgres is something I am concerned about.
I don't think there's a pressing need. But it's possible I am not seeing something (it won't be my first time).
If you feel so, make a meeting. I'd like Miro to be included as he is Control Service Guru currently.

I've made a minor change switching back to CockroachDB default. PostgreSQL support is what's important, switching it to default is a bigger discussion, that won't be solved by this PR.

@antoniivanov
Copy link
Collaborator

Please use conversation mode, it's easier to have context and reply.

What is conversation mode? I use "Quote Reply" option in github to make easier to track conversation ?

@ivakoleva ivakoleva requested a review from murphp15 November 9, 2022 08:03
@ivakoleva ivakoleva enabled auto-merge (squash) November 9, 2022 09:41
@ivakoleva ivakoleva merged commit 6c7235d into main Nov 11, 2022
@ivakoleva ivakoleva deleted the person/ikoleva/postgresql branch November 11, 2022 09:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants