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

local-up-cluster kubelet option opening readonly port #96903

Merged
merged 1 commit into from
Dec 9, 2020
Merged

local-up-cluster kubelet option opening readonly port #96903

merged 1 commit into from
Dec 9, 2020

Conversation

JornShen
Copy link
Member

@JornShen JornShen commented Nov 27, 2020 •
edited
Loading

What type of PR is this?
/kind bug

What this PR does / why we need it:

After MR(#96095) merged, I run local-up-cluster.sh, find that kubelet wont open readonly port which is different from the before.

λ /opt/workspace/myspace/src/k8s.io/kubernetes/ master* netstat -anp | grep kubelet
tcp        0      0 127.0.0.1:10248         0.0.0.0:*               LISTEN      524188/kubelet      
tcp        0      0 127.0.0.1:37449         0.0.0.0:*               LISTEN      524188/kubelet      
tcp        0      0 127.0.0.1:10250         0.0.0.0:*               LISTEN      524188/kubelet      
tcp6       0      0 ::1:36670               ::1:6443                ESTABLISHED 524188/kubelet      
tcp6       0      0 ::1:36754               ::1:6443                ESTABLISHED 524188/kubelet

openning it is convenient for us to debug something.

The problem is that MR(#96095) migrate the kubelet the starting parameter to config file.
In the code, kubeletConfig will reload config from file.

kubernetes/cmd/kubelet/app/server.go

Lines 195 to 199 in 5ed4b76

if configFile := kubeletFlags.KubeletConfigFile; len(configFile) > 0 {
kubeletConfig, err = loadConfigFile(configFile)
if err != nil {
klog.Fatal(err)
}

And it will cover the default config setting before in

kubernetes/cmd/kubelet/app/options/options.go

Line 267 in 5ed4b76

kc.ReadOnlyPort = ports.KubeletReadOnlyPort

Does this PR introduce a user-facing change?:

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/bug Categorizes issue or PR as related to a bug. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Nov 27, 2020
@JornShen
Copy link
Member Author

/cc @masap @dims

@k8s-ci-robot
Copy link
Contributor

@JornShen: GitHub didn't allow me to request PR reviews from the following users: masap.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @masap @dims

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 requested a review from dims November 27, 2020 07:38
@JornShen
Copy link
Member Author

/sig testing
/test pull-kubernetes-e2e-kind
/test pull-kubernetes-e2e-gce-ubuntu-containerd

@k8s-ci-robot k8s-ci-robot added sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Nov 27, 2020
@dims
Copy link
Member

dims commented Nov 27, 2020

/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 27, 2020
@aojea
Copy link
Member

aojea commented Nov 30, 2020

/assign @spiffxp @BenTheElder
Maybe this affect some jobs and is needed for 1.20 🤔

@spiffxp
Copy link
Member

spiffxp commented Nov 30, 2020

/triage accepted
/milestone v1.20
/hold
@kubernetes/release-team I think this should land for v1.20 vs. v1.20.1, what say you? Remove /hold if you agree, remove milestone if you disagree

  • pro: fixes a regression introduced in hack/local-up-cluster.sh used by some for local dev, does not impact binary artifacts
  • con: landing will change the SHA if we're at the point where we're tracking continuous passes on a SHA

The other alternative is reverting the PR which introduced the regression in the first place, which has been in place for 26 days: #96095

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. triage/accepted Indicates an issue or PR is ready to be actively worked on. labels Nov 30, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.20 milestone Nov 30, 2020
@k8s-ci-robot k8s-ci-robot removed the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Nov 30, 2020
@jeremyrickard
Copy link
Contributor

/hold cancel

Thanks for the fix @JornShen
Thanks for the triage and comments @spiffxp.

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 30, 2020
@liggitt
Copy link
Member

liggitt commented Nov 30, 2020

/hold

it is intentional that running from a config file does not serve on the insecure port by default... I expected the local-up-cluster kubelet to pick up those improved defaults when switching to run from a config file

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 1, 2020
@JornShen JornShen changed the title local-up-cluster kubelet resume opening readonly port local-up-cluster kubelet option opening readonly port Dec 1, 2020
@JornShen JornShen force-pushed the local_up_resume_readonly_port branch from cbe2156 to fedbbb9 Compare December 1, 2020 03:39
@JornShen
Copy link
Member Author

JornShen commented Dec 1, 2020

/recc @dims @spiffxp @liggitt @jeremyrickard

aojea
aojea reviewed Dec 1, 2020
View reviewed changes
hack/local-up-cluster.sh Outdated Show resolved Hide resolved
@saschagrunert
Copy link
Member

Hey @JornShen, we're about to cut v1.20.0-rc.0 which also creates the release-1.20 branch. From that point on we have to cherry-pick all changes which should be included in v1.20.0.

@JornShen
Copy link
Member Author

JornShen commented Dec 1, 2020

Hey @JornShen, we're about to cut v1.20.0-rc.0 which also creates the release-1.20 branch. From that point on we have to cherry-pick all changes which should be included in v1.20.0.

@saschagrunert yeah, still in reviewing to this place. Maybe not catching up with that time. ==

@JornShen JornShen force-pushed the local_up_resume_readonly_port branch from fedbbb9 to da80876 Compare December 2, 2020 01:52
@JornShen
Copy link
Member Author

JornShen commented Dec 2, 2020

/test pull-kubernetes-e2e-gce-ubuntu-containerd

@liggitt
Copy link
Member

liggitt commented Dec 2, 2020

/hold cancel
/lgtm
/approve
/milestone v1.21

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 2, 2020
@k8s-ci-robot k8s-ci-robot modified the milestones: v1.20, v1.21 Dec 2, 2020
@liggitt
Copy link
Member

liggitt commented Dec 2, 2020

/priority backlog

@k8s-ci-robot k8s-ci-robot added priority/backlog Higher priority than priority/awaiting-more-evidence. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Dec 2, 2020
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 2, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, JornShen, liggitt

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

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

2 similar comments
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@JornShen
Copy link
Member Author

JornShen commented Dec 3, 2020

/test pull-kubernetes-e2e-gce-ubuntu-containerd
flaking: #97002

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

4 similar comments
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@k8s-ci-robot k8s-ci-robot merged commit 018942a into kubernetes:master Dec 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aojea aojea aojea left review comments

@liggitt liggitt liggitt left review comments

@SataQiu SataQiu Awaiting requested review from SataQiu

@vishh vishh Awaiting requested review from vishh

@dims dims Awaiting requested review from dims

Assignees

@dims dims

@spiffxp spiffxp

@BenTheElder BenTheElder

@liggitt liggitt

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/backlog Higher priority than priority/awaiting-more-evidence. release-note-none Denotes a PR that doesn't merit a release note. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Milestone
v1.21
Development

Successfully merging this pull request may close these issues.
10 participants
@JornShen @k8s-ci-robot @dims @aojea @spiffxp @jeremyrickard @liggitt @BenTheElder @saschagrunert @fejta-bot