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

Adds an overwrite flag to force annotation updates #70

Merged
merged 5 commits into from
Oct 27, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions docs/content/reference/pgo_backup.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,10 @@ Backup cluster

### Synopsis

Backup allows you to take a backup of a PostgreSQL cluster, either using
Backup allows you to backup a PostgreSQL cluster either by using
the current "spec.backups.pgbackrest.manual" settings on the PostgreSQL cluster
or by overwriting those settings using the flags
or by using flags to write your settings. Overwriting those settings may require
the --force-conflicts flag.

### RBAC Requirements
Resources Verbs
Expand All @@ -33,6 +34,9 @@ pgo backup hippo
# on the 'hippo' postgrescluster and trigger a backup
pgo backup hippo --repoName="repo1" --options="--type=full"

# Resolve ownership conflict
pgo backup hippo --force-conflicts

```
### Example output
```
Expand All @@ -42,6 +46,7 @@ postgresclusters/hippo backup initiated
### Options

```
--force-conflicts take ownership and overwrite the backup settings
-h, --help help for backup
--options stringArray options for taking a backup; can be used multiple times
--repoName string repoName to backup to
Expand Down
5 changes: 5 additions & 0 deletions docs/content/reference/pgo_restore.md
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,16 @@ WARNING: This action is destructive and PostgreSQL will be unavailable while its

Do you want to continue? (yes/no): yes
postgresclusters/hippo patched

# Resolve ownership conflict
pgo restore hippo --force-conflicts

```

### Options

```
--force-conflicts take ownership and overwrite the restore settings
-h, --help help for restore
--options stringArray options to pass to the "pgbackrest restore" command; can be used multiple times
--repoName string repository to restore from
Expand Down
24 changes: 17 additions & 7 deletions internal/cmd/backup.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,10 @@ func newBackupCommand(config *internal.Config) *cobra.Command {
cmdBackup := &cobra.Command{
Use: "backup CLUSTER_NAME",
Short: "Backup cluster",
Long: `Backup allows you to take a backup of a PostgreSQL cluster, either using
Long: `Backup allows you to backup a PostgreSQL cluster either by using
the current "spec.backups.pgbackrest.manual" settings on the PostgreSQL cluster
or by overwriting those settings using the flags
or by using flags to write your settings. Overwriting those settings may require
the --force-conflicts flag.

### RBAC Requirements
Resources Verbs
Expand All @@ -55,16 +56,20 @@ pgo backup hippo
# on the 'hippo' postgrescluster and trigger a backup
pgo backup hippo --repoName="repo1" --options="--type=full"

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not your change, but can you look at line 40 -- I don't like that it uses the word "overwrite" when that's not what we do unless you use the overwrite flag

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does the new text work?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think there's ~3 cases here that we (maybe) want to cover:
a) take a backup with the settings already in the spec
b) take a backup with new settings that you write to the spec
c) take a backup with new settings that you write to the spec -- even if someone else owns those settings.

Maybe something like (but better than):

Backup allows you to take a backup of a PostgreSQL cluster, either using
the current "spec.backups.pgbackrest.manual" settings on the PostgreSQL cluster
or by writing those settings using the flags (potentially overwriting them with the
`--force-conflicts` flag).

(I don't think we actually want parentheses in our docs. Maybe we just want to say "you can take backups with the settings that are there or write those settings" and mention the overwrite situation in the flag description? No, I think people will run into that, we probably want to bring it up in this summary.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about this?

# Resolve ownership conflict
pgo backup hippo --force-conflicts

### Example output
postgresclusters/hippo backup initiated`)

// Limit the number of args, that is, only one cluster name
cmdBackup.Args = cobra.ExactArgs(1)

// `backup` command accepts `repoName` and `options` flags;
// `backup` command accepts `repoName`, `force-conflicts` and `options` flags;
// multiple options flags can be used, with each becoming a new line
// in the options array on the spec
backup := pgBackRestBackup{}
cmdBackup.Flags().BoolVar(&backup.ForceConflicts, "force-conflicts", false, "take ownership and overwrite the backup settings")
cmdBackup.Flags().StringVar(&backup.RepoName, "repoName", "", "repoName to backup to")
cmdBackup.Flags().StringArrayVar(&backup.Options, "options", []string{},
"options for taking a backup; can be used multiple times")
Expand Down Expand Up @@ -110,12 +115,16 @@ postgresclusters/hippo backup initiated`)

// Update the spec/annotate
// TODO(benjaminjb): Would we want to allow a dry-run option here?
// TODO(benjaminjb): Would we want to allow a force option here?
patchOptions := metav1.PatchOptions{}
if backup.ForceConflicts {
b := true
patchOptions.Force = &b
}
_, err = client.Namespace(configNamespace).Patch(ctx,
args[0], // the name of the cluster object, limited to one name through `ExactArgs(1)`
types.ApplyPatchType,
patch,
config.Patch.PatchOptions(metav1.PatchOptions{}),
config.Patch.PatchOptions(patchOptions),
)

if err != nil {
Expand All @@ -134,8 +143,9 @@ postgresclusters/hippo backup initiated`)
}

type pgBackRestBackup struct {
Options []string
RepoName string
Options []string
RepoName string
ForceConflicts bool
}

