Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add sync backup volume option to allow users to manually synchronize backup volume #725

Merged
merged 4 commits into from
May 14, 2024
Merged

Add sync backup volume option to allow users to manually synchronize backup volume #725

merged 4 commits into from
May 14, 2024

Conversation

a110605
Copy link
Contributor

@a110605 a110605 commented May 10, 2024 •
edited
Loading

Goal

  • Add Sync Backup Volume option for each backup volume to allow users to request synchronized backup volume.
  • Add Sync all Backup Volumes button to sync all backup volumes.
Screenshot 2024-05-10 at 4 25 19 PM

Issue

longhorn/longhorn#8501

Test Result

Success

Screenshot 2024-05-11 at 12 43 20 AM 1f2"> Screenshot 2024-05-10 at 5 18 32 PM

Failed

Screenshot 2024-05-11 at 12 34 56 AM

@a110605 a110605 force-pushed the enable_backupVolumeSync branch 3 times, most recently from 70e867f to b5eee34 Compare May 10, 2024 09:17
@a110605 a110605 marked this pull request as ready for review May 10, 2024 09:22
a110605
a110605 commented May 10, 2024
View reviewed changes
src/models/backup.js Outdated Show resolved Hide resolved
@a110605 a110605 changed the title Add sync backup volume option to allow user manually synchronizing backup volume Add sync backup volume option to allow users to manually synchronize backup volume May 10, 2024
@mantissahz mantissahz requested a review from a team May 13, 2024 04:02
mantissahz
mantissahz previously approved these changes May 13, 2024
View reviewed changes
Copy link
Contributor

@mantissahz mantissahz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

src/services/backup.js Outdated Show resolved Hide resolved
src/models/backup.js Outdated Show resolved Hide resolved
src/routes/backup/BackupVolumeList.js
@@ -248,6 +250,7 @@ class List extends React.Component {
<DropOption menuOptions={[
{ key: 'recovery', name: 'Create Disaster Recovery Volume', disabled: !record.lastBackupName || (record.messages && record.messages.error) },
{ key: 'restoreLatestBackup', name: 'Restore Latest Backup', disabled: !record.lastBackupName || (record.messages && record.messages.error) },
{ key: 'syncBackupVolume', name: 'Sync Backup Volume' },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Not sure about the wording.

@a110605
add sync backup volume option to allow user manually synchronizing ba…
77fd578 
…ckup volumes

Signed-off-by: andy.lee <andy.lee@suse.com>
@a110605
add message for success sync API call
1a970a5 
Signed-off-by: andy.lee <andy.lee@suse.com>
@a110605
change success message and extend message exist timeout to 5 seconds
14d37bb 
Signed-off-by: andy.lee <andy.lee@suse.com>
@derekbit
Copy link
Member

Can we have a button that syncs all backup volumes?
WDYT @a110605 @mantissahz @innobead

@a110605
Copy link
Contributor Author

a110605 commented May 14, 2024 •
edited
Loading

Can we have a button that syncs all backup volumes? WDYT @a110605 @mantissahz @innobead

As far as I know, longhorn-manager supports PUT SyncAllBackupVolumes: true to sync all backup volume.

But it's NOT to sync the SELECTED backup volume on table. If we wish to sync selected backup volume, it need to do batch POST SyncBackupVolume: true for selected backup volume.

Or we just want one button named Sync all backup volumes on the top to sync all backup volume in page ?

@derekbit
Copy link
Member

Can we have a button that syncs all backup volumes? WDYT @a110605 @mantissahz @innobead

As far as I know, longhorn-manager supports PUT SyncAllBackupVolumes: true to sync all backup volume.

But it's NOT to sync the SELECTED backup volume on table. If we wish to sync selected backup volume, it need to do batch POST SyncBackupVolume: true for selected backup volume.

Or we just one button named Sync all backup volumes on the top to sync all backup volume in page (would be easy case)?

Selected volumes might be over-engineering. I think sync all backup volume is good enough.

@a110605
Copy link
Contributor Author

a110605 commented May 14, 2024 •
edited
Loading

Can we have a button that syncs all backup volumes? WDYT @a110605 @mantissahz @innobead

As far as I know, longhorn-manager supports PUT SyncAllBackupVolumes: true to sync all backup volume.
But it's NOT to sync the SELECTED backup volume on table. If we wish to sync selected backup volume, it need to do batch POST SyncBackupVolume: true for selected backup volume.
Or we just one button named Sync all backup volumes on the top to sync all backup volume in page (would be easy case)?

Selected volumes might be over-engineering. I think sync all backup volume is good enough.

ok, let me add new commit for that sync all button.

@a110605
Add Sync all Backup Volumes button
6e51c8d 
Signed-off-by: andy.lee <andy.lee@suse.com>
@a110605
Copy link
Contributor Author

a110605 commented May 14, 2024 •
edited
Loading

@derekbit , @mantissahz , added in 6e51c8d

add

@a110605 a110605 requested a review from mantissahz May 14, 2024 03:56
mantissahz
mantissahz approved these changes May 14, 2024
View reviewed changes
Copy link
Contributor

@mantissahz mantissahz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@a110605 a110605 merged commit 21a0d48 into longhorn:master May 14, 2024
5 checks passed
@a110605
Copy link
Contributor Author

a110605 commented May 14, 2024

@mergify backport v1.6.x

@mergify Mergify
Copy link

mergify bot commented May 14, 2024 •
edited
Loading

backport v1.6.x

✅ Backports have been created

@mergify mergify bot mentioned this pull request May 14, 2024
Merged
@mergify Mergify
Copy link

mergify bot commented May 14, 2024

backport v1.6.x

✅ Backports have been created

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mantissahz mantissahz mantissahz approved these changes

@derekbit derekbit Awaiting requested review from derekbit

Assignees

@a110605 a110605

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@a110605 @derekbit @mantissahz