Support dynamiclly set glog.logging.verbosity

[hzxuzhonghu]

Support dynamically set glog logging level, which is convenient for debug.

Release note:

Expose `/debug/flags/v` to allow dynamically set glog logging level, if want to change glog level to 3, you only have to send a PUT request with like `curl -X PUT http://127.0.0.1:8080/debug/flags/v -d "3"`.

[hzxuzhonghu]

/assign sttts

[sttts]

is this protected?

[hzxuzhonghu]

Yes,

func (l *Level) Set(value string) error {
	v, err := strconv.Atoi(value)
	if err != nil {
		return err
	}
	logging.mu.Lock()
	defer logging.mu.Unlock()
	logging.setVState(Level(v), logging.vmodule.filter, false)
	return nil
}

[sttts]

I mean via authn/z. This also needs a test checking that authn/z is applied.

[hzxuzhonghu]

Yes, it is like /debug/pprof, protected by authn/z .

This also needs a test checking that authn/z is applied.

I have to look into it, never wrote this case before. But to make sure
this really acceptable.

[hzxuzhonghu]

Done, add a case test auth

[dims]

@hzxuzhonghu Can we please query the current level using HTTP GET too?

[hzxuzhonghu]

@dims Sorry, we cann't. because glog do not expose a method to query, if we want to do this , I think we can cache the level after first set. But we have no way to query without firstly set.

[dims]

Ack thanks @hzxuzhonghu

[sttts]

There were discussion to have a limited ComponentConfig for apiservers. I wonder whether this shouldn't go into a dynamic reconfiguration mechanism based on that.

[hzxuzhonghu]

I wonder whether this shouldn't go into a dynamic reconfiguration mechanism based on that.

I met many cases, like in production env master exception. Developers want to improve the log level, but have no way.

[sttts]

I met many cases, like in production env master exception. Developers want to improve the log level, but have no way.

I agree. But then we have other values operators want to change on-the-fly: max-inflight requests for example.

[hzxuzhonghu]

But then we have other values operators want to change on-the-fly: max-inflight requests for example

Yeah, maybe max-inflight is one flag we want to change. But mostly we want to improve log level is when we meet exception(may cause fatal), it is of higher priority. Many other languages or golang logging lib support dynamically changing level.

[hzxuzhonghu]

fixed test failure

[hzxuzhonghu]

/test pull-kubernetes-integration

[hzxuzhonghu]

/test pull-kubernetes-e2e-gce

[dims]

/uncc @dims

[sttts]

Reasonable usecase. I tend to be fine with this. But would like to hear some more opinions whether we want to add those special purpose endpoints.

/assign @deads2k

Does this also apply to the controller manager (and scheduler when the options PR merges) ?

[hzxuzhonghu]

Does this also apply to the controller manager (and scheduler when the options PR merges) ?

No, but we can install this handler later in https://github.com/kubernetes/kubernetes/blob/master/cmd/controller-manager/app/serve.go#L49 for controller-manager and https://github.com/kubernetes/kubernetes/blob/master/cmd/kube-scheduler/app/server.go#L525 for kube-scheduler.

[deads2k]

I like the idea, but I think we should have a versioned endpoint for it and it should plan ahead to set other glog fields like vmodule.

Dynamically setting log levels helps when you're live troubleshooting a hard to reproduce scenario. It doesn't come up often, but when you need it there are few substitutes. It is different than "normal" config in that restarting often destroys the thing you wanted to watch and because loglevel doesn't overtly change capability (latency sensitive behavior may be affected though)

@liggitt we were just talking about this
@lavalamp we talked about this a while back.
@kubernetes/sig-api-machinery-misc

[lavalamp]

• Disagree about making this versioned (it's for human debugging, not for tools to use). I do not want to offer this as an api with a contract that I have to support forever.
• Binaries in Google all offer a web page that lets you change flag values on the fly (not guaranteed to work, you need to know what you're doing and how the flag is used), so even more broadly scoped than just glog flags.

[hzxuzhonghu]

@lavalamp Any idea about which flags should support modification on the fly? And want to know how you make this in Google?

[sttts]

How about putting them under /debug, e.g. /debug/flags/v. For the endpoints there we don't give any compatibility guarantee. They are what Golang provides to us. Also this style would allow to have more similar flags in the future, aligned with the flag names.

[hzxuzhonghu]

How about putting them under /debug, e.g. /debug/flags/v. For the endpoints there we don't give any compatibility guarantee. They are what Golang provides to us. Also this style would allow to have more similar flags in the future, aligned with the flag names.

agree

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, hzxuzhonghu, lavalamp

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:

• staging/src/k8s.io/apiserver/OWNERS [deads2k,lavalamp]

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

[lavalamp]

/hold cancel

[fejta-bot]

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

[fejta-bot]

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

[k8s-ci-robot]

@hzxuzhonghu: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-kubemark-e2e-gce-big	73a22b2	link	/test pull-kubernetes-kubemark-e2e-gce-big

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

[fejta-bot]

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

[hzxuzhonghu]

@deads2k @lavalamp @sttts for milestone, so we can make it into v1.11.

[m1093782566]

@hzxuzhonghu I assume you want to get this PR in.

/milestone v1.11
/status approved-for-milestone

[k8s-github-robot]

[MILESTONENOTIFIER] Milestone Pull Request Labels Incomplete

@deads2k @hzxuzhonghu @lavalamp @sttts

Action required: This pull request requires label changes. If the required changes are not made within 3 days, the pull request will be moved out of the v1.11 milestone.

kind: Must specify exactly one of kind/bug, kind/cleanup or kind/feature.
priority: Must specify exactly one of priority/critical-urgent, priority/important-longterm or priority/important-soon.

Help

• Additional instructions
• Commands for setting labels

[hzxuzhonghu]

Thanks @m1093782566

[k8s-github-robot]

Automatic merge from submit-queue (batch tested with PRs 59938, 63777, 64577, 63999, 64431). If you want to cherry-pick this change to another branch, please follow the instructions here.

[tallclair]

Why is this gated on profiling being enabled?

[hzxuzhonghu]

That's @lavalamp suggested: #63777 (comment)

[lavalamp]

So that admins must deliberately enable them. It was convenient to reuse the same flag.

…

On Mon, Aug 13, 2018, 5:59 PM Tim Allclair (St. Clair) < ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In staging/src/k8s.io/apiserver/pkg/server/config.go <#63777 (comment)> : > @@ -575,6 +576,16 @@ func installAPI(s *GenericAPIServer, c *Config) { if c.EnableContentionProfiling { goruntime.SetBlockProfileRate(1) } + // so far, only logging related endpoints are considered valid to add for these debug flags. + routes.DebugFlags{}.Install(s.Handler.NonGoRestfulMux, "v", routes.StringFlagPutHandler( Why is this gated on profiling being enabled? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#63777 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAnglj36huNNWkQ8fWaR3PHG815HJZrkks5uQiDUgaJpZM4T9W2u> .

[tallclair]

Makes sense to flag-gate it, I'm just questioning the choice to tie it to profiling, as they seem somewhat unrelated.