Skip to content
This repository was archived by the owner on Nov 24, 2025. It is now read-only.

Fix TO atscfg missing max origin conns#4344

Merged
mitchell852 merged 1 commit into
apache:masterfrom
rob05c:to-ats-config-fix-misordered-scan
Jan 29, 2020
Merged

Fix TO atscfg missing max origin conns#4344
mitchell852 merged 1 commit into
apache:masterfrom
rob05c:to-ats-config-fix-misordered-scan

Conversation

@rob05c

@rob05c rob05c commented Jan 27, 2020

Copy link
Copy Markdown
Member

Fixes a misordered SQL scan, which caused the Server Status to be
missing, causing the config gen to think there were no online servers,
and thus not create the max_origin_connections entry.

Note this is only in the old TO endpoints, not the config generator. The atstccfg generator doesn't have this issue.

  • This PR is not related to any other Issue

Includes TO API (Integration) tests.
No docs, no interface change.
No changelog, no interface change.

Which Traffic Control components are affected by this PR?

  • Traffic Ops

What is the best way to verify this PR?

Run TO API Tests.
Set Max Origin Connections on a DS, request the header rewrite endpoint for that DS, verify it includes a max_origin_connections entry.

If this is a bug fix, what versions of Traffic Control are affected?

  • master
  • 4.0.0 RC2

The following criteria are ALL met by this PR

  • This PR includes tests OR I have explained why tests are unnecessary
  • This PR includes documentation OR I have explained why documentation is unnecessary
  • This PR includes an update to CHANGELOG.md OR such an update is not necessary
  • This PR includes any and all required license headers
  • This PR ensures that database migration sequence is correct OR this PR does not include a database migration
  • This PR DOES NOT FIX A SERIOUS SECURITY VULNERABILITY (see the Apache Software Foundation's security guidelines for details)

Additional Information

@rob05c rob05c added Traffic Ops related to Traffic Ops cache-config Cache config generation labels Jan 27, 2020
@rob05c
rob05c requested a review from mitchell852 January 27, 2020 23:05
@mitchell852 mitchell852 added the bug something isn't working as intended label Jan 28, 2020
@rob05c
rob05c force-pushed the to-ats-config-fix-misordered-scan branch from 61a6a45 to d4532b1 Compare January 28, 2020 15:26

@mitchell852 mitchell852 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

When i set max origin conns > 0 for delivery services that do NOT employ the mid-tier (i.e. DNS_LIVE) this looks good. When I call GET api/1.4/cdns/cdn-name/configfiles/ats/hdr_rw_xml-id.config, I see the following:

# DO NOT EDIT - Generated for cdn-name by Traffic Ops (https://trafficops.domain.net/) on Tue Jan 28 15:40:07 UTC 2020
cond %{REMAP_PSEUDO_HOOK}
set-config proxy.config.http.origin_max_connections 1000 [L]

But when i set max origin conns > 0 for delivery services that DO employ the mid-tier (i.e. DNS), I am not seeing an entry in any of the hdr_rw_mid_xml-id.config files.

@rawlinp rawlinp added automation related to automated testing/deployment/packaging etc. and removed automation related to automated testing/deployment/packaging etc. labels Jan 28, 2020
Fixes a misordered SQL scan, which caused the Server Status to be
missing, causing the config gen to think there were no online servers,
and thus not create the max_origin_connections entry.
@rob05c
rob05c force-pushed the to-ats-config-fix-misordered-scan branch from d4532b1 to 9f24b66 Compare January 29, 2020 15:41
@rob05c

rob05c commented Jan 29, 2020

Copy link
Copy Markdown
Member Author

@mitchell852 Ah, the Mid query had the same bug. I fixed that too, both bugs should be fixed now.

@mitchell852 mitchell852 self-assigned this Jan 29, 2020

@mitchell852 mitchell852 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I tested delivery services that employ the mid-tier and ones that do not and set max origin conns to >0 and 0 and saw the appropriate entry in hdr_rw_xml-id.config or hdr_rw_mid_xml-id.config using the following apis:

GET /api/1.4/cdns/cdn-name/configfiles/ats/hdr_rw_mid_xml-id.config
GET /api/1.4/cdns/cdn-name/configfiles/ats/hdr_rw_xml-id.config

I also checked that the location params were created and assigned to the appropriate profiles.

All TO API tests passed as well.

@mitchell852
mitchell852 merged commit a14d3d4 into apache:master Jan 29, 2020
@mitchell852

Copy link
Copy Markdown
Member

@rob05c - reminder to backport this to 4.0.x branch

@mhoppa mhoppa left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

FWIW I am getting some API test failures since this merged

hdr_rw_dot_config_test.go:110: expected 6 lines in the config (actual = 8)
hdr_rw_dot_config_test.go:90: expected 6 lines in the config (actual = 8)

@rob05c would you mind taking a look? or just run them and reconfirm that yours passed and mine should too :)

rawlinp pushed a commit to rawlinp/trafficcontrol that referenced this pull request Jan 30, 2020
Fixes a misordered SQL scan, which caused the Server Status to be
missing, causing the config gen to think there were no online servers,
and thus not create the max_origin_connections entry.

(cherry picked from commit a14d3d4)
rawlinp added a commit that referenced this pull request Jan 30, 2020
Fixes a misordered SQL scan, which caused the Server Status to be
missing, causing the config gen to think there were no online servers,
and thus not create the max_origin_connections entry.

(cherry picked from commit a14d3d4)

Co-authored-by: Robert Butts <rob05c@users.noreply.github.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

bug something isn't working as intended cache-config Cache config generation Traffic Ops related to Traffic Ops

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants