Get rid of redundant logs in HCN version range checks #1053
Conversation
|
@elweb9858 @vikas-bh @Keith-Mange @erfrimod @netal. Talked with Betsy briefly but seems like these shouldn't be very useful in the unit tests either but wanted to make sure beforehand. |
|
lgtm but would like a sanity check from someone else on my team. for reference this is called in kube-proxy here. the logging in this function seems unnecessary because any failed version checks will get a separate log message in kube-proxy, as Danny mentioned in the PR description |
|
It's called in a couple other places where checking for specific features too https://github.com/kubernetes/kubernetes/blob/2453f07e933a6d2ac0be1eb35061a2154ccce5b8/pkg/proxy/winkernel/proxier.go#L588 |
|
@dcantah ohhhh I didn't realize all those specific feature checks call this too. this is so much more logging spam than I originally thought! |
|
I would advise not to remove these. I'd suggest moving them to "Debugf" or "Tracef" levels if they are too noisy. We needed these traces to debug feature availability in the past, so they are useful. |
|
Also, as per our discussion, Kube-Proxy is repeatedly calling into this code, but this is less than efficient (since the response will never change). In an ideal world, kube proxy should check it once (in an init() block), cache the info, and use it later on. |
|
After talking with @JocelynBerrendonner it sounds like it might make more sense to just have a log print out (once) the supported features and the version of HNS, so I'll add this. Sounds like there was some use for these in assistance for investigating issues but the spam is completely agreed upon. |
|
@JocelynBerrendonner Updated! Not sure what else we can log if hcsshim is vendored into the project (kubernetes for example). If you have an idea let me know |
hcn/hcnsupport.go
Outdated
| logrus.WithFields(logrus.Fields{ | ||
| "Major": globals.Version.Major, | ||
| "Minor": globals.Version.Minor, | ||
| }).Infof("HNS version") |
There was a problem hiding this comment.
Also think this should probably not be Info level. Can we make it Debug or Trace instead?
There was a problem hiding this comment.
I don't believe k8s uses logrus so they don't set the level/expose configuring the logrus level by virtue of that. This would likely mean these logs won't show up
There was a problem hiding this comment.
Do we believe it's important for this log to show up in k8s output?
There was a problem hiding this comment.
@JocelynBerrendonner Had mentioned this would help with investigations regarding when a customer is questioning why certain HCN feature(s) aren't working/supported.
There was a problem hiding this comment.
If it's helpful to have for that case, then I think it's okay to leave it at Info, given it only logs once.
Kubeproxy logs are filled with redudnant version check spam from an unexported call that's invoked as part of checking if a feature is supported. The logs don't detail what feature(s) are even being checked so it just seems like spam. With the way things are implemented all of the hcn features are checked for support in any of the `hcn.XSupported()` calls not just the one being checked, so these logs come up quite a bit if there's many features that aren't supported on the machine. Add two new logs in a sync.Once that logs the HNS version and supported features. This should be enough to investigate version issues. Should remedy microsoft#1043 Signed-off-by: Daniel Canter <dcanter@microsoft.com>
There was a problem hiding this comment.
lgtm, assuming @JocelynBerrendonner is okay with the removed logs
|
Thanks a lot, Danny for taking all my feedback and getting my suggestion in! This is much appreciated! |
Related work items: microsoft#930, microsoft#962, microsoft#1004, microsoft#1008, microsoft#1039, microsoft#1045, microsoft#1046, microsoft#1047, microsoft#1052, microsoft#1053, microsoft#1054, microsoft#1057, microsoft#1058, microsoft#1060, microsoft#1061, microsoft#1063, microsoft#1064, microsoft#1068, microsoft#1069, microsoft#1070, microsoft#1071, microsoft#1074, microsoft#1078, microsoft#1079, microsoft#1081, microsoft#1082, microsoft#1083, microsoft#1084, microsoft#1088, microsoft#1090, microsoft#1091, microsoft#1093, microsoft#1094, microsoft#1096, microsoft#1098, microsoft#1099, microsoft#1102, microsoft#1103, microsoft#1105, microsoft#1106, microsoft#1108, microsoft#1109, microsoft#1115, microsoft#1116, microsoft#1122, microsoft#1123, microsoft#1126
Kubeproxy logs are filled with redudnant version check spam from an unexported call that's invoked
as part of checking if a feature is supported. The logs don't detail what feature(s) are even being checked
so it just seems like spam. With the way things are implemented all of the hcn features are checked for support in
any of the
hcn.XSupported()calls not just the one being checked, so these logs come up quite a bit if there'smany features that aren't supported on the machine.
Add two new logs in a sync.Once that logs the HNS version and supported features. This should be enough
to investigate version issues.
Should remedy #1043
Signed-off-by: Daniel Canter dcanter@microsoft.com