Updates to always pass spec.nodeName as --hostname-override #71283
Conversation
k8s-ci-robot
added
release-note-none
|
Hi @Klaven. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
k8s-ci-robot
added
needs-ok-to-test
|
@detiber @neolit123 @anguslees I have made the above PR per the comments in the kubernetes/kubeadm#857 ticket. If I have understood everything correctly this was all that was needed done. pleast let me know your thoughts. |
|
/priority critical-urgent |
k8s-ci-robot
added
priority/critical-urgent
|
I kind of wanted to write some type of test for this... but was not quite sure what would be best/useful. if you all have any suggestions I would love to hear it! |
|
/milestone v1.13 |
|
/retest pull-kubernetes-integration |
| @@ -80,6 +80,7 @@ spec: | |||
| command: | |||
| - /usr/local/bin/kube-proxy | |||
| - --config=/var/lib/kube-proxy/{{ .ProxyConfigMapKey }} | |||
| - --hostname-override=$NODE_NAME | |||
There was a problem hiding this comment.
I think the syntax needs to be $(NODE_NAME) (with the parens) to be substituted by kubelet. Otherwise it's asking a shell somewhere down the line to do the substitution - and I don't think there's a shell involved in this specific example. Indeed, if it does also work as you've written, then I'm kind of alarmed, since I think that means the image entrypoint(?) is re-evaluating its arguments and hence introducing a bunch of shell escaping / word-splitting bugs.
My test, just to confirm I wasn't crazy (on my local k8s 1.9 cluster):
apiVersion: v1
kind: Pod
metadata:
name: test-pod
spec:
containers:
- name: test
image: busybox
command:
- echo
- A=$FOO
- B=$(FOO)
- C=${FOO}
env:
- name: FOO
value: foo=>
A=$FOO B=foo C=${FOO}
There was a problem hiding this comment.
mine was substituted. aka A == foo . @neolit123 and I did talk about what the proper syntax was prior to this. But I can update. running a build and will build a cluster with the (...)
There was a problem hiding this comment.
thanks @anguslees
i suggested the variable without ( ), but wasn't 100% sure about it.
@Klaven mentioned that this works in his tests, but we should probably use ( ... ).
Indeed, if it does also work as you've written, then I'm kind of alarmed, since I think that means the image entrypoint(?) is re-evaluating its arguments and hence introducing a bunch of shell escaping / word-splitting bugs.
yes this is puzzling. i'd suggest what we just get it right under command, as it's unlikely we will have time to investigate further. too many items to look for 1.13.
There was a problem hiding this comment.
ok, @anguslees before I destroyed the old cluster I checked again. --hostname-override != $NODE_NAME as this would imply. building new cluster now. I'm good with putting the (...) in if it works the same. will update you when i'm finished testing.
There was a problem hiding this comment.
by the way, do you have a good way of automated testing of this bug? I wanted too.... but ran out of time.
There was a problem hiding this comment.
IMO, integration or e2e for this are out of scope for this PR.
There was a problem hiding this comment.
yep, e2e is out of scope imo.
It's clear that there is nowhere (or not much) in the current e2e tests that exercises hostname != node name, since there are frequent bugs around this. It would be great to change the hostnames/node-names for all the e2e tests, but that's quite separate to this PR.
|
/test pull-kubernetes-integration |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Klaven, timothysc The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
updated manifest file to always pass spec.nodeName as the --hostname-override.
There was a problem hiding this comment.
/lgtm
thanks to @Klaven and @anguslees for hinting surrounding issues for this.
k8s-ci-robot
added
the
lgtm
| @@ -80,6 +80,7 @@ spec: | |||
| command: | |||
| - /usr/local/bin/kube-proxy | |||
| - --config=/var/lib/kube-proxy/{{ .ProxyConfigMapKey }} | |||
| - --hostname-override=$NODE_NAME | |||
There was a problem hiding this comment.
yep, e2e is out of scope imo.
It's clear that there is nowhere (or not much) in the current e2e tests that exercises hostname != node name, since there are frequent bugs around this. It would be great to change the hostnames/node-names for all the e2e tests, but that's quite separate to this PR.
k8s-ci-robot
added
release-note
per kubernetes/kubeadm#857 updated manifest file to always pass spec.nodeName as the --hostname-override.
What type of PR is this?
/kind bug
What this PR does / why we need it:
Fixes kubernetes/kubeadm#857
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #kubernetes/kubeadm#857
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
-->