Skip to content

Conversation

johngmyers
Copy link
Contributor

Issue #, if available:

None

Description of changes:

Remove configuration settings that don't affect Queue Processor mode (assuming #743 merges) from the Deployment template, so that they don't act as red herrings.

Document the settings that only apply to IMDS mode.

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

@johngmyers johngmyers requested a review from a team as a code owner December 10, 2022 22:21
| `linuxTolerations` | Override `daemonsetTolerations` for the Linux DaemonSet. | `[]` |
| `windowsTolerations` | Override `daemonsetTolerations` for the Linux DaemonSet. | `[]` |
| `enableProbesServer` | If `true`, start an http server exposing `/healthz` endpoint for probes. | `false` |
| `enableProbesServer` | If `true`, start an http server exposing `/healthz` endpoint for probes. Only used in IMDS mode. | `false` |
Copy link
Contributor

Choose a reason for hiding this comment

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

This is incorrect. The probes server can be exposed whether NTH runs as a daemonset or as a deployment and is not related to IMDS mode. Our default configurations here only enable it for the daemonset, not the deployment, but that is not a technical limitation.

Indeed, our release artifacts explicitly include an aggregated YAML configuration with probes server enabled in Queue Processor mode as a deployment (see the artifact all-resources-queue-processor.yaml from any v1 release). Here's the code that generates that file:

## Queue Processor Mode
$BUILD_DIR/helm template aws-node-termination-handler \
--namespace $NAMESPACE \
--set enableSqsTerminationDraining="true" \
--set enableProbesServer="true" \
$SCRIPTPATH/../config/helm/aws-node-termination-handler/ > $QP_AGG_RESOURCES_YAML

windowsTolerations: []

# If the probes server is running for the Daemonset
# If the probes server is running for the Daemonset. Only used in IMDS mode.
Copy link
Contributor

Choose a reason for hiding this comment

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

To be more correct, let's remove "for the Daemonset", since the probes server can be used in a deployment and is not related to IMDS mode.

Suggested change
# If the probes server is running for the Daemonset. Only used in IMDS mode.
# If the probes server is running

@snay2 snay2 requested a review from jillmon December 14, 2022 16:23
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.

2 participants