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

Implement the "MaxAdvertisedBandwidth" hack ... #191

Merged
merged 17 commits into from Jun 23, 2018

Conversation

Projects
None yet
3 participants
@pastly
Copy link
Member

commented Jun 14, 2018

Use a relay's {,Relay}BandwidthRate/MaxAdvertisedBandwidth as an
upper bound on the measurements we make for it.

Implement the "MaxAdvertisedBandwidth" hack ...
Use a relay's {,Relay}BandwidthRate/MaxAdvertisedBandwidth as an
upper bound on the measurements we make for it.

@pastly pastly requested a review from juga0 Jun 14, 2018

@juga0

This comment has been minimized.

Copy link
Contributor

commented Jun 15, 2018

I didn't work on this yet because i wanted to look at tor code and check what teor commented in #8494, but i haven't done it yet.
This PR looks good to me, but we don't have any way to test it.
Would be easy to create a test for this?.

@pastly

This comment has been minimized.

Copy link
Member Author

commented Jun 15, 2018

I could make another testnet like simple-ipv4 and simple-ipv6 made up of relays with a variety of MaxAdvertisedBandwidth settings. The test would not be run automatically.

@teor2345

This comment has been minimized.

Copy link
Member

commented Jun 15, 2018

I didn't work on this yet because i wanted to look at tor code and check what teor commented in #8494, but i haven't done it yet.

I didn't see any questions for me in #8494, so I left a comment saying what still needs to be done to close that ticket:
https://trac.torproject.org/projects/tor/ticket/8494#comment:10

@juga0

This comment has been minimized.

Copy link
Contributor

commented Jun 17, 2018

@juga0

This comment has been minimized.

Copy link
Contributor

commented Jun 17, 2018

@juga0

This comment has been minimized.

Copy link
Contributor

commented Jun 17, 2018

While the changes here would work, now i'm thinking that it would be better to keep track on which was the real measured bandwidth, it might be useful at some point to debug issues.
It also feels less hacking that doing amount = average_bandwidth * duration to obtain average_bandwidth when we divide amount by duration.
What i would propose it that we add average_bandwidth to ResultSuccess, and we limit the measured bandwidth only in the moment where it's actually calculated to be written in the file: https://github.com/pastly/simple-bw-scanner/blob/master/sbws/lib/v3bwfile.py#L308
What do you think about this?, let me know if you prefer i would implement this.

@juga0

This comment has been minimized.

Copy link
Contributor

commented Jun 17, 2018

s/hacking/hacky/

@pastly

This comment has been minimized.

Copy link
Member Author

commented Jun 17, 2018

ow i'm thinking that it would be better to keep track on which was the real measured bandwidth, it might be useful at some point to debug issues.

I can make the requested changes.

i have not followed how much differ the previous testnets and the current integration net.tar.

IIRC net.tar is essentially simple-ipv4.

I think I've realized a way we can test this feature without having to literally run sbws scanner and will work on implementing my idea. (Essentially call measure_relay with a target relay ... don't know why that wasn't obvious to me)

@pastly

This comment has been minimized.

Copy link
Member Author

commented Jun 20, 2018

All that I think is left is either fixing or removing the broken unit tests.

@pastly

This comment has been minimized.

Copy link
Member Author

commented Jun 21, 2018

I'm ready for review again.

@juga0

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2018

I can check this on saturday, if it's blocking something else, just merge.
In a really quick look, i see it's about tests, not sure if there's also what commented in #191 (comment)

@pastly

This comment has been minimized.

Copy link
Member Author

commented Jun 21, 2018

The new commits are essentially

Tests

  • add relay with MaxAdvertisedBandwidth to net.tar
  • add relay with RelayBandwidthRate to net.tar
  • add integration tests that measure the aforementioned relays
  • remove tests for sbws generate

Be less hacky

  • change ResultSuccess to store the relay's average_bandwidth as it was at measurement time (this required a bump in result version)
  • remove the hack you didn't like (because you're right, it was hacky)
  • limit bandwidths at sbws generate time, not at measurement time
@juga0

This comment has been minimized.

Copy link
Contributor

commented Jun 23, 2018

change ResultSuccess to store the relay's average_bandwidth as it was at measurement time (this required a bump in result version)

perfect, i see the commit c7eda42

remove tests for sbws generate

i guess this is because they were breaking and we wanted to change them anyway?

add relay with MaxAdvertisedBandwidth to net.tar
add relay with RelayBandwidthRate to net.tar

not related to this ticket, i don't know if net.tar should be just a dir, so we see the changes, though i know it's big and mostly key files

In any case, LGTM, so i'll merge after your reply (unless you would like to do some other change).

@pastly

This comment has been minimized.

Copy link
Member Author

commented Jun 23, 2018

@juga0

This comment has been minimized.

Copy link
Contributor

commented Jun 23, 2018

realized CHANGELOG.md should be rebased to master, just that and i merge :)

@juga0 juga0 merged commit cf3e478 into master Jun 23, 2018

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details

@pastly pastly deleted the issue155 branch Jul 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.