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

On GCI, cleanup kubelet startup #35319

Merged
merged 1 commit into from
Oct 29, 2016
Merged

On GCI, cleanup kubelet startup #35319

merged 1 commit into from
Oct 29, 2016

Conversation

vishh
Copy link
Contributor

@vishh vishh commented Oct 21, 2016 •
edited by k8s-oncall

-->

* Avoid overriding system and kubelet cgroups on GCI
* Make the kubectl from k8s release the default on GCI

cc @kubernetes/sig-node @mtaufen

This change is Reviewable

@vishh vishh added priority/backlog Higher priority than priority/awaiting-more-evidence. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Oct 21, 2016
@vishh vishh added this to the v1.4 milestone Oct 21, 2016
@k8s-github-robot k8s-github-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Oct 21, 2016
@k8s-ci-robot
Copy link
Contributor

Jenkins verification failed for commit 184e355. Full PR test history.

The magic incantation to run this job again is @k8s-bot verify test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@vishh
Copy link
Contributor Author

vishh commented Oct 21, 2016

@k8s-bot test this

euank
euank reviewed Oct 21, 2016
View reviewed changes
cluster/gce/gci/configure-helper.sh
@@ -1167,6 +1165,9 @@ For Kubernetes copyright and licensing information, see:
EOF
}

function override-kubectl {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer if we could edit the user's path rather than use a bindmount.

This bindmount will not survive a reboot, and afaik this isn't re-run on each boot, so this won't persist.

Editing the PATH (preferably with something like Gentoo's /etc/env.d if GCI has such an option) seems saner to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@adityakali Any opinions?

Copy link
Contributor

Choose a reason for hiding this comment

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

You can add it under /etc/profile.d/. For ex:

cat /etc/profile.d/kubelet_env.sh 
export PATH="/home/kubernetes/bin:$PATH"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sweet. Updated. PTAL

@k8s-ci-robot
Copy link
Contributor

Jenkins Kubemark GCE e2e failed for commit 184e355. Full PR test history.

The magic incantation to run this job again is @k8s-bot kubemark e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCI GCE e2e failed for commit 184e355. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCE etcd3 e2e failed for commit 184e355. Full PR test history.

The magic incantation to run this job again is @k8s-bot gce etcd3 e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCE e2e failed for commit 184e355. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GKE smoke e2e failed for commit 184e355. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCI GKE smoke e2e failed for commit 184e355. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@vishh
Copy link
Contributor Author

vishh commented Oct 27, 2016

@euank @adityakali @mtaufen PTAL

@euank
Copy link
Contributor

euank commented Oct 27, 2016

LGTM

@vishh vishh mentioned this pull request Oct 27, 2016
Closed
adityakali
adityakali reviewed Oct 27, 2016
View reviewed changes
cluster/gce/gci/configure-helper.sh
@@ -1167,6 +1165,9 @@ For Kubernetes copyright and licensing information, see:
EOF
}

function override-kubectl {
echo "export PATH={KUBE_HOME}/bin:$PATH" > /etc/profile.d/kube_env.sh
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't $PATH end up getting resolved in echo command itself? You may want to escape the $. Try it out.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, and, it looks like KUBE_HOME is missing the $ as well (we would want it to be resolved here)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Err. Sorry for being sloppy. Fixed it.

@vishh
On GCI, remove kubelet cgroup overrides and override host kubectl wit…
f6f4f91 
…h that of the release

Signed-off-by: Vishnu kannan <vishnuk@google.com>
@k8s-github-robot k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 28, 2016
@jessfraz jessfraz mentioned this pull request Oct 28, 2016
Closed
@jessfraz jessfraz added cherrypick-candidate cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. labels Oct 28, 2016
@vishh vishh added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 28, 2016
@jessfraz jessfraz mentioned this pull request Oct 28, 2016
Merged
jessfraz added a commit that referenced this pull request Oct 28, 2016
@jessfraz
Merge pull request #35820 from jessfraz/automated-cherry-pick-of-#353…
85fde21 
…19-origin-release-1.4

Automated cherry pick of #35319
@jessfraz jessfraz self-assigned this Oct 29, 2016
@k8s-github-robot
Copy link

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

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 9b021a9 into kubernetes:master Oct 29, 2016
@jessfraz
Copy link
Contributor

this was cherrypicked into 1.4.5

shyamjvs pushed a commit to shyamjvs/kubernetes that referenced this pull request Dec 1, 2016
@jessfraz
Merge pull request kubernetes#35820 from jessfraz/automated-cherry-pi…
ea7ac01 
…ck-of-#35319-origin-release-1.4

Automated cherry pick of kubernetes#35319
@chentao1596 chentao1596 mentioned this pull request Dec 5, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@euank euank euank left review comments

@adityakali adityakali adityakali left review comments

Assignees

@mtaufen mtaufen

@jessfraz jessfraz

@euank euank

Labels
cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. 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 Denotes a PR that will be considered when it comes time to generate release notes. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
v1.4
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@vishh @k8s-ci-robot @euank @k8s-github-robot @jessfraz @adityakali @mtaufen @googlebot