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 Amazon S3 support to the backup/restore features #606

Merged
merged 7 commits into from
Jul 2, 2019

Conversation

sokada1221
Copy link
Contributor

What problem does this PR solve?

#604

What is changed and how it works?

Respective commit comment should explain what's being changed. Added Amazon S3 support to the backup/restore feature.

The problem with the current design is that Ceph and S3 has to share the same set of environment variables (AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY) and thus both cannot be enabled at the same time unless we make changes to uploader and downloader. Given that they're planned to be deprecated with rclone, I wouldn't address this limitation in this PR.

Check List

Tests

  • Unit test
  • Manual test (add detailed scripts or steps below)
  1. Enable scheduledBackup, and upload to Amazon S3 everyday - uploaded successfully to the specified S3 bucket
  2. Enable scheduledBackup, and upload to Amazon S3 every 15 minutes - uploaded successfully to the specified S3 bucket
  3. Restore manually according to the documentation - downloaded and loaded the data successfully from the specified S3 bucket

Code changes

  • Has Helm charts change
  • Has Go code change
  • Has documents change

Side effects

  • N/A

Related changes

  • N/A

Does this PR introduce a user-facing change?:

Full backup data can be uploaded to or downloaded from Amazon S3

@sre-bot
Copy link
Contributor

sre-bot commented Jun 25, 2019

Hi contributor, thanks for your PR.

This patch needs to be approved by someone of admins. They should reply with "/ok-to-test" to accept this PR for running test automatically.

@CLAassistant
Copy link

CLAassistant commented Jun 25, 2019

CLA assistant check
All committers have signed the CLA.

@gregwebs
Copy link
Contributor

/ok-to-test

@@ -224,7 +224,7 @@ fullbackup:
binlogImage: pingcap/tidb-binlog:v3.0.0-rc.1
binlogImagePullPolicy: IfNotPresent
# https://github.com/tennix/tidb-cloud-backup
mydumperImage: pingcap/tidb-cloud-backup:latest
mydumperImage: pingcap/tidb-cloud-backup:20190610
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this change was made for testing and shouldn't be committed? Perhaps we need to improve our e2e testing workflow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay to revert the changes to e2e testing. Just thought it'd make more sense to test with the same version of tidb-cloud-backup as the main code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another thing is - is this tag 20190610 appropriate to be used in tidb-operator? The only reason why I replaced this is because the latest tag is outdated, and doesn't support Amazon S3.

Copy link
Contributor

Choose a reason for hiding this comment

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

the 20190610 is the latest of tidb-cloud-backup image. use it instead of latest.

Copy link
Member

Choose a reason for hiding this comment

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

@shinnosuke-okada Do we have to update the tidb-cloud-backup code to support S3 backup/restore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tennix Yes, the latest tag is outdated, and cannot be used to upload to/download from Amazon S3.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tennix My bad! Just realized that I misread your message here last time. No need to change the code. The latest code is good enough to support S3 backup/restore, and it's available via 20190610. If you plan to rebuild the latest tag or some other tag, we can use that as well.

@@ -2032,7 +2032,7 @@ func (oa *operatorActions) getBackupDir(info *TidbClusterConfig) ([]string, erro
Containers: []corev1.Container{
{
Name: getBackupDirPodName,
Image: "pingcap/tidb-cloud-backup:latest",
Image: "pingcap/tidb-cloud-backup:20190610",
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this change was made for testing and shouldn't be committed?

@weekface
Copy link
Contributor

@tennix PTAL

Copy link
Member

@tennix tennix left a comment

Choose a reason for hiding this comment

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

LGTM

@weekface
Copy link
Contributor

weekface commented Jul 1, 2019

/run-e2e-tests

Copy link
Contributor

@weekface weekface left a comment

Choose a reason for hiding this comment

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

lgtm

@weekface
Copy link
Contributor

weekface commented Jul 1, 2019

/run-e2e-tests

@weekface
Copy link
Contributor

weekface commented Jul 2, 2019

The e2e was broken now, I am working on it.

@weekface
Copy link
Contributor

weekface commented Jul 2, 2019

/run-e2e-tests

1 similar comment
@weekface
Copy link
Contributor

weekface commented Jul 2, 2019

/run-e2e-tests

@weekface weekface merged commit 0435344 into pingcap:master Jul 2, 2019
@sokada1221 sokada1221 deleted the support-s3-backup branch July 4, 2019 19:08
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.

6 participants