From 8e52ec5de11452fb4cbc9a8ab600e7845cdecb39 Mon Sep 17 00:00:00 2001 From: Milla Samuel Date: Mon, 11 Aug 2025 20:08:11 +0800 Subject: [PATCH 1/6] Modify --- api/v1beta2/foundationdbbackup_types.go | 6 ++++++ controllers/admin_client_test.go | 5 +++++ controllers/modify_backup.go | 7 ++++++- fdbclient/admin_client.go | 2 ++ pkg/fdbadminclient/mock/admin_client_mock.go | 1 + 5 files changed, 20 insertions(+), 1 deletion(-) diff --git a/api/v1beta2/foundationdbbackup_types.go b/api/v1beta2/foundationdbbackup_types.go index bb17a645e..98bfa6aee 100644 --- a/api/v1beta2/foundationdbbackup_types.go +++ b/api/v1beta2/foundationdbbackup_types.go @@ -332,6 +332,12 @@ func (backup *FoundationDBBackup) CheckReconciliation() (bool, error) { reconciled = false } + desiredBackupURL := backup.BackupURL() + if backup.Status.BackupDetails.URL != desiredBackupURL { + backup.Status.Generations.NeedsBackupReconfiguration = backup.Generation + reconciled = false + } + isRunning := backup.Status.BackupDetails != nil && backup.Status.BackupDetails.Running isPaused := backup.Status.BackupDetails != nil && backup.Status.BackupDetails.Paused diff --git a/controllers/admin_client_test.go b/controllers/admin_client_test.go index 806178116..0e858bd35 100644 --- a/controllers/admin_client_test.go +++ b/controllers/admin_client_test.go @@ -333,7 +333,12 @@ 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() { 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/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 } From 1094584c4ad063cfac527deba6354a2e7409e7ca Mon Sep 17 00:00:00 2001 From: Milla Samuel Date: Mon, 11 Aug 2025 20:12:19 +0800 Subject: [PATCH 2/6] Update test and docs --- controllers/admin_client_test.go | 18 +++++++++++++++++- docs/manual/technical_design.md | 2 +- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/controllers/admin_client_test.go b/controllers/admin_client_test.go index 0e858bd35..683c52d75 100644 --- a/controllers/admin_client_test.go +++ b/controllers/admin_client_test.go @@ -341,12 +341,28 @@ var _ = Describe("admin_client_test", func() { }) - 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@test-service/test-backup-2")) + }) + }) }) }) 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) From b543387cda9370b8a66235025042d1ca4b69392d Mon Sep 17 00:00:00 2001 From: Milla Samuel Date: Tue, 12 Aug 2025 14:22:06 +0800 Subject: [PATCH 3/6] Fix test --- api/v1beta2/foundationdbbackup_types.go | 11 +++++------ api/v1beta2/foundationdbbackup_types_test.go | 6 ++++-- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/api/v1beta2/foundationdbbackup_types.go b/api/v1beta2/foundationdbbackup_types.go index 98bfa6aee..6cb41d378 100644 --- a/api/v1beta2/foundationdbbackup_types.go +++ b/api/v1beta2/foundationdbbackup_types.go @@ -332,12 +332,6 @@ func (backup *FoundationDBBackup) CheckReconciliation() (bool, error) { reconciled = false } - desiredBackupURL := backup.BackupURL() - if backup.Status.BackupDetails.URL != desiredBackupURL { - backup.Status.Generations.NeedsBackupReconfiguration = backup.Generation - reconciled = false - } - isRunning := backup.Status.BackupDetails != nil && backup.Status.BackupDetails.Running isPaused := backup.Status.BackupDetails != nil && backup.Status.BackupDetails.Paused @@ -362,6 +356,11 @@ func (backup *FoundationDBBackup) CheckReconciliation() (bool, error) { reconciled = false } + if isRunning && backup.BackupURL() != backup.Status.BackupDetails.URL { + backup.Status.Generations.NeedsBackupReconfiguration = backup.Generation + reconciled = false + } + if reconciled { backup.Status.Generations = BackupGenerationStatus{ Reconciled: backup.Generation, 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()) From bcf7ca167f01602f65640a9947d8058551997d8e Mon Sep 17 00:00:00 2001 From: Milla Samuel Date: Mon, 25 Aug 2025 12:16:09 +0800 Subject: [PATCH 4/6] Dedicated method --- api/v1beta2/foundationdbbackup_types.go | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/api/v1beta2/foundationdbbackup_types.go b/api/v1beta2/foundationdbbackup_types.go index 6cb41d378..af487826a 100644 --- a/api/v1beta2/foundationdbbackup_types.go +++ b/api/v1beta2/foundationdbbackup_types.go @@ -321,6 +321,13 @@ func (backup *FoundationDBBackup) GetDesiredAgentCount() int { return ptr.Deref(backup.Spec.AgentCount, 2) } +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,13 +357,7 @@ func (backup *FoundationDBBackup) CheckReconciliation() (bool, error) { reconciled = false } - if isRunning && - backup.SnapshotPeriodSeconds() != backup.Status.BackupDetails.SnapshotPeriodSeconds { - backup.Status.Generations.NeedsBackupReconfiguration = backup.Generation - reconciled = false - } - - if isRunning && backup.BackupURL() != backup.Status.BackupDetails.URL { + if isRunning && backup.NeedsBackupReconfiguration() { backup.Status.Generations.NeedsBackupReconfiguration = backup.Generation reconciled = false } From f14c9814094bac034cb58e40423140adf68f5b73 Mon Sep 17 00:00:00 2001 From: Milla Samuel Date: Mon, 25 Aug 2025 12:37:45 +0800 Subject: [PATCH 5/6] fix test --- controllers/admin_client_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controllers/admin_client_test.go b/controllers/admin_client_test.go index 683c52d75..0335366be 100644 --- a/controllers/admin_client_test.go +++ b/controllers/admin_client_test.go @@ -360,7 +360,7 @@ var _ = Describe("admin_client_test", func() { It("should have modified the backup url", func() { Expect( status.DestinationURL, - ).To(Equal("blobstore://test@test-service/test-backup-2")) + ).To(Equal("blobstore://test:443/test-backup-2?bucket=fdb-backups")) }) }) }) From 3d0d37af9a7af1bfee9ded74e827476f1546790d Mon Sep 17 00:00:00 2001 From: Milla Samuel Date: Mon, 25 Aug 2025 15:56:51 +0800 Subject: [PATCH 6/6] lint --- api/v1beta2/foundationdbbackup_types.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/api/v1beta2/foundationdbbackup_types.go b/api/v1beta2/foundationdbbackup_types.go index af487826a..ad3234df7 100644 --- a/api/v1beta2/foundationdbbackup_types.go +++ b/api/v1beta2/foundationdbbackup_types.go @@ -321,11 +321,12 @@ 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 + hasBackupURLChanged := backup.BackupURL() != backup.Status.BackupDetails.URL - return hasSnapshotSecondsChanged || hasBackupUrlChanged + return hasSnapshotSecondsChanged || hasBackupURLChanged } // CheckReconciliation compares the spec and the status to determine if