rawlinp commented on a change in pull request #5350:
URL: https://github.com/apache/trafficcontrol/pull/5350#discussion_r537841621
##########
File path: traffic_ops/testing/api/v3/topologies_test.go
##########
@@ -240,6 +241,39 @@ func UpdateTestTopologies(t *testing.T) {
}
}
+func UpdateValidateTopologyORGServerCacheGroup(t *testing.T) {
+ params := url.Values{}
+ params.Set("xmlId", "ds-top")
+
+ //Get the correct DS
+ remoteDS, _, err := TOSession.GetDeliveryServicesV30WithHdr(nil, params)
+ if err != nil {
+ t.Errorf("cannot GET Delivery Services: %v", err)
+ }
+
+ //Assign ORG server to DS
+ assignServer := []string{"denver-mso-org-01"}
+ _, _, err = TOSession.AssignServersToDeliveryService(assignServer,
*remoteDS[0].XMLID)
+ if err != nil {
+ t.Errorf("cannot assign server to Delivery Services: %v", err)
+ }
+
+ //Get Topology node to update and remove ORG server nodes
+ resp, _, err := TOSession.GetTopologyWithHdr(*remoteDS[0].Topology, nil)
+ if err != nil {
+ t.Fatalf("couldn't find any topologies: %v", err)
+ }
+ var p []int
+ newNodes := []tc.TopologyNode{{Id: 0, Cachegroup:
"topology-edge-cg-01", Parents: p, LastUpdated: nil}}
+ if *remoteDS[0].Topology == resp.Name {
+ resp.Nodes = newNodes
+ }
+ _, _, err = TOSession.UpdateTopology(*remoteDS[0].Topology, *resp)
+ if err == nil {
+ t.Fatalf("shouldnot UPDATE topology:%v, but update was a
success", *remoteDS[0].Topology)
+ }
Review comment:
Here we should probably unassign the `denver-mso-org-01` server from
`ds-top` just to keep the data in a clean state.
##########
File path: CHANGELOG.md
##########
@@ -39,6 +39,8 @@ The format is based on [Keep a
Changelog](http://keepachangelog.com/en/1.0.0/).
- Traffic Router: Added support for topology-based delivery services
- Traffic Monitor: Added the ability to mark topology-based delivery
services as available
- CDN-in-a-Box: Add a second mid to CDN-in-a-Box, add topology
`demo1-top`, and make the `demo1` delivery service topology-based
+ - Traffic Ops: Added validation to ensure assigned ORG server cachegroups
are in the topology when updating a delivery service
+ - Traffic Ops: Added validation to ensure that the cachegroups of a
delivery services' assigned ORG servers are present in the topology
Review comment:
This 2nd change needs to go in the `unreleased` section, because only
the 1st change has made it into the latest 5.0 release candidate.
##########
File path: traffic_ops/traffic_ops_golang/topology/topologies.go
##########
@@ -150,6 +150,20 @@ func (topology *TOTopology) Validate() error {
rules["empty cachegroups"] =
topology_validation.CheckForEmptyCacheGroups(topology.ReqInfo.Tx,
cacheGroupIds, dsCDNs, false, nil)
rules["required capabilities"] =
topology.validateDSRequiredCapabilities()
+ for _, cdnId := range dsCDNs {
+ err, dsId :=
dbhelpers.GetDSByCDNIdTopology(topology.ReqInfo.Tx.Tx, cdnId, topology.Name)
+ if err != nil {
+ return fmt.Errorf("failed to get a response from DB:
%s", err)
+ }
+ for _, id := range dsId {
+ topoCachegroupNames := topology.getCachegroupNames()
+ userErr, sysErr, _ :=
dbhelpers.CheckOriginServerInCacheGroupTopology(topology.ReqInfo.Tx.Tx, id,
topology.Name, topoCachegroupNames)
Review comment:
Ok, I think I might've been wrong about being able to reuse this same
`CheckOriginServerInCacheGroupTopology` function for this validation. When we
update a topology, we don't want to make N DB queries (where N is the number of
delivery services assigned to the topology). Preferably, we would make a single
DB query that gets all the DS data we need, then we can do the validation from
there.
----------------------------------------------------------------
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:
[email protected]
