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

t3c: strategies.yaml hash_key only for consistent_hash #7204

Merged
merged 4 commits into from
Dec 14, 2022

Conversation

traeak
Copy link
Contributor

@traeak traeak commented Nov 17, 2022

The t3c strategies.yaml file when generating a strategy always populates the hash_key yaml field although it only applies to consistent_hash policy.

Currently with this configuration ATS 9.2 will bump into the unexpected hash_key field for non consistent_hash policy and abort loading the strategies file entirely.

Note the ATS docs here where hash_key only applies to consistent_hash policy.
https://docs.trafficserver.apache.org/en/latest/admin-guide/files/strategies.yaml.en.html?highlight=strategies#strategies-definitions


Which Traffic Control components are affected by this PR?

  • Traffic Control Cache Config (t3c, formerly ORT)

What is the best way to verify this PR?

Create DS services, one with consistent_hash and another with round robin.
Run t3c to generate strategies.yaml
Ensure that the DS with consistent_hash policy also has a hash_key key/value
Ensure that the DS with round robin (rr_strict or rr_ip) does not have a hash_key entry.

As a bonus, drop the strategies.yaml file into an ats config directory, fire up ATS9.2 and verify that the strategies.yaml file loads without generating an error.

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

7.1.0

PR submission checklist

@traeak traeak changed the title t3c: strategies.yaml has_key only for consistent_hash t3c: strategies.yaml hash_key only for consistent_hash Nov 17, 2022
@ocket8888 ocket8888 added bug something isn't working as intended low impact affects only a small portion of a CDN, and cannot itself break one cache-config Cache config generation labels Nov 17, 2022
@codecov
Copy link

codecov bot commented Dec 7, 2022

Codecov Report

Merging #7204 (4624888) into master (bf8e18f) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##             master    #7204   +/-   ##
=========================================
  Coverage     28.37%   28.37%           
  Complexity       98       98           
=========================================
  Files           617      617           
  Lines         69197    69202    +5     
  Branches         90       90           
=========================================
+ Hits          19634    19639    +5     
  Misses        47754    47754           
  Partials       1809     1809           
Flag Coverage Δ
golib_unit 53.10% <100.00%> (+0.02%) ⬆️
grove_unit 4.60% <ø> (ø)
t3c_generate_unit 24.96% <ø> (ø)
traffic_monitor_unit 20.43% <ø> (ø)
traffic_ops_unit 19.81% <ø> (ø)
traffic_stats_unit 10.41% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
lib/go-atscfg/strategiesdotconfig.go 91.78% <100.00%> (+0.03%) ⬆️
lib/go-atscfg/ipallowdotyaml.go 71.61% <0.00%> (+0.25%) ⬆️
lib/go-atscfg/ipallowdotconfig.go 69.62% <0.00%> (+0.28%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@jpappa200 jpappa200 left a comment

Choose a reason for hiding this comment

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

This looks good to me, just need to remove the typo in CHANGELOG.md

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
@ocket8888 ocket8888 merged commit b297605 into apache:master Dec 14, 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.

3 participants