feat(chart): refactor helm chart #540

stevehipwell · 2021-11-30T17:42:55Z

Description of changes:

This PR updates the Helm chart to use the Helm 3 v2 API and to follow the latest idiomatic pattern; this should lower the cognitive complexity of the chart and make it easier to keep maintained moving forwards.

A summary of the important changes are as follows:

Update API version to v2 and only support Helm 3
Limit chart to K8s v1.16 or later
Align chart to default chart template
Align Daemonset and Deployment templates
Remove support for deprecated beta node labels
Improve default security context
Remove un-needed configuration copied between Deployment/Daemonset
Require probes for deployment
Extract Daemonset specific configuration into new values (with fall back logic)
Use correct jobLabel logic in ServiceMonitor/PodMonitor
Update chart template lint tests to use idiomatic queue and IMDS installations

Fixes #518.

Breaking Changes:

The following changes are technically breaking but should have no impact on the use of NTH in Queue mode and require only minor changes for NTH in IMDS mode.

All Modes:

Only support Helm 3
Only support Kubernetes v1.16 and up
Remove use of beta K8s node labels
Namespace isn't set in templates
Removed gracePeriod

Queue Mode:

Can't set hostNetwork or dnsPolicy
Probes server is always enabled

IMDS Mode:

Ignore priorityClassName and use daemonsetPriorityClassName
Only one node selector, affinity and toleration key can be set (see README)
Removed linuxUpdateStrategy & windowsUpdateStrategy

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

stevehipwell · 2021-11-30T17:43:15Z

FYI @bwagner5

stevehipwell · 2021-12-01T16:01:13Z

@bwagner5 I'm getting an interesting failure on the fast tests that doesn't look anything to do with this PR?

bwagner5 · 2021-12-15T16:30:56Z

Looks like a kubectl error having to do with a selector:

error: You must provide one or more resources by argument or filename.
Example resource specifications include:
   '-f rsrc.yaml'
   '--filename=rsrc.json'
   '<resource> <name>'
   '<resource>'

stevehipwell · 2021-12-15T16:34:49Z

@bwagner5 I wonder if it's related to my change of NOTES.txt? I've been a bit busy but will try and update the README and then test the E2E scripts locally.

It'd be good to get your thoughts on the changes if there is anything you want clarifying or changing before I spend the effort on the README.

bwagner5 · 2021-12-15T22:40:38Z

Just a note that #529 was merged recently (between when you opened this PR) that adds customLabels to the helm chart.

bwagner5

looks like this is trending in the right direction! Thanks for doing this!

config/helm/aws-node-termination-handler/values.yaml

test/helm/test-imds-values.yaml

test/helm/helm-lint

stevehipwell · 2021-12-16T11:10:10Z

Just a note that #529 was merged recently (between when you opened this PR) that adds customLabels to the helm chart.

@bwagner5 I think these changes align pretty well with the ones I've made, I'll merge it in.

github-actions · 2021-12-31T17:07:54Z

This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want this PR to never become stale, please ask a maintainer to apply the "stalebot-ignore" label.

stevehipwell · 2021-12-31T17:14:02Z

/not-stale

bwagner5 · 2022-01-04T01:49:13Z

Sorry for the delay! I've been off for the holidays.

stevehipwell · 2022-01-04T16:33:22Z

@bwagner5 I've got the tests fixed, it was a side effect of the old daemonset values being applied to a deployment making the tests work.

If you're happy with the work so far I'll finish off the README for final review?

bwagner5 · 2022-01-04T17:35:59Z

@bwagner5 I've got the tests fixed, it was a side effect of the old daemonset values being applied to a deployment making the tests work.

If you're happy with the work so far I'll finish off the README for final review?

Yep! LGTM!

stevehipwell · 2022-01-05T12:36:35Z

@bwagner5 after making a few minor changes I've updated the PR description and the chart README ready for review.

I looked through the code and it looks like the ASG tagging is only used in the queue processor mode and the metadata config is only used by the IMDS mode, could you confirm this?

bwagner5 · 2022-01-06T23:24:33Z

@bwagner5 after making a few minor changes I've updated the PR description and the chart README ready for review.

I looked through the code and it looks like the ASG tagging is only used in the queue processor mode and the metadata config is only used by the IMDS mode, could you confirm this?

Yes, that is correct

config/helm/aws-node-termination-handler/Chart.yaml

bwagner5

Just the one comment on the chart version and then I think this is good to merge!

Signed-off-by: Steve Hipwell <steve.hipwell@gmail.com>

stevehipwell · 2022-01-07T10:25:36Z

@bwagner5 I've reverted the chart version so once the tests go though I think we're ready.

bwagner5

lgtm! Thanks!

cainelli · 2022-03-08T12:24:12Z

config/helm/aws-node-termination-handler/templates/_helpers.tpl

+{{- define "aws-node-termination-handler.selectorLabelsDeployment" -}}
+{{ include "aws-node-termination-handler.selectorLabels" . }}
+app.kubernetes.io/component: deployment
+{{- end -}}


