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

Added find max bandwidth feature. #96

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

kaldughayem
Copy link

@kaldughayem kaldughayem commented Aug 12, 2019

Usage: add the flag -findMax and specify a server address with -s, it will run multiple 3 seconds bandwidth tests until it finds the maximum bandwidth.

Testing:

  • Using another AS inside a docker container running the server, and limit the bandwidth on that AS's interface using a tool. I use tcconfig for that, tc the traffic control.

  • Measure the bandwidth to a remote AS, the attachment points for example, and the returned bandwidth should roughly correspond to the ratio of upload:download speed available for the machine you're running the tests from.


This change is Reviewable

the flag to use it is -findMax and specify a server address, it will
run multiple 3 seconds tests to find the maximum bandwidth.
@kaldughayem kaldughayem marked this pull request as ready for review August 12, 2019 17:09
Copy link
Contributor

@FR4NK-W FR4NK-W left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 7 unresolved discussions (waiting on @kaldughayem)


bwtester/bwtestclient/bwtestclient.go, line 599 at r1 (raw file):

	)

	clientBw := int64(512000)

initialTargetBw := 512e3 // 512 kbps


bwtester/bwtestclient/bwtestclient.go, line 606 at r1 (raw file):

// Setup the paths

l. 607-628 duplicates code from 342-363, refactor into a function.


bwtester/bwtestclient/bwtestclient.go, line 630 at r1 (raw file):

CCConn

Client control channel setup (630-652) is also duplicates code from l. 369-390 => refactor into a function.


bwtester/bwtestclient/bwtestclient.go, line 660 at r1 (raw file):

//Data channel connection

This section duplicates code from line 392 onwards. Refactor it so that the target bandwidth is a parameter,


bwtester/bwtestclient/bwtestclient.go, line 672 at r1 (raw file):

Do the test for 10 seconds

The comment does not match the value used.


bwtester/bwtestclient/bwtestclient.go, line 674 at r1 (raw file):

clientBwpStr := fmt.Sprintf("3,?,?,%dbps", clientBw)

Don't build the string to then again parse it, set the BwtestParameters directly.


bwtester/bwtestclient/bwtestclient.go, line 1000 at r1 (raw file):

var newBandwidth, newThreshold int64

Use named return values

`bwtestclient`:
- `receiveDone` is a channel now
- Refactored code to reduce duplication and make it easier to read
- Changed some error logging messages

`bwtestlib`:
- `HandleDCConnReceive` takes a channel as a parameter instead of Lock, and signals it when it is done
Copy link
Author

@kaldughayem kaldughayem left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 7 unresolved discussions (waiting on @FR4NK-W)


bwtester/bwtestclient/bwtestclient.go, line 599 at r1 (raw file):

Previously, FR4NK-W wrote…

initialTargetBw := 512e3 // 512 kbps

Done.


bwtester/bwtestclient/bwtestclient.go, line 606 at r1 (raw file):

Previously, FR4NK-W wrote…
// Setup the paths

l. 607-628 duplicates code from 342-363, refactor into a function.

Done.


bwtester/bwtestclient/bwtestclient.go, line 630 at r1 (raw file):

Previously, FR4NK-W wrote…
CCConn

Client control channel setup (630-652) is also duplicates code from l. 369-390 => refactor into a function.

Done.


bwtester/bwtestclient/bwtestclient.go, line 660 at r1 (raw file):

Previously, FR4NK-W wrote…
//Data channel connection

This section duplicates code from line 392 onwards. Refactor it so that the target bandwidth is a parameter,

Done.


bwtester/bwtestclient/bwtestclient.go, line 672 at r1 (raw file):

Previously, FR4NK-W wrote…
Do the test for 10 seconds

The comment does not match the value used.

Forgot to change the value after testing.
Done


bwtester/bwtestclient/bwtestclient.go, line 674 at r1 (raw file):

Previously, FR4NK-W wrote…
clientBwpStr := fmt.Sprintf("3,?,?,%dbps", clientBw)

Don't build the string to then again parse it, set the BwtestParameters directly.

Done.


bwtester/bwtestclient/bwtestclient.go, line 1000 at r1 (raw file):

Previously, FR4NK-W wrote…
var newBandwidth, newThreshold int64

Use named return values

Done.

`bwtestclient`:
- `receiveDone` is a channel now
- Refactored code to reduce duplication and make it easier to read
- Changed some error logging messages

`bwtestlib`:
- `HandleDCConnReceive` takes a channel as a parameter instead of mutex lock, and signals it when it is done
…com:kaldughayem/scion-apps into kaldughayem/bwtestclient-find-max-bandwidth
Copy link
Contributor

@matzf matzf left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 9 unresolved discussions (waiting on @FR4NK-W and @kaldughayem)


bwtester/bwtestclient/bwtestclient.go, line 77 at r2 (raw file):

	maxBandwidth bool

	pathEntry *sciond.PathReplyEntry

Just making everything global variables is really ugly. Most of these can and should be converted to appropriately scoped local variable without much effort.


bwtester/bwtestclient/bwtestclient.go, line 557 at r2 (raw file):

// connection, sets up the Data connection addresses, starts the Data Channel
// connection, then it updates packet size.
func initConns() {

To avoid the global variables: this can take client and server addresses as parameters and return the two connection objects (and an error code).

…lags with find max bandwidth option

- Reduced global variables to 2 variables, all others are scoped local
variables.
- initConns function takes client and server addresses as input (along
with pathAlgo and interactive for choosing the paths while setting up
the addresses), and returns the connections and an error.
- find max bandwidth option can be used with the 'cs' and/or the 'sc'
flags to set the duration, packet sizes, and starting bandwidth for the
 test. If the flags are not set, default values are used
@kaldughayem kaldughayem force-pushed the kaldughayem/bwtestclient-find-max-bandwidth branch from a1bf40c to 8f025a8 Compare August 29, 2019 11:39
Copy link
Author

@kaldughayem kaldughayem left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 9 unresolved discussions (waiting on @FR4NK-W and @matzf)


bwtester/bwtestclient/bwtestclient.go, line 77 at r2 (raw file):

Previously, matzf (Matthias Frei) wrote…

Just making everything global variables is really ugly. Most of these can and should be converted to appropriately scoped local variable without much effort.

Done.
Only two global variables are there now.


bwtester/bwtestclient/bwtestclient.go, line 557 at r2 (raw file):

Previously, matzf (Matthias Frei) wrote…

To avoid the global variables: this can take client and server addresses as parameters and return the two connection objects (and an error code).

Done.

@matzf matzf added the stale label Oct 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants