-
Notifications
You must be signed in to change notification settings - Fork 494
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
Validate resources.request.storage
for each item in spec.tiflash.storageClaims
#4635
base: master
Are you sure you want to change the base?
Conversation
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
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
/merge |
This pull request has been accepted and is ready to merge. Commit hash: 6fc2786
|
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #4635 +/- ##
==========================================
- Coverage 59.01% 59.01% -0.01%
==========================================
Files 227 227
Lines 26357 26357
==========================================
- Hits 15555 15554 -1
- Misses 9298 9299 +1
Partials 1504 1504
|
/run-all-tests |
/test pull-e2e-kind-across-kubernetes |
/run-all-tests |
1 similar comment
/run-all-tests |
/test pull-e2e-kind pull-e2e-kind-serial |
bdb8aad
to
ebdd0e2
Compare
Merge canceled because a new commit is pushed. |
Hi @KanShiori I just rebased it, is it possible for you to take a look and merge this fix? |
What problem does this PR solve?
This PR solves the problem that assigning empty objects under
storageClaims
of tiflash can bypass the validation of the operator.We found that when we specified tiflash spec with some empty objects under
storageClaims
, we got rejection from the events saying thatstorage
is a required field. However, we did not get error messages or alerts from the operator indicating thatstorage
is required.Additionally, from the source code of TiDB operator, we discovered that assigning empty objects under
storageClaims
can bypass the operator's validation code. For details, please refer to the issue we submitted: #4613What is changed and how does it work?
We believed it is necessary to check whether
storage
is set when specifyingstorageClaims
for tiflash. We added a sanity check in the functionvalidateTiFlashSpec
to see whetherstorage
exists in the resource requests.Code changes
Tests
Side effects
Related changes
Release Notes
Please refer to Release Notes Language Style Guide before writing the release note.