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

[api] support custom node affinity per-isogroup #106

Merged
merged 3 commits into from
Mar 22, 2019

Conversation

schallert
Copy link
Collaborator

@schallert schallert commented Mar 20, 2019

Previously we had been using the name of an IsolationGroup as its
the value selector in a StatefulSet's NodeAffinity, with the key
hardcoded to failure-domain.beta.kubernetes.io/zone. This meant that
users had to have clusters across 3 unique zones.

This relaxes those constraints and allows users to specify the
NodeAffinity selectors they wish to use, meaning they can have clusters
within a single zone or other failure domain.

Fixes #68

Previously we had been using the name of an IsolationGroup as its
the value selector in a StatefulSet's NodeAffinity, with the key
hardcoded to `failure-domain.beta.kubernetes.io/zone`. This meant that
users had to have clusters across 3 unique zones.

This relaxes those constraints and allows users to specify the
NodeAffinity selectors they wish to use, meaning they can have clusters
within a single zone or other failure domain.
@schallert schallert requested review from nerd0 and CK-Ward March 20, 2019 18:27
@schallert schallert marked this pull request as ready for review March 20, 2019 18:50
@schallert schallert force-pushed the schallert/sts_isogroup_new branch 4 times, most recently from 236ac4e to 2f6c77d Compare March 22, 2019 02:36
@codecov
Copy link

codecov bot commented Mar 22, 2019

Codecov Report

Merging #106 into master will increase coverage by 0.53%.
The diff coverage is 88.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #106      +/-   ##
==========================================
+ Coverage   67.63%   68.16%   +0.53%     
==========================================
  Files          26       26              
  Lines        1894     1888       -6     
==========================================
+ Hits         1281     1287       +6     
+ Misses        510      501       -9     
+ Partials      103      100       -3

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 388a045...9fff7c7. Read the comment docs.

Copy link
Contributor

@nerd0 nerd0 left a comment

Choose a reason for hiding this comment

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

Looks great overall, a couple of questions.

@@ -98,10 +97,6 @@ func main() {

defer logger.Sync()

logger.Info("Go", zap.Any("VERSION", runtime.Version()))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why get ride of this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since I added the m3x build reporter we get all of this info for free in a startup log from there

Copy link
Contributor

Choose a reason for hiding this comment

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

nice.

)

var (
errEmptyConfigMap = errors.New("ConfigMapName cannot be empty if non-nil")
)

// EnsurePlacement ensures that a placement exists otherwise create one
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume this code was replaced beforehand, didn't see it replaced within this PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah turns out I couldn't find it used anywhere

@@ -46,7 +46,7 @@ function main() {

trap cleanup EXIT

kubectl proxy &
kubectl proxy &>/dev/null &
Copy link
Contributor

Choose a reason for hiding this comment

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

Why silence the output, couldn't it help if the test borks?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So the annoying this was this would interleave the proxy output with the test and it was confusing what was coming from where, but I guess that's better than nothing so will re-add

Copy link

Choose a reason for hiding this comment

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

You could consider something like:
kubectl proxy |sed -e 's/^/kube proxy: /'&

Copy link
Contributor

@nerd0 nerd0 left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -98,10 +97,6 @@ func main() {

defer logger.Sync()

logger.Info("Go", zap.Any("VERSION", runtime.Version()))
Copy link
Contributor

Choose a reason for hiding this comment

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

nice.

@schallert schallert merged commit 5b9a494 into master Mar 22, 2019
@schallert schallert deleted the schallert/sts_isogroup_new branch March 22, 2019 20:29
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.

3 participants