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.

Sign up for GitHub

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

Jump to bottom

br: add additional volumes to backup/restore/warmup pods #5414

Merged
merged 10 commits into from
Nov 30, 2023
Merged

br: add additional volumes to backup/restore/warmup pods #5414

merged 10 commits into from
Nov 30, 2023

Conversation

WangLe1321
Copy link
Contributor

@WangLe1321 WangLe1321 commented Nov 23, 2023 •
edited

What problem does this PR solve?

Add additional volumes and volume mounts to backup/restore/warmup pods, then we can support users to mount configMap these pods.

Closes #5404

Add deadline to backup-init job of 10 minutes, in case it blocks gc and pd schedule indefinitely.

Closes #5387
Closes #5409

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
  • No code

I add additionalVolumes and additionalVolumeMounts to the VolumeBackup

    additionalVolumeMounts:
    - mountPath: /etc/foo
      name: foo
      readOnly: true
    additionalVolumes:
    - configMap:
        name: br-test
      name: foo

I see the volume and volume mount appears in the backup and backup-init pod

    volumeMounts:
    - mountPath: /etc/foo
      name: foo
      readOnly: true

  volumes:
  - configMap:
      defaultMode: 420
      name: br-test
    name: foo

for deadline of backup init job, I see the corresponding fields appearing in the job activeDeadlineSeconds: 600

and the job failed due to the deadline

status:
  conditions:
  - lastProbeTime: "2023-11-23T09:07:17Z"
    lastTransitionTime: "2023-11-23T09:07:17Z"
    message: Job was active longer than specified deadline
    reason: DeadlineExceeded
    status: "True"
    type: Failed

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.

@ti-chi-bot ti-chi-bot bot added the size/XXL label Nov 23, 2023
@WangLe1321
Copy link
Contributor Author

/run-pull-e2e-kind-br

@codecov-commenter
Copy link

codecov-commenter commented Nov 23, 2023 •
edited

Codecov Report

Merging #5414 (1aa3f2f) into master (02a6c02) will decrease coverage by 0.01%.
The diff coverage is 21.48%.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #5414      +/-   ##
==========================================
- Coverage   61.60%   61.59%   -0.01%     
==========================================
  Files         228      239      +11     
  Lines       28953    32828    +3875     
==========================================
+ Hits        17836    20222    +2386     
- Misses       9376    10745    +1369     
- Partials     1741     1861     +120
Flag Coverage Δ
e2e 22.03% <20.00%> (?)
unittest 61.38% <4.54%> (-0.22%) ⬇️

@ti-chi-bot ti-chi-bot bot added the lgtm label Nov 23, 2023
@ti-chi-bot ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Nov 23, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BornChanger

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

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot ti-chi-bot bot added the approved label Nov 23, 2023
@WangLe1321
Copy link
Contributor Author

/pull-e2e-kind-br

@ti-chi-bot ti-chi-bot bot removed the lgtm label Nov 24, 2023
@ti-chi-bot ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Nov 24, 2023

[LGTM Timeline notifier]

Timeline:

  • 2023-11-23 11:44:01.539144672 +0000 UTC m=+491070.204370867: ☑️ agreed by BornChanger.
  • 2023-11-24 02:10:57.497735711 +0000 UTC m=+543086.162961907: ✖️🔁 reset by ti-chi-bot[bot].

@ti-chi-bot ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Nov 24, 2023

New changes are detected. LGTM label has been removed.

@WangLe1321
Copy link
Contributor Author

/pull-e2e-kind-br

@WangLe1321
Copy link
Contributor Author

/run-pull-e2e-kind-br

@WangLe1321
Copy link
Contributor Author

/pull-e2e-kind-br

@WangLe1321
Copy link
Contributor Author

/run-pull-e2e-kind-br

@nkg-
Copy link
Contributor

nkg- commented Nov 27, 2023

Add deadline to backup-init job of 10 minutes, in case it blocks gc and pd schedule indefinitely.

Thanks for supporting this. My 2c is to move to another PR. Since its a logically different change, than indicated in the title.

@WangLe1321
Copy link
Contributor Author

/run-pull-e2e-kind-br

1 similar comment
@WangLe1321
Copy link
Contributor Author

/run-pull-e2e-kind-br

@WangLe1321
Copy link
Contributor Author

/run-pull-e2e-kind-br

@WangLe1321
Copy link
Contributor Author

/run-pull-e2e-kind-br

@csuzhangxc csuzhangxc merged commit 6b6e046 into pingcap:master Nov 30, 2023
5 checks passed
@csuzhangxc
Copy link
Member

/cherry-pick release-1.5

@ti-chi-bot ti-chi-bot mentioned this pull request Nov 30, 2023
Merged
10 tasks
@ti-chi-bot
Copy link
Member

@csuzhangxc: new pull request created to branch release-1.5: #5432.

In response to this:

/cherry-pick release-1.5

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 pushed a commit to ti-chi-bot/tidb-operator that referenced this pull request Nov 30, 2023
@WangLe1321 @ti-chi-bot
br: add additional volumes to backup/restore/warmup pods (pingcap#5414)
c951d65
csuzhangxc pushed a commit that referenced this pull request Nov 30, 2023
@ti-chi-bot @WangLe1321
br: add additional volumes to backup/restore/warmup pods (#5414) (#5432)
3226f55 
Co-authored-by: WangLe1321 <wangle1321@163.com>
wxiaomou
wxiaomou reviewed Nov 30, 2023
View reviewed changes
pkg/backup/backup/backup_manager.go
backup.Spec.FederalVolumeBackupPhase == v1alpha1.FederalVolumeBackupInitialize {
bm.setBackupPodResourceRequirementsEmpty(&job.Spec.Template)
// for volume backup initializing job, set deadline of the job in case it blocks GC and pd schedule indefinitely
job.Spec.ActiveDeadlineSeconds = pointer.Int64(int64(volumeBackupInitJobMaxActiveSeconds))
Copy link
Contributor

Choose a reason for hiding this comment

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

@WangLe1321 this gave me compile error pkg/backup/backup/backup_manager.go:864:44: undefined: pointer.Int64 should we use pointer.Int64Ptr instead?

@BornChanger BornChanger mentioned this pull request Jan 16, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wxiaomou wxiaomou wxiaomou left review comments

@BornChanger BornChanger BornChanger approved these changes

@lichunzhu lichunzhu Awaiting requested review from lichunzhu

@shonge shonge Awaiting requested review from shonge

Assignees
No one assigned
Labels
approved area/ebs-br size/XXL
Projects
None yet
Milestone
No milestone
7 participants
@WangLe1321 @codecov-commenter @nkg- @csuzhangxc @ti-chi-bot @wxiaomou @BornChanger