Skip to content
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

Add logging to sriov-cni #276

Merged
merged 3 commits into from
Oct 11, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -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))
IMAGE_BUILDER ?= docker

export GOPATH
export GOBIN
Expand Down Expand Up @@ -136,7 +137,7 @@ fmt: ; $(info Running gofmt...) @ ## Run gofmt on all source files
# To pass proxy for Docker invoke it as 'make image HTTP_POXY=http://192.168.0.1:8080'
.PHONY: image
image: | $(BASE) ; $(info Building Docker image...) @ ## Build SR-IOV CNI docker image
@docker build -t $(TAG) -f $(DOCKERFILE) $(CURDIR) $(DOCKERARGS)
@$(IMAGE_BUILDER) build -t $(TAG) -f $(DOCKERFILE) $(CURDIR) $(DOCKERARGS)

# Misc

Expand Down
32 changes: 31 additions & 1 deletion cmd/sriov/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
"github.com/containernetworking/plugins/pkg/ipam"
"github.com/containernetworking/plugins/pkg/ns"
"github.com/k8snetworkplumbingwg/sriov-cni/pkg/config"
"github.com/k8snetworkplumbingwg/sriov-cni/pkg/logging"
"github.com/k8snetworkplumbingwg/sriov-cni/pkg/sriov"
"github.com/k8snetworkplumbingwg/sriov-cni/pkg/utils"
"github.com/vishvananda/netlink"
Expand Down Expand Up @@ -43,6 +44,13 @@ func getEnvArgs(envArgsString string) (*envArgs, error) {
}

