-
Notifications
You must be signed in to change notification settings - Fork 84
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
testenv: Remove klog init flags #148
Conversation
b22bf27
to
7c6d0e3
Compare
runtime/testenv/testenv.go
Outdated
// uses it to initializes klog. | ||
// Refer: https://github.com/kubernetes/klog/blob/f8e668dbaa5f6f0e6a5c24ffd7667263840d79ae/examples/coexist_glog/coexist_glog.go | ||
klogFlags := flag.NewFlagSet("klog", flag.ExitOnError) | ||
klog.InitFlags(klogFlags) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need to tell klogr about the flags?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought so too and was trying to find a way to do that but there doesn't seem to be a way to do that.
Concluded that since this is a test, it's okay for the defaults. A user should be able to override the logger that's passed to the controllers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK; I think it is worth pointing out in the signpost comment that it's effectively throwing away the flag values, so that future readers will not skip through and assume they are passed. Since we're talking about global variables here, it would be a reasonable assumption given the comment that say, klog.InitFlags(...)
assigns the flagset to be used by klogr. (Or does it actually do this? There you go, I'm already confused ...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😄 yes, it is a bit confusing. I spent some more time going through the klog repo and understanding how it's supposed to be used. It looks like klog.InitFlags()
is independent of klogr. There's a private variable called logging
that's used for setting the flag values on the given FlagSet
to InitFlags()
.
klogr just imports klog and uses the global klog. There's no API to pass a klog instance. If any values are set on the global klog using the global flagset, it'll be available to klogr. No way to pass a different klog.
The problem in this case is that since the testenv's init gets called at the very beginning, there's nothing that can be done in testenv package from preventing someone trying to register another flag later. In the case of badger, since they use glog, even they don't have a way to not do this without dropping the glog dependency. The only way around this that I can think of is to avoid filling the global flags with klog init flags for this particular situation.
I'll update the comment to mention that the klog flags are not passed to the klogr that's set as the controller runtime logger. It can be manually passed by creating a custom klog and klogr, and overwriting the set controller-runtime logger.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've looked at the klog code now too. The flag values aren't thrown away as I speculated, they are used to populate a global struct, as you say. And klogr
is just an adapter that uses the global. So that's all fine from our point of view, since all we want to do is avoid using the same flagset as glog.
2576e9e
to
402a588
Compare
402a588
to
bb1064d
Compare
I spent more time validating the idea of overriding the controller-runtime(c-n) logger set by the testenv Also, added godoc example to show how a custom logger can be set before creating testenv. |
0d1ea32
to
51132d8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reasoning seems to make sense, thanks for getting to the bottom of it, and for including an example 🥇
runtime/testenv/testenv.go
Outdated
// redeclaration of the same flags. | ||
// This is based on klog's coexist with glog example, refer: | ||
// https://github.com/kubernetes/klog/blob/f8e668dbaa5f6f0e6a5c24ffd7667263840d79ae/examples/coexist_glog/coexist_glog.go | ||
// The klog flags set below are not available to the klogr logger below |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This, we have discovered, is not correct. klogr
uses the public API of klog
, which in turn makes use of the flagset to set itself up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replaced it with a statement about how to override this logger setup.
51132d8
to
6089e4d
Compare
runtime/testenv/testenv.go
Outdated
// assigned to the controller-runtime logger explicitly before creating | ||
// testenv Environment. | ||
klogFlags := flag.NewFlagSet("klog", flag.ExitOnError) | ||
klog.InitFlags(klogFlags) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need to call klogFlags.Parse()
? I don't think klog calls it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Searching for Parse()
in klog repo, I only see them in the example code. So, yes, you're probably right about that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried setting a parse here klogFlags.Parse(os.Args[1:])
, but it results in the following error :
flag provided but not defined: -test.paniconexit0
Usage of klog:
-add_dir_header
If true, adds the file directory to the header of the log messages
-alsologtostderr
log to standard error as well as files
...
Because some extra flags get set in the test run that klog flagset isn't aware of.
I can't think of a way to remove unknown flag args before passing to this Parse() in a library. Maybe this is the reason it's not parsed here in the first place.
This leaves it up to the user to construct their own flagset and configure their logger. The library only does the minimum default setup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So that makes me feel like dropping klog.InitFlags()
completely since the flags are not parsed and used at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And if we do that, we can remove all the extra comments about coexisting with glog. It'll just work with glog without doing anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought that might be the case. And if it can't be done here, I don't think a user of the library has much of a chance either. It worked before because the test environment didn't mandate using klogr
; could this package go back to that arrangement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it did mandate klogr before but didn't care to parse the flags and left no way to change it because of running the logger setup in the init.
I can just remove the separate flagset code and klog.InitFlags()
. That will be equivalent to the previous behavior.
Removes klog.InitFlags() because the klog flags aren't parsed before using klog. To avoid conflicting with other flags, it's better to not populate the global flagset. This results in default klogr to be set as the default controller-runtime logger if not set in the test already. The logger setup not being in init() allows users to set their own logger before creating testenv Environment. Signed-off-by: Sunny <darkowlzz@protonmail.com>
6089e4d
to
c5a17be
Compare
Renamed the title of the PR and changed the details in the commit message because it no longer has to do with glog directly. |
func New(o ...Option) *Environment { | ||
// Set a default logger if not set already. | ||
log.SetLogger(klogr.New()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given this is equivalent to ctrl.SetLogger, it'd be better to use the same in both the example given and the code, otherwise people might wonder why they differ. Also worth explaining that subsequent calls to ctrl.SetLogger have no effect (the comment implies this, but pays to be explicit)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is the right way to go now, and I'm down to nit picks, so ✅
Thanks for your patience Sunny!
This change allows test environments with glog to work with testenv.
Background
image-reflector-controller uses badger for database. dgraph-io/badger go package depends on dgraph-io/ristretto, which depends on glog. When glog is used, it sets
log_dir
flag, refer .Due to the glog transitive dependency of badger, when badger is used in testenv based tests, the testenv package init() initialization of the klog flags and badger set the glog flags conflict, resulting in some redefined flags.
This can be reproduced by adding the following in
TestMain()
:with
"github.com/dgraph-io/badger/v3"
import.Failure output:
Based on an example in klog repo, glog and klog can coexist with separate FlagSets.
With this change, the image-reflector-controller is able to use testenv.