Skip to content

Add TO Go cdns/capacity #2306

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Aug 22, 2019
Merged

Add TO Go cdns/capacity #2306

merged 1 commit into from
Aug 22, 2019

Conversation

rob05c
Copy link
Member

@rob05c rob05c commented May 20, 2018 •
edited by ocket8888
Loading

What does this PR do?

Add TO Go cdns/capacity. Closes #3780

Which TC components are affected by this PR?

  • Documentation
  • Grove
  • Traffic Analytics
  • Traffic Monitor
  • Traffic Ops
  • Traffic Ops ORT
  • Traffic Portal
  • Traffic Router
  • Traffic Stats
  • Traffic Vault
  • Other _________

What is the best way to verify this PR?

GET /api/1.2/cdns/capacity

Check all that apply

  • This PR includes tests
  • This PR includes documentation updates
  • This PR includes an update to CHANGELOG.md
  • This PR includes all required license headers
  • This PR includes a database migration (ensure that migration sequence is correct)
  • This PR fixes a serious security flaw. Read more: www.apache.org/security

@rob05c rob05c added new feature A new feature, capability or behavior Traffic Ops API (golang) labels May 20, 2018
@rob05c rob05c mentioned this pull request May 20, 2018
Closed
@asfgit
Copy link
Contributor

asfgit commented May 20, 2018

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/incubator-trafficcontrol-PR/1625/
Test PASSed.

@rob05c rob05c force-pushed the to-go-cdncapacity branch from a764361 to ceabaf2 Compare May 22, 2018 15:01
@asfgit
Copy link
Contributor

asfgit commented May 22, 2018

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/incubator-trafficcontrol-PR/1645/
Test PASSed.

@rob05c rob05c force-pushed the to-go-cdncapacity branch from ceabaf2 to 90ea458 Compare May 23, 2018 14:24
@asfgit
Copy link
Contributor

asfgit commented May 23, 2018

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/incubator-trafficcontrol-PR/1654/
Test PASSed.

@rob05c rob05c force-pushed the to-go-cdncapacity branch from 90ea458 to 977e81f Compare June 5, 2018 14:51
@asfgit
Copy link
Contributor

asfgit commented Jun 5, 2018

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/incubator-trafficcontrol-PR/1763/
Test PASSed.

@rob05c rob05c force-pushed the to-go-cdncapacity branch from 977e81f to cec28cc Compare June 5, 2018 20:28
@asfgit
Copy link
Contributor

asfgit commented Jun 5, 2018

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/incubator-trafficcontrol-PR/1776/
Test FAILed.

@rob05c rob05c force-pushed the to-go-cdncapacity branch from cec28cc to 9336d78 Compare July 2, 2018 20:41
@asfgit
Copy link
Contributor

asfgit commented Jul 2, 2018

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/1973/
Test FAILed.

@rob05c rob05c force-pushed the to-go-cdncapacity branch from 9336d78 to bb60e95 Compare July 9, 2018 16:53
@asfgit
Copy link
Contributor

asfgit commented Jul 9, 2018

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/2040/
Test PASSed.

@rob05c
Copy link
Member Author

rob05c commented Sep 13, 2018 •
edited
Loading

Rebased, manually tested, should be good to merge.

@asfgit
Copy link
Contributor

asfgit commented Sep 13, 2018

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/2436/
Test PASSed.

@apache apache deleted a comment from rob05c Sep 14, 2018
@mitchell852
Copy link
Member

Is there any sort of API test that can be added? Maybe one to check the response structure or something?

@rob05c
Copy link
Member Author

rob05c commented Sep 14, 2018

@mitchell852 Not until the "API test" framework supports Monitors.

@ocket8888
Copy link
Contributor

This endpoint has already been rewritten to go, so I think this PR should be closed.

@rob05c
Copy link
Member Author

rob05c commented Jun 24, 2019 •
edited
Loading

This hasn't been rewritten in Go, except for this PR. It's still proxying to Perl https://github.com/apache/trafficcontrol/blob/master/traffic_ops/traffic_ops_golang/routing/routes.go#L123

@ocket8888
Copy link
Contributor

Oh, lol, my bad. I forget that some of those routes exist for explicit routing to the proxy. If you wanna rebase for conflicts, I'd be happy to review this.

@ocket8888 ocket8888 mentioned this pull request Aug 6, 2019
Closed
1 task
@rob05c rob05c force-pushed the to-go-cdncapacity branch from 62e129c to 85d6722 Compare August 14, 2019 17:11
@asfgit
Copy link
Contributor

asfgit commented Aug 14, 2019

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/4142/
Test PASSed.

@ocket8888 ocket8888 self-assigned this Aug 14, 2019
mitchell852
mitchell852 previously requested changes Aug 16, 2019
View reviewed changes
Copy link
Member

@mitchell852 mitchell852 left a comment

Choose a reason for hiding this comment

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

found a bug?

@mitchell852 mitchell852 dismissed their stale review August 16, 2019 21:25

inaccurate

mitchell852
mitchell852 reviewed Aug 16, 2019
View reviewed changes
Copy link
Member

@mitchell852 mitchell852 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

a few comments. also, not sure if this is a big deal or not but if all the TMs are OFFLINE, this endpoint will throw an internal server error. Seems like it should probably just return all zeros instead.

scratch that: if at least one TM in each CDN is not ONLINE and reachable, you get an ISE.

@mitchell852 mitchell852 self-assigned this Aug 21, 2019
@mitchell852 mitchell852 added Traffic Ops related to Traffic Ops tech debt rework due to choosing easy/limited solution and removed new feature A new feature, capability or behavior labels Aug 21, 2019
@mitchell852 mitchell852 added this to the Go Rewrite milestone Aug 21, 2019
@rob05c rob05c force-pushed the to-go-cdncapacity branch from 85d6722 to 17a9c59 Compare August 22, 2019 17:23
@rob05c rob05c force-pushed the to-go-cdncapacity branch from 17a9c59 to 8a8c160 Compare August 22, 2019 18:12
@rob05c rob05c requested a review from mitchell852 August 22, 2019 18:15
mitchell852
mitchell852 approved these changes Aug 22, 2019
View reviewed changes
Copy link
Member

@mitchell852 mitchell852 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

tested GET /api/1.1/cdns/capacity:

  • with at least 1 TM set to ONLINE (capacity returned)
  • with no TM's set to ONLINE (internal server error returned)

PR comments were also addressed.

@mitchell852 mitchell852 merged commit 6a3009a into apache:master Aug 22, 2019
@asfgit
Copy link
Contributor

asfgit commented Aug 22, 2019

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/4178/
Test FAILed.

@asfgit
Copy link
Contributor

asfgit commented Aug 23, 2019

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/4179/
Test FAILed.

rob05c added a commit to rob05c/trafficcontrol that referenced this pull request Sep 9, 2019
@rob05c
Add TO Go cdns/capacity (apache#2306)
e471971
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mitchell852 mitchell852 mitchell852 approved these changes

Assignees

@mitchell852 mitchell852

@ocket8888 ocket8888

Labels
tech debt rework due to choosing easy/limited solution Traffic Ops related to Traffic Ops
Projects
None yet
Milestone
Go Rewrite
Development

Successfully merging this pull request may close these issues.

Rewrite /cdns/capacity to Go
4 participants
@rob05c @asfgit @mitchell852 @ocket8888