Add logging to sriov-cni #276

andreaskaris · 2023-07-11T03:49:41Z

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

Reported-at: #275
Signed-off-by: Andreas Karis [email protected]

andreaskaris · 2023-07-11T05:11:10Z

@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 · 2023-07-11T06:16:04Z

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

generally im +1 with adding logs to sriov-cni

andreaskaris · 2023-07-14T16:47:36Z

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

andreaskaris · 2023-07-15T00:19:44Z

@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!

k8snetworkplumbingwg/cni-log#13

coveralls · 2023-07-17T15:04:37Z

Pull Request Test Coverage Report for Build 5516021612

65 of 100 (65.0%) changed or added relevant lines in 4 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+1.9%) to 40.78%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/config/config.go	5	11	45.45%
pkg/logging/logging.go	12	41	29.27%

Totals
Change from base Build 5442888333:	1.9%
Covered Lines:	502
Relevant Lines:	1231

💛 - Coveralls

adrianchiris · 2023-07-20T07:26:29Z

@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

pkg/logging/logging.go

SchSeba · 2023-08-01T14:53:05Z

@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 · 2023-08-24T13:12:54Z

@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 · 2023-09-12T11:09:51Z

pkg/logging/logging.go

+		cnilog.SetLogStderr(true)
+		return
+	}
+	cnilog.SetLogStderr(false)


to remove and error message cni-log: filename is required when logging to stderr is off - will not log anything
please switch between this 2 lines

meaning first put the setLogFile and then the setLogStderr

SchSeba · 2023-09-12T13:21:36Z

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

docs/configuration-reference.md

mlguerrero12 · 2023-09-26T10:47:50Z

docs/configuration-reference.md

@@ -17,6 +17,8 @@ The SR-IOV CNI configures networks through a CNI spec configuration object. In a
 * `min_tx_rate` (int, optional): change the allowed minimum transmit bandwidth, in Mbps, for the VF. Setting this to 0 disables rate limiting. The min_tx_rate value should be <= max_tx_rate. Support of this feature depends on NICs and drivers.
 * `max_tx_rate` (int, optional): change the allowed maximum transmit bandwidth, in Mbps, for the VF.
 Setting this to 0 disables rate limiting.
+* `loglevel` (string, optional): either of panic, error, warning, info, debug with a default of error.


why error and not info for the default value?

Because I originally did not want to print any extra information by default with the introduction of the logging mechanism. That's also the reason why I set logging.Info("Cannot load config file from cache"). Let me switch it to Info

mlguerrero12 · 2023-09-27T07:30:09Z

cmd/sriov/main.go

