Update the documentation to use DaemonSet or Deployment #1735

saschagrunert · 2017-06-11T10:49:31Z

From the discussion of #1706

Waiting for your review. 👍

timoreimann

Thanks! Left a few comments.

@dtomcej and @errm, PTAL as well.

timoreimann · 2017-06-11T11:00:20Z

docs/user-guide/kubernetes.md

+It is possible to use Træfik with a
+[Deployment](https://kubernetes.io/docs/concepts/workloads/controllers/deployment/)
+or a [DaemonSet](https://kubernetes.io/docs/concepts/workloads/controllers/daemonset/)
+object, whereas both options have their own pros and cons: The scalability os much better when


Typo: os -> is

timoreimann · 2017-06-11T11:02:20Z

docs/user-guide/kubernetes.md

+the DeaemonSet. On the other hand the DaemonSet allows you to access any Node
+directly on Port 80 and 443, where you have to setup a 
+[Service](https://kubernetes.io/docs/concepts/services-networking/service/) object
+with a Deployment.


We could also mention that a Daemonset allows you to exclusively run a service (like Traefik) on a dedicated set of machines using taints and tolerations.

Alright, I add a note about this.

timoreimann · 2017-06-11T11:03:32Z

docs/user-guide/kubernetes.md

+  ports:
+    - protocol: TCP
+      port: 80
+      targetPort: 80


AFAIR, we don't need to specify a target port when it matches the service port.

timoreimann · 2017-06-11T11:04:21Z

docs/user-guide/kubernetes.md

+      port: 80
+      targetPort: 80
+    - protocol: TCP
+      port: 8081


I think we should just use 8080, which happens to be the default port for Traefik's administration interface.

timoreimann · 2017-06-11T11:05:36Z

docs/user-guide/kubernetes.md

          hostPort: 80
-        - containerPort: 8080
+        - name: admin
+          containerPort: 8081


8080 would be my preference here too.

timoreimann · 2017-06-11T11:06:41Z

docs/user-guide/kubernetes.md

        - --web
+        - --web.address=:8081


We don't need this after switching to 8080.

timoreimann · 2017-06-11T11:07:24Z

docs/user-guide/kubernetes.md

-> notice that we binding port 80 on the Træfik container to port 80 on the host.
-> With a multi node cluster we might expose Træfik with a NodePort or LoadBalancer service
-> and run more than 1 replica of Træfik for high availability.
+To deploy Træfik to your cluster start by submitting one of the yaml files to the cluster with `kubectl`:


s/yaml/YAML/

saschagrunert · 2017-06-11T11:25:28Z

Changed the files regarding your comments. :)

errm · 2017-06-12T08:33:33Z

docs/user-guide/kubernetes.md

+set of machines using taints and tolerations with a DaemonSet. On the other hand the
+DaemonSet allows you to access any Node directly on Port 80 and 443, where you have to setup a 
+[Service](https://kubernetes.io/docs/concepts/services-networking/service/) object
+with a Deployment.


I would add something here that deployments being suited in environments where there is some sort of external loadbalancer (Like AWS or GCP) to forward traffic to the nodeport.

I would also mention that Deployment offers the facility to upgrade the version / config in place. Whereas DaemonSet offers no ability to do this, short of manually deleting the pods on each node...

I think mostly the DaemonSet option is applicable to development environments like minikube, or bear metal, or some other usecase where you want to expose the hosts directly...

Daemonsets have a rolling deployment option in 1.6+ ;)

Yes you are correct ... I forgot since until recently I am still running a pre 1.6 cluster (for reasons)

errm · 2017-06-12T08:38:46Z

docs/user-guide/kubernetes.md

+Instead of installing Træfik via an own object, you can also use the Træfik Helm chart. This
+allows more complex configuration via Kubernetes
+[ConfigMap](https://kubernetes.io/docs/tasks/configure-pod-container/configmap/) and enabled
+TLS certificates.


Is this true, I don't think we support Kubernetes provided TSL certificates yet...

What do you mean? The helm chart support indeed TLS termination at the ingress.

Yes but I don't think that you are currently able to provide a TLS cert as a kubernetes secret..

Why not? You can create tls secrets with something like:

kubectl create secret tls foo-secret --key /tmp/tls.key --cert /tmp/tls.crt

I think we are probably talking at cross purposes here, I am thinking about the ability to specify a cert to use in the ingress annotation ingress.kubernetes.io/auth-tls-secret. We don't currently support that ...

timoreimann

Two more things, and another general one:

Should we add a general remark that the examples are not (necessarily) meant for production usage? Or at least urge readers to make a conscious decision on the production fitness of our example files?

timoreimann · 2017-06-12T11:07:45Z

docs/user-guide/kubernetes.md

          hostPort: 80
-        - containerPort: 8080
+        - name: admin
+          containerPort: 8080


This one should probably keep its hostPort for symmetry reasons.

Shouldn't this be exposed with ingress via Traefik?

It can, but this is not necessarily needed. It makes the configuration easier.

timoreimann · 2017-06-12T11:08:30Z

examples/k8s/traefik-ds.yaml

@@ -37,11 +37,10 @@ spec:
          containerPort: 80
          hostPort: 80
        - name: admin
-          containerPort: 8081
+          containerPort: 8080


hostPort here too probably.

So shall we add it?

Honestly, according to the kubernetes Configurations best practices documentation (https://kubernetes.io/docs/concepts/configuration/overview/) it clearly states:

Don’t use hostPort unless it is absolutely necessary. They recommend using kubectl proxy or kubectl port-forward. I think if we are providing an example for new users to try, we should at least follow best practices. MANY users have been bitten by hostport usage.

I agree that host ports are tricky in general; but haven't we accepted the deal already when we picked the DaemonSet? After all, we want to reap the benefits of easy accessibility to a known, static port (and, in particular, not add another Service configured with a NodePort).

The kubectl-based approaches are recommended "for debugging purposes". I don't see the admin interface as something you'd only need for rare-ish debug cases, but opinions may vary on this point. To the least, I find it inconsistent that we expose the admin port on the Deployment (through a NodePort) but do not do the same on the DaemonSet. I'm concerned this may lead to confusion.

I agree that Hostport is convenient and easy, but many cloud providers don't allow hostport without escalating privileges. Not sure if this is something we should be "encouraging".

I would be happiest keeping the same nodeports through our examples. Nodeport is fully supported, and pretty easy for a newbie to understand.

timoreimann

To all involved parties: let's try to resolve the open issues and find closure. :-)

timoreimann · 2017-06-17T05:37:56Z

examples/k8s/traefik-ds.yaml

@@ -37,11 +37,10 @@ spec:
          containerPort: 80
          hostPort: 80
        - name: admin
-          containerPort: 8081
+          containerPort: 8080


So shall we add it?

timoreimann · 2017-06-22T23:49:10Z

docs/user-guide/kubernetes.md

+        - name: admin
+          containerPort: 8080
+        securityContext:
+          privileged: true


Do we actually need this? It's not on the Deployment, to the least.

Yes, Hostport on a privileged port requires this IIRC

@dtomcej Per your earlier comment, if you used a more arbitrary port 30080, this would not need to be privileged. In most cases, since there is still an ELB or equivalent in front of traefik, this shouldn't be an issue.

saschagrunert · 2017-07-01T15:03:28Z

I changed the documentation to use the nodeport service instead of host ports. Please have a look again. :)

timoreimann · 2017-07-01T21:48:55Z

If we use Service NodePorts on DaemonSets too, what benefits do we see left that justify using/documenting them at all?

To me they are most valuable when combined with nodeSelectors / taints and tolerations so that you can define a dedicated set of pod-hosting nodes, obviously in our case for Traefik. In such scenarios, you probably wouldn't have to worry about conflicting ports and would be just fine with using host ports, especially when it improves discoverability (port 80 and 8080 are super easy to remember and stable even if you kill your NodePort-providing Service accidentally).

@dtomcej @saschagrunert WDYT?

saschagrunert · 2017-07-02T11:09:46Z

Yes, for me the DS host port is an as valid solution as the Deployment + NodePort Service. I just reverted the last commit. But we have to come to a clear point what we want to add to the documentation for now.

This reverts commit 819931d.

timoreimann · 2017-07-04T19:48:45Z

@dtomcej your turn again. :-)

timoreimann · 2017-07-04T21:30:01Z

Alright @dtomcej, that does make a lot of sense. You convinced me. :-)

Just one question: By "Can implement full pod lifecycle", you probably mean everything related to rolling upgrades? AFAIK, Kubernetes 1.7 brings DaemonSets on par in this regard.

@saschagrunert could you incorporate the points Daniel made (maybe except for the lifecycle one until we've clarified)?

dtomcej · 2017-07-04T22:54:40Z

@timoreimann Correct. Pre-1.6 Daemonsets did not have rolling restarts, rollbacks, history, image changes, etc.

I agree that my term "lifecycle" was incorrect, as yes, Daemonsets still can implement the lifecycle hooks for containers.

dtomcej · 2017-07-06T03:58:39Z

examples/k8s/traefik-deployment.yaml

+  selector:
+    k8s-app: traefik-ingress-lb
+  ports:
+    - protocol: TCP


Need a name for the ports.

dtomcej · 2017-07-06T04:18:50Z

examples/k8s/traefik-deployment.yaml

+apiVersion: v1
+metadata:
+  name: traefik-ingress-service
+spec:


Also need to be part of kube-system due to namespaced pods

ldez · 2017-07-17T15:30:41Z

@saschagrunert Any news on this PR?

saschagrunert · 2017-07-23T10:03:34Z

Alright, please have a look again. 👍

timoreimann

One more thing, then I'm good. :-)

timoreimann · 2017-07-24T08:25:55Z

docs/user-guide/kubernetes.md

+There are some significant differences between using Deployments and DaemonSets. The Deployment has easier
+up and down scaling possibilities. It can implement full pod lifecycle and supports rolling updates from
+Kubernetes 1.2. At least one Pod is needed to run the Deployment. The DaemonSet automatically scales to all nodes that
+meets a specific selector and guarantees to fill nodes one at a time. Rolling updates are supported from Kubernetes 1.6


1.7 really brought DaemonSet rolling upgrades on par with Deployments. I suggest we slightly rephrase to:

Rolling updates are fully supported from Kubernetes 1.7 [...]

(Emphasis by me to highlight the suggested delta, please don't include those. :-) )

saschagrunert · 2017-07-24T08:43:15Z

Like this? :)

timoreimann

I ❤️ it!

LGTM! Thanks with bearing with us, @saschagrunert! 👍

dtomcej

Looks pretty boss.

LGTM

errm · 2017-07-26T14:10:40Z

LGTM...

Just needs commits squashing down ...

ldez · 2017-07-29T10:50:05Z

Bender say:
The first fully automated merge is a success!
🍾 🎉

saschagrunert added 5 commits June 11, 2017 12:06

Create traefik-ds.yaml

87d5a94

Create trafeik-deployment.yaml

681a2dc

Create traefik-deployment.yaml

c040f72

Create traefik-deployment.yaml

8486976

Create kubernetes.md

f25cc67

saschagrunert mentioned this pull request Jun 11, 2017

Update the documentation to use DaemonSet or Deployment #1706

Closed

timoreimann suggested changes Jun 11, 2017

View reviewed changes

saschagrunert added 3 commits June 11, 2017 13:23

Implemented review notes

19ce29e

Implemented review notes

7ce7a99

Implemented review notes

6c893ac

ldez added area/documentation area/provider/k8s/ingress kind/enhancement a new or improved feature. status/2-needs-review labels Jun 11, 2017

ldez added this to the 1.4 milestone Jun 11, 2017

Merge branch 'master' into master

7a55f83

errm suggested changes Jun 12, 2017

View reviewed changes

timoreimann suggested changes Jun 12, 2017

View reviewed changes

timoreimann mentioned this pull request Jun 17, 2017

Kubernetes example references Deployment|RS uses DaemonSet #1764

Closed

timoreimann reviewed Jun 17, 2017

View reviewed changes

timoreimann mentioned this pull request Jun 17, 2017

Kubernetes example inconsistent ingress for cheese websites #1765

Closed

timoreimann reviewed Jun 22, 2017

View reviewed changes

saschagrunert added 2 commits July 1, 2017 16:56

Merge branch 'master' into master

aa82eed

Switched to NodePort service when using the DS

819931d

Revert "Switched to NodePort service when using the DS"

58b1b22

This reverts commit 819931d.

saschagrunert added 2 commits July 4, 2017 22:51

Update traefik-ds.yaml

d4a6631

Update traefik-deployment.yaml

fad25c8

dtomcej suggested changes Jul 6, 2017

View reviewed changes

examples/k8s/traefik-deployment.yaml

selector:

k8s-app: traefik-ingress-lb

ports:

- protocol: TCP

Copy link

Contributor

dtomcej Jul 6, 2017

Choose a reason for hiding this comment

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

Need a name for the ports.

dtomcej suggested changes Jul 6, 2017

View reviewed changes

ldez added the contributor/waiting-for-corrections label Jul 6, 2017

ldez requested a review from a team July 19, 2017 13:35

saschagrunert added 2 commits July 23, 2017 11:51

Merge branch 'master' into master

b632eb2

Implemented review changes

62c82e1

ldez removed the contributor/waiting-for-corrections label Jul 23, 2017

timoreimann suggested changes Jul 24, 2017

View reviewed changes

Update kubernetes.md

5545766

timoreimann approved these changes Jul 24, 2017

View reviewed changes

dtomcej approved these changes Jul 24, 2017

View reviewed changes

errm approved these changes Jul 26, 2017

View reviewed changes

juliens added status/3-needs-merge and removed status/2-needs-review labels Jul 27, 2017

Merge remote-tracking branch 'upstream/master'

61f35f2

ldez added status/4-merge-in-progress and removed status/4-merge-in-progress labels Jul 29, 2017

ldez merged commit 29390a3 into traefik:master Jul 29, 2017

ldez removed the status/3-needs-merge label Jul 29, 2017

timoreimann mentioned this pull request Mar 16, 2018

Set INFO log level in Kubernetes guide and examples. #3037

Merged

Update the documentation to use DaemonSet or Deployment #1735

Update the documentation to use DaemonSet or Deployment #1735

Conversation

saschagrunert commented Jun 11, 2017

timoreimann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saschagrunert commented Jun 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timoreimann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timoreimann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saschagrunert commented Jul 1, 2017

timoreimann commented Jul 1, 2017

saschagrunert commented Jul 2, 2017

timoreimann commented Jul 4, 2017

timoreimann commented Jul 4, 2017

dtomcej commented Jul 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldez commented Jul 17, 2017

saschagrunert commented Jul 23, 2017

timoreimann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saschagrunert commented Jul 24, 2017

timoreimann left a comment

Choose a reason for hiding this comment

dtomcej left a comment

Choose a reason for hiding this comment

errm commented Jul 26, 2017

ldez commented Jul 29, 2017