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

/cdns/capacity returns 500 error: capacity was zero #4892

Merged
merged 9 commits into from
Jul 28, 2020

Conversation

srijeet0406
Copy link
Contributor

@srijeet0406 srijeet0406 commented Jul 17, 2020

What does this PR (Pull Request) do?

Which Traffic Control components are affected by this PR?

  • CDN in a Box
  • Traffic Ops

What is the best way to verify this PR?

Two types of tests:
1.) Test in a system where you dont have any monitors for your cdns. In such a system, if you make a call to GET /cdns/capacity, it should return a 500, and the error logs should show the reason of the failure as no monitors available
2.) Test it in a system that has monitors, etc. (a full setup, like CiaB). Do a snapshot of one of the cdns, and then try to GET the /cdns/capacity. You should get a valid response this time.

If this is a bug fix, what versions of Traffic Control are affected?

  • master

The following criteria are ALL met by this PR

  • This PR includes tests
  • This PR includes documentation
  • This PR includes an update to CHANGELOG.md
  • This PR includes any and all required license headers
  • This PR does not include a database migration
  • This PR DOES NOT FIX A SERIOUS SECURITY VULNERABILITY (see the Apache Software Foundation's security guidelines for details)

Additional Information

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.

I have explained why tests are unnecessary

Non-trivial changes to Traffic Ops need to have tests. Can the checks in the What is the best way to verify this PR? section be added as API or unit tests?


As a general comment, the commit messages are wip, Cleanup, Cleanup, and Adding changelog entry. That last commit message is fine. For the others: Since the commit messages of merged PRs stay in the commit history forever, they should be useful to whoever is reading them. Would you please rebase to reword the commit messages to make them a little more descriptive?

traffic_ops/traffic_ops_golang/cdn/capacity.go Outdated Show resolved Hide resolved
traffic_ops/traffic_ops_golang/cdn/capacity.go Outdated Show resolved Hide resolved
traffic_ops/traffic_ops_golang/cdn/capacity.go Outdated Show resolved Hide resolved
traffic_ops/traffic_ops_golang/cdn/capacity.go Outdated Show resolved Hide resolved
traffic_ops/traffic_ops_golang/cdn/capacity.go Outdated Show resolved Hide resolved
traffic_ops/traffic_ops_golang/cdn/capacity.go Outdated Show resolved Hide resolved
traffic_ops/traffic_ops_golang/cdn/capacity.go Outdated Show resolved Hide resolved
lib/go-tc/enum.go Outdated Show resolved Hide resolved
@mitchell852 mitchell852 added Traffic Ops related to Traffic Ops bug something isn't working as intended labels Jul 22, 2020
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
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.

That was my only comment, so now this is ready whenever the actual reviewer (@zrhoffman) says it is.

traffic_ops/traffic_ops_golang/cdn/capacity.go Outdated Show resolved Hide resolved
traffic_ops/traffic_ops_golang/cdn/capacity.go Outdated Show resolved Hide resolved
traffic_ops/traffic_ops_golang/cdn/capacity_test.go Outdated Show resolved Hide resolved
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.

This all works. From my testing, 10,000 GET requests to /cdns/capacity runs about 7 seconds slower than it did at 0480f8fcd8~ (1 commit before #4700 was merged).

Is there any advantage to querying a list of interfaces and checking only those, rather than just skipping over all array keys named aggregate (that is the only array key that is not an interface name right?)? There is probably only a 1-2 second timesave over 10,000 GET requests to /cdns/capacity from nixing that query, so not a huge difference either way.

traffic_ops/traffic_ops_golang/cdn/capacity.go Outdated Show resolved Hide resolved
traffic_ops/traffic_ops_golang/cdn/capacity.go Outdated Show resolved Hide resolved
traffic_ops/traffic_ops_golang/cdn/capacity.go Outdated Show resolved Hide resolved
@srijeet0406
Copy link
Contributor Author

@zrhoffman About the query performance, we have to traverse over a list of interfaces to get the service interface, because the stats associated with the service interface are the only stats that we are interested in. We dont really want any of the other ones. So, even if we get everything except the aggregate key, we would still have to run through that list and get the stats associated with only the service interface.

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.

/cdns/capacity works, API tests pass, unit tests pass, code is formatted correctly. LGTM!

@ocket8888 ocket8888 merged commit f5048b3 into apache:master Jul 28, 2020
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 Traffic Ops related to Traffic Ops
Projects
None yet
Development

Successfully merging this pull request may close these issues.

/cdns/capacity returns 500 error: capacity was zero
4 participants