fix undefined log levels #402

bwagner5 · 2021-04-08T20:38:56Z

Issue #, if available:
N/A

Description of changes:
Fix undefined log levels:

BEFORE:

2021/04/02 15:44:08 ??? Trying to get token from IMDSv2
2021/04/02 15:44:15 ??? Unable to retrieve an IMDSv2 token, continuing with IMDSv1 error="Put \"http://169.254.169.254/latest/api/token\": context deadline exceeded (Client.Timeout exceeded while awaiting headers)"
2021/04/02 15:44:18 ??? Request failed. Attempts remaining: 2
2021/04/02 15:44:18 ??? Sleep for 2.973889705s seconds
2021/04/02 15:44:23 ??? Request failed. Attempts remaining: 1
2021/04/02 15:44:23 ??? Sleep for 8.84038228s seconds
2021/04/02 15:44:34 ??? Unable to fetch metadata from IMDS error="Unable to parse metadata response: Unable to get a response from IMDS: Get \"http://169.254.169.254/latest/dynamic/instance-identity/document\": context deadline exceeded (Client.Timeout exceeded while awaiting headers)"
2021/04/02 15:44:34 ??? aws-node-termination-handler arguments:

AFTER:

build/node-termination-handler --dry-run --node-name hi
2021/04/08 15:37:37 ERR Unable to retrieve an IMDSv2 token, continuing with IMDSv1 error="Received an http status code 403"
2021/04/08 15:37:37 ERR Unable to fetch metadata from IMDS error="Metadata request received http status code: 403"
2021/04/08 15:37:37 INF aws-node-termination-handler arguments:
	dry-run: true,
	node-name: hi,
	metadata-url: http://169.254.169.254,
	kubernetes-service-host: ,
	kubernetes-service-port: ,
	delete-local-data: true,
	ignore-daemon-sets: true,
	pod-termination-grace-period: -1,
	node-termination-grace-period: 120,
	enable-scheduled-event-draining: false,
	enable-spot-interruption-draining: true,
	enable-sqs-termination-draining: false,
	enable-rebalance-monitoring: false,
	metadata-tries: 3,
	cordon-only: false,
	taint-node: false,
	json-logging: false,
	log-level: INFO,
	webhook-proxy: ,
	webhook-headers: <not-displayed>,
	webhook-url: ,
	webhook-template: <not-displayed>,
	uptime-from-file: ,
	enable-prometheus-server: false,
	prometheus-server-port: 9092,
	aws-region: us-east-1,
	queue-url: ,
	check-asg-tag-before-draining: true,
	managed-asg-tag: aws-node-termination-handler/managed,
	aws-endpoint: ,

2021/04/08 15:37:37 INF Started watching for interruption events
2021/04/08 15:37:37 INF Kubernetes AWS Node Termination Handler has started successfully!
2021/04/08 15:37:37 INF Started watching for event cancellations
2021/04/08 15:37:37 INF Started monitoring for events event_type=SPOT_ITN
2021/04/08 15:37:39 ERR Unable to retrieve an IMDSv2 token, continuing with IMDSv1 error="Received an http status code 403"
2021/04/08 15:37:39 WRN There was a problem monitoring for events error="There was a problem checking for spot ITNs: Metadata request received http status code: 403" event_type=SPOT_ITN
2021/04/08 15:37:41 ERR Unable to retrieve an IMDSv2 token, continuing with IMDSv1 error="Received an http status code 403"
2021/04/08 15:37:41 WRN There was a problem monitoring for events error="There was a problem checking for spot ITNs: Metadata request received http status code: 403" event_type=SPOT_ITN
2021/04/08 15:37:43 ERR Unable to retrieve an IMDSv2 token, continuing with IMDSv1 error="Received an http status code 403"
2021/04/08 15:37:43 WRN There was a problem monitoring for events error="There was a problem checking for spot ITNs: Metadata request received http status code: 403" event_type=SPOT_ITN
2021/04/08 15:37:45 ERR Unable to retrieve an IMDSv2 token, continuing with IMDSv1 error="Received an http status code 403"
2021/04/08 15:37:45 WRN There was a problem monitoring for events error="There was a problem checking for spot ITNs: Metadata request received http status code: 403" event_type=SPOT_ITN
2021/04/08 15:37:45 WRN Stopping NTH - Duplicate Error Threshold hit.
panic: There was a problem checking for spot ITNs: Metadata request received http status code: 403

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

