From 39529a5c1af7f33aee21d686e23a5164d5d9be4a Mon Sep 17 00:00:00 2001 From: Srijeet Chatterjee Date: Wed, 16 Dec 2020 08:34:12 -0700 Subject: [PATCH 01/11] wip --- traffic_ops/traffic_ops_golang/server/servers.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/traffic_ops/traffic_ops_golang/server/servers.go b/traffic_ops/traffic_ops_golang/server/servers.go index 9ed4df102f..ed07bdb74c 100644 --- a/traffic_ops/traffic_ops_golang/server/servers.go +++ b/traffic_ops/traffic_ops_golang/server/servers.go @@ -73,12 +73,10 @@ JOIN deliveryservice td ON td.topology = tc.topology JOIN type t ON s.type = t.id LEFT JOIN deliveryservice_server dss ON s.id = dss."server" + AND dss.deliveryservice IS NOT NULL AND dss.deliveryservice = td.id WHERE td.id = :dsId -AND ( - t.name != '` + tc.OriginTypeName + `' - OR dss.deliveryservice IS NOT NULL -)), +), ` /* language=SQL */ From ea1188389fe6f368243cb6f2edcdf604d8a11228 Mon Sep 17 00:00:00 2001 From: Srijeet Chatterjee Date: Thu, 17 Dec 2020 17:09:21 -0700 Subject: [PATCH 02/11] wip --- .../traffic_ops_golang/server/servers.go | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/traffic_ops/traffic_ops_golang/server/servers.go b/traffic_ops/traffic_ops_golang/server/servers.go index ed07bdb74c..31f40b7bf3 100644 --- a/traffic_ops/traffic_ops_golang/server/servers.go +++ b/traffic_ops/traffic_ops_golang/server/servers.go @@ -795,6 +795,9 @@ func getServers(h http.Header, params map[string]string, tx *sqlx.Tx, user *auth queryAddition := "" dsHasRequiredCapabilities := false var cdnID int + var serverCountOrigin uint64 + rowsAffected := 0 + if dsIDStr, ok := params[`dsId`]; ok { // don't allow query on ds outside user's tenant dsID, err := strconv.Atoi(dsIDStr) @@ -834,6 +837,10 @@ func getServers(h http.Header, params map[string]string, tx *sqlx.Tx, user *auth } usesMids = dsType.UsesMidCache() log.Debugf("Servers for ds %d; uses mids? %v\n", dsID, usesMids) + row := tx.QueryRow(serverCountQuery+` JOIN deliveryservice_server dsorg ON dsorg.server = s.id WHERE t.name='ORG' AND dsorg.deliveryservice=$1`, dsID) + if err := row.Scan(&serverCountOrigin); err != nil { + return nil, 0, nil, fmt.Errorf("failed to read servers count: %v", err), http.StatusInternalServerError, nil + } } if _, ok := params[`topology`]; ok { @@ -858,7 +865,7 @@ func getServers(h http.Header, params map[string]string, tx *sqlx.Tx, user *auth return nil, 0, nil, fmt.Errorf("failed to get servers count: %v", err), http.StatusInternalServerError, nil } defer countRows.Close() - rowsAffected := 0 + rowsAffected = 0 for countRows.Next() { if err = countRows.Scan(&serverCount); err != nil { return nil, 0, nil, fmt.Errorf("failed to read servers count: %v", err), http.StatusInternalServerError, nil @@ -869,6 +876,7 @@ func getServers(h http.Header, params map[string]string, tx *sqlx.Tx, user *auth return nil, 0, nil, fmt.Errorf("incorrect rows returned for server count, want: 1 got: %v", rowsAffected), http.StatusInternalServerError, nil } + serverCount += serverCountOrigin serversList := []tc.ServerNullable{} if useIMS { runSecond, maxTime = ims.TryIfModifiedSinceQuery(tx, h, queryValues, selectMaxLastUpdatedQuery(queryAddition, where)) @@ -881,9 +889,16 @@ func getServers(h http.Header, params map[string]string, tx *sqlx.Tx, user *auth log.Debugln("Non IMS request") } + if dsIDStr, ok := params[`dsId`]; ok { + _, err := strconv.Atoi(dsIDStr) + if err != nil { + return nil, 0, errors.New("dsId must be an integer"), nil, http.StatusNotFound, nil + } + where += `UNION `+selectQuery + ` JOIN deliveryservice_server dsorg ON dsorg.server = s.id WHERE t.name='ORG' AND dsorg.deliveryservice=:dsId` + } query := selectQuery + queryAddition + where + orderBy + pagination + fmt.Println("Query is ", query) log.Debugln("Query is ", query) - rows, err := tx.NamedQuery(query, queryValues) if err != nil { return nil, serverCount, nil, errors.New("querying: " + err.Error()), http.StatusInternalServerError, nil From c2d77e5a1178a82f72046f80eb1f23e8b8fd9f96 Mon Sep 17 00:00:00 2001 From: Srijeet Chatterjee Date: Fri, 18 Dec 2020 12:30:53 -0700 Subject: [PATCH 03/11] modified query --- .../traffic_ops_golang/server/servers.go | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/traffic_ops/traffic_ops_golang/server/servers.go b/traffic_ops/traffic_ops_golang/server/servers.go index 31f40b7bf3..95c6062c26 100644 --- a/traffic_ops/traffic_ops_golang/server/servers.go +++ b/traffic_ops/traffic_ops_golang/server/servers.go @@ -73,10 +73,12 @@ JOIN deliveryservice td ON td.topology = tc.topology JOIN type t ON s.type = t.id LEFT JOIN deliveryservice_server dss ON s.id = dss."server" - AND dss.deliveryservice IS NOT NULL AND dss.deliveryservice = td.id WHERE td.id = :dsId -), +AND ( + t.name != '` + tc.OriginTypeName + `' + OR dss.deliveryservice IS NOT NULL +)), ` /* language=SQL */ @@ -889,15 +891,11 @@ func getServers(h http.Header, params map[string]string, tx *sqlx.Tx, user *auth log.Debugln("Non IMS request") } - if dsIDStr, ok := params[`dsId`]; ok { - _, err := strconv.Atoi(dsIDStr) - if err != nil { - return nil, 0, errors.New("dsId must be an integer"), nil, http.StatusNotFound, nil - } - where += `UNION `+selectQuery + ` JOIN deliveryservice_server dsorg ON dsorg.server = s.id WHERE t.name='ORG' AND dsorg.deliveryservice=:dsId` - } query := selectQuery + queryAddition + where + orderBy + pagination - fmt.Println("Query is ", query) + if _, ok := params[`dsId`]; ok { + query = `(` + selectQuery + queryAddition + where + orderBy + pagination + `) UNION ` + selectQuery + ` JOIN deliveryservice_server dsorg ON dsorg.server = s.id WHERE t.name='ORG' AND dsorg.deliveryservice=:dsId` + } + log.Debugln("Query is ", query) rows, err := tx.NamedQuery(query, queryValues) if err != nil { From 5bb06960d29c71ecd87afbf564a67720d99df3ee Mon Sep 17 00:00:00 2001 From: Srijeet Chatterjee Date: Fri, 18 Dec 2020 12:58:36 -0700 Subject: [PATCH 04/11] adding test --- ...vers_to_deliveryservice_assignment_test.go | 25 ++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/traffic_ops/testing/api/v3/servers_to_deliveryservice_assignment_test.go b/traffic_ops/testing/api/v3/servers_to_deliveryservice_assignment_test.go index 4ae11f4d30..3d51b6ecdd 100644 --- a/traffic_ops/testing/api/v3/servers_to_deliveryservice_assignment_test.go +++ b/traffic_ops/testing/api/v3/servers_to_deliveryservice_assignment_test.go @@ -17,6 +17,7 @@ package v3 import ( "net/http" "net/url" + "strconv" "testing" "time" @@ -219,10 +220,11 @@ func AssignTopologyBasedDeliveryService(t *testing.T) { } var found bool + var dsID int for _, ds := range response { - if ds.ID != nil && *ds.ID == *firstDS.ID { found = true + dsID = *ds.ID break } } @@ -230,6 +232,27 @@ func AssignTopologyBasedDeliveryService(t *testing.T) { if !found { t.Errorf(`Valid Server/DS assignment was not created!`) } + + _, _, err = TOSession.AssignServersToDeliveryService([]string{"denver-mso-org-01"}, "ds-top") + if err != nil { + t.Errorf("assigning ORG server to ds-top delivery service: %v", err.Error()) + } + params = url.Values{} + params.Add("dsId", strconv.Itoa(dsID)) + params.Add("type", "ORG") + responseServers, _, err := TOSession.GetServersWithHdr(¶ms, nil) + if err != nil { + t.Fatalf("getting servers for ds-top delivery service: %v", err.Error()) + } + if len(responseServers.Response) != 1 { + t.Fatalf("expected just one ORG server in the response, but got %d", len(responseServers.Response)) + } + if responseServers.Response[0].HostName == nil { + t.Fatal("expected a valid host name for the resulting ORG server, but got nothing") + } + if *responseServers.Response[0].HostName != "denver-mso-org-01" { + t.Errorf("expected host name of the resulting ORG server to be %v, but got %v", "denver-mso-org-01", *responseServers.Response[0].HostName) + } } func OriginAssignTopologyBasedDeliveryService(t *testing.T) { From c5b8732d6af2242ce8d383124e0a347f61f60351 Mon Sep 17 00:00:00 2001 From: Srijeet Chatterjee Date: Fri, 18 Dec 2020 14:41:57 -0700 Subject: [PATCH 05/11] Adding new test --- ...vers_to_deliveryservice_assignment_test.go | 24 +++++++++++++++---- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/traffic_ops/testing/api/v3/servers_to_deliveryservice_assignment_test.go b/traffic_ops/testing/api/v3/servers_to_deliveryservice_assignment_test.go index 3d51b6ecdd..ba30edd5d7 100644 --- a/traffic_ops/testing/api/v3/servers_to_deliveryservice_assignment_test.go +++ b/traffic_ops/testing/api/v3/servers_to_deliveryservice_assignment_test.go @@ -31,6 +31,7 @@ func TestAssignments(t *testing.T) { AssignIncorrectTestDeliveryService(t) AssignTopologyBasedDeliveryService(t) OriginAssignTopologyBasedDeliveryService(t) + OriginAssignTopologyBasedDeliveryServiceWithRequiredCapabilities(t) }) } @@ -220,11 +221,10 @@ func AssignTopologyBasedDeliveryService(t *testing.T) { } var found bool - var dsID int for _, ds := range response { + if ds.ID != nil && *ds.ID == *firstDS.ID { found = true - dsID = *ds.ID break } } @@ -232,17 +232,31 @@ func AssignTopologyBasedDeliveryService(t *testing.T) { if !found { t.Errorf(`Valid Server/DS assignment was not created!`) } +} + +func OriginAssignTopologyBasedDeliveryServiceWithRequiredCapabilities(t *testing.T) { + resp, _, err := TOSession.GetDeliveryServiceByXMLIDNullableWithHdr("ds-top-req-cap2", nil) + if err != nil { + t.Errorf("getting delivery service by xml ID: %v", err.Error()) + } + if len(resp) != 1 { + t.Fatalf("expected to get only one delivery service in the response, but got %d", len(resp)) + } + if resp[0].ID == nil { + t.Fatalf("no ID in the resulting delivery service") + } + dsID := *resp[0].ID - _, _, err = TOSession.AssignServersToDeliveryService([]string{"denver-mso-org-01"}, "ds-top") + params := url.Values{} + _, _, err = TOSession.AssignServersToDeliveryService([]string{"denver-mso-org-01"}, "ds-top-req-cap2") if err != nil { t.Errorf("assigning ORG server to ds-top delivery service: %v", err.Error()) } - params = url.Values{} params.Add("dsId", strconv.Itoa(dsID)) params.Add("type", "ORG") responseServers, _, err := TOSession.GetServersWithHdr(¶ms, nil) if err != nil { - t.Fatalf("getting servers for ds-top delivery service: %v", err.Error()) + t.Fatalf("getting servers for ds-top-req-cap2 delivery service: %v", err.Error()) } if len(responseServers.Response) != 1 { t.Fatalf("expected just one ORG server in the response, but got %d", len(responseServers.Response)) From 9505993ddbc0b613bfe3606d94040faf5d85ee68 Mon Sep 17 00:00:00 2001 From: Srijeet Chatterjee Date: Fri, 18 Dec 2020 15:37:45 -0700 Subject: [PATCH 06/11] adding test in the right file --- ...veryservices_required_capabilities_test.go | 36 ++++++++++++++++++ ...vers_to_deliveryservice_assignment_test.go | 37 ------------------- 2 files changed, 36 insertions(+), 37 deletions(-) diff --git a/traffic_ops/testing/api/v3/deliveryservices_required_capabilities_test.go b/traffic_ops/testing/api/v3/deliveryservices_required_capabilities_test.go index ed9ef4b118..0f230f2e06 100644 --- a/traffic_ops/testing/api/v3/deliveryservices_required_capabilities_test.go +++ b/traffic_ops/testing/api/v3/deliveryservices_required_capabilities_test.go @@ -19,6 +19,7 @@ import ( "fmt" "net/http" "net/url" + "strconv" "strings" "testing" "time" @@ -45,9 +46,44 @@ func TestDeliveryServicesRequiredCapabilities(t *testing.T) { func TestTopologyBasedDeliveryServicesRequiredCapabilities(t *testing.T) { WithObjs(t, []TCObj{CDNs, Types, Tenants, Users, Parameters, Profiles, Statuses, Divisions, Regions, PhysLocations, CacheGroups, Servers, ServerCapabilities, ServerServerCapabilitiesForTopologies, Topologies, DeliveryServices, TopologyBasedDeliveryServiceRequiredCapabilities}, func() { GetTestDeliveryServicesRequiredCapabilities(t) + OriginAssignTopologyBasedDeliveryServiceWithRequiredCapabilities(t) }) } +func OriginAssignTopologyBasedDeliveryServiceWithRequiredCapabilities(t *testing.T) { + resp, _, err := TOSession.GetDeliveryServiceByXMLIDNullableWithHdr("ds-top-req-cap2", nil) + if err != nil { + t.Errorf("getting delivery service by xml ID: %v", err.Error()) + } + if len(resp) != 1 { + t.Fatalf("expected to get only one delivery service in the response, but got %d", len(resp)) + } + if resp[0].ID == nil { + t.Fatalf("no ID in the resulting delivery service") + } + dsID := *resp[0].ID + params := url.Values{} + _, _, err = TOSession.AssignServersToDeliveryService([]string{"denver-mso-org-01"}, "ds-top-req-cap2") + if err != nil { + t.Errorf("assigning ORG server to ds-top delivery service: %v", err.Error()) + } + params.Add("dsId", strconv.Itoa(dsID)) + params.Add("type", "ORG") + responseServers, _, err := TOSession.GetServersWithHdr(¶ms, nil) + if err != nil { + t.Fatalf("getting servers for ds-top-req-cap2 delivery service: %v", err.Error()) + } + if len(responseServers.Response) != 1 { + t.Fatalf("expected just one ORG server in the response, but got %d", len(responseServers.Response)) + } + if responseServers.Response[0].HostName == nil { + t.Fatal("expected a valid host name for the resulting ORG server, but got nothing") + } + if *responseServers.Response[0].HostName != "denver-mso-org-01" { + t.Errorf("expected host name of the resulting ORG server to be %v, but got %v", "denver-mso-org-01", *responseServers.Response[0].HostName) + } +} + func GetTestDeliveryServicesRequiredCapabilitiesIMSAfterChange(t *testing.T, header http.Header) { data := testData.DeliveryServicesRequiredCapabilities ds1 := helperGetDeliveryServiceID(t, data[0].XMLID) diff --git a/traffic_ops/testing/api/v3/servers_to_deliveryservice_assignment_test.go b/traffic_ops/testing/api/v3/servers_to_deliveryservice_assignment_test.go index ba30edd5d7..4ae11f4d30 100644 --- a/traffic_ops/testing/api/v3/servers_to_deliveryservice_assignment_test.go +++ b/traffic_ops/testing/api/v3/servers_to_deliveryservice_assignment_test.go @@ -17,7 +17,6 @@ package v3 import ( "net/http" "net/url" - "strconv" "testing" "time" @@ -31,7 +30,6 @@ func TestAssignments(t *testing.T) { AssignIncorrectTestDeliveryService(t) AssignTopologyBasedDeliveryService(t) OriginAssignTopologyBasedDeliveryService(t) - OriginAssignTopologyBasedDeliveryServiceWithRequiredCapabilities(t) }) } @@ -234,41 +232,6 @@ func AssignTopologyBasedDeliveryService(t *testing.T) { } } -func OriginAssignTopologyBasedDeliveryServiceWithRequiredCapabilities(t *testing.T) { - resp, _, err := TOSession.GetDeliveryServiceByXMLIDNullableWithHdr("ds-top-req-cap2", nil) - if err != nil { - t.Errorf("getting delivery service by xml ID: %v", err.Error()) - } - if len(resp) != 1 { - t.Fatalf("expected to get only one delivery service in the response, but got %d", len(resp)) - } - if resp[0].ID == nil { - t.Fatalf("no ID in the resulting delivery service") - } - dsID := *resp[0].ID - - params := url.Values{} - _, _, err = TOSession.AssignServersToDeliveryService([]string{"denver-mso-org-01"}, "ds-top-req-cap2") - if err != nil { - t.Errorf("assigning ORG server to ds-top delivery service: %v", err.Error()) - } - params.Add("dsId", strconv.Itoa(dsID)) - params.Add("type", "ORG") - responseServers, _, err := TOSession.GetServersWithHdr(¶ms, nil) - if err != nil { - t.Fatalf("getting servers for ds-top-req-cap2 delivery service: %v", err.Error()) - } - if len(responseServers.Response) != 1 { - t.Fatalf("expected just one ORG server in the response, but got %d", len(responseServers.Response)) - } - if responseServers.Response[0].HostName == nil { - t.Fatal("expected a valid host name for the resulting ORG server, but got nothing") - } - if *responseServers.Response[0].HostName != "denver-mso-org-01" { - t.Errorf("expected host name of the resulting ORG server to be %v, but got %v", "denver-mso-org-01", *responseServers.Response[0].HostName) - } -} - func OriginAssignTopologyBasedDeliveryService(t *testing.T) { params := url.Values{} params.Add("hostName", "denver-mso-org-01") From 6894221054a697fe23444cfe7e98310f235669b9 Mon Sep 17 00:00:00 2001 From: Srijeet Chatterjee Date: Fri, 18 Dec 2020 16:29:48 -0700 Subject: [PATCH 07/11] refactoring --- .../traffic_ops_golang/server/servers.go | 57 +++++++++++++------ 1 file changed, 39 insertions(+), 18 deletions(-) diff --git a/traffic_ops/traffic_ops_golang/server/servers.go b/traffic_ops/traffic_ops_golang/server/servers.go index 95c6062c26..214f659237 100644 --- a/traffic_ops/traffic_ops_golang/server/servers.go +++ b/traffic_ops/traffic_ops_golang/server/servers.go @@ -399,6 +399,12 @@ RETURNING upd_pending ` +const originServerQuery = ` +JOIN deliveryservice_server dsorg +ON dsorg.server = s.id +WHERE t.name='ORG' +AND dsorg.deliveryservice=:dsId +` const deleteServerQuery = `DELETE FROM server WHERE id=$1` const deleteInterfacesQuery = `DELETE FROM interface WHERE server=$1` const deleteIPsQuery = `DELETE FROM ip_address WHERE server = $1` @@ -767,6 +773,26 @@ JOIN type t ON s.type = t.id ` + select max(last_updated) as t from last_deleted l where l.table_name='server') as res` } +func getServerCount(tx *sqlx.Tx, query string, queryValues map[string]interface{}) (uint64, error) { + rowsAffected := 0 + var serverCount uint64 + countOrigRows, err := tx.NamedQuery(query, queryValues) + if err != nil { + return 0, fmt.Errorf("failed to get origin servers count: %v", err) + } + defer countOrigRows.Close() + for countOrigRows.Next() { + if err = countOrigRows.Scan(&serverCount); err != nil { + return 0, fmt.Errorf("failed to read servers count: %v", err) + } + rowsAffected++ + } + if rowsAffected != 1 { + return 0, fmt.Errorf("incorrect rows returned for server count, want: 1 got: %v", rowsAffected) + } + return serverCount, nil +} + func getServers(h http.Header, params map[string]string, tx *sqlx.Tx, user *auth.CurrentUser, useIMS bool, version api.Version) ([]tc.ServerNullable, uint64, error, error, int, *time.Time) { var maxTime time.Time var runSecond bool @@ -798,7 +824,7 @@ func getServers(h http.Header, params map[string]string, tx *sqlx.Tx, user *auth dsHasRequiredCapabilities := false var cdnID int var serverCountOrigin uint64 - rowsAffected := 0 + var err error if dsIDStr, ok := params[`dsId`]; ok { // don't allow query on ds outside user's tenant @@ -839,10 +865,6 @@ func getServers(h http.Header, params map[string]string, tx *sqlx.Tx, user *auth } usesMids = dsType.UsesMidCache() log.Debugf("Servers for ds %d; uses mids? %v\n", dsID, usesMids) - row := tx.QueryRow(serverCountQuery+` JOIN deliveryservice_server dsorg ON dsorg.server = s.id WHERE t.name='ORG' AND dsorg.deliveryservice=$1`, dsID) - if err := row.Scan(&serverCountOrigin); err != nil { - return nil, 0, nil, fmt.Errorf("failed to read servers count: %v", err), http.StatusInternalServerError, nil - } } if _, ok := params[`topology`]; ok { @@ -853,6 +875,15 @@ func getServers(h http.Header, params map[string]string, tx *sqlx.Tx, user *auth } where, orderBy, pagination, queryValues, errs := dbhelpers.BuildWhereAndOrderByAndPagination(params, queryParamsToSQLCols) + + // get the origin server count + if _, ok := params["dsId"]; ok { + serverCountOrigin, err = getServerCount(tx, serverCountQuery+originServerQuery, queryValues) + if err != nil { + return nil, 0, nil, fmt.Errorf("failed to get origin servers count: %v", err), http.StatusInternalServerError, nil + } + } + if dsHasRequiredCapabilities { where += requiredCapabilitiesCondition } @@ -862,21 +893,10 @@ func getServers(h http.Header, params map[string]string, tx *sqlx.Tx, user *auth // TODO there's probably a cleaner way to do this by preparing a NamedStmt first and using its QueryRow method var serverCount uint64 - countRows, err := tx.NamedQuery(serverCountQuery+queryAddition+where, queryValues) + serverCount, err = getServerCount(tx, serverCountQuery+queryAddition+where, queryValues) if err != nil { return nil, 0, nil, fmt.Errorf("failed to get servers count: %v", err), http.StatusInternalServerError, nil } - defer countRows.Close() - rowsAffected = 0 - for countRows.Next() { - if err = countRows.Scan(&serverCount); err != nil { - return nil, 0, nil, fmt.Errorf("failed to read servers count: %v", err), http.StatusInternalServerError, nil - } - rowsAffected++ - } - if rowsAffected != 1 { - return nil, 0, nil, fmt.Errorf("incorrect rows returned for server count, want: 1 got: %v", rowsAffected), http.StatusInternalServerError, nil - } serverCount += serverCountOrigin serversList := []tc.ServerNullable{} @@ -892,8 +912,9 @@ func getServers(h http.Header, params map[string]string, tx *sqlx.Tx, user *auth } query := selectQuery + queryAddition + where + orderBy + pagination + // If you're looking to get the servers for a particular delivery service, make sure you're also querying the ORG servers from the deliveryservice_server table if _, ok := params[`dsId`]; ok { - query = `(` + selectQuery + queryAddition + where + orderBy + pagination + `) UNION ` + selectQuery + ` JOIN deliveryservice_server dsorg ON dsorg.server = s.id WHERE t.name='ORG' AND dsorg.deliveryservice=:dsId` + query = `(` + selectQuery + queryAddition + where + orderBy + pagination + `) UNION ` + selectQuery + originServerQuery } log.Debugln("Query is ", query) From e013041608d5c03cf8787729ba5d48a3786ba3f8 Mon Sep 17 00:00:00 2001 From: Srijeet Chatterjee Date: Fri, 18 Dec 2020 16:31:16 -0700 Subject: [PATCH 08/11] changing error msg --- traffic_ops/traffic_ops_golang/server/servers.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/traffic_ops/traffic_ops_golang/server/servers.go b/traffic_ops/traffic_ops_golang/server/servers.go index 214f659237..36e7d33ef6 100644 --- a/traffic_ops/traffic_ops_golang/server/servers.go +++ b/traffic_ops/traffic_ops_golang/server/servers.go @@ -778,7 +778,7 @@ func getServerCount(tx *sqlx.Tx, query string, queryValues map[string]interface{ var serverCount uint64 countOrigRows, err := tx.NamedQuery(query, queryValues) if err != nil { - return 0, fmt.Errorf("failed to get origin servers count: %v", err) + return 0, fmt.Errorf("failed to get servers count: %v", err) } defer countOrigRows.Close() for countOrigRows.Next() { From b3bc5073d9f6594f3763dc1a44d7f118e24e77ce Mon Sep 17 00:00:00 2001 From: Srijeet Chatterjee Date: Fri, 18 Dec 2020 16:39:26 -0700 Subject: [PATCH 09/11] Adding changelog --- CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b7ab9ab4df..49f073d58d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,12 +7,13 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/). ### Added - Traffic Ops: added a feature so that the user can specify `maxRequestHeaderBytes` on a per delivery service basis - Traffic Router: log warnings when requests to Traffic Monitor return a 503 status code -- #5344 - Add a page that addresses migrating from Traffic Ops API v1 for each endpoint +- [#5344](https://github.com/apache/trafficcontrol/issues/5344) - Add a page that addresses migrating from Traffic Ops API v1 for each endpoint - [#5296](https://github.com/apache/trafficcontrol/issues/5296) - Fixed a bug where users couldn't update any regex in Traffic Ops/ Traffic Portal - Added API endpoints for ACME accounts - Traffic Ops: Added validation to ensure that the cachegroups of a delivery services' assigned ORG servers are present in the topology ### Fixed +- [#5380](https://github.com/apache/trafficcontrol/issues/5380) - Show the correct servers (including ORGs) when a topology based DS with required capabilities + ORG servers is queried for the assigned servers - [#5195](https://github.com/apache/trafficcontrol/issues/5195) - Correctly show CDN ID in Changelog during Snap - Fixed Traffic Router logging unnecessary warnings for IPv6-only caches - Fixed parent.config generation for topology-based delivery services (inline comments not supported) From baa1937792887395dd438071471987e5661753db Mon Sep 17 00:00:00 2001 From: Srijeet Chatterjee Date: Mon, 21 Dec 2020 16:19:53 -0700 Subject: [PATCH 10/11] code review fixes --- .../traffic_ops_golang/server/servers.go | 38 ++++++------------- .../traffic_ops_golang/server/servers_test.go | 4 ++ 2 files changed, 16 insertions(+), 26 deletions(-) diff --git a/traffic_ops/traffic_ops_golang/server/servers.go b/traffic_ops/traffic_ops_golang/server/servers.go index 36e7d33ef6..305878a376 100644 --- a/traffic_ops/traffic_ops_golang/server/servers.go +++ b/traffic_ops/traffic_ops_golang/server/servers.go @@ -774,21 +774,14 @@ JOIN type t ON s.type = t.id ` + } func getServerCount(tx *sqlx.Tx, query string, queryValues map[string]interface{}) (uint64, error) { - rowsAffected := 0 var serverCount uint64 - countOrigRows, err := tx.NamedQuery(query, queryValues) + ns, err := tx.PrepareNamed(query) if err != nil { - return 0, fmt.Errorf("failed to get servers count: %v", err) - } - defer countOrigRows.Close() - for countOrigRows.Next() { - if err = countOrigRows.Scan(&serverCount); err != nil { - return 0, fmt.Errorf("failed to read servers count: %v", err) - } - rowsAffected++ + return 0, fmt.Errorf("couldn't prepare the query to get server count : %v", err) } - if rowsAffected != 1 { - return 0, fmt.Errorf("incorrect rows returned for server count, want: 1 got: %v", rowsAffected) + err = tx.NamedStmt(ns).QueryRow(queryValues).Scan(&serverCount) + if err != nil { + return 0, fmt.Errorf("failed to get servers count: %v", err) } return serverCount, nil } @@ -823,7 +816,7 @@ func getServers(h http.Header, params map[string]string, tx *sqlx.Tx, user *auth queryAddition := "" dsHasRequiredCapabilities := false var cdnID int - var serverCountOrigin uint64 + var serverCount uint64 var err error if dsIDStr, ok := params[`dsId`]; ok { @@ -875,15 +868,6 @@ func getServers(h http.Header, params map[string]string, tx *sqlx.Tx, user *auth } where, orderBy, pagination, queryValues, errs := dbhelpers.BuildWhereAndOrderByAndPagination(params, queryParamsToSQLCols) - - // get the origin server count - if _, ok := params["dsId"]; ok { - serverCountOrigin, err = getServerCount(tx, serverCountQuery+originServerQuery, queryValues) - if err != nil { - return nil, 0, nil, fmt.Errorf("failed to get origin servers count: %v", err), http.StatusInternalServerError, nil - } - } - if dsHasRequiredCapabilities { where += requiredCapabilitiesCondition } @@ -891,14 +875,16 @@ func getServers(h http.Header, params map[string]string, tx *sqlx.Tx, user *auth return nil, 0, util.JoinErrs(errs), nil, http.StatusBadRequest, nil } - // TODO there's probably a cleaner way to do this by preparing a NamedStmt first and using its QueryRow method - var serverCount uint64 - serverCount, err = getServerCount(tx, serverCountQuery+queryAddition+where, queryValues) + countQuery := serverCountQuery + queryAddition + where + // If we are querying for a DS that has reqd capabilities, we need to make sure that we also include all the ORG servers directly assigned to this DS + if _, ok := params["dsId"]; ok && dsHasRequiredCapabilities { + countQuery = `SELECT (` + countQuery + `) + (` + serverCountQuery + originServerQuery + `) AS total` + } + serverCount, err = getServerCount(tx, countQuery, queryValues) if err != nil { return nil, 0, nil, fmt.Errorf("failed to get servers count: %v", err), http.StatusInternalServerError, nil } - serverCount += serverCountOrigin serversList := []tc.ServerNullable{} if useIMS { runSecond, maxTime = ims.TryIfModifiedSinceQuery(tx, h, queryValues, selectMaxLastUpdatedQuery(queryAddition, where)) diff --git a/traffic_ops/traffic_ops_golang/server/servers_test.go b/traffic_ops/traffic_ops_golang/server/servers_test.go index df2c454959..0ea0d02cd7 100644 --- a/traffic_ops/traffic_ops_golang/server/servers_test.go +++ b/traffic_ops/traffic_ops_golang/server/servers_test.go @@ -248,6 +248,8 @@ func TestGetServersByCachegroup(t *testing.T) { } mock.ExpectBegin() + mock.ExpectPrepare("SELECT COUNT\\(s.id\\) FROM s") + mock.ExpectPrepare("SELECT COUNT\\(s.id\\) FROM s") mock.ExpectQuery("SELECT COUNT\\(s.id\\) FROM s").WillReturnRows(unfilteredRows) mock.ExpectQuery("SELECT").WillReturnRows(rows) mock.ExpectQuery("SELECT").WillReturnRows(interfaceRows) @@ -356,6 +358,8 @@ func TestGetMidServers(t *testing.T) { } } mock.ExpectBegin() + mock.ExpectPrepare("SELECT COUNT\\(s.id\\) FROM s") + mock.ExpectPrepare("SELECT COUNT\\(s.id\\) FROM s") mock.ExpectQuery("SELECT COUNT\\(s.id\\) FROM s").WillReturnRows(unfilteredRows) mock.ExpectQuery("SELECT").WillReturnRows(rows) mock.ExpectQuery("SELECT").WillReturnRows(interfaceRows) From 1eb8a257d06cdca5234f2e987cbfb8ef3b810b38 Mon Sep 17 00:00:00 2001 From: Srijeet Chatterjee Date: Tue, 22 Dec 2020 09:38:34 -0700 Subject: [PATCH 11/11] code review fixes --- .../api/v3/deliveryservices_required_capabilities_test.go | 2 +- traffic_ops/traffic_ops_golang/server/servers.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/traffic_ops/testing/api/v3/deliveryservices_required_capabilities_test.go b/traffic_ops/testing/api/v3/deliveryservices_required_capabilities_test.go index 0f230f2e06..6b094619bf 100644 --- a/traffic_ops/testing/api/v3/deliveryservices_required_capabilities_test.go +++ b/traffic_ops/testing/api/v3/deliveryservices_required_capabilities_test.go @@ -68,7 +68,7 @@ func OriginAssignTopologyBasedDeliveryServiceWithRequiredCapabilities(t *testing t.Errorf("assigning ORG server to ds-top delivery service: %v", err.Error()) } params.Add("dsId", strconv.Itoa(dsID)) - params.Add("type", "ORG") + params.Add("type", tc.OriginTypeName) responseServers, _, err := TOSession.GetServersWithHdr(¶ms, nil) if err != nil { t.Fatalf("getting servers for ds-top-req-cap2 delivery service: %v", err.Error()) diff --git a/traffic_ops/traffic_ops_golang/server/servers.go b/traffic_ops/traffic_ops_golang/server/servers.go index 305878a376..f2d8a6548c 100644 --- a/traffic_ops/traffic_ops_golang/server/servers.go +++ b/traffic_ops/traffic_ops_golang/server/servers.go @@ -402,7 +402,7 @@ RETURNING const originServerQuery = ` JOIN deliveryservice_server dsorg ON dsorg.server = s.id -WHERE t.name='ORG' +WHERE t.name = '` + tc.OriginTypeName + `' AND dsorg.deliveryservice=:dsId ` const deleteServerQuery = `DELETE FROM server WHERE id=$1`