Move Server to Deployment instead of DaemonSet

[daviddyball]

See discussion in this issue

Considerations:

• I've set replicas to 3, but most will want to adjust this
• I've assumed that antiAffinity is based on pod co-location and not some other criteria
• The nodeSelector still ensures it only runs on masters, while spec.affinity prevents co-location on the same node
• Would it be worth adding some YAML for Horizontal Pod Autoscaling at this stage?

EDIT: Also relates to issues rolling master nodes causing issues

[daviddyball]

@rhysemmas @Joseph-Irving : I saw that you had recent approval activity on some closed PRs. Are you active maintainers? Are you able to review this PR?

[stefansedich]

Would love to see this one merged, however I wonder @daviddyball if it should provide an option to either run as a DaemonSet or a Deployment depending on what a user prefers.

[daviddyball]

Very true @stefansedich. It's currently not a helm template, so adding some sort of "switching" logic is not really possible. Perhaps just offering two different YAML files is enough? My only concern would be people blindly doing kubectl apply -f deploy/*.yaml 😛

[stefansedich]

Ah sorry @daviddyball it is early and I failed to realize this was not the helm chart! on that note were you planning to update the Helm chart too?

[denniswebb]

If this has gotten stale, I'm open to doing the helm chart work because as it stands, I'm about to have to start hosting the chart myself to do the daemonset vs deployment work. Please ping me if I'm open to do the helm chart work myself on a separate PR.

[daviddyball]

This isn't exactly stale, it's just not gotten reviewed by any maintainers 😕 We've been running kiam-server as a Deployment since September and it's solved most of our issues with masters being rolled.

[jaygorrell]

I'm curious if the maintainers have any opposition to this approach. The chart PR linked above (#332) adds an option for moving to Deployment without changing the default... so hopefully that can get accepted more easily.

If the default is changing, the chart should change to match but also there are things like the README that should be updated as part of this PR. We just need input from a maintainer though.

[daviddyball]

This one doesn't force a default, it just offers a different YAML file if you choose to run it as a Deployment 🤷‍♂️

Scratch that, it did overwrite server.yaml 😂 I'll push a new commit to offer both options.

[jaygorrell]

It's not a different YAML file though - it's changing the DaemonSet file to be Deployment.

I have no authority here, just saying it may be confusing to have the README talk about using a DaemonSet while the example in deploy/server.yaml is a Deployment. I like the change, but updating the README would make sense.

[daviddyball]

I had it in my head that I'd already refactored this to make it optional which deployment method you choose... turns out I hadn't 🙄

Moved to 2x YAML file options under deploy/, server.daemonset.yaml and server.deployment.yaml and updated README.md to include instructions and a note about only deploying one of the two options available.

[jaygorrell]

Looks good to me. I'd approve if I could. :)

[dbyron0]

I think this is missing:

  strategy:
    rollingUpdate:
      maxSurge: 0
      maxUnavailable: 1
    type: RollingUpdate

as the helm chart has here

See https://github.com/uswitch/kiam/pull/332/files#r351312455 for background.