Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix t3c non-topo edge-origin DS params #7043

Merged

Conversation

rob05c
Copy link
Member

@rob05c rob05c commented Aug 29, 2022

Fixes cache config generation for non-topology DSes that are edge->origin (via the edge's cachegroup's parent being the origin cachegroup) for DS Parameters.

Due to a bug in the non-topology logic to determine if a cache is the last cache tier, that scenario was resulting in the config gen thinking the edge wasn't the last tier, and not inserting MSO retry parameters on the parent line.

Fixing that bug exposed another bug, in the unavailableServerRetryResponsesValid regex. I changed that function to simply call ParseRetryResponses and return whether there was an error. Actually parsing is probably faster than the regex anyway, and this is the safest way to prevent bugs and discrepancies in the parse vs can-parse functions.

This also fixes several tests around both of those bugs, which were testing the wrong things. It fixes both the retry parameter tests, which were accidentally testing that the last tier had the first tier params, and a remap.config test that was testing edge->origin http/https rewriting as if it were edge->mid.

Which Traffic Control components are affected by this PR?

  • Documentation
  • Traffic Control Cache Config (t3c, formerly ORT)
  • Traffic Control Health Client (tc-health-client)
  • Traffic Control Client
  • Traffic Monitor
  • Traffic Ops
  • Traffic Portal
  • Traffic Router
  • Traffic Stats
  • Grove
  • CDN in a Box
  • Automation
  • unknown

What is the best way to verify this PR?

Run tests. Generate config for a non-topology DS which is edge->origin and has DS retry parameters, verify retry parameters are inserted into parent.config correctly.

If this is a bugfix, which Traffic Control versions contained the bug?

  • 6.0.0 and newer

PR submission checklist

@rob05c rob05c added bug something isn't working as intended cache-config Cache config generation labels Aug 29, 2022
@rob05c rob05c force-pushed the fix-t3c-non-topo-edge-origin-ds-params branch from 10d42de to bac9742 Compare August 29, 2022 19:06
@ocket8888 ocket8888 added the low impact affects only a small portion of a CDN, and cannot itself break one label Aug 29, 2022
Copy link
Contributor

@traeak traeak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixes look okay, make sense and testing on prod data shows the fix works.

@rob05c rob05c force-pushed the fix-t3c-non-topo-edge-origin-ds-params branch from bac9742 to e736bb0 Compare September 19, 2022 19:00
@rob05c rob05c force-pushed the fix-t3c-non-topo-edge-origin-ds-params branch from e736bb0 to f3c7c82 Compare September 19, 2022 19:27
@zrhoffman zrhoffman merged commit 18f521b into apache:master Sep 19, 2022
zrhoffman pushed a commit to zrhoffman/trafficcontrol that referenced this pull request Oct 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something isn't working as intended cache-config Cache config generation low impact affects only a small portion of a CDN, and cannot itself break one
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants