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

Kill docker daemon after configuring cbr0 #22293

Merged
merged 2 commits into from
Mar 2, 2016
Merged

Kill docker daemon after configuring cbr0 #22293

merged 2 commits into from
Mar 2, 2016

Conversation

dchen1107
Copy link
Member

Address #21523

@k8s-github-robot
Copy link

Labelling this PR as size/M

@k8s-github-robot k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 1, 2016
@k8s-bot
Copy link

k8s-bot commented Mar 1, 2016

GCE e2e build/test passed for commit 297a94c6db758b43fb5c6a069c9f3a15d52fb1ef.

@k8s-bot
Copy link

k8s-bot commented Mar 1, 2016

GCE e2e build/test passed for commit d32987bd7448a4e4679803025ecabd2d5c9cf04d.

@k8s-bot
Copy link

k8s-bot commented Mar 1, 2016

GCE e2e build/test passed for commit 7e48eae82437c0afa8539391e1bd05a5cb5605d0.

@dchen1107 dchen1107 force-pushed the test branch 2 times, most recently from 6d9c6ef to f1d4662 Compare March 1, 2016 21:41
@bprashanth
Copy link
Contributor

kill -9 might have bad effects on plugins if they're relying on a signal from docker when it receives SIGTERM to save state to a checkpoint (that's stored somewhere other than /var/lib/docker/network, so we won't remove it). It's generally not a polite thing. I'm ok with this if the alternative is too complex (--container-runtime-restart-script=/usr/sbin/docker-checker), but please add a todo or comment that explains why we -KILL.

@k8s-bot
Copy link

k8s-bot commented Mar 1, 2016

GCE e2e build/test passed for commit 6d9c6ef35b7f740c3565985a2bed40e400ed35a3.

@k8s-bot
Copy link

k8s-bot commented Mar 1, 2016

GCE e2e build/test passed for commit f1d46627264848a91adf8ca4d8bf2193e0e7f636.

@dchen1107
Copy link
Member Author

Why you care about sigkill here? It should be issued only when node coming up without any kube container running since network configuration is not done, thus the node is not ready. Why you care about checkpoint files through SIGTERM? We are removing all checkpoints /var/lib/docker/network anyway, right?

I can add more comments on why we want to kill, and please note that the newly introduced flag is marked as deprecated already.

@bprashanth
Copy link
Contributor

We are removing all checkpoints /var/lib/docker/network anyway, right?

Not for all plugins. Not even for all network plugins, CNI won't store its checkpoint under /var/lib/docker.

Why you care about sigkill here? It should be issued only when node coming up without any kube container running since network configuration is not done, thus the node is not ready.

SGTM. We decided not to do: #19854, so the CIDR can't change. Please add as comment.

I'll apply lgtm after rebase and comment.

@k8s-github-robot
Copy link

@dchen1107 PR needs rebase

@k8s-github-robot k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/old-docs labels Mar 1, 2016
@dchen1107
Kill docker daemon after configing cbr0 if flag --babysit-daemon is t…
a90ac42 
…rue so that babysitter process can restart it again with proper configurations and checkpoint file.
@dchen1107
set --babysit-daemons=true for gce and aws
960bea3
@dchen1107
Copy link
Member Author

Add more comments. Thanks!

@bprashanth
Copy link
Contributor

LGTM, thanks!

@bprashanth bprashanth added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 2, 2016
bprashanth
bprashanth reviewed Mar 2, 2016
View reviewed changes
cluster/saltbase/salt/kubelet/default
@@ -164,6 +164,11 @@
{% set hairpin_mode = "--hairpin-mode=" + pillar['hairpin_mode'] -%}
{% endif -%}

{% set babysit_daemons = "" -%}
{% if grains['cloud'] is defined and grains.cloud in [ 'aws', 'gce' ] %}
Copy link
Contributor

Choose a reason for hiding this comment

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

is gke included in gce?

@dchen1107 dchen1107 added this to the v1.2 milestone Mar 2, 2016
@dchen1107 dchen1107 added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Mar 2, 2016
@k8s-bot
Copy link

k8s-bot commented Mar 2, 2016

GCE e2e build/test passed for commit 960bea3.

@k8s-github-robot
Copy link

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

@k8s-bot
Copy link

k8s-bot commented Mar 2, 2016

GCE e2e build/test passed for commit 960bea3.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

k8s-github-robot pushed a commit that referenced this pull request Mar 2, 2016
Merge pull request #22293 from dchen1107/test
73b39e9 
Auto commit by PR queue bot
@k8s-github-robot k8s-github-robot merged commit 73b39e9 into kubernetes:master Mar 2, 2016
@bprashanth bprashanth mentioned this pull request Mar 2, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@bprashanth bprashanth

Labels
kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.2
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@dchen1107 @k8s-github-robot @k8s-bot @bprashanth @k8s-teamcity-mesosphere @googlebot