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

Check if options returning an error #78775

Merged

Conversation

@johscheuer
Copy link
Member

commented Jun 6, 2019

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change
/kind bug

/kind cleanup

/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:
This PR fixes that kube-proxy don't log an nil error after a successful run.

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

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

kube-proxy --cleanup will return the correct exit code if the cleanup was successful
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2019

Hi @johscheuer. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@johnbelamaric

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2019

/ok-to-test
/priority important-longterm

@johscheuer

This comment has been minimized.

Copy link
Member Author

commented Jun 6, 2019

So the --cleanup case works now:

sudo /tmp/kube-proxy --cleanup
W0606 20:51:18.343204   18959 server.go:216] WARNING: all flags other than --config, --write-config-to, and --cleanup are deprecated. Please begin using a config file ASAP.

but now I get an Stacktrace when an error is happening:

sudo /tmp/kube-proxy
W0606 20:51:33.797272   19263 server.go:216] WARNING: all flags other than --config, --write-config-to, and --cleanup are deprecated. Please begin using a config file ASAP.
I0606 20:51:33.812251   19263 server.go:502] Neither kubeconfig file nor master URL was specified. Falling back to in-cluster config.
F0606 20:51:33.812290   19263 server.go:451] unable to load in-cluster configuration, KUBERNETES_SERVICE_HOST and KUBERNETES_SERVICE_PORT must be defined
goroutine 1 [running]:
k8s.io/kubernetes/vendor/k8s.io/klog.stacks(0x2744101, 0x3, 0xc0002fc000, 0x9a)
	/Users/jscheuermann/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/klog/klog.go:900 +0xb1
k8s.io/kubernetes/vendor/k8s.io/klog.(*loggingT).output(0x27441a0, 0xc000000003, 0xc00019f9d0, 0x26a8363, 0x9, 0x1c3, 0x0)
	/Users/jscheuermann/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/klog/klog.go:815 +0xe6
k8s.io/kubernetes/vendor/k8s.io/klog.(*loggingT).printDepth(0x27441a0, 0x3, 0x1, 0xc00057bd28, 0x1, 0x1)
	/Users/jscheuermann/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/klog/klog.go:718 +0x12b
k8s.io/kubernetes/vendor/k8s.io/klog.(*loggingT).print(...)
	/Users/jscheuermann/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/klog/klog.go:709
k8s.io/kubernetes/vendor/k8s.io/klog.Fatal(...)
	/Users/jscheuermann/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/klog/klog.go:1289
k8s.io/kubernetes/cmd/kube-proxy/app.NewProxyCommand.func1(0xc0003b4000, 0x2761660, 0x0, 0x0)
	/Users/jscheuermann/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/cmd/kube-proxy/app/server.go:451 +0x1ea
k8s.io/kubernetes/vendor/github.com/spf13/cobra.(*Command).execute(0xc0003b4000, 0xc000086190, 0x0, 0x0, 0xc0003b4000, 0xc000086190)
	/Users/jscheuermann/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/github.com/spf13/cobra/command.go:760 +0x2ae
k8s.io/kubernetes/vendor/github.com/spf13/cobra.(*Command).ExecuteC(0xc0003b4000, 0x179cea8, 0x2743d40, 0x16fd4b8)
	/Users/jscheuermann/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/github.com/spf13/cobra/command.go:846 +0x2ec
k8s.io/kubernetes/vendor/github.com/spf13/cobra.(*Command).Execute(...)
	/Users/jscheuermann/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/github.com/spf13/cobra/command.go:794
main.main()
	_output/local/go/src/k8s.io/kubernetes/cmd/kube-proxy/proxy.go:49 +0x105

goroutine 18 [chan receive]:
k8s.io/kubernetes/vendor/k8s.io/klog.(*loggingT).flushDaemon(0x27441a0)
	/Users/jscheuermann/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/klog/klog.go:1035 +0x8b
created by k8s.io/kubernetes/vendor/k8s.io/klog.init.0
	/Users/jscheuermann/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/klog/klog.go:404 +0x6c

goroutine 13 [select]:
k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/util/wait.JitterUntil(0x179cec0, 0x12a05f200, 0x0, 0xc0003a2001, 0xc0000840c0)
	/Users/jscheuermann/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/util/wait/wait.go:164 +0x181
k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/util/wait.Until(...)
	/Users/jscheuermann/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/util/wait/wait.go:88
k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/util/wait.Forever(0x179cec0, 0x12a05f200)
	/Users/jscheuermann/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/util/wait/wait.go:79 +0x50
