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

Remove unnecessary configuration in Queue Processor mode #745

Merged
merged 2 commits into from
Dec 22, 2022

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
@@ -150,12 +150,12 @@ The configuration in this table applies to AWS Node Termination Handler in IMDS
| `daemonsetTolerations` | Tolerations for DaemonSet pod assignment. For backwards compatibility the `tolerations` has priority over this but shouldn't be used. | `[]` |
| `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

@@ -259,22 +259,22 @@ daemonsetTolerations:
linuxTolerations: []
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