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 parent.config fix for MSO non-topo mid cache group no parent cache groups #7471

Merged
merged 5 commits into from
May 3, 2023

Conversation

traeak
Copy link
Contributor

@traeak traeak commented Apr 28, 2023

Bug fix for #7137 (create topology on the fly for non topo delivery services)
Case:
delivery service with MSO but no topology.
mid cache group does not have primary and secondary parents assigned.
origins belong to any number of origin cache groups.
In this case the origin from only the fist encountered cache group was getting pulled into parent.config


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 delivery service, MSO non topo. Ensure origins belong to different origin cache groups. ensure the mid cache group has neither primary nor secondary cache groups assigned. T3C run parent.config should

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

PR submission checklist

@traeak traeak marked this pull request as ready for review May 1, 2023 13:33
@codecov
Copy link

codecov bot commented May 1, 2023

Codecov Report

Merging #7471 (1ac8abf) into master (c09cbeb) will decrease coverage by 1.83%.
The diff coverage is 80.00%.

@@             Coverage Diff              @@
##             master    #7471      +/-   ##
============================================
- Coverage     29.28%   27.45%   -1.83%     
  Complexity       98       98              
============================================
  Files           782      685      -97     
  Lines         81860    77786    -4074     
  Branches        784       90     -694     
============================================
- Hits          23970    21358    -2612     
+ Misses        55825    54416    -1409     
+ Partials       2065     2012      -53     
Flag Coverage Δ
golib_unit 48.58% <80.00%> (+0.17%) ⬆️
grove_unit 4.60% <ø> (ø)
t3c_unit 5.32% <ø> (ø)
traffic_monitor_unit 21.28% <ø> (ø)
traffic_ops_unit 22.82% <ø> (ø)
traffic_portal_v2 ?
traffic_stats_unit 10.14% <ø> (ø)
unit_tests 24.07% <80.00%> (-2.17%) ⬇️

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

Impacted Files Coverage Δ
lib/go-atscfg/parentdotconfig.go 66.23% <80.00%> (+1.01%) ⬆️

... and 97 files with indirect coverage changes

📣 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.

LGTM, just a couple small things/questions but should be approved.

lib/go-atscfg/parentdotconfig.go Outdated Show resolved Hide resolved
lib/go-atscfg/remapdotconfig_test.go Show resolved Hide resolved
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.

Looks good to me. 👍

@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 May 2, 2023
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.

👍

Copy link
Member

@zrhoffman zrhoffman left a comment

Choose a reason for hiding this comment

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

Looks good, tests pass! The CiaB failure is unrelated.

@zrhoffman zrhoffman merged commit 0d8b516 into apache:master May 3, 2023
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.

4 participants