tumblr / k8s-sidecar-injector

Kubernetes sidecar injection service

Apache License 2.0

345 stars 75 forks source link

restart watcher when receive event with nil Object #16

Closed lxs137 closed 5 years ago

lxs137 commented 5 years ago

What and why?

Fix #15 Be descriptive, give context, and avoid one liners.

Testing Steps

Please provide adequate testing steps (including screenshots if necessary). Include any test fixtures or sample configurations in your commit.

[ ] Added unit tests for this feature (make test)

Reviewers

Required reviewers: @byxorna Request reviews from other people you want to review this PR in the "Reviewers" section on the right.

:warning: this PR must have at least 2 thumbs from the MAINTAINERS.md of the project before merging!

yahoocla commented 5 years ago

Thank you for submitting this pull request, however I do not see a valid CLA on file for you. Before we can merge this request please visit https://yahoocla.herokuapp.com/ and agree to the terms. Thanks! :smile:

lxs137 commented 5 years ago

Actually, this bug is caused by HTTP timeout, which related to --min-request-timeout option of kube-apiserver, if set this option to 1800, watcher will get nil Object every 1800s exactly.

I found there are two solution to fix this:

close and recreate the watcher, just like what informer do: https://github.com/kubernetes/kubernetes/blob/32b37f5a85e8f2987f8367e611147cd4d4dfa4f9/staging/src/k8s.io/client-go/tools/cache/reflector.go#L319-L340
use informer directly, but this will change other function in config/watcher

I want to know which solution do you perfer @byxorna

byxorna commented 5 years ago

thanks for digging into this @lxs137 ! I think stopping the watcher when the http connection times out seems like the easiest approach IMO. I like their solution of defer watcher.Stop() in watcher.Watch() - seems hygienic.

byxorna commented 5 years ago

@lxs137 thank you for this contribution! Awesome work :) I will merge and cut a new release shortly.

Thanks!