Skip to content
This repository has been archived by the owner on Apr 25, 2023. It is now read-only.

Set default log level for kubefed controller manager. #1245

Merged

Conversation

RainbowMango
Copy link
Contributor

@RainbowMango RainbowMango commented Jul 8, 2020

What this PR does / why we need it:
Set default log level to 2 for the controller manager.
With this level, more useful logs would be printed and without much noise.

Which issue(s) this PR fixes :
Fixes #1246

Special notes for your reviewer:

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 8, 2020
@k8s-ci-robot k8s-ci-robot requested review from font and hectorj2f July 8, 2020 11:46
@jimmidyson
Copy link
Contributor

/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 8, 2020
@jimmidyson
Copy link
Contributor

/approve cancel

On second thoughts, can you please make this configurable via the chart, defaulting to 3 as you suggest is reasonable though imo.

@k8s-ci-robot k8s-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 8, 2020
@RainbowMango
Copy link
Contributor Author

On second thoughts, can you please make this configurable via the chart

That would be better I guess, If you don't insist I'd like to try this approach next time. I was thinking upgrade helm to V3 too.

@RainbowMango
Copy link
Contributor Author

I'm debugging an issue and found these hidden logs would help, so just sent this simple PR.

@jimmidyson
Copy link
Contributor

@RainbowMango Could you share what logs you found helpful from changing to --v=3? Are these logs from kubefed code or dependencies? Should those logs be made V1 instead?

I'm happy to merge this as is to, as long as we have issues open for future improvements.

@RainbowMango RainbowMango force-pushed the pr_set_default_log_level branch from 3806f2f to 5cb879d Compare July 9, 2020 02:01
@k8s-ci-robot k8s-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 9, 2020
@RainbowMango
Copy link
Contributor Author

Could you share what logs you found helpful from changing to --v=3?

Just pushed a new commit to change the log level to --v=2, because the following log from client-go will keep printing with --v=3.

I0709 01:33:19.974612       1 request.go:557] Throttling request took 109.98087ms, request: PUT:https://10.96.0.1:443/apis/types.kubefed.io/v1beta1/namespaces/test-namespace/federateddeployments/test-deployment/status
I0709 01:33:20.174552       1 request.go:557] Throttling request took 189.23737ms, request: PUT:https://10.96.0.1:443/apis/types.kubefed.io/v1beta1/namespaces/test-namespace/federateddeployments/test-deployment/status

These kinds of logs would show why a controller not working in abnormal cases like #1241.
https://github.com/kubernetes-sigs/kubefed/blob/bf67d02369e9b2d93281f8224747b94afab3170e/pkg/controller/sync/controller.go#L203

Are these logs from kubefed code or dependencies?

Actually, I tried the log level from 8 to 1 one by one, with --v=4 there would be a bunch of logs from dependencies that quite noisy. And with --v=2 or --v=3, the logs mostly dumped by Kubefed.

I'm happy to merge this as is to, as long as we have issues open for future improvements.

Here is the issue. #1246

@RainbowMango
Copy link
Contributor Author

@jimmidyson Can this PR moving forward now?

And, about how to make this configurable via the chart, could you share your idea?
For now, kubefed-controller-manager and kubefed-admission-webhook are sharing the same variables declared in kubefed-values, but they are using different log level.

@hectorj2f
Copy link
Contributor

@RainbowMango The idea of making configurable is to define a property in the chart that sets the value to 2 by default or to another value. That way anyone can re-define this value when desired. I think this is relevant otherwise it is a bit opinionated to hardcode it to v=2

@@ -22,6 +22,7 @@ spec:
containers:
- command:
- /hyperfed/controller-manager
- "--v=2"
Copy link
Contributor

Choose a reason for hiding this comment

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

add a command property to the values.yaml for the controller so this verbosity level can be re-defined.

@RainbowMango RainbowMango force-pushed the pr_set_default_log_level branch from 5cb879d to 48f0fb2 Compare July 17, 2020 01:44
@RainbowMango
Copy link
Contributor Author

@hectorj2f Thanks. just pushed a new commit, could you please take a look again?

@hectorj2f
Copy link
Contributor

It works for me, I believe we will keep the log level of the webhook hardcores to v=8. We can improve it in the future although ideally we should expose it for both.

@hectorj2f
Copy link
Contributor

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hectorj2f, RainbowMango

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 17, 2020
@hectorj2f
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 17, 2020
@k8s-ci-robot k8s-ci-robot merged commit 5bf41ff into kubernetes-retired:master Jul 17, 2020
@RainbowMango RainbowMango deleted the pr_set_default_log_level branch July 17, 2020 10:04
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make component log level configurable
4 participants