Revert "Modify Backup URL in Reconciler (#2347)"

This reverts commit b1e744e.

Author: Milla Samuel

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
 	}

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())

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"))
-				})
-			})
 		})
 	})
 

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}

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)
 

fdbclient/admin_client.go

@@ -932,8 +932,6 @@ func (client *cliAdminClient) ModifyBackup(backup *fdbv1beta2.FoundationDBBackup
 			"modify",
 			"-s",
 			strconv.Itoa(backup.SnapshotPeriodSeconds()),
-			"-d",
-			backup.BackupURL(),
 		},
 	})
 

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
 }