You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@trafficcontrol.apache.org by zr...@apache.org on 2021/02/01 23:55:52 UTC
[trafficcontrol] branch master updated: TR: Fix potential steering
registry race, improve steering logging (#5482)
This is an automated email from the ASF dual-hosted git repository.
zrhoffman 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 aecab5e TR: Fix potential steering registry race, improve steering logging (#5482)
aecab5e is described below
commit aecab5e386b8583f747a73e280b6646f1dc8a86d
Author: Rawlin Peters <ra...@apache.org>
AuthorDate: Mon Feb 1 16:55:29 2021 -0700
TR: Fix potential steering registry race, improve steering logging (#5482)
* Fix potential steering registry race, improve steering logging
Clearing the existing map and putting all the new steerings into it can
potentially cause a race where the map is empty or partially empty when
doing steering lookups for incoming requests. Instead, swap the old map
with the new map (which is atomic).
Additionally, improve the steering registry logging so that only changes
are logged, rather than re-printing the entire registry when a single
entry is changed.
* Add changelog
* Simplify equals()
---
CHANGELOG.md | 1 +
.../traffic_router/core/ds/Steering.java | 21 ++++-----
.../traffic_router/core/ds/SteeringRegistry.java | 50 ++++++++++------------
3 files changed, 34 insertions(+), 38 deletions(-)
diff --git a/CHANGELOG.md b/CHANGELOG.md
index bcf2f19..d681be0 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/).
- [#5382](https://github.com/apache/trafficcontrol/issues/5382) - Fixed API documentation and TP helptext for "Max DNS Answers" field with respect to DNS, HTTP, Steering Delivery Service
- [#5396](https://github.com/apache/trafficcontrol/issues/5396) - Return the correct error type if user tries to update the root tenant
- [#5378](https://github.com/apache/trafficcontrol/issues/5378) - Updating a non existent DS should return a 404, instead of a 500
+- Fixed a potential Traffic Router race condition that could cause erroneous 503s for CLIENT_STEERING delivery services when loading new steering changes
- [#5195](https://github.com/apache/trafficcontrol/issues/5195) - Correctly show CDN ID in Changelog during Snap
- [#5438](https://github.com/apache/trafficcontrol/issues/5438) - Correctly specify nodejs version requirements in traffic_portal.spec
- Fixed Traffic Router logging unnecessary warnings for IPv6-only caches
diff --git a/traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/ds/Steering.java b/traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/ds/Steering.java
index 841df23..f673dd5 100644
--- a/traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/ds/Steering.java
+++ b/traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/ds/Steering.java
@@ -19,6 +19,7 @@ import com.fasterxml.jackson.annotation.JsonProperty;
import java.util.ArrayList;
import java.util.List;
+import java.util.Objects;
public class Steering {
@JsonProperty
@@ -83,18 +84,18 @@ public class Steering {
}
@Override
- @SuppressWarnings("PMD")
- public boolean equals(Object o) {
- if (this == o) return true;
- if (o == null || getClass() != o.getClass()) return false;
-
- Steering steering = (Steering) o;
-
- if (deliveryService != null ? !deliveryService.equals(steering.deliveryService) : steering.deliveryService != null)
+ public boolean equals(final Object o) {
+ if (this == o) {
+ return true;
+ }
+ if (o == null || getClass() != o.getClass()) {
return false;
- if (targets != null ? !targets.equals(steering.targets) : steering.targets != null) return false;
- return filters != null ? filters.equals(steering.filters) : steering.filters == null;
+ }
+ final Steering steering = (Steering) o;
+ return Objects.equals(deliveryService, steering.deliveryService)
+ && Objects.equals(targets, steering.targets)
+ && Objects.equals(filters, steering.filters);
}
@Override
diff --git a/traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/ds/SteeringRegistry.java b/traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/ds/SteeringRegistry.java
index 0b9437a..41b749a 100644
--- a/traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/ds/SteeringRegistry.java
+++ b/traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/ds/SteeringRegistry.java
@@ -21,7 +21,6 @@ import com.fasterxml.jackson.databind.ObjectMapper;
import org.apache.log4j.Logger;
import java.io.IOException;
-import java.util.ArrayList;
import java.util.Collection;
import java.util.HashMap;
import java.util.List;
@@ -30,7 +29,7 @@ import java.util.Map;
public class SteeringRegistry {
private static final Logger LOGGER = Logger.getLogger(SteeringRegistry.class);
- private final Map<String, Steering> registry = new HashMap<String, Steering>();
+ private Map<String, Steering> registry = new HashMap<>();
private final ObjectMapper objectMapper = new ObjectMapper(new JsonFactory());
@SuppressWarnings({"PMD.CyclomaticComplexity", "PMD.AvoidDuplicateLiterals"})
@@ -53,25 +52,29 @@ public class SteeringRegistry {
newSteerings.put(steering.getDeliveryService(), steering);
}
- registry.clear();
- registry.putAll(newSteerings);
- for (final Steering steering : steerings) {
- for (final SteeringTarget target : steering.getTargets()) {
- if (target.getGeolocation() != null && target.getGeoOrder() != 0) {
- LOGGER.info("Steering " + steering.getDeliveryService() + " target " + target.getDeliveryService() + " now has geolocation [" + target.getLatitude() + ", " + target.getLongitude() + "] and geoOrder " + target.getGeoOrder());
- } else if (target.getGeolocation() != null && target.getWeight() > 0) {
- LOGGER.info("Steering " + steering.getDeliveryService() + " target " + target.getDeliveryService() + " now has geolocation [" + target.getLatitude() + ", " + target.getLongitude() + "] and weight " + target.getWeight());
- } else if (target.getGeolocation() != null) {
- LOGGER.info("Steering " + steering.getDeliveryService() + " target " + target.getDeliveryService() + " now has geolocation [" + target.getLatitude() + ", " + target.getLongitude() + "]");
- } else if (target.getWeight() > 0) {
- LOGGER.info("Steering " + steering.getDeliveryService() + " target " + target.getDeliveryService() + " now has weight " + target.getWeight());
- } else if (target.getOrder() != 0) { // this target has a specific order set
- LOGGER.info("Steering " + steering.getDeliveryService() + " target " + target.getDeliveryService() + " now has order " + target.getOrder());
- } else {
- LOGGER.info("Steering " + steering.getDeliveryService() + " target " + target.getDeliveryService() + " now has weight " + target.getWeight() + " and order " + target.getOrder());
+ newSteerings.forEach((k, newSteering) -> {
+ final Steering old = registry.get(k);
+ if (old == null || !old.equals(newSteering)) {
+ for (final SteeringTarget target : newSteering.getTargets()) {
+ if (target.getGeolocation() != null && target.getGeoOrder() != 0) {
+ LOGGER.info("Steering " + newSteering.getDeliveryService() + " target " + target.getDeliveryService() + " now has geolocation [" + target.getLatitude() + ", " + target.getLongitude() + "] and geoOrder " + target.getGeoOrder());
+ } else if (target.getGeolocation() != null && target.getWeight() > 0) {
+ LOGGER.info("Steering " + newSteering.getDeliveryService() + " target " + target.getDeliveryService() + " now has geolocation [" + target.getLatitude() + ", " + target.getLongitude() + "] and weight " + target.getWeight());
+ } else if (target.getGeolocation() != null) {
+ LOGGER.info("Steering " + newSteering.getDeliveryService() + " target " + target.getDeliveryService() + " now has geolocation [" + target.getLatitude() + ", " + target.getLongitude() + "]");
+ } else if (target.getWeight() > 0) {
+ LOGGER.info("Steering " + newSteering.getDeliveryService() + " target " + target.getDeliveryService() + " now has weight " + target.getWeight());
+ } else if (target.getOrder() != 0) { // this target has a specific order set
+ LOGGER.info("Steering " + newSteering.getDeliveryService() + " target " + target.getDeliveryService() + " now has order " + target.getOrder());
+ } else {
+ LOGGER.info("Steering " + newSteering.getDeliveryService() + " target " + target.getDeliveryService() + " now has weight " + target.getWeight() + " and order " + target.getOrder());
+ }
}
}
- }
+ });
+
+ registry = newSteerings;
+ LOGGER.info("Finished updating steering registry");
}
public boolean verify(final String json) {
@@ -98,13 +101,4 @@ public class SteeringRegistry {
return registry.values();
}
- public Collection<Steering> removeAll(final List<String> steeringIds) {
- final List<Steering> removedEntries = new ArrayList<Steering>();
-
- for (final String steeringId : steeringIds) {
- removedEntries.add(registry.remove(steeringId));
- }
-
- return removedEntries;
- }
}
\ No newline at end of file