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 2020/02/17 23:12:31 UTC
[GitHub] [trafficcontrol] rawlinp opened a new pull request #4409: Optimize
TR DNSSEC zone re-signing
rawlinp opened a new pull request #4409: Optimize TR DNSSEC zone re-signing
URL: https://github.com/apache/trafficcontrol/pull/4409
## What does this PR (Pull Request) do?
If `dnssec.zone.diffing.enabled`, compare the previously generated zone
to the newly generated zone. If the only thing different is the SOA
record's serial number, just reuse the existing zone instead of signing
a new zone.
This drastically reduces the amount of CPU time it takes to process new snapshots or new DNSSEC keys, because only the zones that have actually changed will be re-signed and re-primed.
- [x] This PR is not related to any Issue
## Which Traffic Control components are affected by this PR?
- Documentation
- Traffic Router
## What is the best way to verify this PR?
1. Run the TR unit tests: `mvn clean test -Djava.library.path=/usr/local/opt/tomcat-native/lib`
2. _Without_ the new TR profile parameter enabled, snapshot the CDN a few times, verify that TR re-signs all the zones in the CDN every time. You will see INFO log lines like this: `"Signing records, name for first record is <insert name here>"`
3. Enable the new TR profile parameter and snapshot the CDN, verify that TR re-signs all the zones.
4. With the new TR profile parameter now enabled:
4.1. snapshot the CDN with no actual changes from the previous snapshot, verify that TR performs no zone signing.
4.2. remove/add a delivery service from/to an edge, snapshot, and verify that TR performs zone signing for that single zone.
NOTE: with the new TR profile parameter enabled,
when TR is _not_ going to re-sign a zone, an INFO log line like the following is printed:
```
"found matching ZoneKey for " + domain + " - copying from current Zone cache into new Zone cache - no re-signing necessary"
```
when TR _is_ going to re-sign a zone, an INFO log line like the following is printed:
```
"new zone for " + domain + " is not equal to the old zone - re-signing necessary"
```
## The following criteria are ALL met by this PR
- [x] This PR includes tests
- [x] This PR includes documentation
- [x] This PR includes an update to CHANGELOG.md
- [x] This PR includes any and all required license headers
- [x] This PR does not include a database migration
- [x] This PR **DOES NOT FIX A SERIOUS SECURITY VULNERABILITY** (see [the Apache Software Foundation's security guidelines](https://www.apache.org/security/) for details)
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request
#4409: Optimize TR DNSSEC zone re-signing
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4409: Optimize TR DNSSEC zone re-signing
URL: https://github.com/apache/trafficcontrol/pull/4409#discussion_r382341311
##########
File path: traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/dns/ZoneManager.java
##########
@@ -460,18 +499,37 @@ private static void generateZones(final TrafficRouter tr, final LoadingCache<Zon
LOGGER.fatal("Unable to create zone: " + ex.getMessage(), ex);
}
- primeZoneCache(domain, name, list, tr, zc, dzc, generationTasks, primingTasks, ds);
+ primeZoneCache(domain, name, list, tr, zc, dzc, generationTasks, primingTasks, ds, newDomainsToZoneKeys);
return records;
}
- @SuppressWarnings("PMD.CyclomaticComplexity")
+ @SuppressWarnings({"PMD.CyclomaticComplexity", "PMD.ExcessiveParameterList"})
private static void primeZoneCache(final String domain, final Name name, final List<Record> list, final TrafficRouter tr,
final LoadingCache<ZoneKey, Zone> zc, final LoadingCache<ZoneKey, Zone> dzc, final List<Runnable> generationTasks,
- final BlockingQueue<Runnable> primingTasks, final DeliveryService ds) {
+ final BlockingQueue<Runnable> primingTasks, final DeliveryService ds, final ConcurrentMap<String, ZoneKey> newDomainsToZoneKeys) {
generationTasks.add(() -> {
try {
- final Zone zone = zc.get(signatureManager.generateZoneKey(name, list)); // cause the zone to be loaded into the new cache
+ final ZoneKey newZoneKey = signatureManager.generateZoneKey(name, list);
+ if (tr.isDnssecZoneDiffingEnabled() && domainsToZoneKeys.containsKey(domain)) {
Review comment:
I agree about calling `zc.get(...)` before `newDomainsToZoneKeys.put(domain, newZoneKey)`. As I understand it, the reason you could additionally declare `Zone zone = zc.get(newZoneKey)` after `newDomainsToZoneKeys.put(...)` is that LoadingCache only creates signs the zone if the ZoneKey isn't already there. If it is is, you get the existing zone for free.
Calling `zc.get(newZoneKey)` again doesn't refresh the ZoneKey.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [trafficcontrol] rawlinp commented on a change in pull request
#4409: Optimize TR DNSSEC zone re-signing
Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #4409: Optimize TR DNSSEC zone re-signing
URL: https://github.com/apache/trafficcontrol/pull/4409#discussion_r382641008
##########
File path: traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/dns/ZoneManager.java
##########
@@ -460,18 +499,37 @@ private static void generateZones(final TrafficRouter tr, final LoadingCache<Zon
LOGGER.fatal("Unable to create zone: " + ex.getMessage(), ex);
}
- primeZoneCache(domain, name, list, tr, zc, dzc, generationTasks, primingTasks, ds);
+ primeZoneCache(domain, name, list, tr, zc, dzc, generationTasks, primingTasks, ds, newDomainsToZoneKeys);
return records;
}
- @SuppressWarnings("PMD.CyclomaticComplexity")
+ @SuppressWarnings({"PMD.CyclomaticComplexity", "PMD.ExcessiveParameterList"})
private static void primeZoneCache(final String domain, final Name name, final List<Record> list, final TrafficRouter tr,
final LoadingCache<ZoneKey, Zone> zc, final LoadingCache<ZoneKey, Zone> dzc, final List<Runnable> generationTasks,
- final BlockingQueue<Runnable> primingTasks, final DeliveryService ds) {
+ final BlockingQueue<Runnable> primingTasks, final DeliveryService ds, final ConcurrentMap<String, ZoneKey> newDomainsToZoneKeys) {
generationTasks.add(() -> {
try {
- final Zone zone = zc.get(signatureManager.generateZoneKey(name, list)); // cause the zone to be loaded into the new cache
+ final ZoneKey newZoneKey = signatureManager.generateZoneKey(name, list);
+ if (tr.isDnssecZoneDiffingEnabled() && domainsToZoneKeys.containsKey(domain)) {
Review comment:
Yeah, I think I will just keep the block as-is because I'd rather not add extra `zc.gets` and multi-returns just to remove a level of indentation. In this case I think one extra level of indentation is fine, especially since it's a relatively small block. Once the zone-diffing feature becomes vetted I would like to come back here and remove the feature flag for it, and just always do zone-diffing (with or without dnssec enabled). That will remove a level of indentation here in the future.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [trafficcontrol] ocket8888 merged pull request #4409: Optimize TR
DNSSEC zone re-signing
Posted by GitBox <gi...@apache.org>.
ocket8888 merged pull request #4409: Optimize TR DNSSEC zone re-signing
URL: https://github.com/apache/trafficcontrol/pull/4409
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [trafficcontrol] rawlinp commented on a change in pull request
#4409: Optimize TR DNSSEC zone re-signing
Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #4409: Optimize TR DNSSEC zone re-signing
URL: https://github.com/apache/trafficcontrol/pull/4409#discussion_r382152225
##########
File path: traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/dns/ZoneManager.java
##########
@@ -538,6 +596,42 @@ private static void primeDNSDeliveryServices(final String domain, final TrafficR
}
}
+ // Check if the zones are equal except for the SOA record serial number, NSEC, or RRSIG records
+ private static boolean zonesAreEqual(final List<Record> newRecords, final List<Record> oldRecords) {
+ final List<Record> oldRecordsCopy = oldRecords.stream()
+ .filter(r -> !(r instanceof NSECRecord) && !(r instanceof RRSIGRecord))
+ .collect(Collectors.toList());
+ final List<Record> newRecordsCopy = newRecords.stream()
+ .filter(r -> !(r instanceof NSECRecord) && !(r instanceof RRSIGRecord))
+ .collect(Collectors.toList());
+ if (oldRecordsCopy.size() != newRecordsCopy.size()) {
+ return false;
+ }
+ Collections.sort(oldRecordsCopy);
+ Collections.sort(newRecordsCopy);
+ for (int i = 0; i < newRecordsCopy.size(); i++) {
+ final Record newRec = newRecordsCopy.get(i);
+ final Record oldRec = oldRecordsCopy.get(i);
+ if (newRec instanceof SOARecord && oldRec instanceof SOARecord) {
Review comment:
To me it seems weird to not use `instanceof` when typecasting from Record to one of its subclasses, and if I'm using it for SOARecord down here I think it makes sense to use it up above for consistency within this method as well.
Are there performance concerns about using `instanceof`?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request
#4409: Optimize TR DNSSEC zone re-signing
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4409: Optimize TR DNSSEC zone re-signing
URL: https://github.com/apache/trafficcontrol/pull/4409#discussion_r382249399
##########
File path: traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/dns/ZoneManager.java
##########
@@ -460,18 +499,37 @@ private static void generateZones(final TrafficRouter tr, final LoadingCache<Zon
LOGGER.fatal("Unable to create zone: " + ex.getMessage(), ex);
}
- primeZoneCache(domain, name, list, tr, zc, dzc, generationTasks, primingTasks, ds);
+ primeZoneCache(domain, name, list, tr, zc, dzc, generationTasks, primingTasks, ds, newDomainsToZoneKeys);
return records;
}
- @SuppressWarnings("PMD.CyclomaticComplexity")
+ @SuppressWarnings({"PMD.CyclomaticComplexity", "PMD.ExcessiveParameterList"})
private static void primeZoneCache(final String domain, final Name name, final List<Record> list, final TrafficRouter tr,
final LoadingCache<ZoneKey, Zone> zc, final LoadingCache<ZoneKey, Zone> dzc, final List<Runnable> generationTasks,
- final BlockingQueue<Runnable> primingTasks, final DeliveryService ds) {
+ final BlockingQueue<Runnable> primingTasks, final DeliveryService ds, final ConcurrentMap<String, ZoneKey> newDomainsToZoneKeys) {
generationTasks.add(() -> {
try {
- final Zone zone = zc.get(signatureManager.generateZoneKey(name, list)); // cause the zone to be loaded into the new cache
+ final ZoneKey newZoneKey = signatureManager.generateZoneKey(name, list);
+ if (tr.isDnssecZoneDiffingEnabled() && domainsToZoneKeys.containsKey(domain)) {
Review comment:
That approach would also involve passing in `newZoneKey`.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [trafficcontrol] rawlinp commented on a change in pull request
#4409: Optimize TR DNSSEC zone re-signing
Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #4409: Optimize TR DNSSEC zone re-signing
URL: https://github.com/apache/trafficcontrol/pull/4409#discussion_r382223394
##########
File path: traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/dns/ZoneManager.java
##########
@@ -460,18 +499,37 @@ private static void generateZones(final TrafficRouter tr, final LoadingCache<Zon
LOGGER.fatal("Unable to create zone: " + ex.getMessage(), ex);
}
- primeZoneCache(domain, name, list, tr, zc, dzc, generationTasks, primingTasks, ds);
+ primeZoneCache(domain, name, list, tr, zc, dzc, generationTasks, primingTasks, ds, newDomainsToZoneKeys);
return records;
}
- @SuppressWarnings("PMD.CyclomaticComplexity")
+ @SuppressWarnings({"PMD.CyclomaticComplexity", "PMD.ExcessiveParameterList"})
private static void primeZoneCache(final String domain, final Name name, final List<Record> list, final TrafficRouter tr,
final LoadingCache<ZoneKey, Zone> zc, final LoadingCache<ZoneKey, Zone> dzc, final List<Runnable> generationTasks,
- final BlockingQueue<Runnable> primingTasks, final DeliveryService ds) {
+ final BlockingQueue<Runnable> primingTasks, final DeliveryService ds, final ConcurrentMap<String, ZoneKey> newDomainsToZoneKeys) {
generationTasks.add(() -> {
try {
- final Zone zone = zc.get(signatureManager.generateZoneKey(name, list)); // cause the zone to be loaded into the new cache
+ final ZoneKey newZoneKey = signatureManager.generateZoneKey(name, list);
+ if (tr.isDnssecZoneDiffingEnabled() && domainsToZoneKeys.containsKey(domain)) {
Review comment:
So, I tried it to split this out, but I wanted to include all of L513-532 since I wanted all of the modification of `newDomainsToZoneKeys` to be done in the same place. Then I found that the early return on L522 makes splitting this out trickier because the new method would have to have a `Zone` passed in to be set as one method result but also return a `boolean` for whether or not an existing zone was matched and reused.
That said, I think the way the code is currently would be easier to comprehend than what I just described. What do you think?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request
#4409: Optimize TR DNSSEC zone re-signing
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4409: Optimize TR DNSSEC zone re-signing
URL: https://github.com/apache/trafficcontrol/pull/4409#discussion_r382114010
##########
File path: traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/dns/ZoneManager.java
##########
@@ -460,18 +499,37 @@ private static void generateZones(final TrafficRouter tr, final LoadingCache<Zon
LOGGER.fatal("Unable to create zone: " + ex.getMessage(), ex);
}
- primeZoneCache(domain, name, list, tr, zc, dzc, generationTasks, primingTasks, ds);
+ primeZoneCache(domain, name, list, tr, zc, dzc, generationTasks, primingTasks, ds, newDomainsToZoneKeys);
return records;
}
- @SuppressWarnings("PMD.CyclomaticComplexity")
+ @SuppressWarnings({"PMD.CyclomaticComplexity", "PMD.ExcessiveParameterList"})
private static void primeZoneCache(final String domain, final Name name, final List<Record> list, final TrafficRouter tr,
final LoadingCache<ZoneKey, Zone> zc, final LoadingCache<ZoneKey, Zone> dzc, final List<Runnable> generationTasks,
- final BlockingQueue<Runnable> primingTasks, final DeliveryService ds) {
+ final BlockingQueue<Runnable> primingTasks, final DeliveryService ds, final ConcurrentMap<String, ZoneKey> newDomainsToZoneKeys) {
generationTasks.add(() -> {
try {
- final Zone zone = zc.get(signatureManager.generateZoneKey(name, list)); // cause the zone to be loaded into the new cache
+ final ZoneKey newZoneKey = signatureManager.generateZoneKey(name, list);
+ if (tr.isDnssecZoneDiffingEnabled() && domainsToZoneKeys.containsKey(domain)) {
Review comment:
Consider splitting this block into its own function to avoid 3 levels of `if` statements.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request
#4409: Optimize TR DNSSEC zone re-signing
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4409: Optimize TR DNSSEC zone re-signing
URL: https://github.com/apache/trafficcontrol/pull/4409#discussion_r382341311
##########
File path: traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/dns/ZoneManager.java
##########
@@ -460,18 +499,37 @@ private static void generateZones(final TrafficRouter tr, final LoadingCache<Zon
LOGGER.fatal("Unable to create zone: " + ex.getMessage(), ex);
}
- primeZoneCache(domain, name, list, tr, zc, dzc, generationTasks, primingTasks, ds);
+ primeZoneCache(domain, name, list, tr, zc, dzc, generationTasks, primingTasks, ds, newDomainsToZoneKeys);
return records;
}
- @SuppressWarnings("PMD.CyclomaticComplexity")
+ @SuppressWarnings({"PMD.CyclomaticComplexity", "PMD.ExcessiveParameterList"})
private static void primeZoneCache(final String domain, final Name name, final List<Record> list, final TrafficRouter tr,
final LoadingCache<ZoneKey, Zone> zc, final LoadingCache<ZoneKey, Zone> dzc, final List<Runnable> generationTasks,
- final BlockingQueue<Runnable> primingTasks, final DeliveryService ds) {
+ final BlockingQueue<Runnable> primingTasks, final DeliveryService ds, final ConcurrentMap<String, ZoneKey> newDomainsToZoneKeys) {
generationTasks.add(() -> {
try {
- final Zone zone = zc.get(signatureManager.generateZoneKey(name, list)); // cause the zone to be loaded into the new cache
+ final ZoneKey newZoneKey = signatureManager.generateZoneKey(name, list);
+ if (tr.isDnssecZoneDiffingEnabled() && domainsToZoneKeys.containsKey(domain)) {
Review comment:
I agree about calling `zc.get(...)` before `newDomainsToZoneKeys.put(domain, newZoneKey)`. As I understand it, the reason you could additionally declare `Zone zone = zc.get(newZoneKey)` after `newDomainsToZoneKeys.put(...)` is that LoadingCache only creates and signs the zone if the ZoneKey isn't already there. If it is, you get the existing zone for free.
Calling `zc.get(newZoneKey)` again doesn't refresh the ZoneKey.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request
#4409: Optimize TR DNSSEC zone re-signing
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4409: Optimize TR DNSSEC zone re-signing
URL: https://github.com/apache/trafficcontrol/pull/4409#discussion_r382110327
##########
File path: traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/dns/ZoneManager.java
##########
@@ -240,6 +247,34 @@ protected static void initZoneCache(final TrafficRouter tr) {
}
}
+ private static void copyExistingDynamicZones(final TrafficRouter tr, final LoadingCache<ZoneKey, Zone> dzc) {
+ final Map<String, DeliveryService> allZones = getAllDeliveryServiceDomains(tr);
+ allZones.put(getTopLevelDomain().toString(true), null);
+ final Map<ZoneKey, Zone> dzcMap = dynamicZoneCache.asMap();
+ for (final ZoneKey zoneKey : dzcMap.keySet()) {
+ if (allZones.containsKey(zoneKey.getName().toString(true))) {
+ dzc.put(zoneKey, dzcMap.get(zoneKey));
+ } else {
+ LOGGER.info("domain for old zone " + zoneKey.getName().toString(true) + " not found; will not copy it into new dynamic zone cache");
+ }
+ }
+ }
+
+ private static int calcThreadPoolSize(final JsonNode config) {
+ int poolSize = 1;
+ final double scale = JsonUtils.optDouble(config, "zonemanager.threadpool.scale", 0.75);
+ final int cores = Runtime.getRuntime().availableProcessors();
+
+ if (cores > 2) {
+ final Double s = Math.floor((double) cores * scale);
Review comment:
Any reason `s` can't be an `int`?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request
#4409: Optimize TR DNSSEC zone re-signing
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4409: Optimize TR DNSSEC zone re-signing
URL: https://github.com/apache/trafficcontrol/pull/4409#discussion_r382244989
##########
File path: traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/dns/ZoneManager.java
##########
@@ -460,18 +499,37 @@ private static void generateZones(final TrafficRouter tr, final LoadingCache<Zon
LOGGER.fatal("Unable to create zone: " + ex.getMessage(), ex);
}
- primeZoneCache(domain, name, list, tr, zc, dzc, generationTasks, primingTasks, ds);
+ primeZoneCache(domain, name, list, tr, zc, dzc, generationTasks, primingTasks, ds, newDomainsToZoneKeys);
return records;
}
- @SuppressWarnings("PMD.CyclomaticComplexity")
+ @SuppressWarnings({"PMD.CyclomaticComplexity", "PMD.ExcessiveParameterList"})
private static void primeZoneCache(final String domain, final Name name, final List<Record> list, final TrafficRouter tr,
final LoadingCache<ZoneKey, Zone> zc, final LoadingCache<ZoneKey, Zone> dzc, final List<Runnable> generationTasks,
- final BlockingQueue<Runnable> primingTasks, final DeliveryService ds) {
+ final BlockingQueue<Runnable> primingTasks, final DeliveryService ds, final ConcurrentMap<String, ZoneKey> newDomainsToZoneKeys) {
generationTasks.add(() -> {
try {
- final Zone zone = zc.get(signatureManager.generateZoneKey(name, list)); // cause the zone to be loaded into the new cache
+ final ZoneKey newZoneKey = signatureManager.generateZoneKey(name, list);
+ if (tr.isDnssecZoneDiffingEnabled() && domainsToZoneKeys.containsKey(domain)) {
Review comment:
The new method would not need a `Zone` passed in if you are okay also calling declaring setting `Zone zone = zc.get(newZoneKey)` outside that method [for line 541](https://github.com/apache/trafficcontrol/blob/afc6baab28/traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/dns/ZoneManager.java#L541).
The early return on L522 doesn't need to make splitting this out trickier if you are okay having multiple returns. Something like:
```java
private static boolean diffZones(...) throws ExecutionException {
...
if (!tr.isDnssecZoneDiffingEnabled() || !domainsToZoneKeys.containsKey(domain)) {
return false;
}
...
if (!zonesAreEqual(newZoneKey.getRecords(), oldZoneKey.getRecords())) {
...
return false;
}
...
if (oldZone != null) {
...
}
...
return true;
}
```
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request
#4409: Optimize TR DNSSEC zone re-signing
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4409: Optimize TR DNSSEC zone re-signing
URL: https://github.com/apache/trafficcontrol/pull/4409#discussion_r382165620
##########
File path: traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/dns/ZoneManager.java
##########
@@ -538,6 +596,42 @@ private static void primeDNSDeliveryServices(final String domain, final TrafficR
}
}
+ // Check if the zones are equal except for the SOA record serial number, NSEC, or RRSIG records
+ private static boolean zonesAreEqual(final List<Record> newRecords, final List<Record> oldRecords) {
+ final List<Record> oldRecordsCopy = oldRecords.stream()
+ .filter(r -> !(r instanceof NSECRecord) && !(r instanceof RRSIGRecord))
+ .collect(Collectors.toList());
+ final List<Record> newRecordsCopy = newRecords.stream()
+ .filter(r -> !(r instanceof NSECRecord) && !(r instanceof RRSIGRecord))
+ .collect(Collectors.toList());
+ if (oldRecordsCopy.size() != newRecordsCopy.size()) {
+ return false;
+ }
+ Collections.sort(oldRecordsCopy);
+ Collections.sort(newRecordsCopy);
+ for (int i = 0; i < newRecordsCopy.size(); i++) {
+ final Record newRec = newRecordsCopy.get(i);
+ final Record oldRec = oldRecordsCopy.get(i);
+ if (newRec instanceof SOARecord && oldRec instanceof SOARecord) {
Review comment:
Nope, no performance concerns. The edge case would be if someone ever creates a new subclass of Record of type SOA without extending SOARecord. After your explanation, `instanceof` sounds right here.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [trafficcontrol] mattjackson220 commented on issue #4409: Optimize
TR DNSSEC zone re-signing
Posted by GitBox <gi...@apache.org>.
mattjackson220 commented on issue #4409: Optimize TR DNSSEC zone re-signing
URL: https://github.com/apache/trafficcontrol/pull/4409#issuecomment-589738081
Tested this locally and it works great! with it disabled it resigns everything, with it enabled it only signs the ones that changed. Tests run successfully. Docs build and look good!
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request
#4409: Optimize TR DNSSEC zone re-signing
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4409: Optimize TR DNSSEC zone re-signing
URL: https://github.com/apache/trafficcontrol/pull/4409#discussion_r382166056
##########
File path: traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/dns/ZoneManager.java
##########
@@ -240,6 +247,34 @@ protected static void initZoneCache(final TrafficRouter tr) {
}
}
+ private static void copyExistingDynamicZones(final TrafficRouter tr, final LoadingCache<ZoneKey, Zone> dzc) {
+ final Map<String, DeliveryService> allZones = getAllDeliveryServiceDomains(tr);
+ allZones.put(getTopLevelDomain().toString(true), null);
+ final Map<ZoneKey, Zone> dzcMap = dynamicZoneCache.asMap();
+ for (final ZoneKey zoneKey : dzcMap.keySet()) {
+ if (allZones.containsKey(zoneKey.getName().toString(true))) {
+ dzc.put(zoneKey, dzcMap.get(zoneKey));
+ } else {
+ LOGGER.info("domain for old zone " + zoneKey.getName().toString(true) + " not found; will not copy it into new dynamic zone cache");
+ }
+ }
+ }
+
+ private static int calcThreadPoolSize(final JsonNode config) {
+ int poolSize = 1;
+ final double scale = JsonUtils.optDouble(config, "zonemanager.threadpool.scale", 0.75);
+ final int cores = Runtime.getRuntime().availableProcessors();
+
+ if (cores > 2) {
+ final Double s = Math.floor((double) cores * scale);
Review comment:
Okay, let's not rework without a good reason.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request
#4409: Optimize TR DNSSEC zone re-signing
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4409: Optimize TR DNSSEC zone re-signing
URL: https://github.com/apache/trafficcontrol/pull/4409#discussion_r382273519
##########
File path: traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/dns/ZoneManager.java
##########
@@ -460,18 +499,37 @@ private static void generateZones(final TrafficRouter tr, final LoadingCache<Zon
LOGGER.fatal("Unable to create zone: " + ex.getMessage(), ex);
}
- primeZoneCache(domain, name, list, tr, zc, dzc, generationTasks, primingTasks, ds);
+ primeZoneCache(domain, name, list, tr, zc, dzc, generationTasks, primingTasks, ds, newDomainsToZoneKeys);
return records;
}
- @SuppressWarnings("PMD.CyclomaticComplexity")
+ @SuppressWarnings({"PMD.CyclomaticComplexity", "PMD.ExcessiveParameterList"})
private static void primeZoneCache(final String domain, final Name name, final List<Record> list, final TrafficRouter tr,
final LoadingCache<ZoneKey, Zone> zc, final LoadingCache<ZoneKey, Zone> dzc, final List<Runnable> generationTasks,
- final BlockingQueue<Runnable> primingTasks, final DeliveryService ds) {
+ final BlockingQueue<Runnable> primingTasks, final DeliveryService ds, final ConcurrentMap<String, ZoneKey> newDomainsToZoneKeys) {
generationTasks.add(() -> {
try {
- final Zone zone = zc.get(signatureManager.generateZoneKey(name, list)); // cause the zone to be loaded into the new cache
+ final ZoneKey newZoneKey = signatureManager.generateZoneKey(name, list);
+ if (tr.isDnssecZoneDiffingEnabled() && domainsToZoneKeys.containsKey(domain)) {
Review comment:
But yeah, if that complicates it, I agree it's better to leave as-is.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request
#4409: Optimize TR DNSSEC zone re-signing
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4409: Optimize TR DNSSEC zone re-signing
URL: https://github.com/apache/trafficcontrol/pull/4409#discussion_r382109272
##########
File path: traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/dns/ZoneManager.java
##########
@@ -538,6 +596,42 @@ private static void primeDNSDeliveryServices(final String domain, final TrafficR
}
}
+ // Check if the zones are equal except for the SOA record serial number, NSEC, or RRSIG records
+ private static boolean zonesAreEqual(final List<Record> newRecords, final List<Record> oldRecords) {
+ final List<Record> oldRecordsCopy = oldRecords.stream()
+ .filter(r -> !(r instanceof NSECRecord) && !(r instanceof RRSIGRecord))
Review comment:
Use `getType()` and `Type.NSEC` to check if it is a NSEC record, not `instanceof` (same for RRSIGRecord).
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request
#4409: Optimize TR DNSSEC zone re-signing
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4409: Optimize TR DNSSEC zone re-signing
URL: https://github.com/apache/trafficcontrol/pull/4409#discussion_r382110982
##########
File path: traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/dns/ZoneManager.java
##########
@@ -538,6 +596,42 @@ private static void primeDNSDeliveryServices(final String domain, final TrafficR
}
}
+ // Check if the zones are equal except for the SOA record serial number, NSEC, or RRSIG records
+ private static boolean zonesAreEqual(final List<Record> newRecords, final List<Record> oldRecords) {
+ final List<Record> oldRecordsCopy = oldRecords.stream()
+ .filter(r -> !(r instanceof NSECRecord) && !(r instanceof RRSIGRecord))
+ .collect(Collectors.toList());
+ final List<Record> newRecordsCopy = newRecords.stream()
+ .filter(r -> !(r instanceof NSECRecord) && !(r instanceof RRSIGRecord))
Review comment:
Use `getType()` and `Type.NSEC` to check if it is a NSEC record, not `instanceof` (same for RRSIGRecord).
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [trafficcontrol] rawlinp commented on a change in pull request
#4409: Optimize TR DNSSEC zone re-signing
Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #4409: Optimize TR DNSSEC zone re-signing
URL: https://github.com/apache/trafficcontrol/pull/4409#discussion_r382156731
##########
File path: traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/dns/ZoneManager.java
##########
@@ -460,18 +499,37 @@ private static void generateZones(final TrafficRouter tr, final LoadingCache<Zon
LOGGER.fatal("Unable to create zone: " + ex.getMessage(), ex);
}
- primeZoneCache(domain, name, list, tr, zc, dzc, generationTasks, primingTasks, ds);
+ primeZoneCache(domain, name, list, tr, zc, dzc, generationTasks, primingTasks, ds, newDomainsToZoneKeys);
return records;
}
- @SuppressWarnings("PMD.CyclomaticComplexity")
+ @SuppressWarnings({"PMD.CyclomaticComplexity", "PMD.ExcessiveParameterList"})
private static void primeZoneCache(final String domain, final Name name, final List<Record> list, final TrafficRouter tr,
final LoadingCache<ZoneKey, Zone> zc, final LoadingCache<ZoneKey, Zone> dzc, final List<Runnable> generationTasks,
- final BlockingQueue<Runnable> primingTasks, final DeliveryService ds) {
+ final BlockingQueue<Runnable> primingTasks, final DeliveryService ds, final ConcurrentMap<String, ZoneKey> newDomainsToZoneKeys) {
generationTasks.add(() -> {
try {
- final Zone zone = zc.get(signatureManager.generateZoneKey(name, list)); // cause the zone to be loaded into the new cache
+ final ZoneKey newZoneKey = signatureManager.generateZoneKey(name, list);
+ if (tr.isDnssecZoneDiffingEnabled() && domainsToZoneKeys.containsKey(domain)) {
Review comment:
Sure, I can try that out.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [trafficcontrol] rawlinp commented on a change in pull request
#4409: Optimize TR DNSSEC zone re-signing
Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #4409: Optimize TR DNSSEC zone re-signing
URL: https://github.com/apache/trafficcontrol/pull/4409#discussion_r382153635
##########
File path: traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/dns/ZoneManager.java
##########
@@ -240,6 +247,34 @@ protected static void initZoneCache(final TrafficRouter tr) {
}
}
+ private static void copyExistingDynamicZones(final TrafficRouter tr, final LoadingCache<ZoneKey, Zone> dzc) {
+ final Map<String, DeliveryService> allZones = getAllDeliveryServiceDomains(tr);
+ allZones.put(getTopLevelDomain().toString(true), null);
+ final Map<ZoneKey, Zone> dzcMap = dynamicZoneCache.asMap();
+ for (final ZoneKey zoneKey : dzcMap.keySet()) {
+ if (allZones.containsKey(zoneKey.getName().toString(true))) {
+ dzc.put(zoneKey, dzcMap.get(zoneKey));
+ } else {
+ LOGGER.info("domain for old zone " + zoneKey.getName().toString(true) + " not found; will not copy it into new dynamic zone cache");
+ }
+ }
+ }
+
+ private static int calcThreadPoolSize(final JsonNode config) {
+ int poolSize = 1;
+ final double scale = JsonUtils.optDouble(config, "zonemanager.threadpool.scale", 0.75);
+ final int cores = Runtime.getRuntime().availableProcessors();
+
+ if (cores > 2) {
+ final Double s = Math.floor((double) cores * scale);
Review comment:
I didn't write this code I just refactored it into its own method, but here `scale` is double which is why I'm guessing it was done this way. I'd rather not rework existing code unless there is a good reason to.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [trafficcontrol] rawlinp commented on a change in pull request
#4409: Optimize TR DNSSEC zone re-signing
Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #4409: Optimize TR DNSSEC zone re-signing
URL: https://github.com/apache/trafficcontrol/pull/4409#discussion_r382296940
##########
File path: traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/dns/ZoneManager.java
##########
@@ -460,18 +499,37 @@ private static void generateZones(final TrafficRouter tr, final LoadingCache<Zon
LOGGER.fatal("Unable to create zone: " + ex.getMessage(), ex);
}
- primeZoneCache(domain, name, list, tr, zc, dzc, generationTasks, primingTasks, ds);
+ primeZoneCache(domain, name, list, tr, zc, dzc, generationTasks, primingTasks, ds, newDomainsToZoneKeys);
return records;
}
- @SuppressWarnings("PMD.CyclomaticComplexity")
+ @SuppressWarnings({"PMD.CyclomaticComplexity", "PMD.ExcessiveParameterList"})
private static void primeZoneCache(final String domain, final Name name, final List<Record> list, final TrafficRouter tr,
final LoadingCache<ZoneKey, Zone> zc, final LoadingCache<ZoneKey, Zone> dzc, final List<Runnable> generationTasks,
- final BlockingQueue<Runnable> primingTasks, final DeliveryService ds) {
+ final BlockingQueue<Runnable> primingTasks, final DeliveryService ds, final ConcurrentMap<String, ZoneKey> newDomainsToZoneKeys) {
generationTasks.add(() -> {
try {
- final Zone zone = zc.get(signatureManager.generateZoneKey(name, list)); // cause the zone to be loaded into the new cache
+ final ZoneKey newZoneKey = signatureManager.generateZoneKey(name, list);
+ if (tr.isDnssecZoneDiffingEnabled() && domainsToZoneKeys.containsKey(domain)) {
Review comment:
Yeah, the thing is `zc.get(newZoneKey)` is what actually creates and signs the zone if the value isn't there already. So I'm pretty sure I have to `newDomainsToZoneKeys.put(domain, newZoneKey);` _after_ doing `zc.get(...)` so that it's definitely already signed if I want to reuse it. I might be wrong about that, since the reference shouldn't change after adding it to `newDomainsToZoneKeys`, but I feel better about not saving it into the map until it's signed for sure.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request
#4409: Optimize TR DNSSEC zone re-signing
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4409: Optimize TR DNSSEC zone re-signing
URL: https://github.com/apache/trafficcontrol/pull/4409#discussion_r382690581
##########
File path: traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/dns/ZoneManager.java
##########
@@ -460,18 +499,37 @@ private static void generateZones(final TrafficRouter tr, final LoadingCache<Zon
LOGGER.fatal("Unable to create zone: " + ex.getMessage(), ex);
}
- primeZoneCache(domain, name, list, tr, zc, dzc, generationTasks, primingTasks, ds);
+ primeZoneCache(domain, name, list, tr, zc, dzc, generationTasks, primingTasks, ds, newDomainsToZoneKeys);
return records;
}
- @SuppressWarnings("PMD.CyclomaticComplexity")
+ @SuppressWarnings({"PMD.CyclomaticComplexity", "PMD.ExcessiveParameterList"})
private static void primeZoneCache(final String domain, final Name name, final List<Record> list, final TrafficRouter tr,
final LoadingCache<ZoneKey, Zone> zc, final LoadingCache<ZoneKey, Zone> dzc, final List<Runnable> generationTasks,
- final BlockingQueue<Runnable> primingTasks, final DeliveryService ds) {
+ final BlockingQueue<Runnable> primingTasks, final DeliveryService ds, final ConcurrentMap<String, ZoneKey> newDomainsToZoneKeys) {
generationTasks.add(() -> {
try {
- final Zone zone = zc.get(signatureManager.generateZoneKey(name, list)); // cause the zone to be loaded into the new cache
+ final ZoneKey newZoneKey = signatureManager.generateZoneKey(name, list);
+ if (tr.isDnssecZoneDiffingEnabled() && domainsToZoneKeys.containsKey(domain)) {
Review comment:
Fair enough, sounds good to me.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request
#4409: Optimize TR DNSSEC zone re-signing
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4409: Optimize TR DNSSEC zone re-signing
URL: https://github.com/apache/trafficcontrol/pull/4409#discussion_r382111752
##########
File path: traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/dns/ZoneManager.java
##########
@@ -538,6 +596,42 @@ private static void primeDNSDeliveryServices(final String domain, final TrafficR
}
}
+ // Check if the zones are equal except for the SOA record serial number, NSEC, or RRSIG records
+ private static boolean zonesAreEqual(final List<Record> newRecords, final List<Record> oldRecords) {
+ final List<Record> oldRecordsCopy = oldRecords.stream()
+ .filter(r -> !(r instanceof NSECRecord) && !(r instanceof RRSIGRecord))
+ .collect(Collectors.toList());
+ final List<Record> newRecordsCopy = newRecords.stream()
+ .filter(r -> !(r instanceof NSECRecord) && !(r instanceof RRSIGRecord))
+ .collect(Collectors.toList());
+ if (oldRecordsCopy.size() != newRecordsCopy.size()) {
+ return false;
+ }
+ Collections.sort(oldRecordsCopy);
+ Collections.sort(newRecordsCopy);
+ for (int i = 0; i < newRecordsCopy.size(); i++) {
+ final Record newRec = newRecordsCopy.get(i);
+ final Record oldRec = oldRecordsCopy.get(i);
+ if (newRec instanceof SOARecord && oldRec instanceof SOARecord) {
Review comment:
Use `getType()` and `Type.SOA` to check if they are SOA records, not `instanceof`.
----------------------------------------------------------------
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
With regards,
Apache Git Services