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 backup manager tool #694

Merged
merged 27 commits into from
Aug 8, 2019
Merged

Conversation

onlymellb
Copy link
Contributor

What problem does this PR solve?

What is changed and how does it work?

This tool is used by backup and restore controller

Check List

Tests

  • Unit test
  • E2E test
  • Stability test
  • Manual test (add detailed scripts or steps below)
  • No code

Code changes

  • Has Go code change

Side effects

Related changes

Does this PR introduce a user-facing change?:

add backup-manager tool for support backup、restore and clean backup data

@onlymellb onlymellb changed the title [DNM] Add backup manager tool Add backup manager tool Jul 30, 2019
@onlymellb onlymellb requested a review from cofyc July 30, 2019 09:48
BackupScheduleLabelKey string = "tidb.pingcap.com/backup-schedule"

// BackupProtectionFinalizer is the name of finalizer on backups
BackupProtectionFinalizer string = "tidb.pingcap.com/backup-protection"
Copy link
Contributor

Choose a reason for hiding this comment

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

where are those constants used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will be used in backup controller.

cmd/backup-manager/app/backup/backup.go Outdated Show resolved Hide resolved
cmd/backup-manager/app/backup/backup.go Outdated Show resolved Hide resolved
"--verbose=3",
}

dumper := exec.Command("/mydumper", args...)
Copy link
Contributor

Choose a reason for hiding this comment

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

why not use exec.Command().CombinedOutput(), then you can get the output when failed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On the one hand, it is divided into multiple steps to make the error finer. On the other hand, it is possible to introduce streaming backup data in the future. This requires exporting the stdout of the command, refer to this.

onlymellb and others added 2 commits August 1, 2019 10:35
Co-Authored-By: weekface <weekface@gmail.com>
Makefile Outdated
@@ -45,6 +45,12 @@ discovery:
admission-controller:
$(GO) -ldflags '$(LDFLAGS)' -o images/tidb-operator/bin/tidb-admission-controller cmd/admission-controller/main.go

backup-manager:
$(GO) -ldflags '$(LDFLAGS)' -o images/backup-manager/bin/backup-manager cmd/backup-manager/main.go
Copy link
Contributor

Choose a reason for hiding this comment

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

should we add tidb- prefix too? see tidb-admission-controller

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch


func (bo *BackupOpts) getTikvGCLifeTime(db *sql.DB) (string, error) {
var tikvGCTime string
row := db.QueryRow("select variable_value from ? where variable_name= ?", constants.TidbMetaTable, constants.TikvGCVariable)
Copy link
Contributor

Choose a reason for hiding this comment

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

This has SQL injection problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On the one hand, the input parameters of this SQL are controllable. In addition, parameterized statements use parameters instead of concatenating input variables in embedded SQL statements is safe. Refer to here

}

func (bo *BackupOpts) setTikvGClifeTime(db *sql.DB, gcTime string) error {
_, err := db.Exec("update ? set variable_value = ? where variable_name = ?", constants.TidbMetaTable, gcTime, constants.TikvGCVariable)
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

return nil
}

func (bo *BackupOpts) cleanRemoteBackupData(bucket string) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to clean local backup data?

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 will add the cleanup logic of local backup data after the manual test passes

return bm.StatusUpdater.Update(backup, &v1alpha1.BackupCondition{
Type: v1alpha1.BackupFailed,
Status: corev1.ConditionTrue,
Reason: "GetBackupSizeFailed",
Copy link
Contributor

Choose a reason for hiding this comment

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

define these as const?

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 will handle it in the next PR

weekface
weekface previously approved these changes Aug 8, 2019
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

xiaojingchen
xiaojingchen previously approved these changes Aug 8, 2019
@weekface
Copy link
Contributor

weekface commented Aug 8, 2019

/run-e2e-tests

1 similar comment
@xiaojingchen
Copy link
Contributor

/run-e2e-tests

@onlymellb onlymellb dismissed stale reviews from xiaojingchen and weekface via b2eeb0e August 8, 2019 13:04
@onlymellb
Copy link
Contributor Author

/run-e2e-tests

@onlymellb
Copy link
Contributor Author

/run-e2e-tests

@onlymellb onlymellb removed the request for review from tennix August 8, 2019 13:55
@onlymellb onlymellb requested a review from tennix August 8, 2019 14:09
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

@onlymellb onlymellb merged commit d8be966 into pingcap:master Aug 8, 2019
@onlymellb onlymellb deleted the add-backup-manager-tool branch August 8, 2019 15:56
@sre-bot
Copy link
Contributor

sre-bot commented Aug 8, 2019

cherry pick to release-1.1 failed

&& unzip rclone-${VERSION}-linux-amd64.zip \
&& mv rclone-${VERSION}-linux-amd64/rclone /rclone \
&& chmod 755 /rclone \
&& rm -rf rclone-${VERSION}-linux-amd64.zip
Copy link
Contributor

Choose a reason for hiding this comment

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

need to rm rclone-${VERSION}-linux-amd64 directory as well

$BACKUP_BIN $VERBOSE schedule-backup "$@"
;;
*)
echo "Usage: $0 {backup|restore|clean}"
Copy link
Contributor

@LinuxGit LinuxGit Aug 16, 2019

Choose a reason for hiding this comment

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

add schedule-backup in Usage

yahonda pushed a commit that referenced this pull request Dec 27, 2021
* .github: add external link check

* align pingcap/docs-dm#370

* update regex to exclude links

* fix dead external links

Signed-off-by: Ran <huangran@pingcap.com>

Co-authored-by: Ran <huangran@pingcap.com>
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