diff --git a/api/v1beta2/foundationdbbackup_types.go b/api/v1beta2/foundationdbbackup_types.go index bb17a645e..ad3234df7 100644 --- a/api/v1beta2/foundationdbbackup_types.go +++ b/api/v1beta2/foundationdbbackup_types.go @@ -321,6 +321,14 @@ func (backup *FoundationDBBackup) GetDesiredAgentCount() int { return ptr.Deref(backup.Spec.AgentCount, 2) } +// NeedsBackupReconfiguration determines if the backup needs to be reconfigured. +func (backup *FoundationDBBackup) NeedsBackupReconfiguration() bool { + hasSnapshotSecondsChanged := backup.SnapshotPeriodSeconds() != backup.Status.BackupDetails.SnapshotPeriodSeconds + hasBackupURLChanged := backup.BackupURL() != backup.Status.BackupDetails.URL + + return hasSnapshotSecondsChanged || hasBackupURLChanged +} + // CheckReconciliation compares the spec and the status to determine if // reconciliation is complete. func (backup *FoundationDBBackup) CheckReconciliation() (bool, error) { @@ -350,8 +358,7 @@ func (backup *FoundationDBBackup) CheckReconciliation() (bool, error) { reconciled = false } - if isRunning && - backup.SnapshotPeriodSeconds() != backup.Status.BackupDetails.SnapshotPeriodSeconds { + if isRunning && backup.NeedsBackupReconfiguration() { backup.Status.Generations.NeedsBackupReconfiguration = backup.Generation reconciled = false } diff --git a/api/v1beta2/foundationdbbackup_types_test.go b/api/v1beta2/foundationdbbackup_types_test.go index 88092a301..8acd1b38f 100644 --- a/api/v1beta2/foundationdbbackup_types_test.go +++ b/api/v1beta2/foundationdbbackup_types_test.go @@ -52,6 +52,9 @@ var _ = Describe("[api] FoundationDBBackup", func() { }, Spec: FoundationDBBackupSpec{ AgentCount: &agentCount, + BlobStoreConfiguration: &BlobStoreConfiguration{ + AccountName: "test@test-service", + }, }, Status: FoundationDBBackupStatus{ Generations: BackupGenerationStatus{ @@ -60,7 +63,7 @@ var _ = Describe("[api] FoundationDBBackup", func() { AgentCount: 3, DeploymentConfigured: true, BackupDetails: &FoundationDBBackupStatusBackupDetails{ - URL: "blobstore://test@test-service/sample-cluster?bucket=fdb-backups", + URL: "blobstore://test@test-service:443/sample-cluster?bucket=fdb-backups", Running: true, SnapshotPeriodSeconds: 864000, }, @@ -69,7 +72,6 @@ var _ = Describe("[api] FoundationDBBackup", func() { } backup = createBackup() - result, err := backup.CheckReconciliation() Expect(result).To(BeTrue()) Expect(err).NotTo(HaveOccurred()) diff --git a/controllers/admin_client_test.go b/controllers/admin_client_test.go index 806178116..0335366be 100644 --- a/controllers/admin_client_test.go +++ b/controllers/admin_client_test.go @@ -333,15 +333,36 @@ var _ = Describe("admin_client_test", func() { Context("with a modification to the snapshot time", func() { BeforeEach(func() { backup.Spec.SnapshotPeriodSeconds = ptr.To(20) + backup.Spec.BlobStoreConfiguration = &fdbv1beta2.BlobStoreConfiguration{ + BackupName: "test-backup", + AccountName: "test", + } Expect(mockAdminClient.ModifyBackup(backup)).NotTo(HaveOccurred()) + }) - It("should mark the backup as stopped", func() { + It("should have modified the snapshot time", func() { Expect( status.SnapshotIntervalSeconds, ).To(BeNumerically("==", backup.SnapshotPeriodSeconds())) }) }) + + Context("with a modification to the backup url", func() { + BeforeEach(func() { + backup.Spec.BlobStoreConfiguration = &fdbv1beta2.BlobStoreConfiguration{ + BackupName: "test-backup-2", + AccountName: "test", + } + Expect(mockAdminClient.ModifyBackup(backup)).NotTo(HaveOccurred()) + }) + + It("should have modified the backup url", func() { + Expect( + status.DestinationURL, + ).To(Equal("blobstore://test:443/test-backup-2?bucket=fdb-backups")) + }) + }) }) }) diff --git a/controllers/modify_backup.go b/controllers/modify_backup.go index 24ea2fa49..a7bca59c0 100644 --- a/controllers/modify_backup.go +++ b/controllers/modify_backup.go @@ -41,7 +41,12 @@ func (s modifyBackup) reconcile( return nil } - if backup.Status.BackupDetails.SnapshotPeriodSeconds != backup.SnapshotPeriodSeconds() { + snapshotPeriod := backup.SnapshotPeriodSeconds() + backupURL := backup.BackupURL() + + shouldModify := backup.Status.BackupDetails.SnapshotPeriodSeconds != snapshotPeriod || + backup.Status.BackupDetails.URL != backupURL + if shouldModify { adminClient, err := r.adminClientForBackup(ctx, backup) if err != nil { return &requeue{curError: err} diff --git a/docs/manual/technical_design.md b/docs/manual/technical_design.md index 2a04a8edd..e1b9aff31 100644 --- a/docs/manual/technical_design.md +++ b/docs/manual/technical_design.md @@ -387,7 +387,7 @@ The `ToggleBackupPaused` subreconciler is responsible for pausing and unpausing The `ModifyBackup` command ensures that any properties that can be configured on a live backup are configured to the values in the backup spec. This will run the `modify` command in `fdbbackup` to set the properties from the spec. -Currently, this only supports the `snapshotPeriodSeconds` property. +Currently, this only supports the `snapshotPeriodSeconds` and `url` property. ### UpdateBackupStatus (again) diff --git a/fdbclient/admin_client.go b/fdbclient/admin_client.go index 8382dfcc1..0d396feb4 100644 --- a/fdbclient/admin_client.go +++ b/fdbclient/admin_client.go @@ -882,6 +882,8 @@ func (client *cliAdminClient) ModifyBackup(backup *fdbv1beta2.FoundationDBBackup "modify", "-s", strconv.Itoa(backup.SnapshotPeriodSeconds()), + "-d", + backup.BackupURL(), }, }) diff --git a/pkg/fdbadminclient/mock/admin_client_mock.go b/pkg/fdbadminclient/mock/admin_client_mock.go index 7a2096627..df1cef3cd 100644 --- a/pkg/fdbadminclient/mock/admin_client_mock.go +++ b/pkg/fdbadminclient/mock/admin_client_mock.go @@ -919,6 +919,7 @@ func (client *AdminClient) ModifyBackup(backup *fdbv1beta2.FoundationDBBackup) e currentBackup := client.Backups["default"] currentBackup.SnapshotPeriodSeconds = backup.SnapshotPeriodSeconds() + currentBackup.URL = backup.BackupURL() client.Backups["default"] = currentBackup return nil }