[GitHub] [trafficcontrol] rawlinp commented on a change in pull request #5144: Prohibit adding a cache group with no servers to a topology

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

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



##########
File path: CHANGELOG.md
##########
@@ -21,6 +21,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
     - Traffic Ops: Added validation to prohibit assigning caches to topology-based delivery services
     - Traffic Ops: Added validation to prohibit removing a capability from a server if no other server in the same cachegroup can satisfy the required capabilities of the delivery services assigned to it via topologies.
     - Traffic Ops: Added validation to ensure that at least one server per cachegroup in a delivery service's topology has the delivery service's required capabilities.
+    - Traffic Ops: Added validation to ensure that at least one server exists in each cachegroup.

Review comment:
       "... that that is used in a Topology"

##########
File path: traffic_ops/traffic_ops_golang/topology/topologies.go
##########
@@ -563,6 +614,23 @@ JOIN topology_cachegroup tc on t.name = tc.topology
 	return query
 }
 
+func selectEmptyCacheGroupsQuery() string {
+	// language=SQL
+	query := `
+		SELECT c."name", COUNT((
+			SELECT s2.id FROM server s2
+			JOIN "type" t ON s."type" = t.id
+			WHERE s2.id = s.id
+		)) server_count
+		FROM cachegroup c
+		LEFT JOIN "server" s ON c.id = s.cachegroup
+		WHERE c."name" = ANY(CAST(:cachegroup_names AS TEXT[]))
+		GROUP BY c."name"
+		ORDER BY server_count

Review comment:
       Can this simply be
   ```
   SELECT c."name", COUNT(*) AS server_count
           FROM cachegroup c
           LEFT JOIN "server" s ON c.id = s.cachegroup
           WHERE c."name" = ANY(CAST(:cachegroup_names AS TEXT[]))
           GROUP BY c."name"
           ORDER BY server_count;
   ```
   instead?

##########
File path: traffic_ops/traffic_ops_golang/topology/topologies.go
##########
@@ -146,13 +149,61 @@ func (topology *TOTopology) Validate() error {
 	return util.JoinErrs(tovalidate.ToErrors(rules))
 }
 
+func (topology *TOTopology) checkForEmptyCacheGroups() error {
+	var (
+		cacheGroupNames = make([]string, len(topology.Nodes))
+		baseError       = errors.New("unable to check for cachegroups with no servers")
+		systemError     = "checking for cachegroups with no servers: %s"
+		parameters      = map[string]interface{}{}
+		query           = selectEmptyCacheGroupsQuery()
+	)
+	for index, node := range topology.Nodes {
+		cacheGroupNames[index] = node.Cachegroup
+	}
+	parameters["cachegroup_names"] = pq.Array(cacheGroupNames)
+
+	rows, err := topology.ReqInfo.Tx.NamedQuery(query, parameters)
+	if err != nil {
+		log.Errorf(systemError, err.Error())
+		return baseError
+	}
+
+	var (
+		serverCount int
+		cacheGroup  string
+		cacheGroups []string
+	)
+	defer log.Close(rows, "unable to close DB connection when checking for cachegroups with no servers")
+	for rows.Next() {
+		if err := rows.Scan(&cacheGroup, &serverCount); err != nil {
+			log.Errorf(systemError, err.Error())
+			return baseError
+		}
+		if serverCount != 0 {
+			break
+		}
+		cacheGroups = append(cacheGroups, cacheGroup)
+	}
+
+	if cacheGroups != nil {

Review comment:
       nit: might read better as `len(cacheGroups) > 0`




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