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

br: support ebs snapshot #4698

Merged
merged 75 commits into from
Oct 21, 2022
Merged

br: support ebs snapshot #4698

merged 75 commits into from
Oct 21, 2022

Conversation

sleepymole
Copy link
Contributor

@sleepymole sleepymole commented Sep 9, 2022

What problem does this PR solve?

What is changed and how does it work?

Code changes

  • Has Go code change
  • Has CI related scripts change

Tests

  • Unit test
  • E2E test
  • Manual test
  1. basic backup functionality -- passed
  2. basic restore functionality -- passed
  3. backup management - get backup/delete backup -- passed
  4. restore to the original EKS cluster -- passed
  5. restore with the new cluster name -- passed
  6. restore with the new namespace -- passed
  7. backup with multi-volumes, raft-engine.dir and storage.data-dir. restore to original EKS cluster. do checksum --passed
  • No code

Side effects

  • Breaking backward compatibility
  • Other side effects:

Related changes

  • Need to cherry-pick to the release branch
  • Need to update the documentation

Release Notes

Please refer to Release Notes Language Style Guide before writing the release note.

br: support ebs snapshot

xu21yingan and others added 30 commits May 24, 2022 03:37
Support BR on Azure Blob Storage
@sleepymole
Copy link
Contributor Author

/test pull-e2e-kind-across-kubernetes

@sleepymole
Copy link
Contributor Author

/test pull-e2e-kind
/test pull-e2e-kind-across-kubernetes

cmd/backup-manager/app/restore/restore.go Show resolved Hide resolved
pkg/backup/snapshotter/snapshotter.go Outdated Show resolved Hide resolved
@@ -663,6 +679,19 @@ func canSkipSetGCLifeTime(image string) bool {
return true
}

func BytesToString(b []byte) string {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
func BytesToString(b []byte) string {
func BytesToStringWithoutAllocation(b []byte) string {

Maybe declaring the meaning of this function in name is better? Or it is hard to understand.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I doubt if we really need to do this hack to improve performance while it's not used in hot path. cc @fengou1

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

@WizardXiao WizardXiao left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-chi-bot
Copy link
Member

@WizardXiao: Thanks for your review. The bot only counts approvals from reviewers and higher roles in list, but you're still welcome to leave your comments.

In response to this:

LGTM

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

Copy link
Contributor

@fengou1 fengou1 left a comment

Choose a reason for hiding this comment

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

currently, my validation shows the PR is good enough to go.

@ti-chi-bot
Copy link
Member

@fengou1: Thanks for your review. The bot only counts approvals from reviewers and higher roles in list, but you're still welcome to leave your comments.

In response to this:

currently, my validation shows the PR is good enough to go.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

@ti-chi-bot
Copy link
Member

@WangLe1321: Thanks for your review. The bot only counts approvals from reviewers and higher roles in list, but you're still welcome to leave your comments.

In response to this:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

@csuzhangxc
Copy link
Member

/merge

@ti-chi-bot
Copy link
Member

@csuzhangxc: /merge in this pull request requires 2 approval(s).

In response to this:

/merge

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

Copy link
Contributor

@fengou1 fengou1 left a comment

Choose a reason for hiding this comment

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

it is good to go

@sleepymole
Copy link
Contributor Author

/test pull-e2e-kind

@sleepymole
Copy link
Contributor Author

/merge

@ti-chi-bot
Copy link
Member

This pull request has been accepted and is ready to merge.

Commit hash: d12420c

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

Successfully merging this pull request may close these issues.

10 participants