From dbe45aa7c71ca1e55fed38599275907f9f23330d Mon Sep 17 00:00:00 2001 From: ocket8888 Date: Wed, 21 Oct 2020 18:43:00 -0600 Subject: [PATCH] Remove unnecessary JOIN (#5184) * Remove unnecessary JOIN * Fix tests, also some context leaks * Add to CHANGELOG (cherry picked from commit 330ac87294439d617c59047642042edd2cc49172) --- CHANGELOG.md | 2 + .../monitoring/monitoring.go | 23 ++++------- .../monitoring/monitoring_test.go | 41 ++++++++----------- 3 files changed, 27 insertions(+), 39 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ea22362104..5e7e813ced 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,8 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/). ### Fixed - Fixed #5188 - DSR (delivery service request) incorrectly marked as complete and error message not displaying when DSR fulfilled and DS update fails in Traffic Portal. [Related Github issues](https://github.com/apache/trafficcontrol/issues/5188) - Fixed #5006 - Traffic Ops now generates the Monitoring on-the-fly if the snapshot doesn't exist, and logs an error. This fixes upgrading to 4.x to not break the CDN until a Snapshot is done. +- Fixed #5180 - Global Max Mbps and Tps is not send to TM +- Fixed #3528 - Fix Traffic Ops monitoring.json missing DeliveryServices - Fixed #5074 - Traffic Monitor logging "CreateStats not adding availability data for server: not found in DeliveryServices" for MID cachces - Fixed an issue that causes Traffic Router to mistakenly route to caches that had recently been set from ADMIN_DOWN to OFFLINE - Traffic Ops Ort: Disabled ntpd verification (ntpd is deprecated in CentOS) diff --git a/traffic_ops/traffic_ops_golang/monitoring/monitoring.go b/traffic_ops/traffic_ops_golang/monitoring/monitoring.go index f269025863..07519cd395 100644 --- a/traffic_ops/traffic_ops_golang/monitoring/monitoring.go +++ b/traffic_ops/traffic_ops_golang/monitoring/monitoring.go @@ -1,3 +1,5 @@ +// Package monitoring contains handlers and supporting logic for the +// /cdns/{{CDN Name}}/configs/monitoring Traffic Ops API endpoint. package monitoring /* @@ -130,7 +132,7 @@ func GetMonitoringJSON(tx *sql.Tx, cdnName string) (*Monitoring, error) { return nil, fmt.Errorf("error getting profiles: %v", err) } - deliveryServices, err := getDeliveryServices(tx, routers) + deliveryServices, err := getDeliveryServices(tx) if err != nil { return nil, fmt.Errorf("error getting deliveryservices: %v", err) } @@ -337,20 +339,13 @@ WHERE pr.config_file = $2; return profilesArr, nil } -func getDeliveryServices(tx *sql.Tx, routers []Router) ([]DeliveryService, error) { - profileNames := []string{} - for _, router := range routers { - profileNames = append(profileNames, router.Profile) - } - +func getDeliveryServices(tx *sql.Tx) ([]DeliveryService, error) { query := ` -SELECT ds.xml_id, ds.global_max_tps, ds.global_max_mbps -FROM deliveryservice ds -JOIN profile profile ON profile.id = ds.profile -WHERE profile.name = ANY($1) -AND ds.active = true -` - rows, err := tx.Query(query, pq.Array(profileNames)) + SELECT ds.xml_id, ds.global_max_tps, ds.global_max_mbps + FROM deliveryservice ds + WHERE ds.active = true + ` + rows, err := tx.Query(query) 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 053a9fdd0e..c4a59fbe64 100644 --- a/traffic_ops/traffic_ops_golang/monitoring/monitoring_test.go +++ b/traffic_ops/traffic_ops_golang/monitoring/monitoring_test.go @@ -151,7 +151,8 @@ func TestGetCachegroups(t *testing.T) { mock.ExpectQuery("SELECT").WithArgs(cdn).WillReturnRows(rows) - dbCtx, _ := context.WithTimeout(context.TODO(), time.Duration(10)*time.Second) + dbCtx, f := context.WithTimeout(context.TODO(), time.Duration(10)*time.Second) + defer f() tx, err := db.BeginTx(dbCtx, nil) if err != nil { t.Fatalf("creating transaction: %v", err) @@ -325,7 +326,8 @@ func TestGetProfiles(t *testing.T) { mock.ExpectQuery("SELECT").WithArgs(pq.Array(profileNames), CacheMonitorConfigFile).WillReturnRows(rows) - dbCtx, _ := context.WithTimeout(context.TODO(), time.Duration(10)*time.Second) + dbCtx, f := context.WithTimeout(context.TODO(), time.Duration(10)*time.Second) + defer f() tx, err := db.BeginTx(dbCtx, nil) if err != nil { t.Fatalf("creating transaction: %v", err) @@ -373,11 +375,6 @@ func TestGetDeliveryServices(t *testing.T) { db := sqlx.NewDb(mockDB, "sqlmock") defer db.Close() - router := Router{ - Type: RouterType, - Profile: "routerProfile", - } - deliveryservice := DeliveryService{ XMLID: "myDsid", TotalTPSThreshold: 42.42, @@ -386,7 +383,6 @@ func TestGetDeliveryServices(t *testing.T) { } deliveryservices := []DeliveryService{deliveryservice} - routers := []Router{router} mock.ExpectBegin() rows := sqlmock.NewRows([]string{"xml_id", "global_max_tps", "global_max_mbps"}) @@ -394,23 +390,22 @@ func TestGetDeliveryServices(t *testing.T) { rows = rows.AddRow(deliveryservice.XMLID, deliveryservice.TotalTPSThreshold, deliveryservice.TotalKBPSThreshold/KilobitsPerMegabit) } - profileNames := []string{router.Profile} - - mock.ExpectQuery("SELECT").WithArgs(pq.Array(profileNames)).WillReturnRows(rows) + mock.ExpectQuery("SELECT").WillReturnRows(rows) - dbCtx, _ := context.WithTimeout(context.TODO(), time.Duration(10)*time.Second) + dbCtx, f := context.WithTimeout(context.TODO(), time.Duration(10)*time.Second) + defer f() tx, err := db.BeginTx(dbCtx, nil) if err != nil { t.Fatalf("creating transaction: %v", err) } - sqlDeliveryservices, err := getDeliveryServices(tx, routers) + sqlDeliveryservices, err := getDeliveryServices(tx) if err != nil { - t.Errorf("getProfiles expected: nil error, actual: %v", err) + t.Errorf("getDeliveryServices expected: nil error, actual: %v", err) } if len(deliveryservices) != len(sqlDeliveryservices) { - t.Errorf("getProfiles expected: %+v actual: %+v", deliveryservices, sqlDeliveryservices) + t.Fatalf("getDeliveryServices expected: %+v actual: %+v", deliveryservices, sqlDeliveryservices) } for i, sqlDeliveryservice := range sqlDeliveryservices { @@ -448,7 +443,8 @@ func TestGetConfig(t *testing.T) { mock.ExpectQuery("SELECT").WillReturnRows(rows) - dbCtx, _ := context.WithTimeout(context.TODO(), time.Duration(10)*time.Second) + dbCtx, f := context.WithTimeout(context.TODO(), time.Duration(10)*time.Second) + defer f() tx, err := db.BeginTx(dbCtx, nil) if err != nil { t.Fatalf("creating transaction: %v", err) @@ -610,10 +606,6 @@ func TestGetMonitoringJSON(t *testing.T) { // // getDeliveryServices // - router := Router{ - Type: RouterType, - Profile: "routerProfile", - } deliveryservice := DeliveryService{ XMLID: "myDsid", @@ -630,9 +622,7 @@ func TestGetMonitoringJSON(t *testing.T) { rows = rows.AddRow(deliveryservice.XMLID, deliveryservice.TotalTPSThreshold, deliveryservice.TotalKBPSThreshold/KilobitsPerMegabit) } - profileNames := []string{router.Profile} - - mock.ExpectQuery("SELECT").WithArgs(pq.Array(profileNames)).WillReturnRows(rows) + mock.ExpectQuery("SELECT").WillReturnRows(rows) resp.Response.DeliveryServices = deliveryservices } { @@ -653,7 +643,8 @@ func TestGetMonitoringJSON(t *testing.T) { resp.Response.Config = config } - dbCtx, _ := context.WithTimeout(context.TODO(), time.Duration(10)*time.Second) + dbCtx, f := context.WithTimeout(context.TODO(), time.Duration(10)*time.Second) + defer f() tx, err := db.BeginTx(dbCtx, nil) if err != nil { t.Fatalf("creating transaction: %v", err) @@ -661,7 +652,7 @@ func TestGetMonitoringJSON(t *testing.T) { sqlResp, err := GetMonitoringJSON(tx, cdn) if err != nil { - t.Errorf("GetMonitoringJSON expected: nil error, actual: %v", err) + t.Fatalf("GetMonitoringJSON expected: nil error, actual: %v", err) } resp.Response.TrafficServers = sortCaches(resp.Response.TrafficServers)