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.

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

Add VM schedules pages #1103

Merged
merged 6 commits into from
Sep 27, 2024
Merged

Add VM schedules pages #1103

merged 6 commits into from
Sep 27, 2024

Conversation

a110605
Copy link
Collaborator

@a110605 a110605 commented Aug 14, 2024

Summary

  • add VM schedule list / edit / details pages
  • add VM schedule column in vmbackup / vmsnapshot list pages
  • add Create VM Scheduleaction for VM operation menu
  • show volume backup error message and readyToUse in backup/snapshot detail volume tab <- discussion conclusion with the team
  • upgrade cron-validator and cronstrue to the latest version

Note. this UI PR needs wait for backend PR harvester/harvester#6149 merge first.

PR Checklist

  • Is this a multi-tenancy feature/bug?
    • Yes, the relevant RBAC changes are at:
  • Do we need to backport changes to the old Rancher UI, such as RKE1?
    • Yes, the relevant PR is at:
  • Are backend engineers aware of UI changes?

Related Issue #
harvester/harvester#6058

Technical notes summary

run yarn install before run up the harvester dashboard

Screenshot/Video

  1. Create VM Schedule
create_scheduling.mov
  1. Edit VM Schedule
update_schedule.mov
  1. See VM Schedule detail
schedule_detail.mov
  1. show error banner if not set backup target
show_error_backup_target.mov
  1. When create back up failed and reach Max Failure threshold, the schedule job would auto suspended.

auto_stop_schedule

  1. Show volume backup error message and readyToUse in backup/snapshot detail volume tab
volume_backup_error.mov

@a110605 a110605 self-assigned this Aug 14, 2024
@a110605 a110605 force-pushed the issue-6058 branch 2 times, most recently from 6bc1799 to 2c50eb2 Compare August 14, 2024 10:13
@a110605 a110605 marked this pull request as ready for review August 14, 2024 10:30
@a110605 a110605 force-pushed the issue-6058 branch 2 times, most recently from 0846983 to a68a6cf Compare August 21, 2024 02:07
@a110605 a110605 added the New Feature New feature for up coming release label Aug 26, 2024
@a110605
Copy link
Collaborator Author

a110605 commented Sep 9, 2024

Let me rebase and test it again since backend PR has some minor change.

@a110605
Copy link
Collaborator Author

a110605 commented Sep 19, 2024

Hi @WebberHuang1118 , @torchiaf , I've rebased this PR and ready to review.

Copy link
Collaborator

@torchiaf torchiaf left a comment

Choose a reason for hiding this comment

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

I'm working on the review, before I look at the code, I found the following error, could you check ?

  • 'Create Schedule' title starts with ':'
  • Cannot read properties of undefined (reading 'linkFor') when click create button.

image

@a110605
Copy link
Collaborator Author

a110605 commented Sep 23, 2024

I'm working on the review, before I look at the code, I found the following error, could you check ?

  • 'Create Schedule' title starts with ':'
  • Cannot read properties of undefined (reading 'linkFor') when click create button.

The backend PR is merged. Could you use master-head deployment and try again?

Should be looks like
Screenshot 2024-09-23 at 11 12 39 AM

Copy link

mergify bot commented Sep 24, 2024

This pull request is now in conflict. Could you fix it @a110605? 🙏

@a110605
Copy link
Collaborator Author

a110605 commented Sep 24, 2024

This pull request is now in conflict. Could you fix it @a110605? 🙏

Done rebase.

@WebberHuang1118
Copy link
Member

LGTM, thans for the PR.

Just a point, if the schedule is suspended, the GUI doesn't show the condition?

For example, if the user suspends the schedule proactively, ScheduleVMBackup CR's status.conditions will have related message and reason.

@a110605
Copy link
Collaborator Author

a110605 commented Sep 24, 2024

LGTM, thans for the PR.

Just a point, if the schedule is suspended, the GUI doesn't show the condition?

For example, if the user suspends the schedule proactively, ScheduleVMBackup CR's status.conditions will have related message and reason.

The stateDescription() is grabbing from this.state.conditions.message, will look like

