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

[#545][FOLLOWUP]feat(operator): support specifying custom tolerations #638

Closed
wants to merge 3 commits into from

Conversation

crain-cn
Copy link
Contributor

What changes were proposed in this pull request?

Support coordinator/shuffler server's tolerations to the fields of RSS spec

Why are the changes needed?

  1. Production environment deployment needs to use.
  2. In order to change parameters flexibly.

Does this PR introduce any user-facing change?

For RSS cluster admin, they can set custom tolerations for shuffle servers and coordinators.

How was this patch tested?

Manually verified.
image

@zuston
Copy link
Member

zuston commented Feb 21, 2023

PTAL @wangao1236 @advancedxy

@kaijchen
Copy link
Contributor

Thanks @crain-cn for the work. Please take a look at the test failures in CI.

@crain-cn
Copy link
Contributor Author

Thanks @crain-cn for the work. Please take a look at the test failures in CI.

Thanks @kaijchen

Copy link
Contributor

@advancedxy advancedxy left a comment

Choose a reason for hiding this comment

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

Cloud you add some UTs for this new field?

@advancedxy advancedxy changed the title feat(operator): support specifying custom tolerations [#545][FOLLOWUP]feat(operator): support specifying custom tolerations Feb 21, 2023
@codecov-commenter
Copy link

codecov-commenter commented Feb 21, 2023

Codecov Report

Merging #638 (3ee4035) into master (c45c187) will decrease coverage by 0.22%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master     #638      +/-   ##
============================================
- Coverage     60.89%   60.67%   -0.22%     
- Complexity     1798     1799       +1     
============================================
  Files           214      216       +2     
  Lines         12381    12468      +87     
  Branches       1042     1052      +10     
============================================
+ Hits           7539     7565      +26     
- Misses         4438     4496      +58     
- Partials        404      407       +3     
Impacted Files Coverage Δ
...tor/pkg/controller/sync/coordinator/coordinator.go 97.89% <100.00%> (+0.01%) ⬆️
...pkg/controller/sync/shuffleserver/shuffleserver.go 86.06% <100.00%> (+0.28%) ⬆️
...a/org/apache/uniffle/server/RegisterHeartBeat.java 43.85% <0.00%> (-43.86%) ⬇️
.../java/org/apache/uniffle/server/ShuffleServer.java 62.06% <0.00%> (-2.32%) ⬇️
...he/uniffle/server/buffer/ShuffleBufferManager.java 82.74% <0.00%> (-0.36%) ⬇️
...n/java/org/apache/uniffle/common/ServerStatus.java 0.00% <0.00%> (ø)
...ffle/common/exception/InvalidRequestException.java 0.00% <0.00%> (ø)
...a/org/apache/uniffle/server/ShuffleServerConf.java 99.33% <0.00%> (+0.02%) ⬆️
...he/uniffle/server/storage/LocalStorageManager.java 86.44% <0.00%> (+0.56%) ⬆️
... and 1 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@crain-cn crain-cn requested review from advancedxy and removed request for wangao1236 and advancedxy February 21, 2023 14:04
@crain-cn crain-cn force-pushed the featrue/Support-tolerations branch from c221a99 to db53af8 Compare February 21, 2023 15:50
@crain-cn crain-cn force-pushed the featrue/Support-tolerations branch from db53af8 to 39e6157 Compare February 21, 2023 15:53
@jerqi
Copy link
Contributor

jerqi commented Feb 22, 2023

@crain-cn Could you add some uts for this pr?

NodeSelector: rss.Spec.Coordinator.NodeSelector,
Tolerations: rss.Spec.Coordinator.Tolerations,
Volumes: rss.Spec.Coordinator.Volumes,
NodeSelector: rss.Spec.Coordinator.NodeSelector,
}
configurationVolume := corev1.Volume{
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
configurationVolume := corev1.Volume{
if len(podSpec.Tolerations) == 0 {
podSpec.Tolerations = []corev1.Toleration{...} # the default master rule toleration
}
configurationVolume := corev1.Volume{

Copy link
Contributor Author

@crain-cn crain-cn Feb 22, 2023

Choose a reason for hiding this comment

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

Thanks 😄 @advancedxy

@crain-cn
Copy link
Contributor Author

@crain-cn Could you add some uts for this pr?
Yeah.

@crain-cn
Copy link
Contributor Author

@advancedxy Affinity fields , can I submit to the next pr?

@crain-cn crain-cn requested a review from advancedxy February 22, 2023 03:52
@advancedxy
Copy link
Contributor

@advancedxy Affinity fields , can I submit to the next pr?

of course.

@crain-cn
Copy link
Contributor Author

@advancedxy Affinity fields , can I submit to the next pr?

of course.

Thanks ! 😄

@crain-cn
Copy link
Contributor Author

@crain-cn Could you add some uts for this pr?

Can I make it up later?

@crain-cn
Copy link
Contributor Author

@crain-cn Could you add some uts for this pr?
Already written.
#641 0cfc745

@crain-cn
Copy link
Contributor Author

Cloud you add some UTs for this new field?

Already written.
#641 0cfc745

@jerqi
Copy link
Contributor

jerqi commented Feb 22, 2023

@crain-cn Could you add some uts for this pr?
Already written.
#641 0cfc745

We would better add this pr's ut to this pr. If we need integration test, we could add it in another single pr.

@crain-cn
Copy link
Contributor Author

@crain-cn Could you add some uts for this pr?
Already written.
#641 0cfc745

We would better add this pr's ut to this pr. If we need integration, we could add it in another single pr.

PRO asked me to add an affinity field, so I added another pr. #641

@advancedxy
Copy link
Contributor

PRO asked me to add an affinity field, so I added another pr. #641

I just took a look at #641, seems that it cover toleration and affinity both and includes some UT. Do you prefer to close this in favor of #641? Actually you could just add more commits and finishes this PR?

Anyway is fine to me.

@crain-cn
Copy link
Contributor Author

crain-cn commented Feb 22, 2023

Then I close #638

@crain-cn crain-cn closed this Feb 22, 2023
@crain-cn crain-cn deleted the featrue/Support-tolerations branch February 22, 2023 11:26
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