Add '--logging-format' flag to kube-controller-manager #91521

SataQiu · 2020-05-28T08:58:49Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
Add --logging-format flag to kube-controller-manager.

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

kube-controller-manager: add '--logging-format' flag to support structured logging

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

NONE

Signed-off-by: SataQiu <1527062125@qq.com>

SataQiu · 2020-05-28T08:59:27Z

/cc @serathius

serathius · 2020-05-28T09:58:04Z

/retest

serathius · 2020-05-28T09:58:44Z

/lgtm
/assign @sttts

cheftako · 2020-05-28T20:11:31Z

/assign @cheftako
/cc @cici37

cheftako · 2020-05-29T16:50:33Z

I have some of the same concerns as #91501 (comment).

Also a bit confused about the usage. It seems to be set up for dynamic configuration (use of locks etc on Set and Register and the fact that they are public) but the validate does not use locks and will only be checked at start up. This seems prone to race conditions (except for things setup in the init setup).

serathius · 2020-06-08T12:47:42Z

Hey @cheftako
We are working on implementing json format, hope to get it merged soon.

As for usage, flag was not implemented to be thread safe, internal register is, as we use it in a global variable. It was not implemented to support dynamic configuration, not I think we should support it. I'm not familiar with it, so I could be wrong.

I don't think there is a sense to make this flag thread safe. WDYT?

serathius · 2020-06-16T09:05:49Z

Json format was added. #91490
PR to update help listing available options is in flight. #92177

I think we can proceed with this PR. @sttts PTAL

SataQiu · 2020-06-16T12:53:42Z

/retest

cheftako · 2020-06-16T16:31:49Z

As for usage, flag was not implemented to be thread safe, internal register is, as we use it in a global variable. It was not implemented to support dynamic configuration, not I think we should support it. I'm not familiar with it, so I could be wrong.

I don't think there is a sense to make this flag thread safe. WDYT?

I agree. I understand that your usage is not intended to be thread safe. However the logs library seems confused on this. It has a Register() method, whish is public, uses lock to ensure safe addition on new loggers and the documentation that does not mention any restrictions on adding logger methods. To me this implies new loggers can be added at any time. However the init() and Validate() methods in it suggest, as you say, that it does not support dynamic configuration.

Lets take a look once we have Json format.

serathius · 2020-06-20T19:43:53Z

PR updating flag to inform about JSON foramt was merged. Can go move forward with this one? @cheftako

cheftako · 2020-06-22T15:59:02Z

/lgtm

cheftako · 2020-06-22T16:00:37Z

/approve

cheftako · 2020-06-22T16:01:05Z

/priority important-soon

cheftako · 2020-06-22T16:01:28Z

/sig api-machinery

serathius · 2020-06-22T16:53:38Z

/hold
Reason same as #91522 (comment)

serathius · 2020-07-01T14:35:18Z

/unhold
Flag validation was implemented, #92394 (comment)
/assign @sttts

serathius · 2020-07-01T14:35:44Z

/sig instrumentation

serathius · 2020-07-01T14:36:41Z

/milestone v1.19

sttts · 2020-07-01T14:41:58Z

cmd/kube-controller-manager/app/options/options.go

