implement zap for cni #1933

estebancams · 2023-04-27T18:31:14Z

Reason for Change:

This migrates from golang's simple log interface (through our shim) to zap for structured logging and as a necessary step to work on etw logging later (through hooks, which the old logger does not implement)

Issue Fixed:

Requirements:

uses conventional commit messages
includes documentation
adds unit tests

Notes:

dropgz/cmd/root.go

azure-ipam/logger/logger.go

timraymond · 2023-04-27T20:57:10Z

cni/ipam/ipam.go


-	defer func() { log.Printf("[cni-ipam] ADD command completed with result:%+v err:%v.", result, err) }()
+	defer func() {
+		log.Logger.Info("[cni-ipam] ADD command completed",


I'd argue that even this [cni-ipam] sigil should be a zap field. I've seen "service" or "component" names for that.

timraymond · 2023-04-27T20:58:03Z

cni/ipam/plugin/main.go

+		MaxSizeInMB: 5,
+		MaxBackups:  8,


Shouldn't these come from some config?

At the moment standard logger is not taking them from config. We can have a separated task to uniform the approach, but by now I'll follow the current way. I added constants to be neater

estebancams · 2023-05-20T17:14:47Z

/azp run

azure-pipelines · 2023-05-20T17:15:05Z

Azure Pipelines successfully started running 2 pipeline(s).

tamilmani1989 · 2023-06-02T05:33:48Z

cni/log/logger.go

+var Logger *zap.Logger
+
+func New(cfg *Config) (func(), error) {
+	Logger = newFileLogger(cfg)


i know today cni logs only to file... if in future, if we wanted to log to file and also send to logcollector/geneva, how can we expand this?

Zap allows to register hooks, so you don't need to further instrument the code for that extra logging. Whatever you log to zap, will be logged to geneva through a hook, that can be used to do the proper transformations and implementation
That's why we needed to go to Zap in the first place

tamilmani1989 · 2023-06-02T05:36:45Z

cni/ipam/ipam.go

-	defer func() { log.Printf("[cni-ipam] DEL command completed with err:%v.", err) }()
+	defer func() {
+		log.Logger.Info("[cni-ipam] DEL command completed",
+			zap.Any("error", err))


if err is nil, does zap.Any handle it?

Yes, Any chooses the best way to output the field based on its type.

don't use zap.Any when we know the type

tamilmani1989 · 2023-06-02T05:39:30Z

cni/log/logger.go

+
+var Logger *zap.Logger
+
+func New(cfg *Config) (func(), error) {


can you add brief comment for this function on what it does and what it returns?

tamilmani1989 · 2023-06-02T05:44:23Z

cni/network/invoker_cns.go

@@ -305,11 +315,15 @@ func (invoker *CNSIPAMInvoker) Delete(address *net.IPNet, nwCfg *cni.NetworkConf

 			if err = invoker.cnsClient.ReleaseIPAddress(context.TODO(), ipConfig); err != nil {
 				// if the old API fails as well then we just return the error
-				log.Errorf("Failed to release IP address from CNS using ReleaseIPAddress with infracontainerid %s. error: %v", ipConfigs.InfraContainerID, err)
+				log.Logger.Error("Failed to release IP address from CNS using ReleaseIPAddress ",
+					zap.Any("infracontainerid", ipConfigs.InfraContainerID),


zap.String for infraContainerid?

tamilmani1989 · 2023-06-02T05:44:37Z

cni/network/invoker_cns.go

 				return errors.Wrap(err, fmt.Sprintf("failed to release IP %v using ReleaseIPAddress with err ", ipConfig.DesiredIPAddress)+"%w")
 			}
 		} else {
-			log.Errorf("Failed to release IP address with infracontainerid %s from CNS error: %v", ipConfigs.InfraContainerID, err)
+			log.Logger.Error("Failed to release IP address",
+				zap.Any("infracontainerid", ipConfigs.InfraContainerID),


same as above - zap.String

tamilmani1989 · 2023-06-02T05:46:02Z

cni/network/multitenancy.go

@@ -249,13 +255,14 @@ func (m *Multitenancy) getNetworkContainersInternal(
 		return nil, []net.IPNet{}, fmt.Errorf("%w", err)
 	}

-	log.Printf("Network config received from cns %+v", ncConfigs)
+	log.Logger.Info("Network config received from cns", zap.Any("nconfig", ncConfigs))


does this print ncConfig structure with field names or just values?

The whole object

tamilmani1989 · 2023-06-02T05:47:14Z

cni/network/multitenancy.go


 	subnetPrefixes := []net.IPNet{}
 	for i := 0; i < len(ncConfigs); i++ {
 		subnetPrefix := m.netioshim.GetInterfaceSubnetWithSpecificIP(ncConfigs[i].PrimaryInterfaceIdentifier)
 		if subnetPrefix == nil {
-			log.Printf("%w %s", errIfaceNotFound, ncConfigs[i].PrimaryInterfaceIdentifier)
+			log.Logger.Error(errIfaceNotFound.Error(),
+				zap.String("interface", ncConfigs[i].PrimaryInterfaceIdentifier))


let's name this as nodeIP instead of interface

tamilmani1989 · 2023-06-02T05:52:07Z

cni/network/network.go


 	return nil
 }

 // This function for sending CNI metrics to telemetry service
 func logAndSendEvent(plugin *NetPlugin, msg string) {
-	log.Printf(msg)
+	log.Logger.Info(msg)


should we change at the place where we format string?

That's true, good cactch

tamilmani1989 · 2023-06-02T05:57:17Z

cni/network/plugin/main.go

-	if err := log.SetTargetLogDirectory(log.TargetLogfile, ""); err != nil {
+	loggerCfg := &log.Config{
+		Level:       zapcore.DebugLevel,
+		LogPath:     log.LogPath + "azure-vnet.log",


can it be LogPath: log.LogPath + name + ".log"

tamilmani1989 · 2023-06-02T05:59:13Z

cni/ipam/ipam.go

@@ -61,22 +62,22 @@ func (plugin *ipamPlugin) Start(config *common.PluginConfig) error {
 	// Initialize base plugin.
 	err := plugin.Initialize(config)
 	if err != nil {
-		log.Printf("[cni-ipam] Failed to initialize base plugin, err:%v.", err)
+		log.Logger.Error("[cni-ipam] Failed to initialize base plugin.", zap.String("error", err.Error()))


using zap.Any elsewhere for logging error. can we use same here?

cni/log/logger.go

tamilmani1989 · 2023-06-06T17:46:49Z

Thanks for this PR @estebancams

…e new one

Co-authored-by: Timothy J. Raymond <[email protected]>

… fields

Co-authored-by: Evan Baker <[email protected]>

estebancams · 2023-07-03T13:38:36Z

/azp run

azure-pipelines · 2023-07-03T13:38:59Z

Azure Pipelines successfully started running 3 pipeline(s).

tamilmani1989 · 2023-07-03T19:03:03Z

aks-engine tests are failing due to npm pod crash which is unrelated to this change. Also those pipelines succeeded for this PR in prior run, so bypassing those pipeline tests

* feat: added logger package and replaced old log initialization for the new one * feat: changed all log lines to new zap logger * fix: typo * Update azure-ipam/logger/logger.go Co-authored-by: Timothy J. Raymond <[email protected]> * fix: adding constants to describe logger rotations constraints * Renamed logger New method * Replaced logAndSend method by log 1st and send then so we can use zap fields * minor fixes * added logger init for tests * replaced Any by Error * gci ipam_test * fixed govet errors * moved component to a zap field * fixed linit issues * added log mock * fix: gci * fix: added context for logger teardown * Update cni/log/logger.go Co-authored-by: Evan Baker <[email protected]> * moved logger init mock function * fix: lint findings --------- Co-authored-by: Esteban Capillo <[email protected]> Co-authored-by: Timothy J. Raymond <[email protected]> Co-authored-by: Evan Baker <[email protected]>

estebancams added work-in-progress cni Related to CNI. labels Apr 27, 2023

estebancams requested a review from a team as a code owner April 27, 2023 18:31

estebancams requested review from thatmattlong and removed request for a team April 27, 2023 18:31

rbtr reviewed Apr 27, 2023

View reviewed changes

dropgz/cmd/root.go Outdated Show resolved Hide resolved

timraymond reviewed Apr 27, 2023

View reviewed changes

estebancams force-pushed the estebanca/implement-zap-for-cni branch from 030ad17 to e9fd707 Compare May 17, 2023 15:49

estebancams requested a review from a team as a code owner May 17, 2023 15:49

estebancams force-pushed the estebanca/implement-zap-for-cni branch from e9fd707 to 5e58cfd Compare May 17, 2023 16:17

estebancams changed the title ~~wip: implement zap for cni~~ implement zap for cni May 17, 2023

estebancams removed the work-in-progress label May 17, 2023

estebancams force-pushed the estebanca/implement-zap-for-cni branch 3 times, most recently from b9d5917 to 69823dc Compare May 20, 2023 13:50

estebancams requested a review from matmerr as a code owner May 20, 2023 13:50

estebancams force-pushed the estebanca/implement-zap-for-cni branch from 69823dc to 0e4c7c7 Compare May 20, 2023 13:53

estebancams force-pushed the estebanca/implement-zap-for-cni branch 4 times, most recently from 19764d3 to d9ba6be Compare May 22, 2023 20:14

estebancams force-pushed the estebanca/implement-zap-for-cni branch from d9ba6be to a675994 Compare May 31, 2023 19:01

tamilmani1989 reviewed Jun 2, 2023

View reviewed changes

behzad-mir requested changes Jun 2, 2023

View reviewed changes

cni/log/logger.go Show resolved Hide resolved

estebancams force-pushed the estebanca/implement-zap-for-cni branch from a675994 to 6c81eb1 Compare June 4, 2023 15:47

tamilmani1989 previously approved these changes Jun 6, 2023

View reviewed changes

estebancams requested a review from rbtr June 28, 2023 14:53

Esteban Capillo and others added 20 commits June 30, 2023 12:56

feat: added logger package and replaced old log initialization for th…

13013aa

…e new one

feat: changed all log lines to new zap logger

29fefec

fix: typo

0f3ff39

Update azure-ipam/logger/logger.go

3283cb4

Co-authored-by: Timothy J. Raymond <[email protected]>

fix: adding constants to describe logger rotations constraints

70b1a94

Renamed logger New method

02ea33a

Replaced logAndSend method by log 1st and send then so we can use zap…

3965710

… fields

minor fixes

555233e

added logger init for tests

604e0ef

replaced Any by Error

87c8b7f

gci ipam_test

4990b3d

fixed govet errors

789d4c0

moved component to a zap field

a3f49b9

fixed linit issues

8d14f0b

added log mock

3c0a03d

fix: gci

9f108ce

fix: added context for logger teardown

d703324

Update cni/log/logger.go

f20773e

Co-authored-by: Evan Baker <[email protected]>

moved logger init mock function

89ccfd5

fix: lint findings

8f3e413

estebancams force-pushed the estebanca/implement-zap-for-cni branch from 9b4e169 to 8f3e413 Compare June 30, 2023 18:28

timraymond approved these changes Jun 30, 2023

View reviewed changes

rbtr approved these changes Jun 30, 2023

View reviewed changes

tamilmani1989 merged commit 04566d5 into master Jul 3, 2023

tamilmani1989 deleted the estebanca/implement-zap-for-cni branch July 3, 2023 19:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement zap for cni #1933

implement zap for cni #1933

estebancams commented Apr 27, 2023 •

edited

Loading

timraymond Apr 27, 2023

rbtr Jun 13, 2023

timraymond Apr 27, 2023

estebancams May 3, 2023

estebancams commented May 20, 2023

azure-pipelines bot commented May 20, 2023

tamilmani1989 Jun 2, 2023

estebancams Jun 4, 2023

tamilmani1989 Jun 2, 2023

estebancams Jun 4, 2023

rbtr Jun 13, 2023

tamilmani1989 Jun 2, 2023

estebancams Jun 4, 2023

tamilmani1989 Jun 2, 2023

estebancams Jun 4, 2023

tamilmani1989 Jun 2, 2023

estebancams Jun 4, 2023

tamilmani1989 Jun 2, 2023

estebancams Jun 4, 2023

tamilmani1989 Jun 2, 2023

estebancams Jun 4, 2023

tamilmani1989 Jun 2, 2023

estebancams Jun 5, 2023

estebancams Jun 5, 2023

tamilmani1989 Jun 2, 2023

estebancams Jun 5, 2023

tamilmani1989 Jun 2, 2023 •

edited

Loading

estebancams Jun 5, 2023

tamilmani1989 commented Jun 6, 2023

estebancams commented Jul 3, 2023

azure-pipelines bot commented Jul 3, 2023

tamilmani1989 commented Jul 3, 2023


		var Logger *zap.Logger

		func New(cfg *Config) (func(), error) {

implement zap for cni #1933

implement zap for cni #1933

Conversation

estebancams commented Apr 27, 2023 • edited Loading

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

estebancams commented May 20, 2023

azure-pipelines bot commented May 20, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tamilmani1989 Jun 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tamilmani1989 commented Jun 6, 2023

estebancams commented Jul 3, 2023

azure-pipelines bot commented Jul 3, 2023

tamilmani1989 commented Jul 3, 2023

estebancams commented Apr 27, 2023 •

edited

Loading

tamilmani1989 Jun 2, 2023 •

edited

Loading