Screenshot 2024-09-24 at 4 41 24 PM Screenshot 2024-09-24 at 4 38 14 PM

Copy link
Member

@bk201 bk201 left a comment

Choose a reason for hiding this comment

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

The UX lgtm. However, I feel the word "VM Schedule" is confusing. Maybe "VM Backup Schedule" is more clear?
@jillian-maroket @asettle What do you think about this term.

@a110605
Copy link
Collaborator Author

a110605 commented Sep 25, 2024

The UX lgtm. However, I feel the word "VM Schedule" is confusing. Maybe "VM Backup Schedule" is more clear? @jillian-maroket @asettle What do you think about this term.

But user can still create snapshot schedule job from this page, that why I give it Virtual Machine Schedule.

Copy link
Collaborator

@torchiaf torchiaf left a comment

Choose a reason for hiding this comment

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

I left some comments and there are some small regressions:

  • When you click on 'Replace existing' / 'Restore New' from Virtual Machine Snapshots page or Virtual Machine Schedules -> Snapshots tab:

    1. The UI is redirected to Backups create page rather than Snapshots create.
    2. If you hit Cancel the UI is redirected to Backups page. The redirection should be to Virtual Machine Snapshots or Virtual Machine Schedules -> Snapshots tab, depending on navigation history.

Virtual Machine Snapshots

Screencast.from.2024-09-26.14-37-07.webm

Virtual Machine Schedules -> Snapshots tab

Screencast.from.2024-09-26.14-45-53.webm

Consider that some button actions are in common between Snapshots and Backup models.

@@ -32,8 +32,8 @@
"@vue/test-utils": "1.2.1",
"babel-eslint": "10.1.0",
"core-js": "3.25.3",
"cron-validator": "1.2.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should port this fix in rancher/dashboard.

pkg/harvester/dialog/HarvesterScheduleModal.vue Outdated Show resolved Hide resolved
- add VM schedule list/edit/details pages
- add VM schedule column in vmbackup/vmsnapshot list pages
- add action menu for VM
- show volume backup error message in backup/snapshot detail volume tab

Signed-off-by: andy.lee <andy.lee@suse.com>
Signed-off-by: andy.lee <andy.lee@suse.com>
Signed-off-by: andy.lee <andy.lee@suse.com>
Signed-off-by: andy.lee <andy.lee@suse.com>
Signed-off-by: andy.lee <andy.lee@suse.com>
@a110605
Copy link
Collaborator Author

a110605 commented Sep 26, 2024

@WebberHuang1118 , could you help check why I can't restore new / restore existing snapshot or backup using schedule job generated backup / snapshot? It's fine to restore snapshot/backup which is manually created.

image

@torchiaf torchiaf self-requested a review September 26, 2024 15:08
Copy link
Collaborator

@torchiaf torchiaf left a comment

Choose a reason for hiding this comment

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

LGTM UI side.

@torchiaf
Copy link
Collaborator

torchiaf commented Sep 26, 2024

@WebberHuang1118 , could you help check why I can't restore new / restore existing snapshot or backup using schedule job generated backup / snapshot? It's fine to restore snapshot/backup which is manually created.

image

IMHO the 500 error should be handled backend side, but the error message is consistent: If I stop the VM schedule from which the VM backups was created, I can manually restore the backup into a new VM.

@WebberHuang1118
Copy link
Member

@WebberHuang1118 , could you help check why I can't restore new / restore existing snapshot or backup using schedule job generated backup / snapshot? It's fine to restore snapshot/backup which is manually created.

image

Yes, the Harvester controller has the webhook to prevent restoring the snapshot/backup from a running schedule, since the backup/snapshots will be cleansed under the hood, leading to restore failure. By design, before restoring, the user should suspend the schedule first.

@bk201 bk201 added the Backport to v1.4 Backport PR target v1.4 label Sep 27, 2024
@bk201 bk201 merged commit a537c1a into harvester:master Sep 27, 2024
9 checks passed
@mergify mergify bot mentioned this pull request Sep 27, 2024
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backport to v1.4 Backport PR target v1.4 New Feature New feature for up coming release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants