k8snetworkplumbingwg / sriov-cni

DPDK & SR-IOV CNI plugin
Apache License 2.0
298 stars 147 forks source link

Add logging to sriov-cni #276

Closed andreaskaris closed 9 months ago

andreaskaris commented 1 year ago

Adds both configurable debug logging to stderr and optionally to file as well as error logging for issue 275.

Reported-at: https://github.com/k8snetworkplumbingwg/sriov-cni/issues/275 Signed-off-by: Andreas Karis ak.karis@gmail.com

andreaskaris commented 1 year ago

@mlguerrero12 @SchSeba Sorry for the random ping - would any of this here make sense? I'm currently working on a customer case and needed to build a debug image to troubleshoot a race condition, and debug logging would have been helpful to me

adrianchiris commented 1 year ago

@andreaskaris maybe use : https://github.com/k8snetworkplumbingwg/cni-log

generally im +1 with adding logs to sriov-cni

andreaskaris commented 1 year ago

Will do, have a couple of improvements for k8snetworkplumbingwg/cni-log though

andreaskaris commented 1 year ago

@adrianchiris I submitted a few improvements for https://github.com/k8snetworkplumbingwg/cni-log It'd be great if you could have a look before I continue work on this one here! Thanks!

https://github.com/k8snetworkplumbingwg/cni-log/pull/13

coveralls commented 1 year ago

Pull Request Test Coverage Report for Build 5516021612


Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/config/config.go 5 11 45.45%
pkg/logging/logging.go 12 41 29.27%
<!-- Total: 65 100 65.0% -->
Totals Coverage Status
Change from base Build 5442888333: 1.9%
Covered Lines: 502
Relevant Lines: 1231

💛 - Coveralls
adrianchiris commented 1 year ago

@adrianchiris I submitted a few improvements for https://github.com/k8snetworkplumbingwg/cni-log It'd be great if you could have a look before I continue work on this one here! Thanks!

Done! lets get cni-log changes in first

SchSeba commented 12 months ago

@andreaskaris great to see this PR!

I have a question if we put in debug but we leave the file empty will crio call still work? I need to test this one. because I try one time to use fmt.print and that mess with the return to crio

andreaskaris commented 11 months ago

@andreaskaris great to see this PR!

I have a question if we put in debug but we leave the file empty will crio call still work? I need to test this one. because I try one time to use fmt.print and that mess with the return to crio

@SchSeba I have to admit that I don't follow exactly what you mean (my fault, not yours). Can you elaborate a bit more (rough reproducer / test steps) and I'll try it out?

SchSeba commented 10 months ago

One more comment can you please add to all the messages the cni name or something like that and also ContainerID, Netns

so it will be easy to grep stuff if multiple cni calls or looking at the stderr that gets print in multus container

andreaskaris commented 10 months ago

@mlguerrero12 I made a few changes, taking into consideration your comments. I know I didn't address all of your comments but I'd prefer not to rely too much on the underlying cni-log defaults. Lmk what you think, I'm happy to revert/address further comments :-)

mlguerrero12 commented 10 months ago

thank you @andreaskaris.

LGTM

andreaskaris commented 9 months ago

@zeeke can you have a look? Thanks!

andreaskaris commented 9 months ago

@adrianchiris or @Eoghan1232 as well (I was just informed that I need approval from another company, as well) - thanks :)

Eoghan1232 commented 9 months ago

@zeeke are we good to merge?