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

refactor: remove mount logic in harvester #4910

Merged
merged 1 commit into from
Jan 15, 2024

Conversation

FrankYang0529
Copy link
Member

Solution:
LH introduces VolumeAttachment CRD after v1.5. It can automatically attach/detach volume when there is a request from LH API / controller / CSI. We can just leverage LH mechanism and remove our implementation.

Related Issue:
#4907

Test plan:

Setup

  1. Create a 3-node harvester from this branch.
  2. Setup backup-target.

Case 1: snapshot can work on a stopped VM.

  1. Create a VM.
  2. After the VM is ready, stop the VM.
  3. Check VM volumes are detached.
  4. Take a snapshot on the VM. The snapshot can be ready.

Case 2: restore a snapshot from detached volumes can work.

  1. Follow Case 1.
  2. Make sure VM volumes are detached.
  3. Restore the snapshot to a new VM. The new VM can be ready.
  4. Restore the snapshot to replace the old VM. The old VM can be ready.

Case 3: backup can work on a stopped VM.

  1. Create a VM.
  2. After the VM is ready, stop the VM.
  3. Check VM volumes are detached.
  4. Take a backup on the VM. The backup can be ready.

Case 4: race condition doesn't break VMBackup.

  1. Create a VM.
  2. After the VM is ready, stop the VM.
  3. Check VM volumes are detached.
  4. Take multiple backup on the VM in a short time. All backup can be ready.

Case 5: restore a backup from detached volumes can work.

  1. Follow Case 3.
  2. Make sure VM volumes are detached.
  3. Restore the backup to a new VM. The new VM can be ready.
  4. Restore the backup to replace the old VM. The old VM can be ready.

Signed-off-by: PoAn Yang <poan.yang@suse.com>
Copy link
Member

@WebberHuang1118 WebberHuang1118 left a comment

Choose a reason for hiding this comment

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

LGTM, just a question for restore from VM snapshot, thanks.

pkg/controller/master/backup/restore.go Show resolved Hide resolved
Copy link
Contributor

@Vicente-Cheng Vicente-Cheng left a comment

Choose a reason for hiding this comment

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

LGTM, also tested all test cases related to the snapshot/backup.

Copy link
Member

@WebberHuang1118 WebberHuang1118 left a comment

Choose a reason for hiding this comment

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

LGTM, all testing cases work as expected. Thanks.

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.

LGTM!!

@bk201 bk201 merged commit e25e153 into harvester:master Jan 15, 2024
5 checks passed
@FrankYang0529 FrankYang0529 deleted the HARV-4907 branch January 15, 2024 09:02
@FrankYang0529
Copy link
Member Author

@mergify backport v1.3

Copy link

mergify bot commented May 23, 2024

backport v1.3

✅ Backports have been created

  • Backport to branch v1.3 not needed, change already in branch v1.3

@FrankYang0529
Copy link
Member Author

@mergify backport v1.2

Copy link

mergify bot commented May 23, 2024

backport v1.2

✅ Backports have been created

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants