From 6bf436dfd22845441c0b248f33fbcb03eb23d97e Mon Sep 17 00:00:00 2001 From: Srijeet Chatterjee Date: Wed, 22 Dec 2021 15:32:11 -0700 Subject: [PATCH 1/3] TM should not overwrite monitoring snapshot data with crconfig snapshot data --- CHANGELOG.md | 1 + traffic_monitor/towrap/towrap.go | 14 ++++++- .../monitoring/monitoring.go | 37 +++++++++++++++++-- .../monitoring/monitoring_test.go | 11 ++++-- 4 files changed, 55 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 664b19fdcc..ca13c0bfff 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -50,6 +50,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/). - Traffic Portal no longer uses `ruby compass` to compile sass and now uses `dart-sass`. - Changed Invalidation Jobs throughout (TO, TP, T3C, etc.) to account for the ability to do both REFRESH and REFETCH requests for resources. - The `admin` Role is now always guaranteed to exist, and can't be deleted or modified. +- [#6376](https://github.com/apache/trafficcontrol/issues/6376) Updated TO/TM so that TM doesn't overwrite monitoring snapshot data with CR config snapshot data. - Updated `t3c-apply` to reduce mutable state in `TrafficOpsReq` struct. - Updated Golang dependencies diff --git a/traffic_monitor/towrap/towrap.go b/traffic_monitor/towrap/towrap.go index fb330a38f9..f5afa522fd 100644 --- a/traffic_monitor/towrap/towrap.go +++ b/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 m := tc.TrafficMonitor{} if mon.Port != nil { diff --git a/traffic_ops/traffic_ops_golang/monitoring/monitoring.go b/traffic_ops/traffic_ops_golang/monitoring/monitoring.go index 27f5bb579d..d2e7c95225 100644 --- a/traffic_ops/traffic_ops_golang/monitoring/monitoring.go +++ b/traffic_ops/traffic_ops_golang/monitoring/monitoring.go @@ -24,6 +24,7 @@ package monitoring import ( "database/sql" "fmt" + "net" "strconv" "strings" @@ -148,7 +149,7 @@ func GetMonitoringJSON(tx *sql.Tx, cdnName string) (*Monitoring, error) { return nil, fmt.Errorf("error getting profiles: %v", err) } - deliveryServices, err := getDeliveryServices(tx) + deliveryServices, err := getDeliveryServices(tx, cdnName) if err != nil { return nil, fmt.Errorf("error getting deliveryservices: %v", err) } @@ -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) + } monitors = append(monitors, Monitor{ BasicServer: BasicServer{ CommonServerProperties: CommonServerProperties{ @@ -304,6 +331,8 @@ AND cdn.name = $3 HostName: hostName.String, FQDN: fqdn.String, }, + IP: ipStr, + IP6: ipStr6, }, }) } else if (strings.HasPrefix(ttype.String, "EDGE") || strings.HasPrefix(ttype.String, "MID")) && @@ -441,13 +470,15 @@ WHERE pr.config_file = $2; return profilesArr, nil } -func getDeliveryServices(tx *sql.Tx) ([]DeliveryService, error) { +func getDeliveryServices(tx *sql.Tx, cdnName string) ([]DeliveryService, error) { query := ` SELECT ds.xml_id, ds.global_max_tps, ds.global_max_mbps FROM deliveryservice ds + JOIN cdn on cdn.id = ds.cdn_id WHERE ds.active = true + AND cdn.name=$1 ` - rows, err := tx.Query(query) + rows, err := tx.Query(query, cdnName) if err != nil { return nil, err } diff --git a/traffic_ops/traffic_ops_golang/monitoring/monitoring_test.go b/traffic_ops/traffic_ops_golang/monitoring/monitoring_test.go index 6f727579f5..4e9fc0baa7 100644 --- a/traffic_ops/traffic_ops_golang/monitoring/monitoring_test.go +++ b/traffic_ops/traffic_ops_golang/monitoring/monitoring_test.go @@ -89,9 +89,6 @@ func TestGetMonitoringServers(t *testing.T) { } else { testCache = otherCache } - if len(cacheServer.Interfaces) != len(testCache.Interfaces) { - t.Errorf("got %v caches, expecting %v", len(cacheServer.Interfaces), len(testCache.Interfaces)) - } for _, interf := range testCache.Interfaces { if len(interf.IPAddresses) != 4 { @@ -329,7 +326,7 @@ func TestGetDeliveryServices(t *testing.T) { t.Fatalf("creating transaction: %v", err) } - sqlDeliveryservices, err := getDeliveryServices(tx) + sqlDeliveryservices, err := getDeliveryServices(tx, "cdn") if err != nil { t.Errorf("getDeliveryServices expected: nil error, actual: %v", err) } @@ -678,6 +675,8 @@ func createMockMonitor() Monitor { HostName: "monitorHost", FQDN: "monitorFqdn.me", }, + IP: "5.6.7.8", + IP6: "2020::4", }, } } @@ -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") + ipAddressRows = ipAddressRows.AddRow("2020::4", "2001:0db8:85a3:0000:0000:8a2e:0370:7334", false, 2, "monitorCacheInterface") serverRows = serverRows.AddRow("noHostname", "noFqdn", "noStatus", "noGroup", 0, router.Profile, RouterType, "noHashid", 3) mock.ExpectQuery("SELECT (.+) FROM interface i (.+)").WithArgs(cdn).WillReturnRows(interfaceRows) mock.ExpectQuery("SELECT (.+) FROM ip_address ip (.+)").WillReturnRows(ipAddressRows) From 17e49a5e4e3aa126600b4ec0f4e5ee1e92b349c2 Mon Sep 17 00:00:00 2001 From: Srijeet Chatterjee Date: Tue, 4 Jan 2022 12:08:09 -0700 Subject: [PATCH 2/3] code review fixes --- traffic_monitor/towrap/towrap.go | 12 +--- .../monitoring/monitoring.go | 6 +- .../monitoring/monitoring_test.go | 71 +++++++++++++++++-- 3 files changed, 68 insertions(+), 21 deletions(-) diff --git a/traffic_monitor/towrap/towrap.go b/traffic_monitor/towrap/towrap.go index f5afa522fd..d575e581a0 100644 --- a/traffic_monitor/towrap/towrap.go +++ b/traffic_monitor/towrap/towrap.go @@ -680,20 +680,11 @@ func (s TrafficOpsSessionThreadsafe) MonitorCDN(hostName string) (string, error) // Traffic Monitors and Delivery Services found in a CDN Snapshot, and wipe out // all of those that already existed in the configuration map. func CreateMonitorConfig(crConfig tc.CRConfig, mc *tc.TrafficMonitorConfigMap) (*tc.TrafficMonitorConfigMap, error) { - // For unknown reasons, this function used to overwrite the passed set of - // TrafficServer objects. That was problematic, tc.CRConfig structures don't - // contain the same amount of information about their "equivalent" - // ContentServers. - // TODO: This is still overwriting TM instances found in the monitoring - // config - why? It's also doing that for Delivery Services, but that's - // necessary until issue #3528 is resolved. - // Dump the "live" monitoring.json monitors, and populate with the // "snapshotted" CRConfig 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 != "" { @@ -704,8 +695,7 @@ func CreateMonitorConfig(crConfig tc.CRConfig, mc *tc.TrafficMonitorConfigMap) ( } else { continue } - // monitorProfile = *mon.Profile - m := tc.TrafficMonitor{} + m := mc.TrafficMonitor[name] if mon.Port != nil { m.Port = *mon.Port } else { diff --git a/traffic_ops/traffic_ops_golang/monitoring/monitoring.go b/traffic_ops/traffic_ops_golang/monitoring/monitoring.go index d2e7c95225..be7db0e8ee 100644 --- a/traffic_ops/traffic_ops_golang/monitoring/monitoring.go +++ b/traffic_ops/traffic_ops_golang/monitoring/monitoring.go @@ -304,9 +304,9 @@ AND cdn.name = $3 if ip == nil { continue } - if ip.To4() != nil { + if ipStr == "" && ip.To4() != nil { ipStr = ipAddress.Address - } else if ip.To16() != nil { + } else if ipStr6 == "" && ip.To16() != nil { ipStr6 = ipAddress.Address } if ipStr != "" && ipStr6 != "" { @@ -318,8 +318,6 @@ AND cdn.name = $3 break } } - } else { - fmt.Println(interfacesByNameAndServer) } monitors = append(monitors, Monitor{ BasicServer: BasicServer{ diff --git a/traffic_ops/traffic_ops_golang/monitoring/monitoring_test.go b/traffic_ops/traffic_ops_golang/monitoring/monitoring_test.go index 4e9fc0baa7..e3b21a3430 100644 --- a/traffic_ops/traffic_ops_golang/monitoring/monitoring_test.go +++ b/traffic_ops/traffic_ops_golang/monitoring/monitoring_test.go @@ -116,6 +116,36 @@ func TestGetMonitoringServers(t *testing.T) { if err := mock.ExpectationsWereMet(); err != nil { t.Errorf("there were unfulfilled expections: %s", err) } + + // Now test by setting the monitor without an IPv4 address + monitor.IP = "" + + mock.ExpectBegin() + setupMockGetMonitoringServersWithoutIPv4(mock, monitor, router, []Cache{cache, otherCache, cache3}, []uint64{cacheID, otherCacheID, cache3ID}, cdn) + + dbCtx, f = context.WithTimeout(context.Background(), time.Duration(10)*time.Second) + defer f() + tx, err = db.BeginTx(dbCtx, nil) + if err != nil { + t.Fatalf("creating transaction: %v", err) + } + + monitors, _, _, err = getMonitoringServers(tx, cdn) + if err != nil { + t.Fatalf("getMonitoringServers expected: nil error, actual: %v", err) + } + + if len(monitors) != 1 { + t.Fatalf("getMonitoringServers expected: len(monitors) == 1, actual: %v", len(monitors)) + } + sqlMonitor = monitors[0] + if sqlMonitor != monitor { + t.Errorf("getMonitoringServers expected: monitor == %+v, actual: %+v", monitor, sqlMonitor) + } + + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("there were unfulfilled expections: %s", err) + } } func TestGetProfileWithParams(t *testing.T) { @@ -675,17 +705,46 @@ func createMockMonitor() Monitor { HostName: "monitorHost", FQDN: "monitorFqdn.me", }, - IP: "5.6.7.8", - IP6: "2020::4", + IP: "5.6.7.10", + IP6: "2020::10", }, } } +func setupMockGetMonitoringServersWithoutIPv4(mock sqlmock.Sqlmock, monitor Monitor, router Router, caches []Cache, cacheIDs []uint64, cdn string) { + serverRows := sqlmock.NewRows([]string{"hostName", "fqdn", "status", "cachegroup", "port", "profile", "type", "hashId", "serverID"}) + interfaceRows := sqlmock.NewRows([]string{"name", "max_bandwidth", "mtu", "monitor", "server"}) + ipAddressRows := sqlmock.NewRows([]string{"address", "gateway", "service_address", "server", "interface"}) + serverRows = serverRows.AddRow(monitor.HostName, monitor.FQDN, monitor.Status, monitor.Cachegroup, monitor.Port, monitor.Profile, MonitorType, "noHash", 5) + for index, cache := range caches { + serverRows = serverRows.AddRow(cache.HostName, cache.FQDN, cache.Status, cache.Cachegroup, cache.Port, cache.Profile, cache.Type, cache.HashID, cacheIDs[index]) + + interfaceRows = interfaceRows.AddRow("none", nil, 1500, false, 0) + for _, interf := range cache.Interfaces { + interfaceRows = interfaceRows.AddRow(interf.Name, interf.MaxBandwidth, interf.MTU, interf.Monitor, cacheIDs[index]) + + for _, ip := range interf.IPAddresses { + ipAddressRows = ipAddressRows.AddRow(ip.Address, ip.Gateway, ip.ServiceAddress, cacheIDs[index], interf.Name) + //Create two orphaned records + ipAddressRows = ipAddressRows.AddRow("0.0.0.0", "0.0.0.0", false, 0, interf.Name) + ipAddressRows = ipAddressRows.AddRow("0.0.0.0", "0.0.0.0", false, cacheIDs[index], "none") + } + } + } + // Add an interface and only ipv6 ip address for the monitor cache + interfaceRows = interfaceRows.AddRow("monitorCacheInterface", nil, 1500, false, 5) + ipAddressRows = ipAddressRows.AddRow("2020::10", "2001:0db8:85a3:0000:0000:8a2e:0370:7334", false, 5, "monitorCacheInterface") + serverRows = serverRows.AddRow("noHostname", "noFqdn", "noStatus", "noGroup", 0, router.Profile, RouterType, "noHashid", 3) + mock.ExpectQuery("SELECT (.+) FROM interface i (.+)").WithArgs(cdn).WillReturnRows(interfaceRows) + mock.ExpectQuery("SELECT (.+) FROM ip_address ip (.+)").WillReturnRows(ipAddressRows) + mock.ExpectQuery("SELECT (.+) FROM server me (.+)").WithArgs(cdn).WillReturnRows(serverRows) +} + func setupMockGetMonitoringServers(mock sqlmock.Sqlmock, monitor Monitor, router Router, caches []Cache, cacheIDs []uint64, cdn string) { serverRows := sqlmock.NewRows([]string{"hostName", "fqdn", "status", "cachegroup", "port", "profile", "type", "hashId", "serverID"}) interfaceRows := sqlmock.NewRows([]string{"name", "max_bandwidth", "mtu", "monitor", "server"}) ipAddressRows := sqlmock.NewRows([]string{"address", "gateway", "service_address", "server", "interface"}) - serverRows = serverRows.AddRow(monitor.HostName, monitor.FQDN, monitor.Status, monitor.Cachegroup, monitor.Port, monitor.Profile, MonitorType, "noHash", 2) + serverRows = serverRows.AddRow(monitor.HostName, monitor.FQDN, monitor.Status, monitor.Cachegroup, monitor.Port, monitor.Profile, MonitorType, "noHash", 5) for index, cache := range caches { serverRows = serverRows.AddRow(cache.HostName, cache.FQDN, cache.Status, cache.Cachegroup, cache.Port, cache.Profile, cache.Type, cache.HashID, cacheIDs[index]) @@ -702,9 +761,9 @@ 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") - ipAddressRows = ipAddressRows.AddRow("2020::4", "2001:0db8:85a3:0000:0000:8a2e:0370:7334", false, 2, "monitorCacheInterface") + interfaceRows = interfaceRows.AddRow("monitorCacheInterface", nil, 1500, false, 5) + ipAddressRows = ipAddressRows.AddRow("5.6.7.10", "10.0.0.0", true, 5, "monitorCacheInterface") + ipAddressRows = ipAddressRows.AddRow("2020::10", "2001:0db8:85a3:0000:0000:8a2e:0370:7334", false, 5, "monitorCacheInterface") serverRows = serverRows.AddRow("noHostname", "noFqdn", "noStatus", "noGroup", 0, router.Profile, RouterType, "noHashid", 3) mock.ExpectQuery("SELECT (.+) FROM interface i (.+)").WithArgs(cdn).WillReturnRows(interfaceRows) mock.ExpectQuery("SELECT (.+) FROM ip_address ip (.+)").WillReturnRows(ipAddressRows) From 281307e7e806e0de3b28774e15a716d3a7219a0f Mon Sep 17 00:00:00 2001 From: Srijeet Chatterjee Date: Thu, 6 Jan 2022 12:06:51 -0700 Subject: [PATCH 3/3] remove unneeded comment --- traffic_monitor/towrap/towrap.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/traffic_monitor/towrap/towrap.go b/traffic_monitor/towrap/towrap.go index d575e581a0..3a47ddbcf7 100644 --- a/traffic_monitor/towrap/towrap.go +++ b/traffic_monitor/towrap/towrap.go @@ -677,8 +677,7 @@ func (s TrafficOpsSessionThreadsafe) MonitorCDN(hostName string) (string, error) } // CreateMonitorConfig modifies the passed TrafficMonitorConfigMap to add the -// Traffic Monitors and Delivery Services found in a CDN Snapshot, and wipe out -// all of those that already existed in the configuration map. +// Traffic Monitors and Delivery Services found in a CDN Snapshot func CreateMonitorConfig(crConfig tc.CRConfig, mc *tc.TrafficMonitorConfigMap) (*tc.TrafficMonitorConfigMap, error) { // Dump the "live" monitoring.json monitors, and populate with the // "snapshotted" CRConfig