cloudnative-pg / charts

CloudNativePG Helm Charts
Apache License 2.0
138 stars 62 forks source link

feat(cluster): Recovery using pg_basebackup #252

Open Pionerd opened 3 months ago

Pionerd commented 3 months ago

An implementation of the pb_basebackup recovery method.

Tested this helm chart using the existingSecret option and was able to successfully bootstrap a cluster using this method. The TLS implementation I'm unable to test, but it is relatively straightforward, based on the docs.

I assume you can automatically update the values schema / docs? Let me know if anything else is required.

itay-grudev commented 3 months ago

This is great work and we'll merge it, but I will request some changes. I'll allocate some time to do a detailed review later this week.

itay-grudev commented 3 months ago

One discussion point: do you think it may have been a misnomer on my side to consider pg_basebackup a recovery mode? We could alternatively put that as another .Values.mode.

@Pionerd @phisco

Pionerd commented 3 months ago

When working on this, I didn't want to refactor the whole thing straight away, but indeed, to me it makes more sense to take it out of the recovery section

itay-grudev commented 3 months ago

Don't refactor it yet. Let's hear what @phisco thinks as well. But if we want to make that change it should be now, otherwise the API will be fixed for reasons of backwards compatibility.

Pionerd commented 3 months ago

Also, we are testing "bootstrapping" databases using https://cloudnative-pg.io/documentation/1.22/database_import/. E.g.

apiVersion: postgresql.cnpg.io/v1
kind: Cluster
metadata:
  name: cluster-microservice
spec:
  instances: 3

  bootstrap:
    initdb:
      import:
        type: microservice
        databases:
          - angus
        source:
          externalCluster: cluster-pg96
        #postImportApplicationSQL:
        #- |
        #  INSERT YOUR SQL QUERIES HERE
  storage:
    size: 1Gi
  externalClusters:
    - name: cluster-pg96
      connectionParameters:
        # Use the correct IP or host name for the source database
        host: pg96.local
        user: postgres
        dbname: postgres
      password:
        name: cluster-pg96-superuser
        key: password

Currently, I can specify the initdb part, but the externalClusters part would also need to be added. However, to me it feels a bit strange to add another part in the bootstrap where externalClusters are defined. Defining the externalClusters in the values.yaml and then using them one on one feels more natural to me, but maybe this is exactly the logic you want to abstract away with this helm chart. What do you think?