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 support for affinity and tolerations #361

Merged
merged 2 commits into from
Apr 5, 2019
Merged

Add support for affinity and tolerations #361

merged 2 commits into from
Apr 5, 2019

Conversation

objectiser
Copy link
Contributor

Combining values from the specific component and general details has been done on the following basis:

  • Affinity - there is no information about merging the information from two affinity definitions - so seems most appropriate approach is to just allow the more specific component's affinity definition to override any general one

  • Tolerations - an array - the Key field is not unique, so not possible to select on that basis (as with annotations) - so just aggregate all of the tolerations from the specific component and general definitions.

Resolves #292

Signed-off-by: Gary Brown [email protected]

@objectiser objectiser requested a review from jpkrohling April 4, 2019 15:25
@codecov
Copy link

codecov bot commented Apr 4, 2019

Codecov Report

Merging #361 into master will increase coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #361      +/-   ##
========================================
+ Coverage   89.94%    90%   +0.05%     
========================================
  Files          64     64              
  Lines        3054   3071      +17     
========================================
+ Hits         2747   2764      +17     
  Misses        207    207              
  Partials      100    100
Impacted Files Coverage Δ
pkg/apis/jaegertracing/v1/jaeger_types.go 100% <ø> (ø) ⬆️
pkg/deployment/query.go 100% <100%> (ø) ⬆️
pkg/deployment/all-in-one.go 100% <100%> (ø) ⬆️
pkg/deployment/collector.go 100% <100%> (ø) ⬆️
pkg/deployment/ingester.go 100% <100%> (ø) ⬆️
pkg/util/util.go 100% <100%> (ø) ⬆️
pkg/deployment/agent.go 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7f314b5...b781b39. Read the comment docs.

Copy link
Contributor

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

LGTM


// Keys are not unique, so should be aggregation of all tolerations
assert.Len(t, merged.Tolerations, 3)
assert.Equal(t, "toleration1", merged.Tolerations[0].Key)
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks wrong at a first glance, but reading the docs makes it clear that it's OK to have two entries with the same key/operator/value, as long as the effect is different. Perhaps add a link to the relevant docs, to avoid future confusion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@jpkrohling jpkrohling merged commit 390d5d4 into jaegertracing:master Apr 5, 2019
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.

2 participants