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

Fix: add init log option for velero controller-runtime manager. #4341

Merged

Conversation

blackpiglet
Copy link
Contributor

@blackpiglet blackpiglet commented Nov 15, 2021

Signed-off-by: Xun Jiang jxun@vmware.com

Thank you for contributing to Velero!

Please add a summary of your change

Does your change fix a particular issue?

Fixes #3737

Please indicate you've done the following:

@blackpiglet blackpiglet force-pushed the 3737-add-logger-for-crd-manager branch from 0d86285 to dacaa20 Compare November 15, 2021 03:54
@blackpiglet blackpiglet added the kind/changelog-not-required PR does not require a user changelog. Often for docs, website, or build changes label Nov 15, 2021
fix for issue vmware-tanzu#3737
add log option for velero controller-runtime manager to log return error in reconcile loop.

Signed-off-by: Xun Jiang jxun@vmware.com
Signed-off-by: jxun <jxun@jxun-a01.vmware.com>
@blackpiglet blackpiglet force-pushed the 3737-add-logger-for-crd-manager branch from dacaa20 to 4a1943f Compare November 15, 2021 06:09
Copy link
Contributor

@jenting jenting left a comment

Choose a reason for hiding this comment

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

In general, LGTM
But it's better to have a changelog if it's fixing an issue.

@blackpiglet
Copy link
Contributor Author

@jenting
Thanks. I will update the changelog.

fix for issue vmware-tanzu#3737
add log option for velero controller-runtime manager to log return error in reconcile loop.

Signed-off-by: Xun Jiang jxun@vmware.com
Signed-off-by: Xun Jiang <jxun@vmware.com>
@blackpiglet blackpiglet force-pushed the 3737-add-logger-for-crd-manager branch from d2e85bb to 7b89950 Compare November 16, 2021 07:37
@blackpiglet blackpiglet removed the kind/changelog-not-required PR does not require a user changelog. Often for docs, website, or build changes label Nov 16, 2021
fix for issue vmware-tanzu#3737
add log option for velero controller-runtime manager to log return error in reconcile loop.

Signed-off-by: Xun Jiang jxun@vmware.com
Signed-off-by: Xun Jiang <jxun@vmware.com>
jenting
jenting previously approved these changes Nov 16, 2021
@ywk253100
Copy link
Contributor

@blackpiglet Could we try to address the issues mentioned here? If we simply set a zap logger for the controller, there will be two kinds of log formats. And we also need to support setting the log level for the new logger. Implement the logger interface with logrus is one solution

@blackpiglet
Copy link
Contributor Author

@ywk253100

I checked whether it's possible to use logrus logger, but it seems the manager only takes logger implements the github.com/go-logr/logr interface. logrus and logr's interface are not compatible.

Logger logr.Logger

@sseago
Copy link
Collaborator

sseago commented Nov 16, 2021

@blackpiglet check out "github.com/bombsimon/logrusr" -- it allows you to implement the go-logr logr.Logger interface with a logrus logger.

@sseago
Copy link
Collaborator

sseago commented Nov 16, 2021

@blackpiglet For the openshift velero plugin used with OADP, we needed to call a shared func that was used both in the plugin (which uses velero's logrus implementation) and in the crane migration tool (which uses go-logr Loggers).

This line calls logrusr.NewLogger(p.Log), passing in a logrus.FieldLogger, and getting a logr.Logger
https://github.com/openshift/openshift-velero-plugin/blob/master/velero-plugins/imagestream/backup.go#L88

The func we're calling there takes a logr.Logger param rather than a logrus.FieldLogger

@blackpiglet
Copy link
Contributor Author

@sseago
Thanks a lot. Your suggestion really saves a lot of my work!
As a matter of fact, I tried to write a log adaptor by myself yesterday. Although it worked, there are still a lot of things I'm clear about in the code.

logrusr is a open source convertor, which can convert logrus logger into logr.
By using logrusr, velero can use exsiting formatted logrus logger, other than introducing zap as a new logger.

Signed-off-by: Xun Jiang <jxun@vmware.com>
@github-actions github-actions bot added the Dependencies Pull requests that update a dependency file label Nov 17, 2021
@ywk253100
Copy link
Contributor

@blackpiglet And one thing we need to do is to remove the similar code like this which will output duplicated logs after we set logger for the controller.

We can do this during the controller refactoring from kubebuilder v2 to v3, but please open a new issue for this if you don't include the change in this PR

@blackpiglet
Copy link
Contributor Author

@ywk253100
Great catch. After adding the new logger, it may introduce duplicated log entries.
I opened a new issue to trace the potential duplicated logs. #4368

@ywk253100 ywk253100 merged commit 11abff4 into vmware-tanzu:main Nov 18, 2021
blackpiglet pushed a commit to blackpiglet/velero that referenced this pull request Nov 22, 2021
Remove duplicated log entries, which are introduced by logger added in controller manager in pull request vmware-tanzu#4341.

Signed-off-by: Xun Jiang <jxun@vmware.com>
blackpiglet pushed a commit to blackpiglet/velero that referenced this pull request Nov 22, 2021
Remove duplicated log entries, which are introduced by logger added in controller manager in pull request vmware-tanzu#4341.
fix issue vmware-tanzu#4368

Signed-off-by: Xun Jiang <jxun@vmware.com>
@blackpiglet blackpiglet deleted the 3737-add-logger-for-crd-manager branch October 15, 2022 04:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Dependencies Pull requests that update a dependency file has-changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

velero backup logs <backup_name> hangs forever - More debug log needed in velero server.
4 participants