fix(backup): manually synchronizing backup volumes.

[mantissahz]

Which issue(s) this PR fixes:

Issue # longhorn/longhorn#7982

What this PR does / why we need it:

When users make a requst to get backup volumes information by Backup Volume Get/List APIs, it will also trigger the synchronization for these backup volumes once.

Special notes for your reviewer:

Additional documentation or context

[derekbit]

When users make a BackupVolumeGet or BackupVolumeList HTTP call, they can trigger a backup volume refresh.

Is it possible to synchronize backup volumes by simply editing customer resources? Or do we need to a different method that doesn't involve HTTP calls?

[mantissahz]

Is it possible to synchronize backup volumes by simply editing customer resources? Or do we need to a different method that doesn't involve HTTP calls?

Users can modify the field spec.syncRequestedAt of the BackupVolume CR for synchronizing the backup volume.
And the time of the filed spec.syncRequestedAt should be after the time of the filed status.lastSyncedAt

[shuo-wu]

Can we just simply update backuptarget.spec.syncRequestedAt instead? Updating the spec for all backup volumes is probably not enough since there may be backup volume CR creation/deletion.

[mantissahz]

Can we just simply update backuptarget.spec.syncRequestedAt instead? Updating the spec for all backup volumes is probably not enough since there may be backup volume CR creation/deletion.

Yes, we can and modified. Thanks.

[shuo-wu]

The implementation LGTM.

Currently, the main concern is, will the websocket keep calling BackupVolumeList then Longhorn generates tons of requests to the remote? If users are using public cloud services like S3, it will lead to a higher bill.

[innobead]

Should we consider just updating SyncRequestedAt to trigger the sync? We could have a constant value representing that the sync will be just triggered, and the mutator will update it with a qualified time value instead. WDYT?

In this way, for UI or even manifest, users can trigger the sync immediately with the same experience.

backupTarget.Spec.SyncRequestedAt = "now" // metav1.Time{Time: time.Now().UTC()}

P.S. I assume we don't have schema validation for that property.

[shuo-wu]

That's a good idea. And you don't need to worry about the schema validation. I think we can create a separate PR for this improvement.

[mantissahz]

backupTarget.Spec.SyncRequestedAt = "now" // metav1.Time{Time: time.Now().UTC()}

I think it isn't easy to have a constant value representing to trigger the sync , and the mutator will update it with a qualified time value instead because of https://github.com/longhorn/longhorn-manager/blob/master/webhook/admission/request.go#L43.

[derekbit]

Hello @mantissahz Is the PR ready?

[mantissahz]

Hi @derekbit,
Yes, it's ready. Thanks

[shuo-wu]

Currently, will Longhorn UI show all backup volumes for all backup targets? If NO, it's better to add an input BackupTargetName for API SyncBackupTarget.

[derekbit]

LGTM

[innobead]

backupTarget.Spec.SyncRequestedAt = "now" // metav1.Time{Time: time.Now().UTC()}

I think it isn't easy to have a constant value representing to trigger the sync , and the mutator will update it with a qualified time value instead because of master/webhook/admission/request.go#L43.

I meant, you can rely on the mutator to update now to a qualified time.

[innobead]

In general, LGTM. however, still have some feedback required to resolve.

P.S. In the current implementation, it's specific for REST API. How do users trigger it by updating the CR? need to exactly update the right time in the syncat field of backup target or backup volume on their own? I still recommend a way that can benefit both interfaces (UI and CLI kubectl)

[innobead]

In general, LGTM. however, still have some feedback required to resolve.

P.S. In the current implementation, it's specific for REST API. How do users trigger it by updating the CR? need to exactly update the right time in the syncat field of backup target or backup volume on their own? I still recommend a way that can benefit both interfaces (UI and CLI kubectl)

After a quick discussion with @mantissahz , we added type validation for the sync_at field, so it's infeasible to use a non-time value to trigger the sync. so for non-UI users, we can create a CLI command for it. James will create a ticket for the following improvement.

/chart/templates/crds.yaml#L961-L965

              syncRequestedAt:
                description: The time to request run sync the remote backup target.
                format: date-time
                nullable: true
                type: string

[derekbit]

@mantissahz Ready to review?

[mantissahz]

@mantissahz Ready to review?

@derekbit, Yes, it is ready.

Dismiss for merge

[mantissahz]

@Mergifyio backport v1.6.x

[mergify]

backport v1.6.x

✅ Backports have been created

• #2788 fix(backup): manually synchronizing backup volumes. (backport #2660) has been created for branch v1.6.x