Modify Backup URL in Reconciler (#2347)

* Allow to modify the backup URL

Co-authored-by: Milla Samuel <millas@palantir.com>

Author: millasml

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
 	}

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

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

controllers/modify_backup.go

@@ -41,7 +41,7 @@ func (s modifyBackup) reconcile(
 		return nil
 	}
 
-	if backup.Status.BackupDetails.SnapshotPeriodSeconds != backup.SnapshotPeriodSeconds() {
+	if backup.NeedsBackupReconfiguration() {
 		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` property.
+Currently, this only supports the `snapshotPeriodSeconds` and `url` property.
 
 ### UpdateBackupStatus (again)
 

fdbclient/admin_client.go

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

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
 }