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

if no parents exist t3c will use secondary parents if configured #6673

Merged
rob05c merged 7 commits into from
Apr 26, 2022
Merged

if no parents exist t3c will use secondary parents if configured #6673

rob05c merged 7 commits into from
Apr 26, 2022

Conversation

@jpappa200
Copy link
Contributor

@jpappa200 jpappa200 commented Mar 18, 2022 •
edited
Loading

Closes: #6780

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?

In a staging environment offline the primary parent in an MSO and run t3c. you will see the error messages in the warning summary and check parent.config.

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

PR submission checklist

@zrhoffman
Copy link
Member

zrhoffman commented Mar 18, 2022

This PR will address CDN-15204

Will you please file a GitHub issue? As far as the ATC community is concerned, there is no such thing as CDN-15204.

@zrhoffman zrhoffman added cache-config Cache config generation improvement The functionality exists but it could be improved in some way. labels Mar 18, 2022
@ocket8888
Copy link
Contributor

ocket8888 commented Mar 18, 2022

CiaB failure may be addressed by rebasing on master

@zrhoffman
Copy link
Member

zrhoffman commented Mar 18, 2022

CiaB failure may be addressed by rebasing on master

It won't be. The failure is coming from the fact that CDN in a Box expects an RPM named trafficserver-8.1.3-10.965df952e.el8.x86_64.rpm, but an RPM named trafficserver-8.1.4_rc0-0.965df952e.el8.x86_64.rpm is provided. So the fetch-github-branch-sha GHA needs to have its latest-tag-getting logic updated to exclude tags that have characters other than digits and . in them.

@ocket8888 ocket8888 mentioned this pull request Mar 18, 2022
Merged
4 tasks
@zrhoffman
Copy link
Member

zrhoffman commented Mar 18, 2022

CiaB failure may be addressed by rebasing on master

@ocket8888 fixed in #6678

@rob05c
Copy link
Member

rob05c commented Apr 14, 2022

This PR has tests This PR has documentation This PR has a CHANGELOG.md entry are all checked, but it doesn't look like any of those are in the PR. @jpappa200 mind adding them?

I don't think we need docs, this behavior is incidental and the only right thing to do, we don't need to document that level of detail. But that should probably be noted, rather than checking the box that it has docs when it doesn't.

But it should probably have a Changelog entry, so people know how and when it changed.

And it should be straightforward to add a unit test, shouldn't need an Integration Test. In https://github.com/apache/trafficcontrol/blob/3b8c0cdc/lib/go-atscfg/parentdotconfig_test.go just needs to make a test with no Primaries and a Secondary via CacheGroups, which should pass with the code change and fail without it.

Other than that, this change looks good. The previous behavior was generating broken config, this is the right thing to do.

@rob05c
Copy link
Member

rob05c commented Apr 14, 2022 •
edited
Loading

You can probably base the test on TestMakeParentDotConfigHTTPSOrigin in parentdotconfig_test.go, if it helps. The setup should be similar, just needs to set the eCG.SecondaryParentCachegroupID = mid0.CachegroupID and not ParentCachegroupID, I think

@ocket8888
Copy link
Contributor

ocket8888 commented Apr 21, 2022 •
edited
Loading

This is including a ton of changes already merged and not at all related to anything you actually did - I think you may want to rebase.

image

@jpappa200
Copy link
Contributor Author

jpappa200 commented Apr 21, 2022

Yea that's what got me into this mess. Something went wrong.

@ocket8888
Copy link
Contributor

ocket8888 commented Apr 21, 2022

TM tests are known to be flaky. I re-ran them

@jpappa200
Copy link
Contributor Author

jpappa200 commented Apr 25, 2022

This PR will address CDN-15204

Will you please file a GitHub issue? As far as the ATC community is concerned, there is no such thing as CDN-15204.

Created issue #6780

rob05c
rob05c approved these changes Apr 26, 2022
View reviewed changes
Copy link
Member

@rob05c rob05c left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@rob05c rob05c merged commit 29fed11 into apache:master Apr 26, 2022
@asf-ci asf-ci mentioned this pull request May 1, 2022
Merged
4 tasks
zrhoffman pushed a commit to zrhoffman/trafficcontrol that referenced this pull request Oct 2, 2022
@jpappa200
if no parents exist t3c will use secondary parents if configured (apa…
4624961 
…che#6673)

* If there are no parents use secondary parents if configured.

* If there are no parents use secondary parents if configured.

* added 2 tests to use secondary parents when no primary parents are available.

* Substituted admin_down with constant tc.CacheStatusAdminDown

* Added changelog entry
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@rob05c rob05c rob05c approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cache-config Cache config generation improvement The functionality exists but it could be improved in some way.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

t3c should use secondary parent if there is no ONLINE primary parent

4 participants

@jpappa200 @zrhoffman @ocket8888 @rob05c