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.

Sign up for GitHub

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

Jump to bottom

Zap logger #824

Merged
merged 1 commit into from
Mar 23, 2020
Merged

Zap logger #824

merged 1 commit into from
Mar 23, 2020

Conversation

nithu0115
Copy link
Contributor

@nithu0115 nithu0115 commented Jan 31, 2020 •
edited
Loading

Issue #, if available: #511

Description of changes:
Adding zap logger

  • Added zap logger
  • Parameterize config
  • Update Readme
  • Unit tests

Outputs

Screen Shot 2020-03-16 at 2 11 07 PM

Screen Shot 2020-03-16 at 2 19 55 PM

Screen Shot 2020-03-16 at 2 19 23 PM

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

@nithu0115 nithu0115 requested review from mogren and jaypipes January 31, 2020 08:37
@nithu0115 nithu0115 force-pushed the master branch 3 times, most recently from 5aba5b9 to 10aafc6 Compare January 31, 2020 20:10
mogren
mogren suggested changes Jan 31, 2020
View reviewed changes
Copy link
Contributor

@mogren mogren left a comment

Choose a reason for hiding this comment

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

Nice work! Added a few comments so far.

Could you add some sample output in the overview as well?

@nithu0115 nithu0115 force-pushed the master branch 2 times, most recently from fad5934 to 43eefe7 Compare February 3, 2020 15:17
@nithu0115 nithu0115 requested a review from mogren February 3, 2020 15:18
jaypipes
jaypipes suggested changes Feb 4, 2020
View reviewed changes
Copy link
Contributor

@jaypipes jaypipes left a comment

Choose a reason for hiding this comment

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

Stay with this, @nithu0115. A number of things to address but it's getting there. :)

@nithu0115 nithu0115 force-pushed the master branch 7 times, most recently from 0d38b18 to beb584c Compare February 13, 2020 19:25
jaypipes
jaypipes suggested changes Feb 20, 2020
View reviewed changes
Copy link
Contributor

@jaypipes jaypipes left a comment

Choose a reason for hiding this comment

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

Hi @nithu0115, still some work left to do on this one...

nithu0115
nithu0115 commented Feb 25, 2020
View reviewed changes
Copy link
Contributor Author

@nithu0115 nithu0115 left a comment

Choose a reason for hiding this comment

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

Thanks for the review @jaypipes and sorry for the delay. I was sick 🤒 I will work on the feedback and update you.

@nithu0115 nithu0115 force-pushed the master branch 6 times, most recently from 3bb2821 to d017743 Compare March 16, 2020 19:09
@nithu0115 nithu0115 requested review from jaypipes and mogren March 16, 2020 19:16
@nithu0115 nithu0115 closed this Mar 17, 2020
@nithu0115 nithu0115 reopened this Mar 17, 2020
mogren
mogren suggested changes Mar 18, 2020
View reviewed changes
Copy link
Contributor

@mogren mogren left a comment

Choose a reason for hiding this comment

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

Nice! Thanks a lot for all this works. Some comments... :)

cmd/aws-k8s-agent/main.go Outdated
const (
defaultLogFilePath = "/host/var/log/aws-routed-eni/ipamd.log"
)
const binaryName = "L-IPamD"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think ipamd is a fine abbreviation of "IP Address Management Daemon". The Linux part is not needed, and neither the special casing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Heh, TIL that the "L-" stands for "Linux"...

Agreed, though, that "ipamd" is a better and more Linux-appropriate daemon name.

If you make this change in a future PR, however, please remember to update https://github.com/awslabs/amazon-eks-ami/blob/master/log-collector-script/linux/eks-log-collector.sh

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think ipamd is a fine abbreviation of "IP Address Management Daemon". The Linux part is not needed, and neither the special casing.

I wanted it to be consistent with the log message. I can change it to "ipamd".

jaypipes
jaypipes suggested changes Mar 18, 2020
View reviewed changes
Copy link
Contributor

@jaypipes jaypipes left a comment

Choose a reason for hiding this comment

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

Only required change here is the need to restore the MTU config default check. Otherwise, looks excellent, thanks!

@nithu0115 nithu0115 force-pushed the master branch 4 times, most recently from 596375b to f0629f1 Compare March 18, 2020 23:59
@nithu0115
Copy link
Contributor Author

Updated, rebased as per your comments @jaypipes @mogren. I am planning to run a benchmark test and will post the results here.

mogren
mogren suggested changes Mar 19, 2020
View reviewed changes
Copy link
Contributor

@mogren mogren left a comment

Choose a reason for hiding this comment

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

Just some tiny things left, almost there!

pkg/k8sapi/discovery.go
@@ -256,7 +254,7 @@ func (d *Controller) handlePodUpdate(key string) error {
d.workerPodsLock.Lock()
defer d.workerPodsLock.Unlock()

log.Tracef("Update for pod %s: %+v, %+v", podName, pod.Status, pod.Spec)
log.Debugf("Update for pod %s: %+v, %+v", podName, pod.Status, pod.Spec)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this might become too verbose, since it is printing the full pod.Spec on every pod update on the node. The info level log on line 267 is probably enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would like to leave it as is for now as this will provide us some better info while troubleshooting.

@nithu0115 nithu0115 force-pushed the master branch 5 times, most recently from cfd3162 to a00483a Compare March 19, 2020 03:19
nithu0115
nithu0115 commented Mar 19, 2020
View reviewed changes
Copy link
Contributor Author

@nithu0115 nithu0115 left a comment

Choose a reason for hiding this comment

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

Rebased, updated as per comments.

@nithu0115 nithu0115 requested review from mogren and jaypipes March 19, 2020 16:33
@nithu0115
Copy link
Contributor Author

@mogren @jaypipes, the original 602401143452.dkr.ecr.us-west-2.amazonaws.com/amazon-k8s-cni:v1.6.0 compressed size is (linux/amd64) 106.27 MB and the new image which includes this commit is 90.19 MB

@jaypipes
Copy link
Contributor

@mogren @jaypipes, the original 602401143452.dkr.ecr.us-west-2.amazonaws.com/amazon-k8s-cni:v1.6.0 compressed size is (linux/amd64) 106.27 MB and the new image which includes this commit is 90.19 MB

So, around a 15% reduction in image size. Awesome.

jaypipes
jaypipes approved these changes Mar 22, 2020
View reviewed changes
Copy link
Contributor

@jaypipes jaypipes left a comment

Choose a reason for hiding this comment

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

I'm good with this. A future commit can get rid of the DefaultLogger module-level log variables, but I think getting the number of logging libraries down to just one (and a tested, vetted one that is maintained) is worthwhile.

@nithu0115 nithu0115 force-pushed the master branch 3 times, most recently from d5ef59b to 4387b7c Compare March 23, 2020 16:24
mogren
mogren reviewed Mar 23, 2020
View reviewed changes
@mogren mogren merged commit 8ffcd8a into aws:master Mar 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mogren mogren mogren approved these changes

@jaypipes jaypipes jaypipes approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nithu0115 @jaypipes @mogren