@@ -202,6 +235,9 @@ func cmdDel(args *skel.CmdArgs) error {
 		// Return nil when LoadConfFromCache fails since the rest
 		// of cmdDel() code relies on netconf as input argument
 		// and there is no meaning to continue.
+		logging.Info("Cannot load config file from cache",


shouldn't it be error level?

Because I originally did not want to print any extra information by default with the introduction of the logging mechanism. Let me change this though

mlguerrero12 · 2023-09-27T07:30:52Z

pkg/types/types.go

@@ -50,4 +50,6 @@ type NetConf struct {
 	RuntimeConfig struct {
 		Mac string `json:"mac,omitempty"`
 	} `json:"runtimeConfig,omitempty"`
+	LogLevel string `json:"loglevel,omitempty"`


let's use logLevel or log_level to be consistent with other parameters

logLevel / logFile it is

mlguerrero12 · 2023-09-27T07:37:29Z

cmd/sriov/main.go

@@ -43,6 +44,18 @@ func getEnvArgs(envArgsString string) (*envArgs, error) {
 }

 func cmdAdd(args *skel.CmdArgs) error {
+	// Set logging related info. The default log level is error, hence also do logging.Debug inside the block.
+	if logLevel, logFile, err := config.LoadLoggingConf(args.StdinData); err == nil {


I think it's better to create a function to setup the logging. It could be created in config.go. Of course, the debug log should be the only one that remains in cmdAdd and cmdDel.

perhaps, you should only set the logLevel and logFile when these are not ""

I prefer functions be omnipotent though and not rely too much on the defaults of the underlying library

mlguerrero12 · 2023-09-27T07:45:20Z

pkg/logging/logging.go

+
+func init() {
+	SetLogFile("")
+	SetLogLevel("")


init of cni-log pkg calls initloger which already sets empty file and enalbe stderr. So, no need to have it here again. If you decide to set error as log level (instead of info as set by cni-log, see comment above). I think this should be the place.

I removed init() altogether

mlguerrero12 · 2023-09-27T07:45:50Z

pkg/logging/logging.go

+
+// SetLogFile sets the log file for logging. If the empty string is provided, it uses stderr.
+func SetLogFile(fileName string) {
+	if fileName == "" {


no needed if you remove it from init and set in main.go only if it is not empty.

if you set the default level in init, then you can do:

if ll:= cnilog.StringToLevel(l), ll != cnilog.InvalidLevel {
cnilog.SetLogLevel(ll)
}

mlguerrero12 · 2023-09-27T07:46:16Z

pkg/logging/logging.go

+		return
+	}
+	cnilog.SetLogFile(fileName)
+	cnilog.SetLogStderr(false)


so, is it exclusive? only file or only stderr, but not both?

yeah that was my idea to have an "either log to file or log to stderr". The other model would be: "log to stderr and optionally log to file as well" .. or yet another option would be to expose the stderr switch to users as well and have 3 options. But I find the either/or one most intuitive. That said, I'm open to changing it if you strongly favor something else

No, it's fine like it is. Just wanted to check if it was really intended.

andreaskaris · 2023-09-27T10:28:18Z

@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 · 2023-09-27T13:00:16Z

pkg/config/config.go

@@ -16,6 +17,22 @@ var (
 	DefaultCNIDir = "/var/lib/cni/sriov"
 )

+// SetLogging sets global logging parameters.
+func SetLogging(stdinData []byte, containerID, netns, ifName string) {
+	if logLevel, logFile, err := loadLoggingConf(stdinData); err == nil {


I think that this is a valid error. If we are not able to unmarshall it, the plugin should return an error. What do you think?

Also, no need to have 2 separate functions. You can embed loadLoggingConf here. It's fine if you prefer to it separated.

mlguerrero12 · 2023-09-27T13:01:41Z

pkg/logging/logging.go

+
+// Init initializes logging with the requested parameters.
+func Init(logLevel, logFile, containerID, netns, ifName string) {
+	SetLogLevel(logLevel)


Let's not export the setters anymore. They are only used in this package. Also, let's remove the setters for continaerid, netns and ifname.

mlguerrero12 · 2023-09-27T15:06:32Z

thank you @andreaskaris.

LGTM

andreaskaris · 2023-10-10T08:43:32Z

@zeeke can you have a look? Thanks!

andreaskaris · 2023-10-10T08:57:56Z

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

zeeke

Left a minor comment, not blocking the PR for that.

LGTM

pkg/logging/logging.go

Eoghan1232 · 2023-10-11T10:07:59Z

Makefile

@@ -14,6 +14,7 @@ BASE=$(GOPATH)/src/$(REPO_PATH)
 GOFILES = $(shell find . -name *.go | grep -vE "(\/vendor\/)|(_test.go)")
 PKGS     = $(or $(PKG),$(shell cd $(BASE) && env GOPATH=$(GOPATH) $(GO) list ./... | grep -v "^$(PACKAGE)/vendor/"))
 TESTPKGS = $(shell env GOPATH=$(GOPATH) $(GO) list -f '{{ if or .TestGoFiles .XTestGoFiles }}{{ .ImportPath }}{{ end }}' $(PKGS))
+CONTAINER_ENGINE ?= $(shell /bin/bash -c "if command -v podman &>/dev/null; then echo podman; else echo docker; fi")


IMAGE_BUILDER ?= docker

I would prefer something like this, check if the env var is set, if so, take the value, otherwise default.
I would prefer docker to be the default as well.

WDYT?

done: fdbccf1

Provide a way to choose the container engine with IMAGE_BUILDER var. Signed-off-by: Andreas Karis <[email protected]>

Signed-off-by: Andreas Karis <[email protected]>

Eoghan1232

LGTM!

Eoghan1232 · 2023-10-11T11:11:13Z

@zeeke are we good to merge?

andreaskaris force-pushed the add-logging branch 6 times, most recently from 8ba3960 to f2ea5fe Compare July 11, 2023 05:06

andreaskaris force-pushed the add-logging branch from f2ea5fe to 4388de4 Compare July 26, 2023 12:58

andreaskaris marked this pull request as draft July 26, 2023 13:00

andreaskaris force-pushed the add-logging branch 2 times, most recently from b46c194 to 47510ee Compare July 27, 2023 18:16

andreaskaris commented Jul 27, 2023

View reviewed changes

pkg/logging/logging.go Outdated Show resolved Hide resolved

andreaskaris force-pushed the add-logging branch from 47510ee to 63b16aa Compare August 24, 2023 13:05

andreaskaris marked this pull request as ready for review August 24, 2023 13:11

SchSeba reviewed Sep 12, 2023

View reviewed changes

andreaskaris force-pushed the add-logging branch from 63b16aa to 3d9bd5f Compare September 20, 2023 16:19

andreaskaris marked this pull request as draft September 20, 2023 16:19

andreaskaris marked this pull request as ready for review September 20, 2023 16:39

andreaskaris force-pushed the add-logging branch 2 times, most recently from 287ce97 to 780f460 Compare September 20, 2023 16:46

andreaskaris marked this pull request as draft September 20, 2023 16:53

andreaskaris force-pushed the add-logging branch from 780f460 to 622649c Compare September 21, 2023 10:20

andreaskaris marked this pull request as ready for review September 21, 2023 10:22

andreaskaris force-pushed the add-logging branch 2 times, most recently from bce91df to 73c4c4e Compare September 21, 2023 10:54

andreaskaris mentioned this pull request Sep 22, 2023

Support loglevel and logfile for SR-IOV plugin k8snetworkplumbingwg/sriov-network-operator#509

Merged

SchSeba reviewed Sep 26, 2023

View reviewed changes

docs/configuration-reference.md Outdated Show resolved Hide resolved

andreaskaris force-pushed the add-logging branch 4 times, most recently from bb375e5 to 3f8d047 Compare September 26, 2023 14:55

mlguerrero12 reviewed Sep 27, 2023

View reviewed changes

andreaskaris force-pushed the add-logging branch from 3f8d047 to 940ac44 Compare September 27, 2023 10:22

mlguerrero12 reviewed Sep 27, 2023

View reviewed changes

andreaskaris force-pushed the add-logging branch from 940ac44 to a989652 Compare September 27, 2023 14:54

mlguerrero12 approved these changes Sep 27, 2023

View reviewed changes

zeeke approved these changes Oct 10, 2023

View reviewed changes

pkg/logging/logging.go Outdated Show resolved Hide resolved

andreaskaris force-pushed the add-logging branch from a989652 to 63c097b Compare October 11, 2023 10:00

Eoghan1232 reviewed Oct 11, 2023

View reviewed changes

andreaskaris added 3 commits October 11, 2023 12:57

Makefile: Provide switch for container engine

fdbccf1

Provide a way to choose the container engine with IMAGE_BUILDER var. Signed-off-by: Andreas Karis <[email protected]>

Add vendoring for logging

c2188d2

Signed-off-by: Andreas Karis <[email protected]>

Add debug logging for SR-IOV plugin

f996a66

Signed-off-by: Andreas Karis <[email protected]>

andreaskaris force-pushed the add-logging branch from 63c097b to f996a66 Compare October 11, 2023 10:58

Eoghan1232 approved these changes Oct 11, 2023

View reviewed changes

zeeke merged commit 1bf2f23 into k8snetworkplumbingwg:master Oct 11, 2023
10 checks passed

Add logging to sriov-cni #276

Add logging to sriov-cni #276

Conversation

andreaskaris commented Jul 11, 2023 • edited Loading

andreaskaris commented Jul 11, 2023

adrianchiris commented Jul 11, 2023

andreaskaris commented Jul 14, 2023

andreaskaris commented Jul 15, 2023 • edited Loading

coveralls commented Jul 17, 2023

Pull Request Test Coverage Report for Build 5516021612

💛 - Coveralls

adrianchiris commented Jul 20, 2023

SchSeba commented Aug 1, 2023

andreaskaris commented Aug 24, 2023

Choose a reason for hiding this comment

SchSeba commented Sep 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreaskaris commented Sep 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlguerrero12 commented Sep 27, 2023

andreaskaris commented Oct 10, 2023

andreaskaris commented Oct 10, 2023

zeeke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eoghan1232 left a comment

Choose a reason for hiding this comment

Eoghan1232 commented Oct 11, 2023

andreaskaris commented Jul 11, 2023 •

edited

Loading

andreaskaris commented Jul 15, 2023 •

edited

Loading