Please be mindful when changing immutable fields, it requires additional work when updating the helm chart to ensure high availability.

@cainelli this work was intended to be a breaking change and these changes were fully intentional and made with the understanding of the implications. I think there might have been a some missed communication when this was released (see above) but technically all changes to a pre v1 SemVer package are breaking.

Thanks for clarifying @stevehipwell and sorry for the grumpiness.

tamirhad · 2022-03-14T17:41:56Z

@stevehipwell Hey,
why ENABLE_SPOT_INTERRUPTION_DRAINING always false when using deployment(sqs mode)?

stevehipwell · 2022-03-14T17:45:18Z

@tamirhad because it's only relevant to the IMDS mode. I did notice earlier that in some of the AWS docs they incorrectly specify setting it for queue mode.

stevehipwell force-pushed the helm-chart-refactor branch from b59eac6 to 41d1a9d Compare December 1, 2021 15:41

stevehipwell force-pushed the helm-chart-refactor branch 4 times, most recently from 62542e1 to 7a50f1e Compare December 15, 2021 16:29

bwagner5 reviewed Dec 15, 2021

View reviewed changes

stevehipwell force-pushed the helm-chart-refactor branch from 7a50f1e to cececa7 Compare December 16, 2021 11:16

github-actions bot added the stale Issues / PRs with no activity label Dec 31, 2021

github-actions bot removed the stale Issues / PRs with no activity label Jan 1, 2022

stevehipwell force-pushed the helm-chart-refactor branch 11 times, most recently from 978ff1d to 2b66407 Compare January 4, 2022 16:02

stevehipwell force-pushed the helm-chart-refactor branch from 2b66407 to 1e8adf1 Compare January 5, 2022 12:34

stevehipwell marked this pull request as ready for review January 5, 2022 12:36

stevehipwell requested a review from a team as a code owner January 5, 2022 12:36

stevehipwell force-pushed the helm-chart-refactor branch from 1e8adf1 to f24aae9 Compare January 5, 2022 13:01

bwagner5 reviewed Jan 6, 2022

View reviewed changes

config/helm/aws-node-termination-handler/Chart.yaml Outdated Show resolved Hide resolved

bwagner5 requested changes Jan 6, 2022

View reviewed changes

feat(chart): refactor helm chart

72416ae

Signed-off-by: Steve Hipwell <steve.hipwell@gmail.com>

stevehipwell force-pushed the helm-chart-refactor branch from f24aae9 to 72416ae Compare January 7, 2022 10:24

bwagner5 approved these changes Jan 12, 2022

View reviewed changes

bwagner5 merged commit 1c61c2d into aws:main Jan 12, 2022

stevehipwell deleted the helm-chart-refactor branch January 12, 2022 19:39

stevehipwell mentioned this pull request Jan 24, 2022

Unable to set an custom nodeAffinity via values.yaml #495

Closed

ec2-bot mentioned this pull request Jan 31, 2022

🥳 node-termination-handler v1.14.1 Automated Release! 🥑 aws/eks-charts#675

Merged

stevehipwell mentioned this pull request Feb 1, 2022

🥑🤖 v1.14.1 release prep #565

Merged

danquack mentioned this pull request Feb 3, 2022

Helm chart has improper indentation with enablePrometheusServer #573

Closed

cainelli reviewed Mar 8, 2022

View reviewed changes

hamishforbes mentioned this pull request Aug 16, 2022

helm: Apply extraEnv to daemonsets #674

Merged

feat(chart): refactor helm chart #540

feat(chart): refactor helm chart #540

Uh oh!

Conversation

stevehipwell commented Nov 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stevehipwell commented Nov 30, 2021

Uh oh!

stevehipwell commented Dec 1, 2021

Uh oh!

bwagner5 commented Dec 15, 2021

Uh oh!

stevehipwell commented Dec 15, 2021

Uh oh!

bwagner5 commented Dec 15, 2021

Uh oh!

bwagner5 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stevehipwell commented Dec 16, 2021

Uh oh!

github-actions bot commented Dec 31, 2021

Uh oh!

stevehipwell commented Dec 31, 2021

Uh oh!

bwagner5 commented Jan 4, 2022

Uh oh!

stevehipwell commented Jan 4, 2022

Uh oh!

bwagner5 commented Jan 4, 2022

Uh oh!

stevehipwell commented Jan 5, 2022

Uh oh!

bwagner5 commented Jan 6, 2022

Uh oh!

Uh oh!

bwagner5 left a comment

Choose a reason for hiding this comment

Uh oh!

stevehipwell commented Jan 7, 2022

Uh oh!

bwagner5 left a comment

Choose a reason for hiding this comment

Uh oh!

cainelli Mar 8, 2022

Choose a reason for hiding this comment

Uh oh!

stevehipwell Mar 8, 2022

Choose a reason for hiding this comment

Uh oh!

cainelli Mar 8, 2022

Choose a reason for hiding this comment

Uh oh!

tamirhad commented Mar 14, 2022

Uh oh!

stevehipwell commented Mar 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

stevehipwell commented Nov 30, 2021 •

edited

Loading