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

Reduce TM dependency on CRConfig#6625

Merged
ocket8888 merged 4 commits intoapache:masterfrom
rawlinp:tm-reduce-crconfig-dep
Mar 28, 2022
Merged

Reduce TM dependency on CRConfig#6625
ocket8888 merged 4 commits intoapache:masterfrom
rawlinp:tm-reduce-crconfig-dep

Conversation

@rawlinp
Copy link
Copy Markdown
Contributor

@rawlinp rawlinp commented Mar 9, 2022

Do not use CRConfig data when the same data already exists in the
TMConfig. For data that TM needs but doesn't exist in TMConfig, add it
to TMConfig. This way, TM only uses data in the TMConfig and can become
simply a shuttle for the CRConfig without actually using any CRConfig
data.

Closes: #6512
Related: #1739
Related: #5367


Which Traffic Control components are affected by this PR?

  • Documentation
  • Traffic Monitor
  • Traffic Ops

What is the best way to verify this PR?

Ensure the GHA tests pass. Take a snapshot of a CDN before this change, then take a snapshot after this change and compare the TMConfig snapshot to see the newly added fields. Compare the newly added fields to the corresponding data in the CRConfig snapshot (they should be the same). Run TM with the new snapshot and ensure it still polls and functions normally.

PR submission checklist

@rawlinp rawlinp added Traffic Ops related to Traffic Ops Traffic Monitor related to Traffic Monitor WIP "Work-in-Progress" - do not merge! (use 'draft' pull requests from now on) improvement The functionality exists but it could be improved in some way. labels Mar 9, 2022
@rawlinp rawlinp force-pushed the tm-reduce-crconfig-dep branch from c6a95a6 to 37599f1 Compare March 9, 2022 18:45
@rawlinp rawlinp force-pushed the tm-reduce-crconfig-dep branch from 37599f1 to 62cb43c Compare March 9, 2022 19:11
@rawlinp rawlinp removed the WIP "Work-in-Progress" - do not merge! (use 'draft' pull requests from now on) label Mar 9, 2022
@rawlinp rawlinp force-pushed the tm-reduce-crconfig-dep branch from 62cb43c to 0afd6fd Compare March 10, 2022 18:25
@rawlinp
Copy link
Copy Markdown
Contributor Author

rawlinp commented Mar 10, 2022

Rebased to resolve changelog merge conflicts.

@rawlinp rawlinp added the WIP "Work-in-Progress" - do not merge! (use 'draft' pull requests from now on) label Mar 14, 2022
@rawlinp rawlinp force-pushed the tm-reduce-crconfig-dep branch from 0afd6fd to 2fb2f49 Compare March 15, 2022 15:33
@rawlinp rawlinp removed the WIP "Work-in-Progress" - do not merge! (use 'draft' pull requests from now on) label Mar 15, 2022
@ocket8888 ocket8888 self-assigned this Mar 15, 2022
@ocket8888 ocket8888 added the medium impact impacts a significant portion of a CDN, or has the potential to do so label Mar 15, 2022
Copy link
Copy Markdown
Contributor

@ocket8888 ocket8888 left a comment

Choose a reason for hiding this comment

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

I only made docs comments for the first version I encountered to avoid being overly repetitive. On that note: shouldn't these changes be made only to APIv4? I know the Snapshots have historically been unversioned, and "monitoring dot JSON" has always been a quasi-part of that snapshot, but it's used in TO Go client call signatures, so won't changing that be a breaking compile-time change?

Comment thread docs/source/api/v2/cdns_name_configs_monitoring.rst Outdated
Comment thread docs/source/api/v2/cdns_name_configs_monitoring.rst Outdated
Comment thread docs/source/api/v2/cdns_name_configs_monitoring.rst Outdated
Comment thread docs/source/api/v2/cdns_name_configs_monitoring.rst Outdated
Comment thread docs/source/api/v2/cdns_name_configs_monitoring.rst Outdated
Comment thread traffic_monitor/todata/todata.go Outdated
Comment thread traffic_monitor/todata/todata.go Outdated
Comment thread traffic_ops/traffic_ops_golang/crconfig/servers.go Outdated
Comment thread traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go Outdated
Comment thread traffic_ops/traffic_ops_golang/monitoring/monitoring.go Outdated
Copy link
Copy Markdown
Contributor Author

@rawlinp rawlinp left a comment

Choose a reason for hiding this comment

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

shouldn't these changes be made only to APIv4?

I don't see why it can't be made to all versions, since this is non-breaking to the expected JSON formatting because it only adds new fields and doesn't change data types or rename existing fields.

As far as the TO client goes, this does technically make a breaking change to the TrafficServer and LegacyTrafficServer structs by changing the type of DeliveryServices from []tsdeliveryService to []TSDeliveryService, but the odds that any external client is actually using that field, when it wasn't even populated in the monitoring.json snapshot in the first place, seem drastically low. Therefore, I felt that is an OK breaking change.

Comment thread docs/source/api/v2/cdns_name_configs_monitoring.rst Outdated
Comment thread docs/source/api/v2/cdns_name_configs_monitoring.rst Outdated
Comment thread docs/source/api/v2/cdns_name_configs_monitoring.rst Outdated
Comment thread docs/source/api/v2/cdns_name_configs_monitoring.rst Outdated
Comment thread docs/source/api/v2/cdns_name_configs_monitoring.rst Outdated
Comment thread traffic_monitor/todata/todata.go Outdated
Comment thread traffic_monitor/todata/todata.go Outdated
Comment thread traffic_ops/traffic_ops_golang/crconfig/servers.go Outdated
Comment thread traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go Outdated
Comment thread traffic_ops/traffic_ops_golang/monitoring/monitoring.go Outdated
@ocket8888
Copy link
Copy Markdown
Contributor

