-
Notifications
You must be signed in to change notification settings - Fork 500
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
support rclone options in backup and restore cr #2318
Conversation
pkg/apis/pingcap/v1alpha1/types.go
Outdated
@@ -843,6 +843,8 @@ type S3StorageProvider struct { | |||
Prefix string `json:"prefix,omitempty"` | |||
// SSE Sever-Side Encryption. | |||
SSE string `json:"sse,omitempty"` | |||
// Options Rclone options for backup and restore with mydumper and lightning. | |||
Options string `json:"options,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not use string slice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure whether expose args by a string is ok to users. Can we import the rclone configFlag as rcloneConfig in the Backup
and Restore
Spec like tidb/tikv/pd config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be too much config fields. Since mydumper+lightning is not our focus already, I think it's OK to support like this.
I will change it to string slice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we don't have to set all the configuration of rclone, just use the configuration we need.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@cofyc comments addressed, PTAL. |
/merge |
/run-all-tests |
@DanielZhangQD merge failed. |
/merge |
/run-all-tests |
cherry pick to release-1.1 in PR #2326 |
What problem does this PR solve?
Fix #2270
What is changed and how does it work?
Support specifying rclone options in the backup and restore CR which can be used during backup and restore with mydumper and lightning.
For the issue in #2270, we need to set
--ignore-checksum
option for rclone, for detail, please check the doc.CR example:
Check List
Tests
Code changes
Related changes
Does this PR introduce a user-facing change?: