[GitHub] [trafficcontrol] rawlinp commented on a change in pull request #5307: Validate ORG server for topology based DS

[GitBox <gi...@apache.org>]

rawlinp commented on a change in pull request #5307:
URL: https://github.com/apache/trafficcontrol/pull/5307#discussion_r527192806



##########
File path: traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go
##########
@@ -1009,3 +1010,49 @@ GROUP BY t.name, ds.topology
 	}
 	return dsType, reqCap, topology, true, nil
 }
+
+// CheckOriginServerInCacheGroupTopology checks if a DS has ORG server and if it does, to make sure the cachegroup is part of DS
+func CheckOriginServerInCacheGroupTopology(tx *sql.Tx, dsID int, dsTopology string) (error, error, int) {
+	// get servers and respective cachegroup name that have ORG type in a delivery service
+	q := `
+		SELECT s2.host_name, c.name 
+		FROM server s2
+			INNER JOIN deliveryservice_server ds ON ds.server = s2.id
+			inner join type t on t.id = s2.type

Review comment:
       for consistency we should capitalize INNER JOIN, ON, and AND in this query

##########
File path: traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go
##########
@@ -1009,3 +1010,49 @@ GROUP BY t.name, ds.topology
 	}
 	return dsType, reqCap, topology, true, nil
 }
+
+// CheckOriginServerInCacheGroupTopology checks if a DS has ORG server and if it does, to make sure the cachegroup is part of DS
+func CheckOriginServerInCacheGroupTopology(tx *sql.Tx, dsID int, dsTopology string) (error, error, int) {
+	// get servers and respective cachegroup name that have ORG type in a delivery service
+	q := `
+		SELECT s2.host_name, c.name 
+		FROM server s2
+			INNER JOIN deliveryservice_server ds ON ds.server = s2.id
+			inner join type t on t.id = s2.type
+			INNER JOIN cachegroup c ON c.id = s2.cachegroup
+		WHERE ds.deliveryservice=$1 and t.name='ORG'
+		GROUP BY s2.host_name, c.name

Review comment:
       the `GROUP BY` is unnecessary here since this isn't doing an aggregate query

##########
File path: traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go
##########
@@ -1009,3 +1010,49 @@ GROUP BY t.name, ds.topology
 	}
 	return dsType, reqCap, topology, true, nil
 }
+
+// CheckOriginServerInCacheGroupTopology checks if a DS has ORG server and if it does, to make sure the cachegroup is part of DS
+func CheckOriginServerInCacheGroupTopology(tx *sql.Tx, dsID int, dsTopology string) (error, error, int) {
+	// get servers and respective cachegroup name that have ORG type in a delivery service
+	q := `
+		SELECT s2.host_name, c.name 
+		FROM server s2
+			INNER JOIN deliveryservice_server ds ON ds.server = s2.id
+			inner join type t on t.id = s2.type
+			INNER JOIN cachegroup c ON c.id = s2.cachegroup
+		WHERE ds.deliveryservice=$1 and t.name='ORG'

Review comment:
       we should use `tc.OriginTypeName` in this query

##########
File path: traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go
##########
@@ -1009,3 +1010,49 @@ GROUP BY t.name, ds.topology
 	}
 	return dsType, reqCap, topology, true, nil
 }
+
+// CheckOriginServerInCacheGroupTopology checks if a DS has ORG server and if it does, to make sure the cachegroup is part of DS
+func CheckOriginServerInCacheGroupTopology(tx *sql.Tx, dsID int, dsTopology string) (error, error, int) {
+	// get servers and respective cachegroup name that have ORG type in a delivery service
+	q := `
+		SELECT s2.host_name, c.name 

Review comment:
       instead of `s2` we should just alias it to `s` -- the `2` is confusing

##########
File path: traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go
##########
@@ -1009,3 +1010,49 @@ GROUP BY t.name, ds.topology
 	}
 	return dsType, reqCap, topology, true, nil
 }