shouldn't these changes be made only to APIv4?

I don't see why it can't be made to all versions, since this is non-breaking to the expected JSON formatting because it only adds new fields and doesn't change data types or rename existing fields.

As far as the TO client goes, this does technically make a breaking change to the TrafficServer and LegacyTrafficServer structs by changing the type of DeliveryServices from []tsdeliveryService to []TSDeliveryService, but the odds that any external client is actually using that field, when it wasn't even populated in the monitoring.json snapshot in the first place, seem drastically low. Therefore, I felt that is an OK breaking change.

That's fine with me, I suppose. It's not like we haven't broken that call signature in the recent past anyway - which I think was actually my doing. The docs pages for monitoring and Snapshots should really have a disclaimer about their instability. Right now monitoring has nothing and I think the Snapshots page has a "Caution" thing I put in saying how basically the guy writing the docs couldn't figure out a definite "shape" for that endpoint's data, so it needed updates from people who did. Should just be changed to "this, historically, just can't be relied upon to respect its versioning in the API". Not that I think you need to do that in this PR - although you're welcome to if you want.

And, of course, I'll then need you to tank aggro when @rob05c finds out about it.

@rawlinp
Copy link
Copy Markdown
Contributor Author

rawlinp commented Mar 15, 2022

I'll tank if you heal 😄

IMO all snapshots are unversioned because they should be handled as raw JSON data to which we can freely add new fields without making breaking changes. Plus, the fact that they're read-only means they're immune to round-trip data loss which could occur if a client were to drop unknown fields from a GET then make a PUT/POST without those fields.

@rawlinp rawlinp force-pushed the tm-reduce-crconfig-dep branch from 8a261df to 58c64d3 Compare March 17, 2022 23:20
@ocket8888
Copy link
Copy Markdown
Contributor

ocket8888 commented Mar 18, 2022

I think you need to rebase this on master to get the CiaB CI to pass nvm - that won't help yet

@rawlinp rawlinp force-pushed the tm-reduce-crconfig-dep branch from 58c64d3 to 3b44bd1 Compare March 18, 2022 21:28
@zrhoffman
Copy link
Copy Markdown
Member

I think you need to rebase this on master to get the CiaB CI to pass nvm - that won't help yet

fixed in #6678

@rawlinp rawlinp force-pushed the tm-reduce-crconfig-dep branch 2 times, most recently from 506f160 to ce8c251 Compare March 22, 2022 15:13
@rawlinp
Copy link
Copy Markdown
Contributor Author

rawlinp commented Mar 22, 2022

ERROR: for to_server  (<Service: to_server>, 'Build failed')
Service 'to_server' failed to build : Build failed
Error: Process completed with exit code 1.

However, this builds fine for me locally using docker... I can't really tell why it's failing here.

rawlinp added 3 commits March 24, 2022 09:22
Do not use CRConfig data when the same data already exists in the
TMConfig. For data that TM needs but doesn't exist in TMConfig, add it
to TMConfig. This way, TM only uses data in the TMConfig and can become
simply a shuttle for the CRConfig without actually using any CRConfig
data.

Closes: apache#6512
Related: apache#1739
Related: apache#5367
@rawlinp rawlinp force-pushed the tm-reduce-crconfig-dep branch from ce8c251 to 96625bc Compare March 24, 2022 15:22
@ocket8888 ocket8888 merged commit 0533b7a into apache:master Mar 28, 2022
@rawlinp rawlinp deleted the tm-reduce-crconfig-dep branch March 28, 2022 22:34
@zrhoffman zrhoffman mentioned this pull request Apr 7, 2022
4 tasks
rawlinp added a commit to rawlinp/trafficcontrol that referenced this pull request Apr 13, 2022
PR apache#6625 reduced the TM dependency on CRConfig data, but it accidentally
caused TM to no longer poll for new CRConfig snapshots. This adds the
CRConfig request back into the polling loop so that it updates its
in-memory copy for internal use (if necessary) and for serving to TR.
ocket8888 pushed a commit that referenced this pull request Apr 13, 2022
* Fix TM issue where CRConfig is never updated

PR #6625 reduced the TM dependency on CRConfig data, but it accidentally
caused TM to no longer poll for new CRConfig snapshots. This adds the
CRConfig request back into the polling loop so that it updates its
in-memory copy for internal use (if necessary) and for serving to TR.

* Add --buildvcs=false to 'go test' action
zrhoffman pushed a commit to zrhoffman/trafficcontrol that referenced this pull request Oct 2, 2022
* Fix TM issue where CRConfig is never updated

PR apache#6625 reduced the TM dependency on CRConfig data, but it accidentally
caused TM to no longer poll for new CRConfig snapshots. This adds the
CRConfig request back into the polling loop so that it updates its
in-memory copy for internal use (if necessary) and for serving to TR.

* Add --buildvcs=false to 'go test' action
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

improvement The functionality exists but it could be improved in some way. medium impact impacts a significant portion of a CDN, or has the potential to do so Traffic Monitor related to Traffic Monitor Traffic Ops related to Traffic Ops

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Reduce Traffic Monitor dependency on CRConfig data

3 participants