func (config pgBackRestBackup) modifyIntent(
Expand Down
33 changes: 26 additions & 7 deletions internal/cmd/restore.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,11 @@ WARNING: You are about to restore from pgBackRest with {options:[] repoName:repo
WARNING: This action is destructive and PostgreSQL will be unavailable while its data is restored.

Do you want to continue? (yes/no): yes
postgresclusters/hippo patched`)
postgresclusters/hippo patched

# Resolve ownership conflict
pgo restore hippo --force-conflicts
`)

restore := pgBackRestRestore{Config: config}

Expand All @@ -64,6 +68,8 @@ postgresclusters/hippo patched`)
cmd.Flags().StringVar(&restore.RepoName, "repoName", "",
"repository to restore from")

cmd.Flags().BoolVar(&restore.ForceConflicts, "force-conflicts", false, "take ownership and overwrite the restore settings")

// Only one positional argument: the PostgresCluster name.
cmd.Args = cobra.ExactArgs(1)

Expand Down Expand Up @@ -119,8 +125,9 @@ postgresclusters/hippo patched`)
type pgBackRestRestore struct {
*internal.Config

Options []string
RepoName string
Options []string
RepoName string
ForceConflicts bool

PostgresCluster string
}
Expand Down Expand Up @@ -168,14 +175,20 @@ func (config pgBackRestRestore) Run(ctx context.Context) error {
if err != nil {
return err
}
patchOptions := metav1.PatchOptions{
DryRun: []string{metav1.DryRunAll},
}

if config.ForceConflicts {
b := true
patchOptions.Force = &b
}

// Perform a dry-run patch to understand what settings will be used should
// the restore proceed.
cluster, err = client.Namespace(namespace).Patch(ctx,
config.PostgresCluster, types.ApplyPatchType, patch,
config.Patch.PatchOptions(metav1.PatchOptions{
DryRun: []string{metav1.DryRunAll},
}))
config.Patch.PatchOptions(patchOptions))
if err != nil {
return err
}
Expand All @@ -191,10 +204,16 @@ func (config pgBackRestRestore) Run(ctx context.Context) error {
return nil
}

patchOptions = metav1.PatchOptions{}
if config.ForceConflicts {
b := true
patchOptions.Force = &b
}

// They agreed to continue. Send the patch again without dry-run.
_, err = client.Namespace(namespace).Patch(ctx,
config.PostgresCluster, types.ApplyPatchType, patch,
config.Patch.PatchOptions(metav1.PatchOptions{}))
config.Patch.PatchOptions(patchOptions))

if err == nil {
fmt.Fprintf(config.Out, "%s/%s patched\n",
Expand Down
29 changes: 29 additions & 0 deletions testing/kuttl/e2e/backup/12--backup-with-force-conflicts.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
---
apiVersion: kuttl.dev/v1beta1
kind: TestStep
commands:
- script: |
pgbackrest_backup_annotation() {
kubectl get --namespace "${NAMESPACE}" postgrescluster/backup-cluster \
--output 'go-template={{ index .metadata.annotations "postgres-operator.crunchydata.com/pgbackrest-backup" }}'
}

kubectl --namespace "${NAMESPACE}" annotate postgrescluster/backup-cluster \
postgres-operator.crunchydata.com/pgbackrest-backup="$(date)" --overwrite || exit

PRIOR=$(pgbackrest_backup_annotation)
RESULT=$(kubectl-pgo --namespace "${NAMESPACE}" backup backup-cluster --force-conflicts)
CURRENT=$(pgbackrest_backup_annotation)

if [ "${CURRENT}" = "${PRIOR}" ]; then
printf 'Expected annotation to change, got %q' "${CURRENT}"
exit 1
fi

echo "RESULT from taking backup: ${RESULT}"

if [[ -n $RESULT && "$RESULT" == "postgresclusters/backup-cluster backup initiated" ]]; then
exit 0
fi

exit 1
27 changes: 27 additions & 0 deletions testing/kuttl/e2e/restore/31--force-conflicts.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
---
apiVersion: kuttl.dev/v1beta1
kind: TestStep
commands:
- script: |
pgbackrest_restore_annotation() {
kubectl --namespace "${NAMESPACE}" get postgrescluster/restore-cluster \
--output "jsonpath-as-json={.metadata.annotations['postgres-operator\.crunchydata\.com/pgbackrest-restore']}"
}

kubectl --namespace "${NAMESPACE}" annotate postgrescluster/restore-cluster \
postgres-operator.crunchydata.com/pgbackrest-restore="$(date)" --overwrite || exit


PRIOR=$(pgbackrest_restore_annotation)
# Running restore will update the annotation.
echo yes | kubectl-pgo --namespace="${NAMESPACE}" restore restore-cluster --options="--db-include=restore-cluster" --repoName="repo2" --force-conflicts
CURRENT=$(pgbackrest_restore_annotation)

if [ "${CURRENT}" != "${PRIOR}" ]; then
exit 0
fi

printf 'Expected annotation to change, got PRIOR %q CURRENT %q' "${PRIOR}" "${CURRENT}"
echo "RESULT from taking restore: ${RESULT}"

exit 1