@@ -249,6 +252,7 @@ func (s *KubeControllerManagerOptions) Flags(allControllers []string, disabledBy
 	s.SAController.AddFlags(fss.FlagSet("serviceaccount controller"))
 	s.TTLAfterFinishedController.AddFlags(fss.FlagSet("ttl-after-finished controller"))
 	s.Metrics.AddFlags(fss.FlagSet("metrics"))
+	s.Logs.AddFlags(fss.FlagSet("logs"))


Does this fit better into generic? Is it just one flag? Where are the other klog flags?

Other klog flags are Global flags. I think this flag should be in same flagSet logs as in other control plane components.

https://gist.github.com/fd93b5d6044d7c256d2e70fcabf503ae does it. All logging flags will be under the logs section.

My gist does not move log-flush-frequency yet. That's also defined in component-base/logs and has to go to the named flag set.

As the proposed change is bigger (removes the init func behaviour of the package), I am fine to postpone this to 1.20. But would like to see a PR started with the gist, tagged with the relevant people before approving this PR here.

Done #92707

We agreed to keep this new logs section for now, and move klog flags over in a follow-up (in 1.19 or 1.20 if we miss code freeze).

sttts · 2020-07-01T18:15:10Z

/approve

k8s-ci-robot · 2020-07-01T18:16:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cheftako, SataQiu, sttts

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:

~~cmd/kube-controller-manager/OWNERS~~ [sttts]

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

SataQiu · 2020-07-02T02:08:08Z

/test pull-kubernetes-e2e-gce-100-performance

add '--logging-format' flag to kube-controller-manager

17f3cd4

Signed-off-by: SataQiu <1527062125@qq.com>

k8s-ci-robot requested review from errordeveloper and sjenning May 28, 2020 08:59

k8s-ci-robot requested a review from serathius May 28, 2020 08:59

serathius mentioned this pull request May 28, 2020

Structured logging kubernetes/enhancements#1602

Open

39 tasks

k8s-ci-robot assigned sttts and serathius May 28, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 28, 2020

k8s-ci-robot assigned cheftako May 28, 2020

k8s-ci-robot requested a review from cici37 May 28, 2020 20:11

k8s-ci-robot added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Jun 22, 2020

k8s-ci-robot removed the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Jun 22, 2020

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 22, 2020

serathius mentioned this pull request Jun 29, 2020

add --logging-format flag to kubelet #91532

Merged

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 1, 2020

k8s-ci-robot added sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jul 1, 2020

k8s-ci-robot added this to the v1.19 milestone Jul 1, 2020

sttts reviewed Jul 1, 2020

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 1, 2020

k8s-ci-robot merged commit e7a949f into kubernetes:master Jul 2, 2020

github-actions bot mentioned this pull request Jul 8, 2020

Week Ending July 5, 2020 dev-obs/actus#190

Open

ari-becker mentioned this pull request May 23, 2021

Support structured logging ('--logging-format=json') kubernetes/kops#11576

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add '--logging-format' flag to kube-controller-manager #91521

Add '--logging-format' flag to kube-controller-manager #91521

SataQiu commented May 28, 2020 •

edited

Loading

SataQiu commented May 28, 2020

serathius commented May 28, 2020

serathius commented May 28, 2020

cheftako commented May 28, 2020

cheftako commented May 29, 2020 •

edited

Loading

serathius commented Jun 8, 2020

serathius commented Jun 16, 2020 •

edited

Loading

SataQiu commented Jun 16, 2020

cheftako commented Jun 16, 2020

serathius commented Jun 20, 2020

cheftako commented Jun 22, 2020

cheftako commented Jun 22, 2020

cheftako commented Jun 22, 2020

cheftako commented Jun 22, 2020

serathius commented Jun 22, 2020

serathius commented Jul 1, 2020

serathius commented Jul 1, 2020

serathius commented Jul 1, 2020

sttts Jul 1, 2020

serathius Jul 1, 2020 •

edited

Loading

sttts Jul 1, 2020

sttts Jul 1, 2020

sttts Jul 1, 2020

serathius Jul 1, 2020

sttts Jul 1, 2020

sttts commented Jul 1, 2020

k8s-ci-robot commented Jul 1, 2020

SataQiu commented Jul 2, 2020

Add '--logging-format' flag to kube-controller-manager #91521

Add '--logging-format' flag to kube-controller-manager #91521

Conversation

SataQiu commented May 28, 2020 • edited Loading

SataQiu commented May 28, 2020

serathius commented May 28, 2020

serathius commented May 28, 2020

cheftako commented May 28, 2020

cheftako commented May 29, 2020 • edited Loading

serathius commented Jun 8, 2020

serathius commented Jun 16, 2020 • edited Loading

SataQiu commented Jun 16, 2020

cheftako commented Jun 16, 2020

serathius commented Jun 20, 2020

cheftako commented Jun 22, 2020

cheftako commented Jun 22, 2020

cheftako commented Jun 22, 2020

cheftako commented Jun 22, 2020

serathius commented Jun 22, 2020

serathius commented Jul 1, 2020

serathius commented Jul 1, 2020

serathius commented Jul 1, 2020

sttts Jul 1, 2020

Choose a reason for hiding this comment

serathius Jul 1, 2020 • edited Loading

Choose a reason for hiding this comment

sttts Jul 1, 2020

Choose a reason for hiding this comment

sttts Jul 1, 2020

Choose a reason for hiding this comment

sttts Jul 1, 2020

Choose a reason for hiding this comment

serathius Jul 1, 2020

Choose a reason for hiding this comment

sttts Jul 1, 2020

Choose a reason for hiding this comment

sttts commented Jul 1, 2020

k8s-ci-robot commented Jul 1, 2020

SataQiu commented Jul 2, 2020

SataQiu commented May 28, 2020 •

edited

Loading

cheftako commented May 29, 2020 •

edited

Loading

serathius commented Jun 16, 2020 •

edited

Loading

serathius Jul 1, 2020 •

edited

Loading