honeycombio / helm-charts

Helm repository and charts for Honeycomb
Apache License 2.0
30 stars 39 forks source link

feat: Enable configuring rollout strategy for Refinery deployment #340

Closed bixu closed 9 months ago

bixu commented 9 months ago

Which problem is this PR solving?

We've observed Refinery updates to be disruptive in some cases. A rolling update with conservative defaults should help give Refinery nodes more time to rebalance traffic across the cluster.

bixu commented 9 months ago

CI is failing in a fairly opaque way. I'm happy to contribute more changes if someone can shed some light on the test suite behavior.

TylerHelmuth commented 9 months ago

@bixu the CI tests failed installing the chart. Have you tested this change locally? I believe the Deployment spec is incorrect: https://kubernetes.io/docs/concepts/workloads/controllers/deployment/#strategy

bixu commented 9 months ago

@TylerHelmuth, good catch on the spec nesting. Fixed that, and tested successfully in local dev (added a Brewfile the PR to speed up local test setup):

ct install --config ./ct.yaml

<snip>

------------------------------------------------------------------------------------------------------------------------
 ✔︎ honeycomb => (version: "1.8.2", path: "charts/honeycomb")
 ✔︎ network-agent => (version: "0.3.0", path: "charts/network-agent")
 ✔︎ refinery => (version: "2.7.0", path: "charts/refinery")
------------------------------------------------------------------------------------------------------------------------
All charts installed successfully

I'm still fuzzy about why CircleCI is failing.

TylerHelmuth commented 9 months ago

@bixu it looks like you've committed some extra files. Also please handle the merge conflict.

bixu commented 9 months ago

@TylerHelmuth: done. I was only able to get working linting locally by using kubeconform. Would y'all be open to another PR introducing kubeconform linting?

bixu commented 9 months ago

@bixu this looks good, but to make it more configurable for other strategy types lets implement something like we did in the collector helm chart:

I was hoping to get some feedback on the interface -- thanks! Will pull in this prior pattern.

Also we'll stick with chart-testing as our linter.

Noted :)