+
+// CheckOriginServerInCacheGroupTopology checks if a DS has ORG server and if it does, to make sure the cachegroup is part of DS
+func CheckOriginServerInCacheGroupTopology(tx *sql.Tx, dsID int, dsTopology string) (error, error, int) {
+	// get servers and respective cachegroup name that have ORG type in a delivery service
+	q := `
+		SELECT s2.host_name, c.name 
+		FROM server s2
+			INNER JOIN deliveryservice_server ds ON ds.server = s2.id
+			inner join type t on t.id = s2.type
+			INNER JOIN cachegroup c ON c.id = s2.cachegroup
+		WHERE ds.deliveryservice=$1 and t.name='ORG'
+		GROUP BY s2.host_name, c.name
+	`
+
+	serverName := ""
+	cacheGroupName := ""
+	rows, err := tx.Query(q, dsID)
+	if err != nil {
+		return nil, fmt.Errorf("querying deliveryservice origin server: %s", err), http.StatusInternalServerError
+	}
+	for rows.Next() {
+		if err := rows.Scan(&serverName, &cacheGroupName); err != nil {
+			return nil, fmt.Errorf("querying deliveryservice origin server: %s", err), http.StatusInternalServerError
+		}
+	}
+	fmt.Println("CacheName:", cacheGroupName)

Review comment:
       I know this is a draft, but these `fmt.Println` statements should be removed before we merge

##########
File path: traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go
##########
@@ -1009,3 +1010,49 @@ GROUP BY t.name, ds.topology
 	}
 	return dsType, reqCap, topology, true, nil
 }
+
+// CheckOriginServerInCacheGroupTopology checks if a DS has ORG server and if it does, to make sure the cachegroup is part of DS
+func CheckOriginServerInCacheGroupTopology(tx *sql.Tx, dsID int, dsTopology string) (error, error, int) {
+	// get servers and respective cachegroup name that have ORG type in a delivery service
+	q := `
+		SELECT s2.host_name, c.name 
+		FROM server s2
+			INNER JOIN deliveryservice_server ds ON ds.server = s2.id
+			inner join type t on t.id = s2.type
+			INNER JOIN cachegroup c ON c.id = s2.cachegroup
+		WHERE ds.deliveryservice=$1 and t.name='ORG'
+		GROUP BY s2.host_name, c.name
+	`
+
+	serverName := ""
+	cacheGroupName := ""
+	rows, err := tx.Query(q, dsID)
+	if err != nil {
+		return nil, fmt.Errorf("querying deliveryservice origin server: %s", err), http.StatusInternalServerError
+	}
+	for rows.Next() {
+		if err := rows.Scan(&serverName, &cacheGroupName); err != nil {
+			return nil, fmt.Errorf("querying deliveryservice origin server: %s", err), http.StatusInternalServerError
+		}
+	}
+	fmt.Println("CacheName:", cacheGroupName)
+
+	servers := make(map[string]string)
+	servers[cacheGroupName] = serverName
+	fmt.Println(servers)
+
+	if len(servers) > 0 {
+		cachegroups, sysErr := GetTopologyCachegroups(tx, dsTopology)
+		if sysErr != nil {
+			return nil, fmt.Errorf("validating %s servers in topology: %v", tc.OriginTypeName, sysErr), http.StatusInternalServerError
+		}
+		for _, cg := range cachegroups {
+			if _, ok := servers[cg]; ok {

Review comment:
       This logic is slightly off. We need to iterate over all the ORG server cachegroups to make sure they're all in the topology cachegroups. If any of them are not, we should return an error message that includes _all_ the offending ORG servers/cachegroups.
   
   I would recommend converting `cachegroups` into a `map[string]struct{}` (essentially a set), then:
   ```
   for cg, s := range servers {
       if _, ok := cachegroups[cg]; !ok {
           // add to list of offending servers/cachegroups
       }
   }
   // use strings.Join() to merge the offending servers/cachegroups into a single error message
   ```




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org