func cmdAdd(args *skel.CmdArgs) error {
if err := config.SetLogging(args.StdinData, args.ContainerID, args.Netns, args.IfName); err != nil {
return err
}
logging.Debug("function called",
"func", "cmdAdd",
"args.Path", args.Path, "args.StdinData", string(args.StdinData), "args.Args", args.Args)

netConf, err := config.LoadConf(args.StdinData)
if err != nil {
return fmt.Errorf("SRIOV-CNI failed to load netconf: %v", err)
Expand Down Expand Up @@ -179,12 +187,20 @@ func cmdAdd(args *skel.CmdArgs) error {
}

// Cache NetConf for CmdDel
logging.Debug("Cache NetConf for CmdDel",
"func", "cmdAdd",
"config.DefaultCNIDir", config.DefaultCNIDir,
"netConf", netConf)
if err = utils.SaveNetConf(args.ContainerID, config.DefaultCNIDir, args.IfName, netConf); err != nil {
return fmt.Errorf("error saving NetConf %q", err)
}

// Mark the pci address as in use.
logging.Debug("Mark the PCI address as in use",
"func", "cmdAdd",
"config.DefaultCNIDir", config.DefaultCNIDir,
"netConf.DeviceID", netConf.DeviceID)
allocator := utils.NewPCIAllocator(config.DefaultCNIDir)
// Mark the pci address as in used
if err = allocator.SaveAllocatedPCI(netConf.DeviceID, args.Netns); err != nil {
return fmt.Errorf("error saving the pci allocation for vf pci address %s: %v", netConf.DeviceID, err)
}
Expand All @@ -193,6 +209,13 @@ func cmdAdd(args *skel.CmdArgs) error {
}

func cmdDel(args *skel.CmdArgs) error {
if err := config.SetLogging(args.StdinData, args.ContainerID, args.Netns, args.IfName); err != nil {
return err
}
logging.Debug("function called",
"func", "cmdDel",
"args.Path", args.Path, "args.StdinData", string(args.StdinData), "args.Args", args.Args)

netConf, cRefPath, err := config.LoadConfFromCache(args)
if err != nil {
// If cmdDel() fails, cached netconf is cleaned up by
Expand All @@ -202,6 +225,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.Error("Cannot load config file from cache",
"func", "cmdDel",
"err", err)
return nil
}

Expand Down Expand Up @@ -261,6 +287,10 @@ func cmdDel(args *skel.CmdArgs) error {
}

// Mark the pci address as released
logging.Debug("Mark the PCI address as released",
"func", "cmdDel",
"config.DefaultCNIDir", config.DefaultCNIDir,
"netConf.DeviceID", netConf.DeviceID)
allocator := utils.NewPCIAllocator(config.DefaultCNIDir)
if err = allocator.DeleteAllocatedPCI(netConf.DeviceID); err != nil {
return fmt.Errorf("error cleaning the pci allocation for vf pci address %s: %v", netConf.DeviceID, err)
Expand Down
7 changes: 6 additions & 1 deletion docs/configuration-reference.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@ 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 info.
* `logFile` (string, optional): path to file for log output. By default, this will log to stderr. Logging to stderr
means that the logs will show up in crio logs (in the journal in most configurations) and in multus pod logs.


An SR-IOV CNI config with each field filled out looks like:
Expand All @@ -34,7 +37,9 @@ An SR-IOV CNI config with each field filled out looks like:
"max_tx_rate": 200,
"spoofchk": "off",
"trust": "on",
"link_state": "enable"
"link_state": "enable",
"logLevel": "debug",
"logFile": "/tmp/sriov.log"
}
```

Expand Down
3 changes: 3 additions & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ go 1.20
require (
github.com/containernetworking/cni v1.1.2
github.com/containernetworking/plugins v1.2.0
github.com/k8snetworkplumbingwg/cni-log v0.0.0-20230801160229-b6e062c9e0f2
github.com/onsi/ginkgo/v2 v2.9.2
github.com/onsi/gomega v1.27.5
github.com/stretchr/testify v1.6.1
Expand All @@ -26,6 +27,8 @@ require (
golang.org/x/sys v0.6.0 // indirect
golang.org/x/text v0.8.0 // indirect
golang.org/x/tools v0.7.0 // indirect
gopkg.in/natefinch/lumberjack.v2 v2.0.0 // indirect
gopkg.in/yaml.v2 v2.4.0 // indirect
gopkg.in/yaml.v3 v3.0.1 // indirect
)

Expand Down
9 changes: 9 additions & 0 deletions go.sum
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
github.com/BurntSushi/toml v1.1.0 h1:ksErzDEI1khOiGPgpwuI7x2ebx/uXQNw7xJpn9Eq1+I=
github.com/chzyer/logex v1.1.10/go.mod h1:+Ywpsq7O8HXn0nuIou7OrIPyXbp3wmkHB+jjWRnGsAI=
github.com/chzyer/readline v0.0.0-20180603132655-2972be24d48e/go.mod h1:nSuG5e5PlCu98SY8svDHJxuZscDgtXS6KTTbou5AhLI=
github.com/chzyer/test v0.0.0-20180213035817-a1ea475d72b1/go.mod h1:Q3SI9o4m/ZMnBNeIyt5eFwwo7qiLfzFZmjNmxjkiQlU=
Expand Down Expand Up @@ -25,6 +26,10 @@ github.com/google/pprof v0.0.0-20210407192527-94a9f03dee38/go.mod h1:kpwsk12EmLe
github.com/google/pprof v0.0.0-20230323073829-e72429f035bd h1:r8yyd+DJDmsUhGrRBxH5Pj7KeFK5l+Y3FsgT8keqKtk=
github.com/google/pprof v0.0.0-20230323073829-e72429f035bd/go.mod h1:79YE0hCXdHag9sBkw2o+N/YnZtTkXi0UT9Nnixa5eYk=
github.com/ianlancetaylor/demangle v0.0.0-20200824232613-28f6c0f3b639/go.mod h1:aSSvb/t6k1mPoxDqO4vJh6VOCGPwU4O0C2/Eqndh1Sc=
github.com/k8snetworkplumbingwg/cni-log v0.0.0-20230725074452-455465349b0d h1:fXp43GgMdHBigXEnKrsFqlan6+JSTOwAquzmXaKmzkA=
github.com/k8snetworkplumbingwg/cni-log v0.0.0-20230725074452-455465349b0d/go.mod h1:/x45AlZDoJVSSV4ECDb5TcHLzrVRDllsCMDzMrtHKwk=
github.com/k8snetworkplumbingwg/cni-log v0.0.0-20230801160229-b6e062c9e0f2 h1:KB8UPZQwLge4Abuk9tNmvzffdCJgqXSN341BX98QTHg=
github.com/k8snetworkplumbingwg/cni-log v0.0.0-20230801160229-b6e062c9e0f2/go.mod h1:/x45AlZDoJVSSV4ECDb5TcHLzrVRDllsCMDzMrtHKwk=
github.com/nxadm/tail v1.4.8 h1:nPr65rt6Y5JFSKQO7qToXr7pePgD6Gwiw05lkbyAQTE=
github.com/onsi/ginkgo v1.16.5 h1:8xi0RTUf59SOSfEtZMvwTvXYMzG4gV23XVHOZiXNtnE=
github.com/onsi/ginkgo/v2 v2.9.2 h1:BA2GMJOtfGAfagzYtrAlufIP0lq6QERkFmHLMLPwFSU=
Expand Down Expand Up @@ -96,7 +101,11 @@ google.golang.org/protobuf v1.28.0 h1:w43yiav+6bVFTBQFZX0r7ipe9JQ1QsbMgHwbBziscL
google.golang.org/protobuf v1.28.0/go.mod h1:HV8QOd/L58Z+nl8r43ehVNZIU/HEI6OcFqwMG9pJV4I=
gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM=
gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
gopkg.in/natefinch/lumberjack.v2 v2.0.0 h1:1Lc07Kr7qY4U2YPouBjpCLxpiyxIVoxqXgkXLknAOE8=
gopkg.in/natefinch/lumberjack.v2 v2.0.0/go.mod h1:l0ndWWf7gzL7RNwBG7wST/UCcT4T24xpD6X8LsfU/+k=
gopkg.in/tomb.v1 v1.0.0-20141024135613-dd632973f1e7 h1:uRGJdciOHaEIrze2W8Q3AKkepLTh2hOroT7a+7czfdQ=
gopkg.in/yaml.v2 v2.4.0 h1:D8xgwECY7CYvx+Y2n4sBz93Jn9JRvxdiyyo8CTfuKaY=
gopkg.in/yaml.v2 v2.4.0/go.mod h1:RDklbk79AGWmwhnvt/jBztapEOGDOx6ZbXqjP6csGnQ=
gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM=
gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA=
gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM=
18 changes: 17 additions & 1 deletion pkg/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"strings"

"github.com/containernetworking/cni/pkg/skel"
"github.com/k8snetworkplumbingwg/sriov-cni/pkg/logging"
sriovtypes "github.com/k8snetworkplumbingwg/sriov-cni/pkg/types"
"github.com/k8snetworkplumbingwg/sriov-cni/pkg/utils"
)
Expand All @@ -16,6 +17,17 @@ var (
DefaultCNIDir = "/var/lib/cni/sriov"
)

// SetLogging sets global logging parameters.
func SetLogging(stdinData []byte, containerID, netns, ifName string) error {
n := &sriovtypes.NetConf{}
if err := json.Unmarshal(stdinData, n); err != nil {
return fmt.Errorf("SetLogging(): failed to load netconf: %v", err)
}

logging.Init(n.LogLevel, n.LogFile, containerID, netns, ifName)
return nil
}

// LoadConf parses and validates stdin netconf and returns NetConf object
func LoadConf(bytes []byte) (*sriovtypes.NetConf, error) {
n := &sriovtypes.NetConf{}
Expand All @@ -36,11 +48,15 @@ func LoadConf(bytes []byte) (*sriovtypes.NetConf, error) {
return nil, fmt.Errorf("LoadConf(): VF pci addr is required")
}

allocator := utils.NewPCIAllocator(DefaultCNIDir)
// Check if the device is already allocated.
// This is to prevent issues where kubelet request to delete a pod and in the same time a new pod using the same
// vf is started. we can have an issue where the cmdDel of the old pod is called AFTER the cmdAdd of the new one
// This will block the new pod creation until the cmdDel is done.
logging.Debug("Check if the device is already allocated",
"func", "LoadConf",
"DefaultCNIDir", DefaultCNIDir,
"n.DeviceID", n.DeviceID)
allocator := utils.NewPCIAllocator(DefaultCNIDir)
isAllocated, err := allocator.IsAllocated(n.DeviceID)
if err != nil {
return n, err
Expand Down
93 changes: 93 additions & 0 deletions pkg/logging/logging.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
// package logging is a small wrapper around github.com/k8snetworkplumbingwg/cni-log

package logging

import (
cnilog "github.com/k8snetworkplumbingwg/cni-log"
)

const (
labelCNIName = "cniName"
labelContainerID = "containerID"
labelNetNS = "netns"
labelIFName = "ifname"
cniName = "sriov-cni"
)

var (
logLevelDefault = cnilog.InfoLevel
containerID = ""
netNS = ""
ifName = ""
)

// Init initializes logging with the requested parameters in this order: log level, log file, container ID,
// network namespace and interface name.
func Init(logLevel, logFile, containerIdentification, networkNamespace, interfaceName string) {
setLogLevel(logLevel)
setLogFile(logFile)
containerID = containerIdentification
netNS = networkNamespace
ifName = interfaceName
}

// setLogLevel sets the log level to either verbose, debug, info, warn, error or panic. If an invalid string is
// provided, it uses error.
func setLogLevel(l string) {
ll := cnilog.StringToLevel(l)
if ll == cnilog.InvalidLevel {
ll = logLevelDefault
}
cnilog.SetLogLevel(ll)
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

cnilog.SetLogStderr(true)
cnilog.SetLogFile("")
return
}
cnilog.SetLogFile(fileName)
cnilog.SetLogStderr(false)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

// Debug provides structured logging for log level >= debug.
func Debug(msg string, args ...interface{}) {
cnilog.DebugStructured(msg, prependArgs(args)...)
}

// Info provides structured logging for log level >= info.
func Info(msg string, args ...interface{}) {
cnilog.InfoStructured(msg, prependArgs(args)...)
}

// Warning provides structured logging for log level >= warning.
func Warning(msg string, args ...interface{}) {
cnilog.WarningStructured(msg, prependArgs(args)...)
}

// Error provides structured logging for log level >= error.
func Error(msg string, args ...interface{}) {
_ = cnilog.ErrorStructured(msg, prependArgs(args)...)
}

// Panic provides structured logging for log level >= panic.
func Panic(msg string, args ...interface{}) {
cnilog.PanicStructured(msg, prependArgs(args)...)
}

// prependArgs prepends cniName, containerID, netNS and ifName to the args of every log message.
func prependArgs(args []interface{}) []interface{} {
if ifName != "" {
args = append([]interface{}{labelIFName, ifName}, args...)
}
if netNS != "" {
args = append([]interface{}{labelNetNS, netNS}, args...)
}
if containerID != "" {
args = append([]interface{}{labelContainerID, containerID}, args...)
}
args = append([]interface{}{labelCNIName, cniName}, args...)
return args
}
19 changes: 19 additions & 0 deletions pkg/logging/logging_suite_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
package logging

import (
"testing"

g "github.com/onsi/ginkgo/v2"
o "github.com/onsi/gomega"
)

func TestConfig(t *testing.T) {
o.RegisterFailHandler(g.Fail)
g.RunSpecs(t, "Logging Suite")
}

var _ = g.BeforeSuite(func() {
})

var _ = g.AfterSuite(func() {
})
Loading
Loading