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

[DNM] refactor backup CR related logic #3723

Closed
wants to merge 6 commits into from

Conversation

dragonly
Copy link
Contributor

@dragonly dragonly commented Jan 19, 2021

What problem does this PR solve?

Found backup CR cleaning related logic confusing.

What is changed and how does it work?

Refactored some function with identical behavior.

Code changes

  • Has Go code change
  • Has CI related scripts change

Tests

  • Unit test

  • E2E test

  • Manual test

    • test backup using following yaml (dumpling)

      apiVersion: pingcap.com/v1alpha1
      kind: Backup
      metadata:
          name: demo1-backup-s3
          namespace: default
      spec:
          from:
              host: tctest-tidb
              port: 4000
              user: root
              secretName: backup-demo1-tidb-secret
          s3:
              provider: aws
              bucket: test-lyl
              secretName: s3-secret
              region: us-west-2
              cluster: tctest
          storageClassName: standard
          storageSize: 1Gi

      backup succeeded

      NAME              STATUS     BACKUPPATH                                      BACKUPSIZE   COMMITTS             AGE
      demo1-backup-s3   Complete   s3://test-lyl/backup-2021-01-27T10:12:04Z.tgz   255 B        422508579913203714   20s
      
    • test backup using following yaml (br)

      apiVersion: pingcap.com/v1alpha1
      kind: Backup
      metadata:
          name: demo1-backup-s3
          namespace: default
      spec:
          br:
              cluster: tctest
          s3:
              provider: aws
              bucket: test-lyl
              secretName: s3-secret
              region: us-west-2
              cluster: tctest
          storageClassName: standard
          storageSize: 1Gi

      backup succeeded

      NAME              STATUS     BACKUPPATH      BACKUPSIZE   COMMITTS             AGE
      demo1-backup-s3   Complete   s3://test-lyl   16 B         422508672830144513   25s
      
    • test backup using following yaml (dumpling)

      apiVersion: pingcap.com/v1alpha1
      kind: Backup
      metadata:
          name: demo1-backup-s3
          namespace: default
      spec:
          cleanPolicy: Delete
          from:
              host: tctest-tidb
              port: 4000
              user: root
              secretName: backup-demo1-tidb-secret
          s3:
              provider: aws
              bucket: test-lyl
              secretName: s3-secret
              region: us-west-2
              cluster: tctest
          storageClassName: standard
          storageSize: 1Gi

      Backup succeeded. When backup deleted, data on s3 deleted

    • test backup schedule with cleanPolicy Retain (br)

      apiVersion: pingcap.com/v1alpha1
      kind: BackupSchedule
      metadata:
          name: demo1-backup-schedule-s3
          namespace: default
      spec:
          maxBackups: 1
          schedule: "*/1 * * * *"
          backupTemplate:
              br:
                  cluster: tctest
              s3:
                  provider: aws
                  bucket: test-lyl
                  secretName: s3-secret
                  region: us-west-2
                  cluster: tctest

      a new backup will be created every minute, last backup deleted

      NAME                                           STATUS     BACKUPPATH                                                 BACKUPSIZE   COMMITTS             AGE
      demo1-backup-schedule-s3-2021-01-27t10-39-00   Complete   s3://test-lyl/tctest-pd.default-2379-2021-01-27t10-39-00   16 B         422509009514790913   15s
      
  • 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.

None

@codecov-io
Copy link

codecov-io commented Jan 19, 2021

Codecov Report

Merging #3723 (cfb5478) into master (6698595) will increase coverage by 0.02%.
The diff coverage is 90.90%.

@@            Coverage Diff             @@
##           master    #3723      +/-   ##
==========================================
+ Coverage   62.32%   62.35%   +0.02%     
==========================================
  Files         165      165              
  Lines       17799    17805       +6     
==========================================
+ Hits        11093    11102       +9     
+ Misses       5631     5629       -2     
+ Partials     1075     1074       -1     
Flag Coverage Δ
unittest 62.35% <90.90%> (?)

Copy link
Contributor

@DanielZhangQD DanielZhangQD left a comment

Choose a reason for hiding this comment

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

LGTM
Please update the test result to the PR description.

@ti-chi-bot
Copy link
Member

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • DanielZhangQD

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by writing /lgtm in a comment.
Reviewer can cancel approval by writing /lgtm cancel in a comment.

@dragonly
Copy link
Contributor Author

/test pull-e2e-kind

@dragonly dragonly changed the title refactor backup CR related logic [DNM] refactor backup CR related logic Jan 28, 2021
@dragonly dragonly closed this Mar 11, 2021
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.

None yet

5 participants