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

support: recovery target variables in helm chart #399

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

sadath-12
Copy link

No description provided.

@gbartolini
Copy link
Contributor

I see you added only targetTime. Can you please ensure there's a way to add the other recovery targets specified in https://cloudnative-pg.io/documentation/current/recovery/#recovery-targets?

@sadath-12
Copy link
Author

sure @gbartolini

@sadath-12
Copy link
Author

@gbartolini have a look thanks

Copy link
Collaborator

@itay-grudev itay-grudev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the purpose of this check?

{{- if or .Values.recovery.pitrTarget.targetTime .Values.recovery.pitrTarget.backupID .Values.recovery.pitrTarget.targetXID .Values.recovery.pitrTarget.targetName .Values.recovery.pitrTarget.targetLSN .Values.recovery.pitrTarget.targetImmediate }}

Copy link
Collaborator

@itay-grudev itay-grudev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we remove the prefix target?

  1. Renaming time to targetTime is a breaking change.
  2. It's repetitive. pitrTarget.target.

@itay-grudev
Copy link
Collaborator

itay-grudev commented Sep 24, 2024

Can we change the section in the values.yaml to the following:

  pitrTarget:
    # -- The ID of the backup from which to start the recovery process. 
    # If empty (default) the operator will automatically detect the backup based on `pitrTarget.time` or `pitrTarget.lsn` if provided.
    # Otherwise use the latest available backup in chronological order.
    backupID: ""

    # -- Recovery ends as soon as a consistent state is reached, that is, as early as possible
    immediate: ""
    
    # -- The target timeline ("latest" or a positive integer)
    tli: ""

    # -- Set the target to be exclusive. If omitted, defaults to false, so that in Postgres, `recovery_target_inclusive` will be true
    exclusive: ""

    # Specify EXACTLY ONE of the following options:
    # -- Point in time recovery Time target in RFC3339 format
    time: ""
    # -- Point in time recovery Transaction ID up to which recovery proceeds. (The precise stopping point is also influenced by the exclusive option.) 
    xid: ""
    # -- Named restore point (created with pg_create_restore_point()) to which recovery proceeds
    name: ""
    # -- LSN (Log Sequence Number) of the write-ahead log location up to which recovery proceeds. (The precise stopping point is also influenced by the exclusive option.)
    lsn: ""

I've structured it in two sections: the configuration above and the target options below. I've also updated some of the descriptions so they are more accurate.

You were missing the targetTLI and exclusive options. Could you please add those as well?

Signed-off-by: sadath-12 <[email protected]>
@sadath-12 sadath-12 changed the title support: backupID in helm chart support: recovery target variables in helm chart Sep 25, 2024
@sadath-12
Copy link
Author

@itay-grudev thanks ,please have a look

@ockhamlabs
Copy link

@itay-grudev curious about status of this and if this is planned to be merged?

@itay-grudev
Copy link
Collaborator

@ockhamlabs It's getting merged. It's excellent work. I would like to test it and/or write a test suite for it, which is why I haven't merged it yet.

@itay-grudev itay-grudev added the chart( cluster ) Related to the cluster chart label Oct 9, 2024
@sadath-12
Copy link
Author

Hi @itay-grudev , gentle reminder

@ockhamlabs
Copy link

@itay-grudev Thanks and let us know if this can be merged now?

@riddhithummar
Copy link

When is this expected to be merged?

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.

5 participants