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

Remove "Restart=on-failure" from kube-node-drainer.service #41

Merged
merged 1 commit into from
Nov 8, 2016

Conversation

camilb
Copy link
Contributor

@camilb camilb commented Nov 7, 2016

Systemd only allows "Restart=no" for "Type=oneshot" services.

Systemd only allows "Restart=no" for  "Type=oneshot" services.
@codecov-io
Copy link

Current coverage is 56.90% (diff: 100%)

Merging #41 into master will not change coverage

@@             master        #41   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 6255751...020a9ce

Copy link
Contributor

@pieterlange pieterlange left a comment

Choose a reason for hiding this comment

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

While we're at it: is the double $$ intentional here? Some weird double escaping? I think just $(hostname) would work?

@pieterlange
Copy link
Contributor

Thanks! Restart= indeed makes no sense on oneshot services.

@camilb
Copy link
Contributor Author

camilb commented Nov 7, 2016

Apart from this, maybe it's a good idea to replace ExecStart: /bin/true with:

        ExecStart=/bin/sh -c '/usr/bin/docker run --rm -v /etc/kubernetes:/etc/kubernetes {{.HyperkubeImageRepo}}:{{.K8sVer}} \
      /hyperkube kubectl \
      --server=https://{{.ExternalDNSName}}:443 \
      --kubeconfig=/etc/kubernetes/worker-kubeconfig.yaml \
      uncordon $(hostname)'

If the node reboots, it will be marked automatically as schedulable.

@mumoshu
Copy link
Contributor

mumoshu commented Nov 7, 2016

@camilb Ah, I know i'm to blame! Thanks for the fix 🙇

@mumoshu
Copy link
Contributor

mumoshu commented Nov 8, 2016

@pieterlange Yes, $$ is intentional. I remember that I referred to

To pass a literal dollar sign, use "$$".
https://www.freedesktop.org/software/systemd/man/systemd.service.html#Command%20lines

Basically I tried my best to make sure $$(hostname) to get expanded at runtime. I guess if $(hostname) isn't recognized as a systemd variable, it also work as we might expect 👍

@mumoshu mumoshu added this to the v0.9.1-rc.2 milestone Nov 8, 2016
@mumoshu mumoshu merged commit 011c11f into kubernetes-retired:master Nov 8, 2016
@camilb camilb deleted the fix-node-drainer branch November 8, 2016 22:46
davidmccormick pushed a commit to HotelsDotCom/kube-aws that referenced this pull request Jul 18, 2018
…working-settings-on-nodes to hcom-flavour

* commit '646a0ccd5d4125c74418c5b893464c4c2bd7ef44':
  Inherit controlplane Kubenetes config in node pools.
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.

4 participants