created by k8s.io/kubernetes/vendor/k8s.io/component-base/logs.InitLogs
	/Users/jscheuermann/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/component-base/logs/logs.go:58 +0x8a

I will take a look where and why the stacktrace is happening 🤔

@johscheuer

This comment has been minimized.

Copy link
Member Author

commented Jun 6, 2019

Okay I just validated with the latest master that I also get the Stacktrace seems to be an issue (or feature??) in klog ?

@johscheuer johscheuer changed the title WIP: Check if options returning an error Check if options returning an error Jun 6, 2019

@vllry

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2019

/assign

@vllry

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2019

Are those stack traces only on fatal errors, or for non-fatal too? Might be worth checking if kube-proxy is restarting.

@johscheuer

This comment has been minimized.

Copy link
Member Author

commented Jun 7, 2019

This happens only on fatals (I just verified it). The fatal error above is raised because I didn't provide any configuration to kube-proxy.

It seems like this is newly introduced into a release of klog. Do we really want to throw an fatal with an stacktrace when we don't provide any configuration to kube-proxy ?

@johnbelamaric

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2019

No. For a CLI, "handling" the error is just letting the user know. I don't see a need to use Fatal here, why not just log as error and exit with a non-zero value?

That said, this seems like a bad change in klog to assume you want a stack trace for any Fatal - maybe that should be a new function FatalWithTrace or something.

@johnbelamaric

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2019

(by CLI i mean that this is related to the CLI options, but I guess that it may be called in automation...but still stack trace implies programmer error not user error)

@vllry

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2019

I suspect the reason is that kube-proxy is part CLI and part daemon... as a cluster admin, I'd really want to know if kube-proxy was crashing.

@johscheuer

This comment has been minimized.

Copy link
Member Author

commented Jun 8, 2019

I adjusted the code to use log level error instead of fatal to suppress the stacktrace, now everything is working as expected:

Running the cleanup:

sudo /tmp/kube-proxy --cleanup
W0608 21:01:10.740895   11955 server.go:216] WARNING: all flags other than --config, --write-config-to, and --cleanup are deprecated. Please begin using a config file ASAP.

Calling without arguments:

/tmp/kube-proxy 
W0608 21:01:22.544614   12261 server.go:216] WARNING: all flags other than --config, --write-config-to, and --cleanup are deprecated. Please begin using a config file ASAP.
I0608 21:01:22.562628   12261 server.go:503] Neither kubeconfig file nor master URL was specified. Falling back to in-cluster config.
E0608 21:01:22.562673   12261 server.go:451] unable to load in-cluster configuration, KUBERNETES_SERVICE_HOST and KUBERNETES_SERVICE_PORT must be defined
@johscheuer

This comment has been minimized.

Copy link
Member Author

commented Jun 10, 2019

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

@vllry

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2019

Do we want to downgrade the log level? I would lean towards leaving it at fatal.

(Also, nit, there's a typo in the release note)

@johscheuer

This comment has been minimized.

Copy link
Member Author

commented Jun 21, 2019

Thanks for the hint I fixed the typo in the release notes.

I take a look if we can leave the log level at fatal and don't print a stacktrace (or do we want to print a stacktrace if no configuration is provided).

@johscheuer

This comment has been minimized.

Copy link
Member Author

commented Jul 24, 2019

Ping @vllry should we fix the upstream klog (to add an option to use fatal without stacktrace) or can we go with the error level ?

@johscheuer johscheuer force-pushed the johscheuer:fix-kube-proxy-cleanup-error branch from 8257bf7 to bc8f0c2 Aug 13, 2019

@danwinship

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2019

looks good, but you can you squash it down into a single commit?

Handle error correctly in kubee-proxy command
Signed-off-by: Johannes M. Scheuermann <joh.scheuer@gmail.com>

@johscheuer johscheuer force-pushed the johscheuer:fix-kube-proxy-cleanup-error branch from bc8f0c2 to 3525647 Aug 14, 2019

@johscheuer

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2019

Squashed the commits

@danwinship

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

/lgtm
/approve
/retest

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danwinship, johscheuer

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 merged commit a7c81c6 into kubernetes:master Aug 14, 2019

23 checks passed

cla/linuxfoundation johscheuer authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-iscsi Skipped.
pull-kubernetes-e2e-gce-iscsi-serial Skipped.
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details

@k8s-ci-robot k8s-ci-robot added this to the v1.16 milestone Aug 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.