From c872e30036cf1475d5f97061dc605a6612995fbc Mon Sep 17 00:00:00 2001 From: Tamilmani Manoharan Date: Thu, 7 Mar 2019 14:34:25 -0800 Subject: [PATCH 01/14] added UT to fix code coverage --- telemetry/telemetry_test.go | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/telemetry/telemetry_test.go b/telemetry/telemetry_test.go index 0daf668f1e..21dafc355d 100644 --- a/telemetry/telemetry_test.go +++ b/telemetry/telemetry_test.go @@ -175,6 +175,31 @@ func TestCloseTelemetryConnection(t *testing.T) { } } +func TestServerCloseTelemetryConnection(t *testing.T) { + // server telemetrybuffer + tb = NewTelemetryBuffer(hostAgentUrl) + err := tb.StartServer() + if err == nil { + go tb.BufferAndPushData(0) + } + + // client telemetrybuffer + tb1 := NewTelemetryBuffer(hostAgentUrl) + if err := tb1.Connect(); err != nil { + fmt.Printf("connection to telemetry server failed %v", err) + } + + // Close server connection + tb.Close() + + // Close client connection + tb1.Close() + + if len(tb.connections) != 0 { + fmt.Printf("server didn't close all connections as expected") + } +} + func TestSetReportState(t *testing.T) { err := reportManager.SetReportState("a.json") if err != nil { From 94bbbb3bc47ed2c6199d2c58ee28d230fa5c55d7 Mon Sep 17 00:00:00 2001 From: Tamilmani Manoharan Date: Thu, 7 Mar 2019 14:38:44 -0800 Subject: [PATCH 02/14] added check before closing client connection --- telemetry/telemetry_test.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/telemetry/telemetry_test.go b/telemetry/telemetry_test.go index 21dafc355d..4128bc7e75 100644 --- a/telemetry/telemetry_test.go +++ b/telemetry/telemetry_test.go @@ -192,12 +192,13 @@ func TestServerCloseTelemetryConnection(t *testing.T) { // Close server connection tb.Close() - // Close client connection - tb1.Close() - if len(tb.connections) != 0 { fmt.Printf("server didn't close all connections as expected") } + + // Close client connection + tb1.Close() + } func TestSetReportState(t *testing.T) { From 5f9650a7b8d1a42bf0bcfde2dfa531ce0d339b30 Mon Sep 17 00:00:00 2001 From: Tamilmani Manoharan Date: Thu, 7 Mar 2019 14:40:22 -0800 Subject: [PATCH 03/14] replaced fmt.Printf with t.Errorf --- telemetry/telemetry_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/telemetry/telemetry_test.go b/telemetry/telemetry_test.go index 4128bc7e75..755a333946 100644 --- a/telemetry/telemetry_test.go +++ b/telemetry/telemetry_test.go @@ -186,14 +186,14 @@ func TestServerCloseTelemetryConnection(t *testing.T) { // client telemetrybuffer tb1 := NewTelemetryBuffer(hostAgentUrl) if err := tb1.Connect(); err != nil { - fmt.Printf("connection to telemetry server failed %v", err) + t.Errorf("connection to telemetry server failed %v", err) } // Close server connection tb.Close() if len(tb.connections) != 0 { - fmt.Printf("server didn't close all connections as expected") + t.Errorf("server didn't close all connections as expected") } // Close client connection From b9f476a931061b6e70024dca0af5ef093dcf5d21 Mon Sep 17 00:00:00 2001 From: Tamilmani Manoharan Date: Thu, 7 Mar 2019 14:42:51 -0800 Subject: [PATCH 04/14] Calling cancel to exit server thread and close connection --- telemetry/telemetry_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/telemetry/telemetry_test.go b/telemetry/telemetry_test.go index 755a333946..619c2abd7c 100644 --- a/telemetry/telemetry_test.go +++ b/telemetry/telemetry_test.go @@ -190,7 +190,8 @@ func TestServerCloseTelemetryConnection(t *testing.T) { } // Close server connection - tb.Close() + tb.Cancel() + time.Sleep(300 * time.Millisecond) if len(tb.connections) != 0 { t.Errorf("server didn't close all connections as expected") From 73033d8a1570da76ccea24eb58e7c6493724842e Mon Sep 17 00:00:00 2001 From: Tamilmani Manoharan Date: Thu, 7 Mar 2019 14:46:51 -0800 Subject: [PATCH 05/14] updated comments --- telemetry/telemetry_test.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/telemetry/telemetry_test.go b/telemetry/telemetry_test.go index 619c2abd7c..d2fa4ecd7a 100644 --- a/telemetry/telemetry_test.go +++ b/telemetry/telemetry_test.go @@ -176,20 +176,20 @@ func TestCloseTelemetryConnection(t *testing.T) { } func TestServerCloseTelemetryConnection(t *testing.T) { - // server telemetrybuffer + // create server telemetrybuffer and start server tb = NewTelemetryBuffer(hostAgentUrl) err := tb.StartServer() if err == nil { go tb.BufferAndPushData(0) } - // client telemetrybuffer + // create client telemetrybuffer and connect to server tb1 := NewTelemetryBuffer(hostAgentUrl) if err := tb1.Connect(); err != nil { t.Errorf("connection to telemetry server failed %v", err) } - // Close server connection + // Exit server thread and close server connection tb.Cancel() time.Sleep(300 * time.Millisecond) @@ -199,7 +199,6 @@ func TestServerCloseTelemetryConnection(t *testing.T) { // Close client connection tb1.Close() - } func TestSetReportState(t *testing.T) { From d0d45ce9cc0dd719cc82a9ad14b5ec797e157fa4 Mon Sep 17 00:00:00 2001 From: Tamilmani Manoharan Date: Thu, 7 Mar 2019 15:04:00 -0800 Subject: [PATCH 06/14] fixed an issue in telemtry server code if server closes client connection before --- telemetry/telemetrybuffer.go | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/telemetry/telemetrybuffer.go b/telemetry/telemetrybuffer.go index cb5436a984..926a69a56a 100644 --- a/telemetry/telemetrybuffer.go +++ b/telemetry/telemetrybuffer.go @@ -38,7 +38,7 @@ const ( cni = "CNI" ) -var telemetryLogger = log.NewLogger(logName, log.LevelInfo, log.TargetStderr) +var telemetryLogger = log.NewLogger(logName, log.LevelInfo, log.TargetLogfile) var payloadSize uint16 = 0 // TelemetryBuffer object @@ -78,10 +78,10 @@ func NewTelemetryBuffer(hostReportURL string) *TelemetryBuffer { tb.payload.NPMReports = make([]NPMReport, 0) tb.payload.CNSReports = make([]CNSReport, 0) - err := telemetryLogger.SetTarget(log.TargetLogfile) - if err != nil { - fmt.Printf("Failed to configure logging: %v\n", err) - } + // err := telemetryLogger.SetTarget(log.TargetLogfile) + // if err != nil { + // fmt.Printf("Failed to configure logging: %v\n", err) + // } return &tb } @@ -135,11 +135,13 @@ func (tb *TelemetryBuffer) StartServer() error { telemetryLogger.Printf("Server closing client connection") for index, value := range tb.connections { if value == conn { + telemetryLogger.Printf("Server closing client connection2") conn.Close() tb.connections = remove(tb.connections, index) return } } + return } } }() From 28c292e54e618385e9f47a2a82c5f372c71bfa8d Mon Sep 17 00:00:00 2001 From: Tamilmani Manoharan Date: Thu, 7 Mar 2019 15:18:16 -0800 Subject: [PATCH 07/14] fixed removing tb connection from array --- telemetry/telemetrybuffer.go | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/telemetry/telemetrybuffer.go b/telemetry/telemetrybuffer.go index 926a69a56a..61b35ca3d7 100644 --- a/telemetry/telemetrybuffer.go +++ b/telemetry/telemetrybuffer.go @@ -133,14 +133,23 @@ func (tb *TelemetryBuffer) StartServer() error { } } else { telemetryLogger.Printf("Server closing client connection") - for index, value := range tb.connections { + var index int + var value net.Conn + var found bool + + for index, value = range tb.connections { if value == conn { telemetryLogger.Printf("Server closing client connection2") conn.Close() - tb.connections = remove(tb.connections, index) - return + found = true + break } } + + if found { + tb.connections = remove(tb.connections, index) + } + return } } From 37a17f51e1fcfb48f1feac8f72a8cfb244cab22d Mon Sep 17 00:00:00 2001 From: Tamilmani Manoharan Date: Thu, 7 Mar 2019 15:21:16 -0800 Subject: [PATCH 08/14] removed dead code --- telemetry/telemetrybuffer.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/telemetry/telemetrybuffer.go b/telemetry/telemetrybuffer.go index 61b35ca3d7..f5ae6c54b5 100644 --- a/telemetry/telemetrybuffer.go +++ b/telemetry/telemetrybuffer.go @@ -78,11 +78,6 @@ func NewTelemetryBuffer(hostReportURL string) *TelemetryBuffer { tb.payload.NPMReports = make([]NPMReport, 0) tb.payload.CNSReports = make([]CNSReport, 0) - // err := telemetryLogger.SetTarget(log.TargetLogfile) - // if err != nil { - // fmt.Printf("Failed to configure logging: %v\n", err) - // } - return &tb } From 28a9c5222924643cd143f2d59ad353ec6429c749 Mon Sep 17 00:00:00 2001 From: Tamilmani Manoharan Date: Fri, 8 Mar 2019 11:37:11 -0800 Subject: [PATCH 09/14] removed duplicate logs --- telemetry/telemetrybuffer.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/telemetry/telemetrybuffer.go b/telemetry/telemetrybuffer.go index f5ae6c54b5..de083321f0 100644 --- a/telemetry/telemetrybuffer.go +++ b/telemetry/telemetrybuffer.go @@ -127,14 +127,13 @@ func (tb *TelemetryBuffer) StartServer() error { tb.data <- cnsReport } } else { - telemetryLogger.Printf("Server closing client connection") var index int var value net.Conn var found bool for index, value = range tb.connections { if value == conn { - telemetryLogger.Printf("Server closing client connection2") + telemetryLogger.Printf("Server closing client connection") conn.Close() found = true break From a7db81f10947b75585433142b52a44a9b9918376 Mon Sep 17 00:00:00 2001 From: Tamilmani Manoharan Date: Fri, 8 Mar 2019 17:27:00 -0800 Subject: [PATCH 10/14] added a check for removing element from tb.connections --- telemetry/cnstelemetry.go | 1 + telemetry/telemetrybuffer.go | 16 +++++++++++++--- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/telemetry/cnstelemetry.go b/telemetry/cnstelemetry.go index b5e15eff04..a554def7f7 100644 --- a/telemetry/cnstelemetry.go +++ b/telemetry/cnstelemetry.go @@ -80,6 +80,7 @@ CONNECT: if err == nil { // If write fails, try to re-establish connections as server/client if _, err = telemetryBuffer.Write(report); err != nil { + log.Printf("[CNS-Telemetry] Telemetry write failed: %v", err) telemetryBuffer.Cancel() goto CONNECT } diff --git a/telemetry/telemetrybuffer.go b/telemetry/telemetrybuffer.go index de083321f0..05bf22b1bc 100644 --- a/telemetry/telemetrybuffer.go +++ b/telemetry/telemetrybuffer.go @@ -38,7 +38,7 @@ const ( cni = "CNI" ) -var telemetryLogger = log.NewLogger(logName, log.LevelInfo, log.TargetLogfile) +var telemetryLogger = log.NewLogger(logName, log.LevelInfo, log.TargetStderr) var payloadSize uint16 = 0 // TelemetryBuffer object @@ -78,12 +78,22 @@ func NewTelemetryBuffer(hostReportURL string) *TelemetryBuffer { tb.payload.NPMReports = make([]NPMReport, 0) tb.payload.CNSReports = make([]CNSReport, 0) + err := telemetryLogger.SetTarget(log.TargetLogfile) + if err != nil { + fmt.Printf("Failed to configure logging: %v\n", err) + } + return &tb } func remove(s []net.Conn, i int) []net.Conn { - s[i] = s[len(s)-1] - return s[:len(s)-1] + if len(s) > 0 && i < len(s) { + s[i] = s[len(s)-1] + return s[:len(s)-1] + } + + telemetryLogger.Printf("tb connections remove failed index %v len %v", i, len(s)) + return s } // Starts Telemetry server listening on unix domain socket From be8c43b51d96c78809bad73f8c434a4f714e4943 Mon Sep 17 00:00:00 2001 From: Tamilmani Manoharan Date: Fri, 8 Mar 2019 18:36:25 -0800 Subject: [PATCH 11/14] added mutex lock as multiple threads accessing tb.connections slice --- telemetry/telemetry_test.go | 9 +++++++-- telemetry/telemetrybuffer.go | 13 ++++++++++++- 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/telemetry/telemetry_test.go b/telemetry/telemetry_test.go index d2fa4ecd7a..8602ab0d6c 100644 --- a/telemetry/telemetry_test.go +++ b/telemetry/telemetry_test.go @@ -193,12 +193,17 @@ func TestServerCloseTelemetryConnection(t *testing.T) { tb.Cancel() time.Sleep(300 * time.Millisecond) - if len(tb.connections) != 0 { - t.Errorf("server didn't close all connections as expected") + b := []byte("tamil") + if _, err := tb1.Write(b); err == nil { + t.Errorf("Client couldn't recognise server close") } // Close client connection tb1.Close() + + if len(tb.connections) != 0 { + t.Errorf("All connections not closed as expected") + } } func TestSetReportState(t *testing.T) { diff --git a/telemetry/telemetrybuffer.go b/telemetry/telemetrybuffer.go index 05bf22b1bc..9ee8a6b948 100644 --- a/telemetry/telemetrybuffer.go +++ b/telemetry/telemetrybuffer.go @@ -12,6 +12,7 @@ import ( "net" "net/http" "strings" + "sync" "time" "github.com/Azure/azure-container-networking/common" @@ -52,6 +53,7 @@ type TelemetryBuffer struct { Connected bool data chan interface{} cancel chan bool + mutex sync.Mutex } // Payload object holds the different types of reports @@ -112,7 +114,9 @@ func (tb *TelemetryBuffer) StartServer() error { // Spawn worker goroutines to communicate with client conn, err := tb.listener.Accept() if err == nil { + tb.mutex.Lock() tb.connections = append(tb.connections, conn) + tb.mutex.Unlock() go func() { for { reportStr, err := read(conn) @@ -141,6 +145,9 @@ func (tb *TelemetryBuffer) StartServer() error { var value net.Conn var found bool + tb.mutex.Lock() + defer tb.mutex.Unlock() + for index, value = range tb.connections { if value == conn { telemetryLogger.Printf("Server closing client connection") @@ -159,6 +166,7 @@ func (tb *TelemetryBuffer) StartServer() error { } }() } else { + telemetryLogger.Printf("Telemetry Server accept error %v", err) return } } @@ -254,9 +262,12 @@ func (tb *TelemetryBuffer) Close() { tb.listener = nil } + tb.mutex.Lock() + defer tb.mutex.Unlock() + for _, conn := range tb.connections { if conn != nil { - telemetryLogger.Printf("connection close") + telemetryLogger.Printf("connection close as server closed") conn.Close() } } From 61dc8225dde9af419f31d6b8286d3e9582845c4f Mon Sep 17 00:00:00 2001 From: Tamilmani Manoharan Date: Fri, 8 Mar 2019 18:38:12 -0800 Subject: [PATCH 12/14] moved the condition before client close --- telemetry/telemetry_test.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/telemetry/telemetry_test.go b/telemetry/telemetry_test.go index 8602ab0d6c..30c1042bdb 100644 --- a/telemetry/telemetry_test.go +++ b/telemetry/telemetry_test.go @@ -198,12 +198,13 @@ func TestServerCloseTelemetryConnection(t *testing.T) { t.Errorf("Client couldn't recognise server close") } - // Close client connection - tb1.Close() - if len(tb.connections) != 0 { t.Errorf("All connections not closed as expected") } + + // Close client connection + tb1.Close() + } func TestSetReportState(t *testing.T) { From 489256ada3af579aaf91b7c24f85ed58157c7ae5 Mon Sep 17 00:00:00 2001 From: Tamilmani Manoharan Date: Fri, 8 Mar 2019 18:52:07 -0800 Subject: [PATCH 13/14] added one more UT to fix code coverage --- telemetry/telemetry_test.go | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/telemetry/telemetry_test.go b/telemetry/telemetry_test.go index 30c1042bdb..53ffb0da4c 100644 --- a/telemetry/telemetry_test.go +++ b/telemetry/telemetry_test.go @@ -204,7 +204,37 @@ func TestServerCloseTelemetryConnection(t *testing.T) { // Close client connection tb1.Close() +} + +func TestClientCloseTelemetryConnection(t *testing.T) { + // create server telemetrybuffer and start server + tb = NewTelemetryBuffer(hostAgentUrl) + err := tb.StartServer() + if err == nil { + go tb.BufferAndPushData(0) + } + + // create client telemetrybuffer and connect to server + tb1 := NewTelemetryBuffer(hostAgentUrl) + if err := tb1.Connect(); err != nil { + t.Errorf("connection to telemetry server failed %v", err) + } + // Close client connection + tb1.Close() + time.Sleep(300 * time.Millisecond) + + if len(tb.connections) != 0 { + t.Errorf("All connections not closed as expected") + } + + b := []byte("tamil") + if _, err := tb1.Write(b); err == nil { + t.Errorf("write succeeded even after client close") + } + + // Exit server thread and close server connection + tb.Cancel() } func TestSetReportState(t *testing.T) { From ca62f538de76106e9f9c3b313cac2134be6c96ea Mon Sep 17 00:00:00 2001 From: Tamilmani Manoharan Date: Mon, 11 Mar 2019 18:09:27 -0700 Subject: [PATCH 14/14] fixed the ut. shud not write after close --- telemetry/telemetry_test.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/telemetry/telemetry_test.go b/telemetry/telemetry_test.go index 53ffb0da4c..a12703d107 100644 --- a/telemetry/telemetry_test.go +++ b/telemetry/telemetry_test.go @@ -228,11 +228,6 @@ func TestClientCloseTelemetryConnection(t *testing.T) { t.Errorf("All connections not closed as expected") } - b := []byte("tamil") - if _, err := tb1.Write(b); err == nil { - t.Errorf("write succeeded even after client close") - } - // Exit server thread and close server connection tb.Cancel() }