You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficcontrol.apache.org by GitBox <gi...@apache.org> on 2021/08/03 13:07:10 UTC
[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #6077: Prevent unassigning all ONLINE ORG servers from an active, MSO-enabled DS
zrhoffman commented on a change in pull request #6077:
URL: https://github.com/apache/trafficcontrol/pull/6077#discussion_r681378105
##########
File path: traffic_ops/traffic_ops_golang/server/servers_assignment.go
##########
@@ -63,37 +63,54 @@ func getConfigFile(prefix string, xmlId string) string {
}
const lastServerInActiveDeliveryServicesQuery = `
-SELECT deliveryservice_server.deliveryservice
-FROM deliveryservice_server
-INNER JOIN server ON server.id = deliveryservice_server.server
-INNER JOIN status ON status.id = server.status
-WHERE deliveryservice IN (
- SELECT deliveryservice_server.deliveryservice
- FROM deliveryservice_server
- INNER JOIN deliveryservice ON deliveryservice.id = deliveryservice_server.deliveryservice
- WHERE deliveryservice_server.server=$1
- AND deliveryservice.active IS TRUE
+SELECT d.id, d.multi_site_origin
+FROM deliveryservice d
+INNER JOIN deliveryservice_server dss ON dss.deliveryservice = d.id
+INNER JOIN server s ON s.id = dss.server
+INNER JOIN status st ON st.id = s.status
+INNER JOIN type t ON t.id = s.type
+WHERE d.id IN (
+ SELECT dss.deliveryservice
+ FROM deliveryservice_server dss
+ INNER JOIN deliveryservice d ON d.id = dss.deliveryservice
+ WHERE dss.server=$1
+ AND d.active
)
-AND NOT (deliveryservice_server.deliveryservice = ANY($2::BIGINT[]))
-AND (status.name = '` + string(tc.CacheStatusOnline) + `' OR status.name = '` + string(tc.CacheStatusReported) + `')
-GROUP BY deliveryservice_server.deliveryservice
-HAVING COUNT(deliveryservice_server.server) = 1
+AND NOT (dss.deliveryservice = ANY($2::BIGINT[]))
+AND (st.name = '` + string(tc.CacheStatusOnline) + `' OR st.name = '` + string(tc.CacheStatusReported) + `')
+AND t.name LIKE $3
+GROUP BY d.id, d.multi_site_origin
+HAVING COUNT(dss.server) = 1
`
-func checkForLastServerInActiveDeliveryServices(serverID int, dsIDs []int, tx *sql.Tx) ([]int, error) {
+func checkForLastServerInActiveDeliveryServices(serverID int, serverType string, dsIDs []int, tx *sql.Tx) ([]int, error) {
violations := []int{}
- rows, err := tx.Query(lastServerInActiveDeliveryServicesQuery, serverID, pq.Array(dsIDs))
+ var like string
+ isEdge := strings.HasPrefix(serverType, tc.CacheTypeEdge.String())
+ isOrigin := strings.HasPrefix(serverType, tc.OriginTypeName)
+ if isEdge {
+ like = tc.CacheTypeEdge.String() + "%"
+ } else if isOrigin {
+ like = tc.OriginTypeName + "%"
+ } else {
+ // by definition, only EDGE-type or ORG-type servers can be assigned
+ return violations, nil
+ }
+ rows, err := tx.Query(lastServerInActiveDeliveryServicesQuery, serverID, pq.Array(dsIDs), like)
if err != nil {
return violations, fmt.Errorf("querying: %v", err)
}
defer rows.Close()
for rows.Next() {
var violation int
- if err = rows.Scan(&violation); err != nil {
+ var mso bool
+ if err = rows.Scan(&violation, &mso); err != nil {
return violations, fmt.Errorf("scanning: %v", err)
}
- violations = append(violations, violation)
+ if isEdge || (isOrigin && mso) {
Review comment:
Nit: The parenthesis are unnecessary
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/servers/delete.go
##########
@@ -35,50 +35,68 @@ import (
// Delete handler for deleting the association between a Delivery Service and a Server
func Delete(w http.ResponseWriter, r *http.Request) {
- delete(w, r)
+ deleteDSS(w, r)
}
-const lastServerQuery = `
-SELECT
-(SELECT (CASE WHEN t.name LIKE '` + string(tc.EdgeTypePrefix) + `%' THEN TRUE ELSE FALSE END) AS available
-FROM type t
-JOIN server s ON s.type = t.id
-WHERE s.id = $2)
-AND
-(SELECT COUNT(*) = 0 AS available
-FROM deliveryservice_server
-JOIN server s ON deliveryservice_server.server = s.id
-JOIN type t ON t.id = s.type
-JOIN status st ON st.id = s.status
-WHERE (st.name = '` + string(tc.CacheStatusOnline) + `' OR st.name = '` + string(tc.CacheStatusReported) + `')
-AND t.name LIKE '` + string(tc.EdgeTypePrefix) + `%'
-AND deliveryservice = $1
-AND server <> $2)
+const lastEdgeAndLastOriginQuery = `
+SELECT (
+ SELECT (CASE WHEN t.name LIKE '` + string(tc.EdgeTypePrefix) + `%' THEN TRUE ELSE FALSE END) AS available
Review comment:
Another way of writing
```sql
(CASE WHEN t.name LIKE '` + string(tc.EdgeTypePrefix) + `%' THEN TRUE ELSE FALSE END)
```
is
```sql
(SELECT t.name LIKE '` + string(tc.EdgeTypePrefix) + `%')
```
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/servers/delete.go
##########
@@ -35,50 +35,68 @@ import (
// Delete handler for deleting the association between a Delivery Service and a Server
func Delete(w http.ResponseWriter, r *http.Request) {
- delete(w, r)
+ deleteDSS(w, r)
}
-const lastServerQuery = `
-SELECT
-(SELECT (CASE WHEN t.name LIKE '` + string(tc.EdgeTypePrefix) + `%' THEN TRUE ELSE FALSE END) AS available
-FROM type t
-JOIN server s ON s.type = t.id
-WHERE s.id = $2)
-AND
-(SELECT COUNT(*) = 0 AS available
-FROM deliveryservice_server
-JOIN server s ON deliveryservice_server.server = s.id
-JOIN type t ON t.id = s.type
-JOIN status st ON st.id = s.status
-WHERE (st.name = '` + string(tc.CacheStatusOnline) + `' OR st.name = '` + string(tc.CacheStatusReported) + `')
-AND t.name LIKE '` + string(tc.EdgeTypePrefix) + `%'
-AND deliveryservice = $1
-AND server <> $2)
+const lastEdgeAndLastOriginQuery = `
+SELECT (
+ SELECT (CASE WHEN t.name LIKE '` + string(tc.EdgeTypePrefix) + `%' THEN TRUE ELSE FALSE END) AS available
+ FROM type t
+ JOIN server s ON s.type = t.id
+ WHERE s.id = $2)
+ AND (
+ SELECT COUNT(*) = 0 AS available
+ FROM deliveryservice_server
+ JOIN server s ON deliveryservice_server.server = s.id
+ JOIN type t ON t.id = s.type
+ JOIN status st ON st.id = s.status
+ WHERE (st.name = '` + string(tc.CacheStatusOnline) + `' OR st.name = '` + string(tc.CacheStatusReported) + `')
+ AND t.name LIKE '` + string(tc.EdgeTypePrefix) + `%'
+ AND deliveryservice = $1
+ AND server <> $2),
+(
+ SELECT (CASE WHEN t.name LIKE '` + string(tc.OriginTypeName) + `%' THEN TRUE ELSE FALSE END) AS available
Review comment:
Another way of writing
```sql
(CASE WHEN t.name LIKE '` + string(tc.OriginTypeName) + `%' THEN TRUE ELSE FALSE END)
```
is
```sql
(SELECT t.name LIKE '` + string(tc.OriginTypeName) + `%')
```
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/servers/servers.go
##########
@@ -1024,14 +987,16 @@ SELECT
ds.cacheurl,
ds.max_origin_connections,
ds.topology,
- ds.cdn_id
+ ds.cdn_id,
+ ds.multi_site_origin
FROM
deliveryservice ds
JOIN type tp ON ds.type = tp.id
`
func scanDSInfoRow(row *sql.Row) (DSInfo, bool, error) {
di := DSInfo{}
+ var useMSO *bool
Review comment:
A reference to `useMSO` is passed to `row.Scan()`:
https://github.com/apache/trafficcontrol/blob/14c0716eb61c5f95c017c633f946bad3c9b70b91/traffic_ops/traffic_ops_golang/deliveryservice/servers/servers.go#L1013
So `useMSO` should be a bool, not a bool pointer.
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/servers/servers.go
##########
@@ -568,48 +530,50 @@ func GetCreateHandler(w http.ResponseWriter, r *http.Request) {
// validateDSSAssignments returns an error if the given servers cannot be assigned to the given delivery service.
func validateDSSAssignments(tx *sql.Tx, ds DSInfo, serverInfos []tc.ServerInfo, replace bool) (error, error, int) {
- valid := false
+ anyAvailableServers := false
userErr, sysErr, status := validateDSS(tx, ds, serverInfos)
if userErr != nil || sysErr != nil {
return userErr, sysErr, status
}
if ds.Active && replace {
ids := make([]int, 0, len(serverInfos))
+ newOrgCount := 0
+ newAvailableEdgeCount := 0
for _, inf := range serverInfos {
ids = append(ids, inf.ID)
// We dont check for the cache type to be = EDGE here because if this is a new DS, and we want to assign an online/ reported ORG to it,
// we should be able to do that.
if inf.Status == string(tc.CacheStatusOnline) || inf.Status == string(tc.CacheStatusReported) {
- valid = true
+ anyAvailableServers = true
+ if inf.Type == tc.OriginTypeName {
+ newOrgCount++
+ }
+ if strings.HasPrefix(inf.Type, tc.CacheTypeEdge.String()) {
+ newAvailableEdgeCount++
+ }
}
}
// Prevent the user from deleting all the servers in an active DS
if len(ids) == 0 {
return fmt.Errorf("this server assignment leaves Active Delivery Service #%d without any '%s' or '%s' servers", ds.ID, tc.CacheStatusOnline, tc.CacheStatusReported), nil, http.StatusConflict
}
+ // prevent the user from deleting all ORG servers from an active, MSO-enabled DS
+ if ds.UseMultiSiteOrigin && newOrgCount < 1 {
+ return fmt.Errorf("this server assignment leaves Active, MSO-enabled Delivery Service #%d without any '%s' or '%s' %s servers", ds.ID, tc.CacheStatusOnline, tc.CacheStatusReported, tc.OriginTypeName), nil, http.StatusConflict
+ }
// The following check is necessary because of the following:
// Consider a brand new active DS that has no server assignments.
// Now, you wish to assign an online/ reported ORG server to it.
// Since this is a new DS and it didnt have any "pre existing" online/ reported EDGEs, this should be possible.
// However, if that DS had a couple of online/ reported EDGEs assigned to it, and now if you wanted to "replace"
// that assignment with the new assignment of an online/ reported ORG, this should be prohibited by TO.
- err, preExistingEdges := checkIfEdgesExistedBefore(tx, ds.ID)
+ currentlyHasAvailableEdgesAssigned, err := hasAvailableEdgesCurrentlyAssigned(tx, ds.ID)
if err != nil {
return nil, fmt.Errorf("checking for pre existing ONLINE/ REPORTED EDGES: %v", err), http.StatusInternalServerError
}
- if preExistingEdges {
- ok, err := verifyAtLeastOneAvailableServer(ids, tx)
- if err != nil {
- return nil, fmt.Errorf("verifying statuses: %v", err), http.StatusInternalServerError
- }
- if !ok {
- return fmt.Errorf("this server assignment leaves Active Delivery Service #%d without any '%s' or '%s' servers", ds.ID, tc.CacheStatusOnline, tc.CacheStatusReported), nil, http.StatusConflict
- }
- } else {
- if !valid {
- return fmt.Errorf("this server assignment leaves Active Delivery Service #%d without any '%s' or '%s' servers", ds.ID, tc.CacheStatusOnline, tc.CacheStatusReported), nil, http.StatusConflict
- }
+ if (currentlyHasAvailableEdgesAssigned && newAvailableEdgeCount < 1) || !anyAvailableServers {
Review comment:
Nit: The parenthesis are unnecessary
##########
File path: traffic_ops/traffic_ops_golang/server/servers.go
##########
@@ -1906,40 +1907,52 @@ func Create(w http.ResponseWriter, r *http.Request) {
}
}
-const lastServerOfDSesQuery = `
-SELECT "deliveryservice"
-FROM deliveryservice_server
-WHERE "deliveryservice" IN (
- SELECT deliveryservice_server.deliveryservice
- FROM deliveryservice_server
- INNER JOIN deliveryservice
- ON deliveryservice_server.deliveryservice = deliveryservice.id
- WHERE deliveryservice_server."server" = $1
- AND deliveryservice.active
-)
-GROUP BY "deliveryservice"
-HAVING COUNT("server") = 1;
+const lastServerTypeOfDSesQuery = `
+SELECT ds.id, ds.multi_site_origin
+FROM deliveryservice_server dss
+JOIN server s ON dss.server = s.id
+JOIN type t ON s.type = t.id
+JOIN deliveryservice ds ON dss.deliveryservice = ds.id
+WHERE t.name LIKE $1 AND ds.active
+GROUP BY ds.id, ds.multi_site_origin
+HAVING COUNT(dss.server) = 1 AND $2 = ANY(ARRAY_AGG(dss.server));
`
-func getActiveDeliveryServicesThatOnlyHaveThisServerAssigned(id int, tx *sql.Tx) ([]int, error) {
+// getActiveDeliveryServicesThatOnlyHaveThisServerAssigned returns the IDs of active delivery services for which the given
+// server ID is either the last EDGE-type server or last ORG-type server (if MSO is enabled) assigned to them.
+func getActiveDeliveryServicesThatOnlyHaveThisServerAssigned(id int, serverType string, tx *sql.Tx) ([]int, error) {
var ids []int
+ var like string
+ isEdge := strings.HasPrefix(serverType, tc.CacheTypeEdge.String())
+ isOrigin := strings.HasPrefix(serverType, tc.OriginTypeName)
+ if isEdge {
+ like = tc.CacheTypeEdge.String() + "%"
+ } else if isOrigin {
+ like = tc.OriginTypeName + "%"
+ } else {
+ // by definition, only EDGE-type or ORG-type servers can be assigned
+ return ids, nil
+ }
if tx == nil {
return ids, errors.New("nil transaction")
}
- rows, err := tx.Query(lastServerOfDSesQuery, id)
+ rows, err := tx.Query(lastServerTypeOfDSesQuery, like, id)
if err != nil {
return ids, fmt.Errorf("querying: %v", err)
}
defer rows.Close()
for rows.Next() {
var dsID int
- err = rows.Scan(&dsID)
+ var mso bool
+ err = rows.Scan(&dsID, &mso)
if err != nil {
return ids, fmt.Errorf("scanning: %v", err)
}
- ids = append(ids, dsID)
+ if isEdge || (isOrigin && mso) {
Review comment:
Nit: The parenthesis are unnecessary
##########
File path: traffic_ops/traffic_ops_golang/server/put_status.go
##########
@@ -56,7 +56,11 @@ func InvalidStatusForDeliveryServicesAlertText(status string, dsIDs []int) strin
}
alertText += fmt.Sprintf("s %s, and #%d", strings.Join(dsNums, ", "), dsIDs[len(dsIDs)-1])
}
- alertText += fmt.Sprintf(" with no '%s' or '%s' servers", tc.CacheStatusOnline, tc.CacheStatusReported)
+ typeMsg := tc.CacheTypeEdge.String()
+ if strings.HasPrefix(serverType, tc.OriginTypeName) {
+ typeMsg = tc.OriginTypeName
+ }
+ alertText += fmt.Sprintf(" with no '%s' or '%s' %s servers", tc.CacheStatusOnline, tc.CacheStatusReported, typeMsg)
Review comment:
The formatted string appended to `alertText` begins with 2 spaces. It was this way before this PR, but do you think both spaces should stay?
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/servers/servers.go
##########
@@ -568,48 +530,50 @@ func GetCreateHandler(w http.ResponseWriter, r *http.Request) {
// validateDSSAssignments returns an error if the given servers cannot be assigned to the given delivery service.
func validateDSSAssignments(tx *sql.Tx, ds DSInfo, serverInfos []tc.ServerInfo, replace bool) (error, error, int) {
- valid := false
+ anyAvailableServers := false
userErr, sysErr, status := validateDSS(tx, ds, serverInfos)
if userErr != nil || sysErr != nil {
return userErr, sysErr, status
}
if ds.Active && replace {
ids := make([]int, 0, len(serverInfos))
+ newOrgCount := 0
+ newAvailableEdgeCount := 0
for _, inf := range serverInfos {
ids = append(ids, inf.ID)
// We dont check for the cache type to be = EDGE here because if this is a new DS, and we want to assign an online/ reported ORG to it,
// we should be able to do that.
if inf.Status == string(tc.CacheStatusOnline) || inf.Status == string(tc.CacheStatusReported) {
- valid = true
+ anyAvailableServers = true
+ if inf.Type == tc.OriginTypeName {
+ newOrgCount++
+ }
+ if strings.HasPrefix(inf.Type, tc.CacheTypeEdge.String()) {
+ newAvailableEdgeCount++
+ }
Review comment:
The second `if` can be an `else if`, right?
--
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