lowercase node name in generated static pod name #59849
Conversation
k8s-ci-robot
added
release-note
k8s-ci-robot
added
the
needs-ok-to-test
|
/assign @dchen1107 |
yue9944882
changed the title
|
/ok-to-test |
k8s-ci-robot
removed
the
needs-ok-to-test
k8s-ci-robot
added
the
lgtm
|
/retest |
yue9944882
force-pushed
the
forcibly-lower-staticpod-name
branch
from
b417d3e to
a331f81
Compare
k8s-github-robot
removed
the
lgtm
|
@errordeveloper I just made some stupid mistake in my previous submit. Plz take another look thx. |
|
@gyliu513 WDYM？A unittest proving the node name is successfully lowercased？ |
|
@yue9944882 yes, add a new unit test case for the function that you updated |
k8s-ci-robot
added
size/M
|
@gyliu513 PTAL thx😉 |
yue9944882
force-pushed
the
forcibly-lower-staticpod-name
branch
2 times, most recently
from
4bec3fe to
d96b2e8
Compare
k8s-ci-robot
added
the
lgtm
|
@yujuhong @tallclair @Random-Liu Do u think about approving this plz? thx |
|
This seems like it might be a deeper issue. If uppercase node names are allowed, what happens when you have 2 nodes with the same name but different capitalization? |
@tallclair I think either one of static pods will fail to be created due to pod name conflict. How do you think about adding random suffix to the static pod name? kubernetes/pkg/kubelet/kubelet.go Line 369 in 71c374d Actually uppercase node name is allowed currently. Solving this issue might be simple as "allow" or "not allow". For former case, we should provide full support for uppercase node name, or rather, making sure every static pod created successfully. OTOH, we should make kubelet with uppercase node name fail-hard before it joining the cluster. WYDT?
|
|
I prefer that we keep current logic of lower case the host name directly as it is not good to have two same host name in one cluster but different capitalization, please take a look at calico, it is already coverting all host name to lowercase automatically projectcalico/calico#1441 (comment) , comments @yue9944882 @tallclair |
|
I am ok with current logic, but want to make sure the cause described at #59849 (comment) has a explicit error for one of failing static pod. @yue9944882 can you add another tests with two nodes with same name but different capitalization? /approve |
k8s-ci-robot
added
the
approved
dchen1107
removed
the
lgtm
|
I approved this pr, but cancelled lgtm. Once you addressed my comment, please feel free to re-apply lgtm. Thanks! |
yue9944882
force-pushed
the
forcibly-lower-staticpod-name
branch
from
d96b2e8 to
fdd3601
Compare
|
@dchen1107 ACK, test added. |
|
/test pull-kubernetes-unit |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dchen1107, errordeveloper, gyliu513, yue9944882 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 |
|
/test all [submit-queue is verifying that this PR is safe to merge] |
|
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here. |
What this PR does / why we need it:
Cast appended node name to lowercase when generating static pod name on kubelet starting.
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 #59801
Special notes for your reviewer:
Not sure about how to deal with other illegal node names e.g. containing invalid no-alphabetic characters. Maybe just let it fail-hard is not a bad idea.
But considering that containing uppercase letter in the hostname is somehow a usual case even in the production environment of some companies, tolerating uppercase and cast it implicitly should be good.
Release note: