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

Reduce FV verbosity #2814

Merged
merged 2 commits into from
May 18, 2021
Merged

Reduce FV verbosity #2814

merged 2 commits into from
May 18, 2021

Conversation

fasaxc
Copy link
Member

@fasaxc fasaxc commented May 17, 2021

Description

  • Ignore infra logs during stop. (API server tends to log a few pages of "can't reach etcd", which is totally useless.)
  • Log command output inline rather than buffer. (Think the buffered approach was added before we understood we could route logrus to ginkgo's writer in a nice way...)
  • Reduce k8s components log levels; I can't remember the last time we've hit a problem where those logs were useful.

Todos

  • Unit tests (full coverage)
  • Integration tests (delete as appropriate) In plan/Not needed/Done
  • Documentation
  • Backport
  • Release note

Release Note

None required

Ignore infra logs during stop.
Log command output inline rather than buffer.
Copy link
Member

@nelljerram nelljerram left a comment

Choose a reason for hiding this comment

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

Just one small thing...

for _, profile := range profiles.Items {
utils.AddToTestOutput(fmt.Sprintf("%v\n", profile))
log.Info(profile)
Copy link
Member

Choose a reason for hiding this comment

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

I did not know you could call Info with a single object arg....

} else {
log.WithFields(log.Fields{
"command": command,
"args": args}).Infof("Command succeeded:\n%s", output)
Copy link
Member

Choose a reason for hiding this comment

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

I think it's going to be a bit confusing having command=kubectl whatever appended to the last line of the output. Can we revert to logging the command on its own first? It's not a big deal for me, but I think that would be better.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, was just thinking the same myself after trying this out for a bit 👍

Copy link
Member

@nelljerram nelljerram left a comment

Choose a reason for hiding this comment

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

Looks good, thanks.

@fasaxc
Copy link
Member Author

fasaxc commented May 18, 2021

/merge-when-ready

@marvin-tigera
Copy link
Contributor

OK, I will merge the pull request when it's ready, leave the commits as is when I merge it, and leave the branch after I've merged it.

@marvin-tigera marvin-tigera merged commit e7a8ae4 into projectcalico:master May 18, 2021
fasaxc pushed a commit to fasaxc/felix that referenced this pull request May 18, 2021
@fasaxc fasaxc mentioned this pull request May 18, 2021
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants