From d64f211a88bef119245881bfb91144f755d35b26 Mon Sep 17 00:00:00 2001 From: Milla Samuel Date: Tue, 11 Nov 2025 13:26:36 +0800 Subject: [PATCH 1/2] Revert "Modify Backup URL in Reconciler (#2347)" This reverts commit b1e744ef9c7fd00b91f53075e9ed85b4a081edd1. --- api/v1beta2/foundationdbbackup_types.go | 11 ++-------- api/v1beta2/foundationdbbackup_types_test.go | 6 ++--- controllers/admin_client_test.go | 23 +------------------- controllers/modify_backup.go | 4 +--- docs/manual/technical_design.md | 2 +- fdbclient/admin_client.go | 2 -- pkg/fdbadminclient/mock/admin_client_mock.go | 1 - 7 files changed, 7 insertions(+), 42 deletions(-) diff --git a/api/v1beta2/foundationdbbackup_types.go b/api/v1beta2/foundationdbbackup_types.go index 97c0a0504..7ee23eda5 100644 --- a/api/v1beta2/foundationdbbackup_types.go +++ b/api/v1beta2/foundationdbbackup_types.go @@ -376,14 +376,6 @@ 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) { @@ -413,7 +405,8 @@ func (backup *FoundationDBBackup) CheckReconciliation() (bool, error) { reconciled = false } - if isRunning && backup.NeedsBackupReconfiguration() { + if isRunning && + backup.SnapshotPeriodSeconds() != backup.Status.BackupDetails.SnapshotPeriodSeconds { 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 f2595f9f5..60d8462a7 100644 --- a/api/v1beta2/foundationdbbackup_types_test.go +++ b/api/v1beta2/foundationdbbackup_types_test.go @@ -52,9 +52,6 @@ var _ = Describe("[api] FoundationDBBackup", func() { }, Spec: FoundationDBBackupSpec{ AgentCount: &agentCount, - BlobStoreConfiguration: &BlobStoreConfiguration{ - AccountName: "test@test-service", - }, }, Status: FoundationDBBackupStatus{ Generations: BackupGenerationStatus{ @@ -63,7 +60,7 @@ var _ = Describe("[api] FoundationDBBackup", func() { AgentCount: 3, DeploymentConfigured: true, BackupDetails: &FoundationDBBackupStatusBackupDetails{ - URL: "blobstore://test@test-service:443/sample-cluster?bucket=fdb-backups", + URL: "blobstore://test@test-service/sample-cluster?bucket=fdb-backups", Running: true, SnapshotPeriodSeconds: 864000, }, @@ -72,6 +69,7 @@ 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 c97c7c24e..d93ac93ae 100644 --- a/controllers/admin_client_test.go +++ b/controllers/admin_client_test.go @@ -333,36 +333,15 @@ 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 have modified the snapshot time", func() { + It("should mark the backup as stopped", 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 2a8d041db..0a4b54b9a 100644 --- a/controllers/modify_backup.go +++ b/controllers/modify_backup.go @@ -41,9 +41,7 @@ func (s modifyBackup) reconcile( return nil } - // The modify command is only required for continuous backups. - if backup.NeedsBackupReconfiguration() && - backup.GetBackupMode() == fdbv1beta2.BackupModeContinuous { + if backup.Status.BackupDetails.SnapshotPeriodSeconds != backup.SnapshotPeriodSeconds() || backup.GetBackupMode() == fdbv1beta2.BackupModeContinuous { 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 e1b9aff31..2a04a8edd 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` and `url` property. +Currently, this only supports the `snapshotPeriodSeconds` property. ### UpdateBackupStatus (again) diff --git a/fdbclient/admin_client.go b/fdbclient/admin_client.go index 4a3500dae..7db9e5e44 100644 --- a/fdbclient/admin_client.go +++ b/fdbclient/admin_client.go @@ -932,8 +932,6 @@ 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 8bad37f8f..f4dc915af 100644 --- a/pkg/fdbadminclient/mock/admin_client_mock.go +++ b/pkg/fdbadminclient/mock/admin_client_mock.go @@ -941,7 +941,6 @@ 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 76e7426e159fc108b600a5f48412b1fbf9f71ce1 Mon Sep 17 00:00:00 2001 From: Milla Samuel Date: Tue, 11 Nov 2025 15:01:51 +0800 Subject: [PATCH 2/2] make clean all --- controllers/modify_backup.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/controllers/modify_backup.go b/controllers/modify_backup.go index 0a4b54b9a..cedcb6b32 100644 --- a/controllers/modify_backup.go +++ b/controllers/modify_backup.go @@ -41,7 +41,8 @@ func (s modifyBackup) reconcile( return nil } - if backup.Status.BackupDetails.SnapshotPeriodSeconds != backup.SnapshotPeriodSeconds() || backup.GetBackupMode() == fdbv1beta2.BackupModeContinuous { + if backup.Status.BackupDetails.SnapshotPeriodSeconds != backup.SnapshotPeriodSeconds() || + backup.GetBackupMode() == fdbv1beta2.BackupModeContinuous { adminClient, err := r.adminClientForBackup(ctx, backup) if err != nil { return &requeue{curError: err}