Skip to content

e2e: fix: get kubelet port from node status instead of hard coding #86071

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
Closed
wants to merge 1 commit into from
Closed

e2e: fix: get kubelet port from node status instead of hard coding #86071

wants to merge 1 commit into from

Conversation

@zhuangqh
Copy link
Contributor

@zhuangqh zhuangqh commented Dec 9, 2019

Signed-off-by: zhuangqh zhuangqhc@gmail.com

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespace from that line:

/kind api-change

/kind bug

/kind cleanup
/kind deprecation
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:

get kubelet port from node status instead of hard coding. Not all the kubelet port is 10250.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-none Denotes a PR that doesn't merit a release note. kind/bug Categorizes issue or PR as related to a bug. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Dec 9, 2019
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Dec 9, 2019

Hi @zhuangqh. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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 k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. area/e2e-test-framework Issues or PRs related to refactoring the kubernetes e2e test framework area/test sig/node Categorizes an issue or PR as relevant to SIG Node. sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Dec 9, 2019
@zhuangqh zhuangqh mentioned this pull request Dec 9, 2019
Closed
35 tasks
@zhuangqh
Copy link
Contributor Author

zhuangqh commented Dec 10, 2019

/assign @timothysc

alejandrox1
alejandrox1 reviewed Dec 16, 2019
View reviewed changes
Copy link
Contributor

@alejandrox1 alejandrox1 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thank you for catching this @zhuangqh !
/lgtm
/ok-to-test
/priority backlog
/kind cleanup

/cc @kubernetes/sig-node-pr-reviews
ptal

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. priority/backlog Higher priority than priority/awaiting-more-evidence. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Dec 16, 2019
@liggitt
Copy link
Member

liggitt commented Dec 16, 2019

Anywhere you are intending to proxy to the kubelet and are currently hard-coding the kubelet port, I would suggest dropping the port altogether. The default when proxying to a node is to proxy to the kubelet port, which the kube-apiserver will look up from the specific kubelet's reported status.

See https://github.com/kubernetes/kubernetes/blob/master/pkg/registry/core/node/strategy.go#L240-L255 and https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/client/kubelet_client.go#L209-L226

timothysc
timothysc approved these changes Jan 7, 2020
View reviewed changes
Copy link
Contributor

@timothysc timothysc left a comment

Choose a reason for hiding this comment

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

I don't mind the change, but is are you changing the ports in your environment?

/approve
/hold

test/e2e/framework/kubelet/kubelet_pods.go Outdated
Copy link
Contributor

@timothysc timothysc Jan 7, 2020

Choose a reason for hiding this comment

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

Why is the casting here necessary, what is the returned type?

Copy link
Contributor Author

@zhuangqh zhuangqh Jan 8, 2020

Choose a reason for hiding this comment

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

@timothysc original type is int32

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 7, 2020
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Jan 7, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: timothysc, zhuangqh

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 7, 2020
@zhuangqh
fix: get kubelet port from node status instead of hard coding
a65325b 
Signed-off-by: zhuangqh <zhuangqhc@gmail.com>
@zhuangqh
Copy link
Contributor Author

zhuangqh commented Jan 8, 2020

I don't mind the change, but is are you changing the ports in your environment?

@timothysc yep. I am working on a multi-tenancy project. There is a fake agent instead of kubelet. The agent's port is different from the real kubelet

@zhuangqh zhuangqh force-pushed the fix-master-kubelet-port branch from 8261190 to a65325b Compare January 8, 2020 02:44
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 8, 2020
@liggitt
Copy link
Member

liggitt commented Jan 8, 2020

#86071 (comment) is still relevant... letting the apiserver do the node port lookup would be more reliable and simpler

@zhuangqh
Copy link
Contributor Author

zhuangqh commented Jan 8, 2020 •
edited
Loading

#86071 (comment) is still relevant... letting the apiserver do the node port lookup would be more reliable and simpler

I totally agree with you. But I still not figure out how to do the node proxy as you said😂
could you give me more input. I am not familiar with k8s code😅

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Jan 8, 2020 •
edited
Loading

@zhuangqh: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-kubernetes-conformance-kind-ipv6 8261190a7b81f41eb5ad8c097ae9e846aa30dcff link /test pull-kubernetes-conformance-kind-ipv6
pull-kubernetes-verify a65325b link /test pull-kubernetes-verify

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@liggitt
Copy link
Member

liggitt commented Jan 8, 2020

But I still not figure out how to do the node proxy as you said😂
could you give me more input. I am not familiar with k8s code😅

the simplest approach I can think of is to accept a port of 0 as a special case, and in ProxyRequest, if the port is zero, construct the proxy request to Name(node)

@liggitt
Copy link
Member

liggitt commented Jan 8, 2020

then you no longer need GetKubeletPort

@zhuangqh
Copy link
Contributor Author

zhuangqh commented Jan 9, 2020

But I still not figure out how to do the node proxy as you said😂
could you give me more input. I am not familiar with k8s code😅

the simplest approach I can think of is to accept a port of 0 as a special case, and in ProxyRequest, if the port is zero, construct the proxy request to Name(node)

good idea. I am trying

@fejta-bot
Copy link

fejta-bot commented Apr 8, 2020

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 8, 2020
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Apr 8, 2020

@zhuangqh: PR needs rebase.

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.

@christopherhein
Copy link
Member

christopherhein commented Apr 22, 2020

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 22, 2020
@fejta-bot
Copy link

fejta-bot commented Jul 21, 2020

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 21, 2020
@fejta-bot
Copy link

fejta-bot commented Aug 20, 2020

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Aug 20, 2020
@fejta-bot
Copy link

fejta-bot commented Sep 19, 2020

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Sep 19, 2020

@fejta-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@derekwaynecarr derekwaynecarr Awaiting requested review from derekwaynecarr

2 more reviewers

@alejandrox1 alejandrox1 alejandrox1 left review comments

@timothysc timothysc timothysc approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@timothysc timothysc

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. area/e2e-test-framework Issues or PRs related to refactoring the kubernetes e2e test framework area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. kind/bug Categorizes issue or PR as related to a bug. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. priority/backlog Higher priority than priority/awaiting-more-evidence. release-note-none Denotes a PR that doesn't merit a release note. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/testing Categorizes an issue or PR as relevant to SIG Testing. 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.

7 participants

@zhuangqh @k8s-ci-robot @liggitt @fejta-bot @christopherhein @timothysc @alejandrox1