Skip to content
This repository has been archived by the owner on Sep 30, 2020. It is now read-only.

Replace docker with rkt in "kube-node-drainer.service". #48

Merged
merged 1 commit into from
Nov 9, 2016

Conversation

camilb
Copy link
Contributor

@camilb camilb commented Nov 8, 2016

Fix kube-node-drainer.service ExecStart and ExecStop errors . #40

@codecov-io
Copy link

codecov-io commented Nov 8, 2016

Current coverage is 56.90% (diff: 100%)

Merging #48 into master will not change coverage

@@             master        #48   diff @@
==========================================
  Files             4          4          
  Lines           949        949          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits            540        540          
  Misses          329        329          
  Partials         80         80          

Powered by Codecov. Last update 011c11f...7af1a45

ExecStart=/bin/true
ExecStop=/bin/sh -c '/usr/bin/docker run --rm -v /etc/kubernetes:/etc/kubernetes {{.HyperkubeImageRepo}}:{{.K8sVer}} \
/hyperkube kubectl \
TimeoutStopSec=30s
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a question but any specific reason why this is 30s?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just wanted to make sure this is properly stopped. This configures the amount of time that systemd will wait when stopping the service. Had several intents with rkt without this and the node wasn't marked SchedulingDisabled.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it!

I was just reading https://www.freedesktop.org/software/systemd/man/systemd.service.html#TimeoutStopSec= which says that TimeoutStopSec defaults to the system-wide value configured via DefaultTimeoutStopSec.

Regardless of the default, setting enough timeout like this would be good.
This is what we couldn't have without your effort 👍

--server=https://{{.ExternalDNSName}}:443 \
--kubeconfig=/etc/kubernetes/worker-kubeconfig.yaml \
drain $$(hostname) \
drain $(hostname) \
Copy link
Contributor

@mumoshu mumoshu Nov 9, 2016

Choose a reason for hiding this comment

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

I now understand that $$ and $ to be equivalent in this context because it isn't recognized/expanded as a systemd variable anyways. Good!

ref #40 (comment)
ref #41 (comment)

--volume=kube,kind=host,source=/etc/kubernetes,readOnly=true \
--mount=volume=kube,target=/etc/kubernetes \
--net=host \
quay.io/coreos/hyperkube:v1.4.5_coreos.0 \
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be ok to keep using {{.HyperkubeImageRepo}}:{{.K8sVer}} instead of quay.io/coreos/hyperkube:v1.4.5_coreos.0 to avoid hard-coding for future ease in maintenance/more customizability?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, think I was copying this from another place.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Would it be ok to let me fix this after merging, or would you like to do on your side?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, no problem.

@mumoshu mumoshu added this to the v0.9.1-rc.2 milestone Nov 9, 2016
@camilb
Copy link
Contributor Author

camilb commented Nov 9, 2016

Fixed {{.HyperkubeImageRepo}}:{{.K8sVer}} using git commit --amend. Notice the comment was erased.

@mumoshu
Copy link
Contributor

mumoshu commented Nov 9, 2016

LGTM 👍
Thanks for your contribution 🙇

@mumoshu mumoshu merged commit b02c31c into kubernetes-retired:master Nov 9, 2016
@camilb camilb deleted the fix-node-drainer branch November 9, 2016 09:27
davidmccormick pushed a commit to HotelsDotCom/kube-aws that referenced this pull request Jul 18, 2018
…ndle-cidr-changes to hcom-flavour

* commit '15698292f0a7d5ac9ef64bff8c900cc4d2187b12':
  Handle changing pod or service CIDRs by cleaning up incompatible api objects.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants