[GitHub] [trafficcontrol] TaylorCFrey commented on a change in pull request #6443: TM should not overwrite monitoring snapshot data with crconfig snapsot data

[GitBox <gi...@apache.org>]

TaylorCFrey commented on a change in pull request #6443:
URL: https://github.com/apache/trafficcontrol/pull/6443#discussion_r777734624



##########
File path: traffic_monitor/towrap/towrap.go
##########
@@ -690,8 +690,20 @@ func CreateMonitorConfig(crConfig tc.CRConfig, mc *tc.TrafficMonitorConfigMap) (
 
 	// Dump the "live" monitoring.json monitors, and populate with the
 	// "snapshotted" CRConfig
-	mc.TrafficMonitor = map[string]tc.TrafficMonitor{}
+	if mc == nil {
+		return mc, errors.New("no TM configmap data")
+	}
+	//mc.TrafficMonitor = map[string]tc.TrafficMonitor{}
 	for name, mon := range crConfig.Monitors {
+		if tmData, ok := mc.TrafficMonitor[name]; ok {
+			if tmData.IP != "" && tmData.IP6 != "" {
+				continue
+			} else {
+				mc.TrafficMonitor[name] = tc.TrafficMonitor{}
+			}
+		} else {
+			continue
+		}
 		// monitorProfile = *mon.Profile

Review comment:
       Commented code to be removed.

##########
File path: traffic_monitor/towrap/towrap.go
##########
@@ -690,8 +690,20 @@ func CreateMonitorConfig(crConfig tc.CRConfig, mc *tc.TrafficMonitorConfigMap) (
 
 	// Dump the "live" monitoring.json monitors, and populate with the
 	// "snapshotted" CRConfig
-	mc.TrafficMonitor = map[string]tc.TrafficMonitor{}
+	if mc == nil {
+		return mc, errors.New("no TM configmap data")
+	}
+	//mc.TrafficMonitor = map[string]tc.TrafficMonitor{}

Review comment:
       Commented code to be removed.

##########
File path: traffic_monitor/towrap/towrap.go
##########
@@ -690,8 +690,20 @@ func CreateMonitorConfig(crConfig tc.CRConfig, mc *tc.TrafficMonitorConfigMap) (
 

Review comment:
       I believe the comment(s) above can be cleaned up. It looks like issue #3528 was resolved in a previous MR.

##########
File path: traffic_ops/traffic_ops_golang/monitoring/monitoring.go
##########
@@ -294,6 +295,32 @@ AND cdn.name = $3
 		cacheStatus := tc.CacheStatusFromString(status.String)
 
 		if ttype.String == tc.MonitorTypeName {
+			var ipStr, ipStr6 string
+			var gotBothIPs bool

Review comment:
       Are we gauranteed to get both ipv4 and ipv6? Will there be a case where one or the other alone is acceptable?

##########
File path: traffic_ops/traffic_ops_golang/monitoring/monitoring.go
##########
@@ -294,6 +295,32 @@ AND cdn.name = $3
 		cacheStatus := tc.CacheStatusFromString(status.String)
 
 		if ttype.String == tc.MonitorTypeName {
+			var ipStr, ipStr6 string
+			var gotBothIPs bool
+			if _, ok := interfacesByNameAndServer[int(serverID.Int64)]; ok {
+				for _, interf := range interfacesByNameAndServer[int(serverID.Int64)] {
+					for _, ipAddress := range interf.IPAddresses {
+						ip := net.ParseIP(ipAddress.Address)
+						if ip == nil {
+							continue
+						}
+						if ip.To4() != nil {
+							ipStr = ipAddress.Address
+						} else if ip.To16() != nil {
+							ipStr6 = ipAddress.Address
+						}
+						if ipStr != "" && ipStr6 != "" {
+							gotBothIPs = true
+							break
+						}
+					}
+					if gotBothIPs {
+						break
+					}
+				}
+			} else {
+				fmt.Println(interfacesByNameAndServer)

Review comment:
       Where is this printing to?

##########
File path: traffic_monitor/towrap/towrap.go
##########
@@ -690,8 +690,20 @@ func CreateMonitorConfig(crConfig tc.CRConfig, mc *tc.TrafficMonitorConfigMap) (
 
 	// Dump the "live" monitoring.json monitors, and populate with the
 	// "snapshotted" CRConfig
-	mc.TrafficMonitor = map[string]tc.TrafficMonitor{}
+	if mc == nil {
+		return mc, errors.New("no TM configmap data")
+	}
+	//mc.TrafficMonitor = map[string]tc.TrafficMonitor{}
 	for name, mon := range crConfig.Monitors {
+		if tmData, ok := mc.TrafficMonitor[name]; ok {
+			if tmData.IP != "" && tmData.IP6 != "" {
+				continue
+			} else {
+				mc.TrafficMonitor[name] = tc.TrafficMonitor{}

Review comment:
       It looks like, if I'm reading this correctly, if there isn't a matching `TrafficMonitor` struct instance, we create one here and assign it. However, a few lines down, we create a new `TrafficMonitor` struct regardless with
   ```
   m := tc.TrafficMonitor{}
   ```
   and, ultimately, assign it to the name/key in the map. I'm wondering if these if-statement are actually necessary or doing what is intended.
   
   Since we're still created a new struct and assigning it I'm not entirely sure we aren't still overwriting.

##########
File path: traffic_ops/traffic_ops_golang/monitoring/monitoring_test.go
##########
@@ -702,6 +701,10 @@ func setupMockGetMonitoringServers(mock sqlmock.Sqlmock, monitor Monitor, router
 			}
 		}
 	}
+	// Add an interface and ip addresses for the monitor cache
+	interfaceRows = interfaceRows.AddRow("monitorCacheInterface", nil, 1500, false, 2)
+	ipAddressRows = ipAddressRows.AddRow("5.6.7.8", "10.0.0.0", true, 2, "monitorCacheInterface")

Review comment:
       Any case or instance where we would need a negative test? Such as missing ip4v/ipv6/both?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org