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

feat(cluster): Recovery using pg_basebackup #252

Merged

Conversation

Pionerd
Copy link
Contributor

@Pionerd Pionerd commented Apr 9, 2024

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.

Pionerd added 2 commits April 9, 2024 18:42
Signed-off-by: Pieter van der Giessen <[email protected]>
Signed-off-by: Pieter van der Giessen <[email protected]>
@Pionerd Pionerd requested a review from itay-grudev as a code owner April 9, 2024 17:17
@itay-grudev itay-grudev changed the title [Cluster] Recovery using pg_basebackup feat(cluster): Recovery using pg_basebackup Apr 10, 2024
@itay-grudev
Copy link
Collaborator

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
Copy link
Collaborator

itay-grudev commented Apr 10, 2024

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
Copy link
Contributor Author

Pionerd commented Apr 10, 2024

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
Copy link
Collaborator

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
Copy link
Contributor Author

Pionerd commented Apr 11, 2024

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?

@itay-grudev itay-grudev added the chart( cluster ) Related to the cluster chart label May 25, 2024
@itay-grudev itay-grudev merged commit c14ed18 into cloudnative-pg:main Aug 28, 2024
4 checks passed
@itay-grudev
Copy link
Collaborator

@Pionerd Sorry it took so long. This was a great work. I added some tests and changed the API so it feels more consistent with the rest of the chart. If you are already using this on production you may need to update your YAML a tiny bit.

hapeho pushed a commit to hapeho/cloudnative-pg-charts that referenced this pull request Dec 18, 2024
feat(cluster): Recovery using pg_basebackup (cloudnative-pg#252)

---------

Signed-off-by: Pieter van der Giessen <[email protected]>
Signed-off-by: Itay Grudev <[email protected]>
Co-authored-by: Itay Grudev <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chart( cluster ) Related to the cluster chart
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants