Prometheus add nodes gauge for SQS mode #1083

phuhung273 · 2024-10-29T14:34:07Z

Issue #, if available:
Close #785

Description of changes:

Count nodes/instances being tracked. Eg:
k get node return 5
aws ec2 describe-instances return 2
Identify 3 nodes no longer under NTH control

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

phuhung273 · 2024-11-16T09:34:23Z

@stevehipwell can you please give me a help on this. Now i only stuck at writing unit test for opentelemetry.go since it doesn
t have any test.

Do you think i should refactor this file along with this PR or should I open another separate PR (eg: make opentelemetry.go testable)

stevehipwell · 2024-11-18T10:50:50Z

@phuhung273 I'm not a maintainer here but I like to see untestable code refactored to be testable, following the boy scout rule. If you do the work in this PR you can always split the refactoring to a separate PR before it's merged.

@LikithaVemulapalli what do you think?

LikithaVemulapalli · 2024-11-18T17:20:51Z

Yes I agree with @stevehipwell here, let's separate refactoring PR to have a clear idea on the changes made. @phuhung273 if you want to test your changes for this PR let me know I will approve and run so for the future commits you can verify if the existing tests are working, if there are any conflicts or not, once you change the PR status to ready I will run the workflow, appreciate for your contribution. Thanks!

github-actions · 2024-12-04T17:03:48Z

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.

phuhung273 · 2024-12-06T01:57:37Z

/remove-lifecycle stale

github-actions · 2024-12-22T17:03:36Z

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.

tiationg-kho · 2024-12-31T23:38:01Z

Hi @phuhung273, could you resolve the conflicts in pkg/node/node_test.go?

phuhung273 · 2025-01-01T02:35:02Z

Thanks and Happy new year @tiationg-kho. Conflicts resolved

tiationg-kho

Hi @phuhung273,

Thanks for resolving the conflicts. I have left some comments.
Also, we can run the e2e test in local (docker desktop) to make sure our modification is valid.

Run all e2e test cases: make e2e-test
Run certain e2e test case: ./test/k8s-local-cluster-test/run-test -a ./test/e2e/<test-case> -d -b e2e-test

pkg/ec2helper/ec2helper.go

pkg/node/node.go

pkg/observability/opentelemetry.go

cmd/node-termination-handler.go

phuhung273 · 2025-01-13T15:37:27Z

Thanks @tiationg-kho for your instruction on the e2e test, took a while to realize where does it fail. I will try to include the 2 new metrics in e2e.

cmd/node-termination-handler.go

phuhung273 · 2025-01-16T14:10:20Z

e2e metrics test in SQS mode added, phew... Please let me know your thought @tiationg-kho, thanks

tiationg-kho

Thank you for adding a new e2e test case! Just left few comments.

@LikithaVemulapalli could you please take a look too?

cmd/node-termination-handler.go

tiationg-kho · 2025-01-22T22:01:19Z

cmd/node-termination-handler.go

@@ -215,6 +215,12 @@ func main() {
 		}
 		log.Debug().Msgf("AWS Credentials retrieved from provider: %s", creds.ProviderName)

+		ec2Client := ec2.New(sess)
+
+		if nthConfig.EnablePrometheus {


Could we add nil check for metrics here in case we have any error during observability.InitMetrics?

I tried to, but the compiler doesn't like it. Can you guys have a try?

Sorry, "nil check" is incorrect. Since we return a zero-value struct when err exists.

Could we rename the error then check that error here?

metrics, initMetricsErr := observability.InitMetrics(nthConfig.EnablePrometheus, nthConfig.PrometheusPort) if initMetricsErr != nil { nthConfig.Print() log.Fatal().Err(initMetricsErr).Msg("Unable to instantiate observability metrics,") }

test/e2e/prometheus-metrics-sqs-test

tiationg-kho · 2025-01-22T23:04:32Z

test/e2e/prometheus-metrics-sqs-test

+if [[ $cordoned -eq 0 ]]; then
+    echo "❌ Worker node was not cordoned"
+else
+    echo "❌ regular-pod-test was not evicted"


This check is not quite correct in the context of this test case.

At first, I just tried to test the 2 new metrics. Later found out that https://github.com/aws/aws-node-termination-handler/blob/main/test/e2e/prometheus-metrics-test only test the metrics in IMDS mode.

So I decided to also include other metrics as well. Thats why the file name is prometheus-metrics-sqs: test all metrics in SQS mode. How do you think ?

Maybe we could remove this if-else check, and replace by adding a status variable before TAINT_CHECK_CYCLES for-loop, and check it after the for-loop to prevent the misleading scenario here (even we already cordoned and evicted but still get a "not evicted" in our log)?

The metric part is good.

LikithaVemulapalli

left some inline comments, thanks for your contribution :)

pkg/node/node.go

LikithaVemulapalli · 2025-01-22T23:27:16Z

cmd/node-termination-handler.go

@@ -215,6 +215,12 @@ func main() {
 		}
 		log.Debug().Msgf("AWS Credentials retrieved from provider: %s", creds.ProviderName)

+		ec2Client := ec2.New(sess)
+
+		if nthConfig.EnablePrometheus {


pkg/ec2helper/ec2helper.go

pkg/ec2helper/ec2helper_test.go

pkg/node/node_test.go

pkg/observability/opentelemetry.go

phuhung273 requested a review from a team as a code owner October 29, 2024 14:34

phuhung273 marked this pull request as draft October 29, 2024 14:34

phuhung273 force-pushed the prometheus-export-node branch from 6e910d0 to 91e34ac Compare October 30, 2024 14:05

phuhung273 changed the title ~~[WIP] Prometheus export nodes counter~~ [WIP] IMDS mode: prometheus add nodes counter Nov 1, 2024

phuhung273 changed the title ~~[WIP] IMDS mode: prometheus add nodes counter~~ [WIP] IMDS mode: prometheus add nodes gauge Nov 1, 2024

phuhung273 changed the title ~~[WIP] IMDS mode: prometheus add nodes gauge~~ [WIP] SQS mode: prometheus add nodes gauge Nov 11, 2024

phuhung273 mentioned this pull request Nov 23, 2024

pkg/observability/opentelemetry.go does not have unit test #1088

Closed

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

github-actions bot removed the stale Issues / PRs with no activity label Dec 6, 2024

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

phuhung273 marked this pull request as ready for review December 26, 2024 15:13

phuhung273 changed the title ~~[WIP] SQS mode: prometheus add nodes gauge~~ Prometheus add nodes gauge for SQS mode Dec 26, 2024

Lu-David removed the stale Issues / PRs with no activity label Dec 27, 2024

tiationg-kho self-assigned this Dec 27, 2024

phuhung273 force-pushed the prometheus-export-node branch from b9e1617 to 95fc5a9 Compare January 1, 2025 02:25

Prometheus add nodes gauge for SQS mode

f4ae6de

phuhung273 force-pushed the prometheus-export-node branch from 95fc5a9 to f4ae6de Compare January 1, 2025 02:32

tiationg-kho reviewed Jan 6, 2025

View reviewed changes

phuhung273 added 2 commits January 8, 2025 15:01

add nil check, fix client creation order

a3ba27e

restore client creation order

d139b86

tiationg-kho reviewed Jan 14, 2025

View reviewed changes

cmd/node-termination-handler.go Outdated Show resolved Hide resolved

phuhung273 added 2 commits January 15, 2025 14:20

EnablePrometheus check

aff2a60

add e2e test

316906e

phuhung273 requested a review from tiationg-kho January 16, 2025 14:13

tiationg-kho reviewed Jan 22, 2025

View reviewed changes

LikithaVemulapalli requested changes Jan 23, 2025

View reviewed changes

improve test + logging

a95474e

phuhung273 force-pushed the prometheus-export-node branch from 654d8f1 to a95474e Compare January 26, 2025 16:48

phuhung273 requested review from tiationg-kho and LikithaVemulapalli January 26, 2025 16:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prometheus add nodes gauge for SQS mode #1083

Prometheus add nodes gauge for SQS mode #1083

phuhung273 commented Oct 29, 2024 •

edited

Loading

phuhung273 commented Nov 16, 2024

stevehipwell commented Nov 18, 2024

LikithaVemulapalli commented Nov 18, 2024

github-actions bot commented Dec 4, 2024

phuhung273 commented Dec 6, 2024

github-actions bot commented Dec 22, 2024

tiationg-kho commented Dec 31, 2024

phuhung273 commented Jan 1, 2025

tiationg-kho left a comment

phuhung273 commented Jan 13, 2025

phuhung273 commented Jan 16, 2025

tiationg-kho left a comment

tiationg-kho Jan 22, 2025

LikithaVemulapalli Jan 22, 2025

phuhung273 Jan 26, 2025 •

edited

Loading

tiationg-kho Feb 5, 2025

tiationg-kho Jan 22, 2025

phuhung273 Jan 26, 2025

tiationg-kho Feb 5, 2025

LikithaVemulapalli left a comment

LikithaVemulapalli Jan 22, 2025

Prometheus add nodes gauge for SQS mode #1083

Are you sure you want to change the base?

Prometheus add nodes gauge for SQS mode #1083

Conversation

phuhung273 commented Oct 29, 2024 • edited Loading

phuhung273 commented Nov 16, 2024

stevehipwell commented Nov 18, 2024

LikithaVemulapalli commented Nov 18, 2024

github-actions bot commented Dec 4, 2024

phuhung273 commented Dec 6, 2024

github-actions bot commented Dec 22, 2024

tiationg-kho commented Dec 31, 2024

phuhung273 commented Jan 1, 2025

tiationg-kho left a comment

Choose a reason for hiding this comment

phuhung273 commented Jan 13, 2025

phuhung273 commented Jan 16, 2025

tiationg-kho left a comment

Choose a reason for hiding this comment

tiationg-kho Jan 22, 2025

Choose a reason for hiding this comment

LikithaVemulapalli Jan 22, 2025

Choose a reason for hiding this comment

phuhung273 Jan 26, 2025 • edited Loading

Choose a reason for hiding this comment

tiationg-kho Feb 5, 2025

Choose a reason for hiding this comment

tiationg-kho Jan 22, 2025

Choose a reason for hiding this comment

phuhung273 Jan 26, 2025

Choose a reason for hiding this comment

tiationg-kho Feb 5, 2025

Choose a reason for hiding this comment

LikithaVemulapalli left a comment

Choose a reason for hiding this comment

LikithaVemulapalli Jan 22, 2025

Choose a reason for hiding this comment

phuhung273 commented Oct 29, 2024 •

edited

Loading

phuhung273 Jan 26, 2025 •

edited

Loading