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

Fixes an issue where multiple interfaces reported by a cache are included in the vitals (bandwidth) #5730

Merged
merged 19 commits into from
Apr 15, 2021

Conversation

tcfdev
Copy link
Collaborator

@tcfdev tcfdev commented Apr 9, 2021

What does this PR (Pull Request) do?

Which Traffic Control components are affected by this PR?

  • Traffic Monitor

What is the best way to verify this PR?

Unit tests have been added to the Traffic Monitor project to ensure this logic will work.

go test apache/trafficcontrol/traffic_monitor/health must pass

Additionally, verify Traffic Monitor still continues to report Bandwidth as reported in:
#5558

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

The following criteria are ALL met by this PR

  • This PR includes tests OR I have explained why tests are unnecessary
  • This PR includes documentation OR I have explained why documentation is unnecessary
  • This PR includes an update to CHANGELOG.md OR such an update is not necessary
  • This PR includes any and all required license headers
  • This PR DOES NOT FIX A SERIOUS SECURITY VULNERABILITY (see the Apache Software Foundation's security guidelines for details)

Additional Information

Taylor Frey and others added 15 commits April 1, 2021 15:18
Per bug report apache#5695, any and all interfaces returned in polling
were used to calculate vitals, such as bytes in or out, bandwidth
and more. However, only designated interfaces should be used for
calculating such metrics and vitals. As such, this commit will
check for monitored interfaces and only use those for the calculations.
Ensure callers for GetVitals must include a valid config
and the config should contain interfaces
@mitchell852 mitchell852 added Traffic Monitor related to Traffic Monitor bug something isn't working as intended labels Apr 9, 2021
CHANGELOG.md Show resolved Hide resolved
traffic_monitor/health/cache.go Outdated Show resolved Hide resolved
traffic_monitor/health/cache.go Outdated Show resolved Hide resolved
traffic_monitor/health/cache.go Show resolved Hide resolved
traffic_monitor/health/cache.go Outdated Show resolved Hide resolved
traffic_monitor/health/cache_test.go Outdated Show resolved Hide resolved
traffic_monitor/health/cache_test.go Outdated Show resolved Hide resolved
traffic_monitor/health/cache.go Show resolved Hide resolved
@zrhoffman zrhoffman added this to the 5.1.2 milestone Apr 15, 2021
Remove extra blank line from CHANGELOG
Condense test structs for sake of clarity
Change log levels to reflect actual logging issues
Add logging clarity to missing or empty caches and interfaces
Copy link
Contributor

@srijeet0406 srijeet0406 left a comment

Choose a reason for hiding this comment

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

Code looks great! All api/ manual tests pass.
Manually tested by linking up my local TM to the staging TO and made sure that the bandwidth shows the value calculated by only those interfaces that are monitored. LGTM!

@ocket8888 ocket8888 merged commit 89669fd into apache:master Apr 15, 2021
ocket8888 pushed a commit that referenced this pull request Apr 26, 2021
…uded in the vitals (bandwidth) (#5730)

* Normalizing README.md for MacOS case insensitive discrepancies

* Removed whitespace at end of line

* Only use monitored interfaces for vitals

Per bug report #5695, any and all interfaces returned in polling
were used to calculate vitals, such as bytes in or out, bandwidth
and more. However, only designated interfaces should be used for
calculating such metrics and vitals. As such, this commit will
check for monitored interfaces and only use those for the calculations.

* Normalized README.md file

* Normalized README file

* Add bugfix info to CHANGELOG

* Check monitor config for nil

Ensure callers for GetVitals must include a valid config
and the config should contain interfaces

* Fix merge issue with CHANGELOG

* Address PR feedback.

Remove extra blank line from CHANGELOG
Condense test structs for sake of clarity
Change log levels to reflect actual logging issues
Add logging clarity to missing or empty caches and interfaces

Co-authored-by: Taylor Frey <[email protected]>
(cherry picked from commit 89669fd)
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 Monitor related to Traffic Monitor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect bandwidth calculations from dual homed servers
5 participants