brycahta

Looks good! Had some minor comments/questions

cmd/node-termination-handler.go

brycahta · 2021-04-08T20:59:40Z

pkg/ec2metadata/ec2metadata.go

 	err = json.NewDecoder(strings.NewReader(identityDoc)).Decode(&metadata)
 	if err != nil {
-		log.Log().Msg("Unable to fetch instance identity document from ec2 metadata")
+		log.Warn().Msg("Unable to fetch instance identity document from ec2 metadata")


log.Err(err) ?

Both comments are nit for sure, but thinking if there's a log within if err != nil then it should be error for consistency

I don't think we can make a strict rule like that. We have a fallback for this error, so I think it can stay a Warn.

Not sure I agree that all loggin within err != nil should be error level, but we should come up with a consistent understanding of what these levels mean

Agreed the rule can't be that strict, but if having a fallback doesn't classify as error, then what about:

ERR Unable to retrieve an IMDSv2 token, continuing with IMDSv1

v1 is the fallback in this case? Are there any formal docs for log-levels similar to semantic versioning or something? Agreed we need some kinda consistency

yeah I changed that one to WARN

I don't think there's anything formal for log levels... I think we probably log way too much in NTH, so maybe we should consider what actually makes sense to log so that someone can view the logs and be confident things are working and have enough information to debug certain error scenarios.

pkg/node/node.go

pkg/webhook/webhook.go

haugenj · 2021-04-08T21:11:54Z

cmd/node-termination-handler.go

 					}
 					if duplicateErrCount >= duplicateErrThreshold {
-						log.Log().Msg("Stopping NTH - Duplicate Error Threshold hit.")
+						log.Warn().Msg("Stopping NTH - Duplicate Error Threshold hit.")


log.panic since we're panicking ?

We could, but IMHO it's clearer when they're separate. I don't think I'd expect a log to panic if I was reading and I didn't know of that weirdness.

cmd/node-termination-handler.go

haugenj · 2021-04-08T21:20:01Z

pkg/ec2metadata/ec2metadata.go

 	err = json.NewDecoder(strings.NewReader(identityDoc)).Decode(&metadata)
 	if err != nil {
-		log.Log().Msg("Unable to fetch instance identity document from ec2 metadata")
+		log.Warn().Msg("Unable to fetch instance identity document from ec2 metadata")


Not sure I agree that all loggin within err != nil should be error level, but we should come up with a consistent understanding of what these levels mean

pkg/ec2metadata/ec2metadata.go

brycahta

🚀

bwagner5 requested review from brycahta and haugenj April 8, 2021 20:39

fix undefined log levels

dbef3ef

bwagner5 force-pushed the fix-log-levels branch from e4b25bd to dbef3ef Compare April 8, 2021 20:47

brycahta suggested changes Apr 8, 2021

View reviewed changes

bwagner5 added 2 commits April 8, 2021 16:10

fix flaky test

98cfbf8

fix wrong levels in some places

b13b2d5

haugenj reviewed Apr 8, 2021

View reviewed changes

bwagner5 requested review from brycahta and haugenj April 8, 2021 21:22

haugenj reviewed Apr 8, 2021

View reviewed changes

pkg/ec2metadata/ec2metadata.go Outdated Show resolved Hide resolved

error to warn ec2 metadata

1d74009

brycahta approved these changes Apr 8, 2021

View reviewed changes

bwagner5 merged commit 4a37023 into aws:main Apr 12, 2021

ec2-bot mentioned this pull request Apr 12, 2021

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

Merged

fix undefined log levels #402

fix undefined log levels #402

Uh oh!

Conversation

bwagner5 commented Apr 8, 2021

Uh oh!

brycahta left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

brycahta left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants