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

Use KubeletPort from API object, salt changes, take 2 #17760

Merged
merged 1 commit into from
Dec 9, 2015
Merged

Use KubeletPort from API object, salt changes, take 2 #17760

merged 1 commit into from
Dec 9, 2015

Conversation

gmarek
Copy link
Contributor

@gmarek gmarek commented Nov 25, 2015

This reverts commit a7425bf, reversing
changes made to 4a9b0fc.

@k8s-github-robot
Copy link

Labelling this PR as size/S

@k8s-github-robot k8s-github-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 25, 2015
@k8s-bot
Copy link

k8s-bot commented Nov 25, 2015

GCE e2e build/test failed for commit 1468861f2c9ad88da755c4c1ce1b62e0e228840f.

@gmarek
Copy link
Contributor Author

gmarek commented Nov 25, 2015

@k8s-bot test this

@k8s-bot
Copy link

k8s-bot commented Nov 25, 2015

GCE e2e test build/test passed for commit 1468861f2c9ad88da755c4c1ce1b62e0e228840f.

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 25, 2015
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 26, 2015
@k8s-bot
Copy link

k8s-bot commented Nov 26, 2015

GCE e2e test build/test passed for commit 7645384f5f4303869d4d1a70014ebbc7d589d1d2.

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 26, 2015
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 26, 2015
@k8s-bot
Copy link

k8s-bot commented Nov 26, 2015

GCE e2e test build/test passed for commit 4bd00a1b5f2a39f4876b04f38ee60b93902642ad.

@gmarek
Copy link
Contributor Author

gmarek commented Nov 26, 2015

@k8s-bot unit test this

roberthbailey
roberthbailey reviewed Nov 30, 2015
View reviewed changes
cluster/gce/configure-vm.sh
@@ -287,6 +287,7 @@ manifest_url: '$(echo "$MANIFEST_URL" | sed -e "s/'/''/g")'
manifest_url_header: '$(echo "$MANIFEST_URL_HEADER" | sed -e "s/'/''/g")'
num_nodes: $(echo "${NUM_NODES}")
e2e_storage_test_environment: '$(echo "$E2E_STORAGE_TEST_ENVIRONMENT" | sed -e "s/'/''/g")'
kubelet_port: '$(echo "$KUBELET_PORT")'
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be passed through sed like the other lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not all lines are passed though the sed, e.g. two lines above there's a NUM_NODES variable which is not. I can add sed to all lines that are missing it, if you think it's needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe @zmerlynn can weigh in.

My guess is that the lines not passed through sed were added recently and we just forgot to make sure they were properly sanitized. Doing a bit of digging in the git history, it looks like these were added in #16071 and #15049 in the last month.

@gmarek gmarek force-pushed the kubelet-port-salt branch from 4bd00a1 to 9e4636a Compare December 1, 2015 10:27
@k8s-bot
Copy link

k8s-bot commented Dec 1, 2015

GCE e2e build/test failed for commit 9e4636aa91bfb35e0e721a6964994759953c44e3.

@gmarek gmarek force-pushed the kubelet-port-salt branch from 9e4636a to 4cf2a11 Compare December 1, 2015 10:51
@k8s-bot
Copy link

k8s-bot commented Dec 1, 2015

GCE e2e test build/test passed for commit 4cf2a11e71d5ebaed58e2dc9e3713bced8c08f6f.

roberthbailey
roberthbailey reviewed Dec 1, 2015
View reviewed changes
cluster/saltbase/salt/kubelet/default
@@ -121,5 +121,10 @@
{% set network_plugin = "--network-plugin=opencontrail" %}
{% endif -%}

{% set kubelet_port = "10250" -%}
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 broken. If the pillar isn't defined, this won't pass --port=10250 but will just pass 10250 on the command line. It'd be better to make the default just be an empty string so that we pick up the default value compiled in (with a shorter command line) if a different port isn't explicitly set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. Done.

@gmarek gmarek force-pushed the kubelet-port-salt branch from 4cf2a11 to e0cbe80 Compare December 3, 2015 16:51
@k8s-bot
Copy link

k8s-bot commented Dec 3, 2015

GCE e2e build/test failed for commit e0cbe808eb796fe910edf14a3989bace6a2c1e1f.

@gmarek gmarek force-pushed the kubelet-port-salt branch from e0cbe80 to d99dac5 Compare December 4, 2015 12:58
@k8s-github-robot
Copy link

Labelling this PR as size/M

@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 4, 2015
@gmarek gmarek force-pushed the kubelet-port-salt branch from d99dac5 to 28da3f0 Compare December 4, 2015 13:13
@k8s-bot
Copy link

k8s-bot commented Dec 4, 2015

GCE e2e test build/test passed for commit d99dac56ccaea998acdb1ef7d39dec9127318f29.

@k8s-bot
Copy link

k8s-bot commented Dec 4, 2015

GCE e2e test build/test passed for commit 28da3f0746ad62a53cd4b4f259a09a2b462c8189.

roberthbailey
roberthbailey reviewed Dec 4, 2015
View reviewed changes
cluster/gce/util.sh
@@ -1303,6 +1303,11 @@ OPENCONTRAIL_KUBERNETES_TAG: $(yaml-quote ${OPENCONTRAIL_KUBERNETES_TAG:-})
OPENCONTRAIL_PUBLIC_SUBNET: $(yaml-quote ${OPENCONTRAIL_PUBLIC_SUBNET:-})
E2E_STORAGE_TEST_ENVIRONMENT: $(yaml-quote ${E2E_STORAGE_TEST_ENVIRONMENT:-})
EOF
if [ -n "${KUBELET_PORT:-}" ]; then
cat >>$file <<EOF
KUBELET_PORT: $(yaml-quote ${KUBELET_PORT:-})
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: you shouldn't need the :- on this line since right above you check that the var isn't empty.

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.

@roberthbailey
Copy link
Contributor

One minor issue; otherwise looks good.

Use KubeletPort from API object, salt changes, take 2
4060eba 
This reverts commit a7425bf, reversing
changes made to 4a9b0fc.
@gmarek gmarek force-pushed the kubelet-port-salt branch from 28da3f0 to 4060eba Compare December 7, 2015 09:31
@k8s-bot
Copy link

k8s-bot commented Dec 7, 2015

GCE e2e test build/test passed for commit 4060eba.

@roberthbailey roberthbailey added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 7, 2015
@roberthbailey
Copy link
Contributor

LGTM.

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Dec 7, 2015

GCE e2e test build/test passed for commit 4060eba.

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Dec 8, 2015

GCE e2e test build/test passed for commit 4060eba.

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Dec 9, 2015

GCE e2e test build/test passed for commit 4060eba.

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Dec 9, 2015

GCE e2e test build/test passed for commit 4060eba.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

k8s-github-robot pushed a commit that referenced this pull request Dec 9, 2015
Merge pull request #17760 from gmarek/kubelet-port-salt
d71e838 
Auto commit by PR queue bot
@k8s-github-robot k8s-github-robot merged commit d71e838 into kubernetes:master Dec 9, 2015
@gmarek gmarek mentioned this pull request Dec 10, 2015
Closed
@gmarek gmarek deleted the kubelet-port-salt branch March 17, 2016 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@roberthbailey roberthbailey

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@gmarek @k8s-github-robot @k8s-bot @roberthbailey @k8s-teamcity-mesosphere @googlebot