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.

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

[kong] add support for DaemonSets #347

Merged
merged 2 commits into from
Apr 20, 2021
Merged

[kong] add support for DaemonSets #347

merged 2 commits into from
Apr 20, 2021

Conversation

rainest
Copy link
Contributor

@rainest rainest commented Apr 19, 2021

What this PR does / why we need it:

Add a toggle to use a DaemonSet instead of a Deployment.

Special notes for your reviewer:

Carried over from #269 with some changes to docs and unintended changes.

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

  • PR is based off the current tip of the next branch and targets next, not main
  • New or modified sections of values.yaml are documented in the README.md
  • Title of the PR and commit headers start with chart name (e.g. [kong])

Add a toggle to use a DaemonSet controller for Kong.

Co-authored-by: arjsrira <arjsrira@cisco.com>
@rainest rainest requested a review from a team as a code owner April 19, 2021 21:03
Copy link
Member

@mflendrich mflendrich left a comment

Choose a reason for hiding this comment

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

I'm torn.

When I first saw this PR, I was surprised by how easy (on the code) introduction of DaemonSets was in this PR. Then I looked at the API spec for DeploymentSpec and DaemonSetSpec in the k8s api to discover that they're similar, but not the same type - in fact, they share a subset of fields, but the types are not identical. This effectively means that this "unified" template be restricted to the set product of Deployment and DaemonSet fields by making this change, or need to introduce some terrible branching depending on which deployment method was chosen.

To me the "prudent" solution for this (in the strongly typed k8s world) would be to define two conditionally enabled templates (one for a deployment, one for a daemonset) with 99%-100% of the spec being generated by more specific templates, thus avoiding the hack of treating schemas of distinct types as identical.

But maybe that would add complexity to an already bloated chart and we'll never need it 🤷. Time will show, I guess.

Use your best judgment.

@rainest
Copy link
Contributor Author

rainest commented Apr 19, 2021

There isn't a whole great deal of difference since most of the actual content is in the Pod template, which is the same object type in both. Split templates could move that into _helpers.tpl, which grows ever larger, and has become a bit cumbersome to deal with.

The single template branching approach is effectively what we're doing with the replicas gate, and is probably fine given that there aren't many differences between the two controllers. As for the rest:

  • paused doesn't look like you'd manage it with the chart, since you'd instead manage it with kubectl rollout pause/kubectl rollout resume.
  • progressDeadlineSeconds we don't use yet, but would need to handle it the same as replicas
  • strategy versus updateStrategy is odd since it's fully user-defined and only rendered if set. We could overload the values setting, populate the correct name depending on which controller you're using, and still leave setting the correct contents up to the user.

If the controller is a DaemonSet, use an "updateStrategy" header for the
updateStrategy value. If it is a Deployment, use "strategy".

DaemonSet and Deployment controllers have similar, but subtly different
specs. This section has a similar purpose in each, but is named
differently. The contents of this section are entirely user-provided, so
the template doesn't need to handle anything beyond the difference in
name.

Somewhat confusingly, though the chart originally supported Deployments
only, the value was always named "updateStrategy", even though the
correct section name for Deployments is "strategy".
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants