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/10/09 20:21:35 UTC
[GitHub] [trafficcontrol] rawlinp opened a new pull request #5133: TR: set cache to unavailable if not found in TM health data
rawlinp opened a new pull request #5133:
URL: https://github.com/apache/trafficcontrol/pull/5133
## What does this PR (Pull Request) do?
In Traffic Router, set a cache to unavailable if it's not found in the TM health data (CrStates). Otherwise, when a cache is set to OFFLINE and removed from TM health data, there is a short window where TR sets it to available until it has finished processing the new snapshot. OFFLINE caches should never be considered available.
## Which Traffic Control components are affected by this PR?
- Traffic Router
## What is the best way to verify this PR?
This is a race condition, which makes it difficult to verify manually, but it can be attempted by:
1. Assign a DS to a single cache
2. Repeatedly request the DS
3. Set the cache to ADMIN_DOWN, snapshot (TR should be returning 5xx shortly)
4. Set the cache to OFFLINE, snapshot
Without this fix, you might be able to get TR to 302 to the offlined cache for a short period until TR has finished processing the new snapshot. With this fix, that should not happen.
## If this is a bug fix, what versions of Traffic Control are affected?
- master
- 4.x
- 3.x
## The following criteria are ALL met by this PR
- [x] This section of the code currently lacks tests, but I did manually verify the expected behavior before/after
- [x] bug fix, no docs necessary
- [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
[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #5133: TR: set cache to unavailable if not found in TM health data
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #5133:
URL: https://github.com/apache/trafficcontrol/pull/5133#discussion_r503473716
##########
File path: traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/edge/Node.java
##########
@@ -200,9 +200,18 @@ public InetAddress getIp6() {
@SuppressWarnings({"PMD.CyclomaticComplexity", "PMD.NPathComplexity"})
public void setState(final JsonNode state) {
- final boolean isAvailable = JsonUtils.optBoolean(state, "isAvailable", true);
- final boolean ipv4Available = JsonUtils.optBoolean(state, "ipv4Available", true);
- final boolean ipv6Available = JsonUtils.optBoolean(state, "ipv6Available", true);
+ final boolean ipv4Available;
+ final boolean ipv6Available;
+ if (state == null) {
+ LOGGER.warn("got null health state for " + fqdn + ". Setting it to unavailable!");
+ isAvailable = false;
+ ipv4Available = false;
+ ipv6Available = false;
+ } else {
+ isAvailable = JsonUtils.optBoolean(state, "isAvailable", true);
+ ipv4Available = JsonUtils.optBoolean(state, "ipv4Available", true);
+ ipv6Available = JsonUtils.optBoolean(state, "ipv6Available", true);
+ }
Review comment:
Previously, if `state` was `null`, `JsonUtils.optBoolean()` would return the provided default value. So does this produce a different result than
```java
isAvailable = JsonUtils.optBoolean(state, "isAvailable", false);
ipv4Available = JsonUtils.optBoolean(state, "ipv4Available", false);
ipv6Available = JsonUtils.optBoolean(state, "ipv6Available", false);
```
?
----------------------------------------------------------------
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
[GitHub] [trafficcontrol] zrhoffman merged pull request #5133: TR: set cache to unavailable if not found in TM health data
Posted by GitBox <gi...@apache.org>.
zrhoffman merged pull request #5133:
URL: https://github.com/apache/trafficcontrol/pull/5133
----------------------------------------------------------------
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
[GitHub] [trafficcontrol] rawlinp commented on a change in pull request #5133: TR: set cache to unavailable if not found in TM health data
Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #5133:
URL: https://github.com/apache/trafficcontrol/pull/5133#discussion_r503484237
##########
File path: traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/edge/Node.java
##########
@@ -200,9 +200,18 @@ public InetAddress getIp6() {
@SuppressWarnings({"PMD.CyclomaticComplexity", "PMD.NPathComplexity"})
public void setState(final JsonNode state) {
- final boolean isAvailable = JsonUtils.optBoolean(state, "isAvailable", true);
- final boolean ipv4Available = JsonUtils.optBoolean(state, "ipv4Available", true);
- final boolean ipv6Available = JsonUtils.optBoolean(state, "ipv6Available", true);
+ final boolean ipv4Available;
+ final boolean ipv6Available;
+ if (state == null) {
+ LOGGER.warn("got null health state for " + fqdn + ". Setting it to unavailable!");
+ isAvailable = false;
+ ipv4Available = false;
+ ipv6Available = false;
+ } else {
+ isAvailable = JsonUtils.optBoolean(state, "isAvailable", true);
+ ipv4Available = JsonUtils.optBoolean(state, "ipv4Available", true);
+ ipv6Available = JsonUtils.optBoolean(state, "ipv6Available", true);
+ }
Review comment:
The behavior is slightly different if we were to do that instead, because there is a difference between the entire state being null and the state missing one of the `ipv4Available` or `ipv6Available` keys (which were added in 4.0 I believe). Upon upgrading, those two would need to default to `true` if missing (since they're new); otherwise, TR would mistakenly treat every cache as unavailable.
----------------------------------------------------------------
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
[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #5133: TR: set cache to unavailable if not found in TM health data
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #5133:
URL: https://github.com/apache/trafficcontrol/pull/5133#discussion_r503483887
##########
File path: traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/edge/Node.java
##########
@@ -200,9 +200,18 @@ public InetAddress getIp6() {
@SuppressWarnings({"PMD.CyclomaticComplexity", "PMD.NPathComplexity"})
public void setState(final JsonNode state) {
- final boolean isAvailable = JsonUtils.optBoolean(state, "isAvailable", true);
- final boolean ipv4Available = JsonUtils.optBoolean(state, "ipv4Available", true);
- final boolean ipv6Available = JsonUtils.optBoolean(state, "ipv6Available", true);
+ final boolean ipv4Available;
+ final boolean ipv6Available;
+ if (state == null) {
+ LOGGER.warn("got null health state for " + fqdn + ". Setting it to unavailable!");
+ isAvailable = false;
+ ipv4Available = false;
+ ipv6Available = false;
+ } else {
+ isAvailable = JsonUtils.optBoolean(state, "isAvailable", true);
+ ipv4Available = JsonUtils.optBoolean(state, "ipv4Available", true);
+ ipv6Available = JsonUtils.optBoolean(state, "ipv6Available", true);
+ }
Review comment:
With the `if` statement we still get truthy defaults in case the TM is <= 4.0.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
[GitHub] [trafficcontrol] rawlinp commented on a change in pull request #5133: TR: set cache to unavailable if not found in TM health data
Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #5133:
URL: https://github.com/apache/trafficcontrol/pull/5133#discussion_r503485675
##########
File path: traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/edge/Node.java
##########
@@ -200,9 +200,18 @@ public InetAddress getIp6() {
@SuppressWarnings({"PMD.CyclomaticComplexity", "PMD.NPathComplexity"})
public void setState(final JsonNode state) {
- final boolean isAvailable = JsonUtils.optBoolean(state, "isAvailable", true);
- final boolean ipv4Available = JsonUtils.optBoolean(state, "ipv4Available", true);
- final boolean ipv6Available = JsonUtils.optBoolean(state, "ipv6Available", true);
+ final boolean ipv4Available;
+ final boolean ipv6Available;
+ if (state == null) {
+ LOGGER.warn("got null health state for " + fqdn + ". Setting it to unavailable!");
+ isAvailable = false;
+ ipv4Available = false;
+ ipv6Available = false;
+ } else {
+ isAvailable = JsonUtils.optBoolean(state, "isAvailable", true);
+ ipv4Available = JsonUtils.optBoolean(state, "ipv4Available", true);
+ ipv6Available = JsonUtils.optBoolean(state, "ipv6Available", true);
+ }
Review comment:
Yep, that is correct.
----------------------------------------------------------------
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