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 2022/03/15 20:24:00 UTC
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #6625: Reduce TM dependency on CRConfig
ocket8888 commented on a change in pull request #6625:
URL: https://github.com/apache/trafficcontrol/pull/6625#discussion_r827341089
##########
File path: docs/source/api/v2/cdns_name_configs_monitoring.rst
##########
@@ -58,9 +58,12 @@ Response Structure
:deliveryServices: An array of objects representing each :term:`Delivery Service` provided by this CDN
+ :hostRegexes: An array of strings which are the Delivery Service's HOST_REGEXP-type regexes
:status: The :term:`Delivery Service`'s status
+ :topology: A string that is the name of the Delivery Service's topology (if assigned one)
:totalKbpsThreshold: A threshold rate of data transfer this :term:`Delivery Service` is configured to handle, in Kilobits per second
:totalTpsThreshold: A threshold amount of transactions per second that this :term:`Delivery Service` is configured to handle
+ :type: A string that is the Delivery Service's type category (HTTP or DNS)
Review comment:
To be clear, this isn't the actual Type of the Delivery Service? It's going to be literally just the strings `"HTTP"` or `"DNS"`?
##########
File path: lib/go-tc/traffic_router.go
##########
@@ -342,7 +249,6 @@ func (ts *TrafficServer) IPv6() string {
return *lid.IP6Address
}
-type tsdeliveryService struct {
- Xmlid string `json:"xmlId"`
- Remaps []string `json:"remaps"`
+type TSDeliveryService struct {
Review comment:
GoDoc on this type?
##########
File path: traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go
##########
@@ -543,6 +543,39 @@ func GetDSTenantIDFromXMLID(tx *sql.Tx, xmlid string) (int, bool, error) {
return id, true, nil
}
+func GetServerDSNamesByCDN(tx *sql.Tx, cdn string) (map[tc.CacheName][]string, error) {
Review comment:
GoDoc?
##########
File path: docs/source/api/v2/cdns_name_configs_monitoring.rst
##########
@@ -58,9 +58,12 @@ Response Structure
:deliveryServices: An array of objects representing each :term:`Delivery Service` provided by this CDN
+ :hostRegexes: An array of strings which are the Delivery Service's HOST_REGEXP-type regexes
:status: The :term:`Delivery Service`'s status
+ :topology: A string that is the name of the Delivery Service's topology (if assigned one)
Review comment:
nit: "Topology" is an English word, so its special meaning in the context of this project is usually indicated by giving it the Title-Casing of proper nouns (also could be a Glossary term)
##########
File path: docs/source/api/v2/cdns_name_configs_monitoring.rst
##########
@@ -80,29 +83,36 @@ Response Structure
:type: A string that names the :ref:`Profile's Type <profile-type>`
+:topologies: A map of :term:`Topology` names to objects
+
+ :nodes: An array of strings which are the names of the EDGE_LOC-type cache groups in the topology
+
:trafficMonitors: An array of objects representing each Traffic Monitor that monitors this CDN (this is used by Traffic Monitor's "peer polling" function)
- :fqdn: An :abbr:`FQDN (Fully Qualified Domain Name)` that resolves to the IPv4 (and/or IPv6) address of the server running this Traffic Monitor instance
- :hostname: The hostname of the server running this Traffic Monitor instance
- :ip6: The IPv6 address of this Traffic Monitor - when applicable
- :ip: The IPv4 address of this Traffic Monitor
- :port: The port on which this Traffic Monitor listens for incoming connections
- :profile: A string that is the :ref:`profile-name` of the :term:`Profile` assigned to this Traffic Monitor
- :status: The status of the server running this Traffic Monitor instance
+ :cachegroup: The :term:`Cache Group` to which this Traffic Monitor belongs
+ :fqdn: An :abbr:`FQDN (Fully Qualified Domain Name)` that resolves to the IPv4 (and/or IPv6) address of the server running this Traffic Monitor instance
+ :hostname: The hostname of the server running this Traffic Monitor instance
+ :ip6: The IPv6 address of this Traffic Monitor - when applicable
+ :ip: The IPv4 address of this Traffic Monitor
+ :port: The port on which this Traffic Monitor listens for incoming connections
+ :profile: A string that is the :ref:`profile-name` of the :term:`Profile` assigned to this Traffic Monitor
+ :status: The status of the server running this Traffic Monitor instance
:trafficServers: An array of objects that represent the :term:`cache servers` being monitored within this CDN
- :cacheGroup: The :term:`Cache Group` to which this :term:`cache server` belongs
- :fqdn: An :abbr:`FQDN (Fully Qualified Domain Name)` that resolves to the :term:`cache server`'s IPv4 (or IPv6) address
- :hashId: The (short) hostname for the :term:`cache server` - named "hashId" for legacy reasons
- :hostName: The (short) hostname of the :term:`cache server`
- :interfacename: The name of the network interface device being used by the :term:`cache server`'s HTTP proxy
- :ip6: The :term:`cache server`'s IPv6 address - when applicable
- :ip: The :term:`cache server`'s IPv4 address
- :port: The port on which the :term:`cache server` listens for incoming connections
- :profile: A string that is the :ref:`profile-name` of the :term:`Profile` assigned to this :term:`cache server`
- :status: The status of the :term:`cache server`
- :type: A string that names the :term:`Type` of the :term:`cache server` - should (ideally) be either ``EDGE`` or ``MID``
+ :cacheGroup: The :term:`Cache Group` to which this :term:`cache server` belongs
+ :deliveryServices: An array of objects which contain the XML IDs of the delivery services to which this cache server is assigned
+
+ :xmlId: A string which is the XML ID of the delivery service
+
+ :fqdn: An :abbr:`FQDN (Fully Qualified Domain Name)` that resolves to the :term:`cache server`'s IPv4 (or IPv6) address
+ :hashId: The (short) hostname for the :term:`cache server` - named "hashId" for legacy reasons
+ :hostName: The (short) hostname of the :term:`cache server`
+ :port: The port on which the :term:`cache server` listens for incoming connections
+ :profile: A string that is the :ref:`profile-name` of the :term:`Profile` assigned to this :term:`cache server`
+ :status: The status of the :term:`cache server`
+ :type: A string that names the :term:`Type` of the :term:`cache server` - should (ideally) be either ``EDGE`` or ``MID``
+ :interfaces: A set of the network interfaces in use by the server. In most scenarios, only one will be present, but it is illegal for this set to be an empty collection.
Review comment:
this property is unexplained and doesn't appear in the request example
##########
File path: docs/source/api/v2/cdns_name_configs_monitoring.rst
##########
@@ -80,29 +83,36 @@ Response Structure
:type: A string that names the :ref:`Profile's Type <profile-type>`
+:topologies: A map of :term:`Topology` names to objects
+
+ :nodes: An array of strings which are the names of the EDGE_LOC-type cache groups in the topology
+
:trafficMonitors: An array of objects representing each Traffic Monitor that monitors this CDN (this is used by Traffic Monitor's "peer polling" function)
- :fqdn: An :abbr:`FQDN (Fully Qualified Domain Name)` that resolves to the IPv4 (and/or IPv6) address of the server running this Traffic Monitor instance
- :hostname: The hostname of the server running this Traffic Monitor instance
- :ip6: The IPv6 address of this Traffic Monitor - when applicable
- :ip: The IPv4 address of this Traffic Monitor
- :port: The port on which this Traffic Monitor listens for incoming connections
- :profile: A string that is the :ref:`profile-name` of the :term:`Profile` assigned to this Traffic Monitor
- :status: The status of the server running this Traffic Monitor instance
+ :cachegroup: The :term:`Cache Group` to which this Traffic Monitor belongs
+ :fqdn: An :abbr:`FQDN (Fully Qualified Domain Name)` that resolves to the IPv4 (and/or IPv6) address of the server running this Traffic Monitor instance
+ :hostname: The hostname of the server running this Traffic Monitor instance
+ :ip6: The IPv6 address of this Traffic Monitor - when applicable
+ :ip: The IPv4 address of this Traffic Monitor
+ :port: The port on which this Traffic Monitor listens for incoming connections
+ :profile: A string that is the :ref:`profile-name` of the :term:`Profile` assigned to this Traffic Monitor
+ :status: The status of the server running this Traffic Monitor instance
:trafficServers: An array of objects that represent the :term:`cache servers` being monitored within this CDN
- :cacheGroup: The :term:`Cache Group` to which this :term:`cache server` belongs
- :fqdn: An :abbr:`FQDN (Fully Qualified Domain Name)` that resolves to the :term:`cache server`'s IPv4 (or IPv6) address
- :hashId: The (short) hostname for the :term:`cache server` - named "hashId" for legacy reasons
- :hostName: The (short) hostname of the :term:`cache server`
- :interfacename: The name of the network interface device being used by the :term:`cache server`'s HTTP proxy
- :ip6: The :term:`cache server`'s IPv6 address - when applicable
- :ip: The :term:`cache server`'s IPv4 address
- :port: The port on which the :term:`cache server` listens for incoming connections
- :profile: A string that is the :ref:`profile-name` of the :term:`Profile` assigned to this :term:`cache server`
- :status: The status of the :term:`cache server`
- :type: A string that names the :term:`Type` of the :term:`cache server` - should (ideally) be either ``EDGE`` or ``MID``
+ :cacheGroup: The :term:`Cache Group` to which this :term:`cache server` belongs
Review comment:
This is confusing. So, I see here you're changing it to camelCase - as is API standard - probably because the `json` struct tag for this property in lib/go-tc specifies it that way. But then you also later change that struct tag to be lowercase. To make matters weirder, on current master that struct tag doesn't appear to make a difference:
```shellsession
$ toget -k cdns/dev/configs/monitoring | jq '.response.trafficServers[0].cacheGroup'
null
$ toget -k cdns/dev/configs/monitoring | jq '.response.trafficServers[0].cachegroup'
"dev"
```
I have no idea what's going on that would mess that up. I triple-checked my struct tags (for the current as well as legacy structs), and they're definitely `camelCase`, but TO doesn't output it that way.
##########
File path: docs/source/api/v2/cdns_name_configs_monitoring.rst
##########
@@ -80,29 +83,36 @@ Response Structure
:type: A string that names the :ref:`Profile's Type <profile-type>`
+:topologies: A map of :term:`Topology` names to objects
+
+ :nodes: An array of strings which are the names of the EDGE_LOC-type cache groups in the topology
+
:trafficMonitors: An array of objects representing each Traffic Monitor that monitors this CDN (this is used by Traffic Monitor's "peer polling" function)
- :fqdn: An :abbr:`FQDN (Fully Qualified Domain Name)` that resolves to the IPv4 (and/or IPv6) address of the server running this Traffic Monitor instance
- :hostname: The hostname of the server running this Traffic Monitor instance
- :ip6: The IPv6 address of this Traffic Monitor - when applicable
- :ip: The IPv4 address of this Traffic Monitor
- :port: The port on which this Traffic Monitor listens for incoming connections
- :profile: A string that is the :ref:`profile-name` of the :term:`Profile` assigned to this Traffic Monitor
- :status: The status of the server running this Traffic Monitor instance
+ :cachegroup: The :term:`Cache Group` to which this Traffic Monitor belongs
+ :fqdn: An :abbr:`FQDN (Fully Qualified Domain Name)` that resolves to the IPv4 (and/or IPv6) address of the server running this Traffic Monitor instance
+ :hostname: The hostname of the server running this Traffic Monitor instance
+ :ip6: The IPv6 address of this Traffic Monitor - when applicable
+ :ip: The IPv4 address of this Traffic Monitor
+ :port: The port on which this Traffic Monitor listens for incoming connections
+ :profile: A string that is the :ref:`profile-name` of the :term:`Profile` assigned to this Traffic Monitor
+ :status: The status of the server running this Traffic Monitor instance
:trafficServers: An array of objects that represent the :term:`cache servers` being monitored within this CDN
- :cacheGroup: The :term:`Cache Group` to which this :term:`cache server` belongs
- :fqdn: An :abbr:`FQDN (Fully Qualified Domain Name)` that resolves to the :term:`cache server`'s IPv4 (or IPv6) address
- :hashId: The (short) hostname for the :term:`cache server` - named "hashId" for legacy reasons
- :hostName: The (short) hostname of the :term:`cache server`
- :interfacename: The name of the network interface device being used by the :term:`cache server`'s HTTP proxy
- :ip6: The :term:`cache server`'s IPv6 address - when applicable
- :ip: The :term:`cache server`'s IPv4 address
- :port: The port on which the :term:`cache server` listens for incoming connections
- :profile: A string that is the :ref:`profile-name` of the :term:`Profile` assigned to this :term:`cache server`
- :status: The status of the :term:`cache server`
- :type: A string that names the :term:`Type` of the :term:`cache server` - should (ideally) be either ``EDGE`` or ``MID``
+ :cacheGroup: The :term:`Cache Group` to which this :term:`cache server` belongs
+ :deliveryServices: An array of objects which contain the XML IDs of the delivery services to which this cache server is assigned
+
+ :xmlId: A string which is the XML ID of the delivery service
Review comment:
The example doesn't have this property - is it omitted when empty? Is it supposed to be empty for "assignments" only done through Topology association?
##########
File path: traffic_monitor/todata/todata.go
##########
@@ -222,20 +241,34 @@ func getDeliveryServiceServers(crc CRConfig) (map[tc.DeliveryServiceName][]tc.Ca
}
}
- return dsServers, serverDses, nil
+ return dsServers, serverDses
}
// getDeliveryServiceRegexes gets the regexes of each delivery service, for the given CDN, from Traffic Ops.
// Returns a map[deliveryService][]regex.
-func getDeliveryServiceRegexes(crc CRConfig) (Regexes, error) {
+func getDeliveryServiceRegexes(crc CRConfig, mc tc.TrafficMonitorConfigMap) (Regexes, error) {
dsRegexes := map[tc.DeliveryServiceName][]string{}
+ if canUseMonitorConfig(mc) {
+ for dsName, dsData := range mc.DeliveryService {
+ if len(dsData.HostRegexes) < 1 {
+ log.Warnln("TMConfig missing regex for delivery service " + string(dsName))
Review comment:
Nit but as a `fmt.Stringer`, `DeliveryServiceName` values can be coerced automatically to strings - all you have to do is get rid of the manual cast and exchange ` " + ` for `, ` i.e.
```go
log.Warnln("TMConfig missing regex for delivery service " + string(dsName))
```
is equivalent to
```go
log.Warnln("TMConfig missing regex for Delivery Service", dsName)
```
##########
File path: docs/source/api/v2/cdns_name_configs_monitoring.rst
##########
@@ -80,29 +83,36 @@ Response Structure
:type: A string that names the :ref:`Profile's Type <profile-type>`
+:topologies: A map of :term:`Topology` names to objects
+
+ :nodes: An array of strings which are the names of the EDGE_LOC-type cache groups in the topology
+
:trafficMonitors: An array of objects representing each Traffic Monitor that monitors this CDN (this is used by Traffic Monitor's "peer polling" function)
- :fqdn: An :abbr:`FQDN (Fully Qualified Domain Name)` that resolves to the IPv4 (and/or IPv6) address of the server running this Traffic Monitor instance
- :hostname: The hostname of the server running this Traffic Monitor instance
- :ip6: The IPv6 address of this Traffic Monitor - when applicable
- :ip: The IPv4 address of this Traffic Monitor
- :port: The port on which this Traffic Monitor listens for incoming connections
- :profile: A string that is the :ref:`profile-name` of the :term:`Profile` assigned to this Traffic Monitor
- :status: The status of the server running this Traffic Monitor instance
+ :cachegroup: The :term:`Cache Group` to which this Traffic Monitor belongs
Review comment:
Since Cache Groups have multiple identifiers (three, I think) it would be clearer to say "The _name of the_ Cache Group..."
##########
File path: traffic_monitor/todata/todata.go
##########
@@ -162,44 +154,71 @@ func (d TODataThreadsafe) Update(to towrap.TrafficOpsSessionThreadsafe, cdn stri
json := jsoniter.ConfigFastest
err = json.Unmarshal(crConfigBytes, &crConfig)
if err != nil {
- return fmt.Errorf("Error unmarshalling CRconfig: %v", err)
+ return fmt.Errorf("unmarshalling CRconfig: %v", err)
}
- newTOData.DeliveryServiceServers, newTOData.ServerDeliveryServices, err = getDeliveryServiceServers(crConfig)
- if err != nil {
- return err
- }
+ // TODO: remove the fallback behavior on the CRConfig in ATC 8.0 (https://github.com/apache/trafficcontrol/issues/6627)
+ newTOData.DeliveryServiceServers, newTOData.ServerDeliveryServices = getDeliveryServiceServers(crConfig, mc)
- newTOData.DeliveryServiceTypes, err = getDeliveryServiceTypes(crConfig)
+ // TODO: remove the fallback behavior on the CRConfig in ATC 8.0 (https://github.com/apache/trafficcontrol/issues/6627)
+ newTOData.DeliveryServiceTypes, err = getDeliveryServiceTypes(crConfig, mc)
if err != nil {
- return fmt.Errorf("Error getting delivery service types from Traffic Ops: %v\n", err)
+ return fmt.Errorf("getting delivery service types from Traffic Ops: %v\n", err)
Review comment:
Can you remove the newlines in these, too? In TO I'd also suggest error-wrapping, but that can be very slow and TM needs all the performance it can get.
##########
File path: traffic_ops/traffic_ops_golang/crconfig/servers.go
##########
@@ -361,10 +322,10 @@ order by dsr.set_number asc
serverDSPatterns := map[tc.CacheName]map[string][]string{}
for server, dses := range serverDSNames {
for _, dsName := range dses {
- dsInfList, ok := dsInfs[string(dsName)]
+ dsInfList, ok := dsInfs[dsName]
if !ok {
- if !dsHasTopology[string(dsName)] {
- log.Warnln("Creating CRConfig: deliveryservice " + string(dsName) + " has no regexes, skipping")
+ if !dsHasTopology[dsName] {
+ log.Warnln("creating CRConfig: deliveryservice " + dsName + " has no regexes, skipping")
Review comment:
why lowercase this?
##########
File path: traffic_ops/traffic_ops_golang/monitoring/monitoring.go
##########
@@ -158,6 +164,10 @@ func GetMonitoringJSON(tx *sql.Tx, cdnName string) (*Monitoring, error) {
if err != nil {
return nil, fmt.Errorf("error getting config: %v", err)
}
+ topologies, err := topology.MakeTopologies(tx)
+ if err != nil {
+ return nil, fmt.Errorf("getting topologies: %s", err.Error())
Review comment:
formatting errors should wrap with `%w`
--
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