You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@trafficcontrol.apache.org by oc...@apache.org on 2021/01/07 23:13:27 UTC
[trafficcontrol] branch master updated: TO unable to unassign ORG
server from delivery services if it's the last assigned (#5404)
This is an automated email from the ASF dual-hosted git repository.
ocket8888 pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/trafficcontrol.git
The following commit(s) were added to refs/heads/master by this push:
new 36a1a84 TO unable to unassign ORG server from delivery services if it's the last assigned (#5404)
36a1a84 is described below
commit 36a1a847b7d216d7606349831e5bcb878a10aba9
Author: Srijeet Chatterjee <30...@users.noreply.github.com>
AuthorDate: Thu Jan 7 16:13:16 2021 -0700
TO unable to unassign ORG server from delivery services if it's the last assigned (#5404)
* Initial commit -> fixed queries, tests
* adding comments/ changelog entry
* change error message
* code review fixes
---
CHANGELOG.md | 1 +
traffic_ops/testing/api/v3/topologies_test.go | 22 ++++----
.../deliveryservice/servers/delete.go | 20 ++++++--
.../deliveryservice/servers/servers.go | 60 ++++++++++++++++++++--
4 files changed, 85 insertions(+), 18 deletions(-)
diff --git a/CHANGELOG.md b/CHANGELOG.md
index b1d15fa..7298bce 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -29,6 +29,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
of just column filters
- #2881 Some API endpoints have incorrect Content-Types
- [#5311](https://github.com/apache/trafficcontrol/issues/5311) - Better TO log messages when failures calling TM CacheStats
+- [#5390](https://github.com/apache/trafficcontrol/issues/5390) - Improve the way TO deals with delivery service server assignments
## [5.0.0] - 2020-10-20
### Added
diff --git a/traffic_ops/testing/api/v3/topologies_test.go b/traffic_ops/testing/api/v3/topologies_test.go
index e2dff86..7611e94 100644
--- a/traffic_ops/testing/api/v3/topologies_test.go
+++ b/traffic_ops/testing/api/v3/topologies_test.go
@@ -279,15 +279,19 @@ func UpdateValidateTopologyORGServerCacheGroup(t *testing.T) {
}
- //TODO: Need to fix the query in deliveryservice/servers/delete.go for DeleteDeliveryServiceServer() to work correctly.
-
- // Remove org server assignment and reset DS back to as it was for further testing
- //params.Set("hostName", "denver-mso-org-01")
- //serverResp, _, err := TOSession.GetServersWithHdr(¶ms, nil)
- //_, _, err = TOSession.DeleteDeliveryServiceServer(*remoteDS[0].ID, *serverResp.Response[0].ID)
- //if err != nil {
- // t.Errorf("cannot delete assigned server from Delivery Services: %v", err)
- //}
+ //Remove org server assignment and reset DS back to as it was for further testing
+ params.Set("hostName", "denver-mso-org-01")
+ serverResp, _, err := TOSession.GetServersWithHdr(¶ms, nil)
+ if len(serverResp.Response) == 0 {
+ t.Fatal("no servers in response, quitting")
+ }
+ if serverResp.Response[0].ID == nil {
+ t.Fatal("ID of the response server is nil, quitting")
+ }
+ _, _, err = TOSession.DeleteDeliveryServiceServer(*remoteDS[0].ID, *serverResp.Response[0].ID)
+ if err != nil {
+ t.Errorf("cannot delete assigned server from Delivery Services: %v", err)
+ }
}
func DeleteTestTopologies(t *testing.T) {
diff --git a/traffic_ops/traffic_ops_golang/deliveryservice/servers/delete.go b/traffic_ops/traffic_ops_golang/deliveryservice/servers/delete.go
index 421d776..0ce6872 100644
--- a/traffic_ops/traffic_ops_golang/deliveryservice/servers/delete.go
+++ b/traffic_ops/traffic_ops_golang/deliveryservice/servers/delete.go
@@ -44,12 +44,22 @@ func DeleteDeprecated(w http.ResponseWriter, r *http.Request) {
delete(w, r, true)
}
-//TODO: Check if the query works correctly when last assigned server is a ORG server
const lastServerQuery = `
-SELECT COUNT(*) = 0
+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
-WHERE deliveryservice = $1
- AND server <> $2
+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)
`
// checkLastServer checks if the given Server ID identifies the last server
@@ -69,7 +79,7 @@ func checkLastServer(dsID, serverID int, tx *sql.Tx) (int, error, error) {
return http.StatusInternalServerError, nil, fmt.Errorf("checking if server #%d is the last one assigned to DS #%d: %v", serverID, dsID, err)
}
if isLast {
- return http.StatusConflict, fmt.Errorf("removing server #%d from active Delivery Service #%d would leave it with no assigned servers", serverID, dsID), nil
+ return http.StatusConflict, fmt.Errorf("removing server #%d from active Delivery Service #%d would leave it with no REPORTED/ONLINE assigned servers", serverID, dsID), nil
}
return http.StatusOK, nil, nil
}
diff --git a/traffic_ops/traffic_ops_golang/deliveryservice/servers/servers.go b/traffic_ops/traffic_ops_golang/deliveryservice/servers/servers.go
index 00ce0c9..4a0a7a0 100644
--- a/traffic_ops/traffic_ops_golang/deliveryservice/servers/servers.go
+++ b/traffic_ops/traffic_ops_golang/deliveryservice/servers/servers.go
@@ -341,9 +341,35 @@ const verifyStatusesQuery = `
SELECT status.name = '` + string(tc.CacheStatusOnline) + `' OR status.name = '` + string(tc.CacheStatusReported) + `'
FROM server
INNER JOIN status ON server.status = status.id
+JOIN type t on server.type = t.id
WHERE server.id = ANY($1::BIGINT[])
+AND t.name like '` + string(tc.EdgeTypePrefix) + `%'
`
+const checkPreExistingEdgeServersQuery = `
+SELECT t.name AS name
+FROM type t
+JOIN server s ON t.id = s.type
+JOIN status st ON s.status = st.id
+JOIN deliveryservice_server dss ON dss.server = s.id
+WHERE s.id = ANY(ARRAY(SELECT server FROM deliveryservice_server WHERE deliveryservice=$1))
+AND (st.name = '` + string(tc.CacheStatusOnline) + `' OR st.name = '` + string(tc.CacheStatusReported) + `')
+AND t.name like '` + string(tc.EdgeTypePrefix) + `%'
+AND dss.deliveryservice=$1
+`
+
+func checkIfEdgesExistedBefore(tx *sql.Tx, dsID int) (error, bool) {
+ rows, err := tx.Query(checkPreExistingEdgeServersQuery, dsID)
+ if err != nil {
+ return fmt.Errorf("couldn't query for pre existing edge servers for this DS: %v", err.Error()), false
+ }
+ defer rows.Close()
+ for rows.Next() {
+ return nil, true
+ }
+ return nil, false
+}
+
func verifyAtLeastOneAvailableServer(ids []int, tx *sql.Tx) (bool, error) {
if len(ids) < 1 {
return false, nil
@@ -531,6 +557,7 @@ 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
userErr, sysErr, status := validateDSS(tx, ds, serverInfos)
if userErr != nil || sysErr != nil {
return userErr, sysErr, status
@@ -540,13 +567,38 @@ func validateDSSAssignments(tx *sql.Tx, ds DSInfo, serverInfos []tc.ServerInfo,
ids := make([]int, 0, len(serverInfos))
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
+ }
}
- ok, err := verifyAtLeastOneAvailableServer(ids, tx)
+ // 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
+ }
+ // 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)
if err != nil {
- return nil, fmt.Errorf("verifying statuses: %v", err), http.StatusInternalServerError
+ return nil, fmt.Errorf("checking for pre existing ONLINE/ REPORTED EDGES: %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
+ 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
+ }
}
}