Opened 7 years ago

Closed 7 years ago

#6131 closed task (fixed)

bwauths learn to recognize Unmeasured=1 in consensus line and treat it differently

Reported by: arma Owned by: mikeperry
Priority: High Milestone:
Component: Core Tor/Torflow Version:
Severity: Keywords:
Cc: mikeperry, karsten, aagbsn Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

In #2286 we're going to change directory authorities to say something like "w Bandwidth=100 Capped=1" for relays that don't have enough Measured lines.

Torflow's BWAuthority needs to learn not to get snookered by the 100 into thinking that the relay's peers are all the other "Bandwidth=100" relays.

Child Tickets

Attachments (3)

6131_capped_flag.patch (2.1 KB) - added by aagbsn 7 years ago.
Set ratio_rank = 1 if Capped=1 is parsed from NS "w " line
6131_capped_flag_2.patch (5.1 KB) - added by aagbsn 7 years ago.
Attempt 2
6131_capped_flag_3.patch (5.0 KB) - added by aagbsn 7 years ago.
attempt 3

Download all attachments as: .zip

Change History (24)

comment:1 Changed 7 years ago by arma

Cc: mikeperry added

comment:2 Changed 7 years ago by arma

Parent ID: #2286

comment:3 Changed 7 years ago by mikeperry

aagbsn: The change that needs to happen here is in the ConsensusTracker code. It calculates a consensus to descriptor ratio in _read_routers() for each router. Add some parsing code to pull out the "Capped" flag for routers, and those routers should get a hard-coded ratio of 1.0.

The RatioPercentileRestriction should then handle things as normal, without modifications.

comment:4 Changed 7 years ago by arma

aagbsn, any chance we could convince you to work on this? I hear it's easy. :)

Changed 7 years ago by aagbsn

Attachment: 6131_capped_flag.patch added

Set ratio_rank = 1 if Capped=1 is parsed from NS "w " line

comment:5 Changed 7 years ago by aagbsn

Status: newneeds_review

comment:6 Changed 7 years ago by mikeperry

Status: needs_reviewneeds_revision

Two points from the review:

  1. You transform the ratio_r[i] object into an integer 1 for the capped case. This will cause explosions. Better than trying to hack the ratio_rank directly, you should alter the ratio_r sorting to use a Router.get_unmeasured_bw() method to return the NetworkStatus bw value instead of desc_bw if unmeasured is true... Then the ratio will work out to 1 that way.
  1. Please change "Capped" to "Unmeasured". See https://trac.torproject.org/projects/tor/ticket/2286#comment:45 for why.

comment:7 in reply to:  6 Changed 7 years ago by aagbsn

Replying to mikeperry:

Two points from the review:

  1. You transform the ratio_r[i] object into an integer 1 for the capped case. This will cause explosions.

Wait? Do you mean ratio_rank? Isn't that supposed to be an integer? If not, what about TorCtl.py:1647

   for i in xrange(len(ratio_r)): ratio_r[i].ratio_rank = i 

Better than trying to hack the ratio_rank directly, you should alter the ratio_r sorting to use a Router.get_unmeasured_bw() method to return the NetworkStatus bw value instead of desc_bw if unmeasured is true... Then the ratio will work out to 1 that way.

  1. Please change "Capped" to "Unmeasured". See https://trac.torproject.org/projects/tor/ticket/2286#comment:45 for why.

Ok.

Changed 7 years ago by aagbsn

Attachment: 6131_capped_flag_2.patch added

Attempt 2

comment:8 Changed 7 years ago by aagbsn

Status: needs_revisionneeds_review

comment:9 Changed 7 years ago by mikeperry

Status: needs_reviewneeds_revision

Two more things:

  1. w.groups(2) doesn't appear to do what you think it does. At least not in my python... I tested your regex in a shell your if statement always ends up true, regardless of if Capped=1 is present or not.
  1. The consensus w keyword is ultimately going to be Unmeasured=1, not Capped=1. You need to update the regex for that, too.

comment:10 Changed 7 years ago by aagbsn

  1. Whoops, good catch.
  2. Hmm.
    aagbsn@debian:~/code/torflow/TorCtl$ python
    Python 2.6.6 (r266:84292, Dec 26 2010, 22:31:48) 
    [GCC 4.4.5] on linux2
    Type "help", "copyright", "credits" or "license" for more information.
    >>> import re
    >>> nsline = "w Bandwidth=100 Unmeasured=1"
    >>> m = re.search(r"^w Bandwidth=(\d+)(?:\s(Unmeasured)=1)?", nsline, re.M)
    >>> m.groups()
    ('100', 'Unmeasured')
    >>> nsline = "w Bandwidth=100"
    >>> m = re.search(r"^w Bandwidth=(\d+)(?:\s(Unmeasured)=1)?", nsline, re.M)
    >>> m.groups()
    ('100', None)
    >>> nsline = "w Bandwidth=100 Unmeasured=0"
    >>> m = re.search(r"^w Bandwidth=(\d+)(?:\s(Unmeasured)=1)?", nsline, re.M)
    >>> m.groups()
    ('100', None)
    >>> 
    

Changed 7 years ago by aagbsn

Attachment: 6131_capped_flag_3.patch added

attempt 3

comment:11 Changed 7 years ago by aagbsn

Status: needs_revisionneeds_review

Did attempt 3 look ok?

comment:12 Changed 7 years ago by mikeperry

Cc: karsten added

I think this looks good. Merge it and be sure it starts running on a bw auth for a while, and verify those results seem sane?

Karsten has scripts for comparing the bw auth votes in the metrics repo somewhere. Perhaps he can point you at them. In the past, I've found them very useful for ensuring new bandwidth auth changes produced results in line with older code running on other bw auths.

comment:13 in reply to:  12 Changed 7 years ago by karsten

Replying to mikeperry:

Karsten has scripts for comparing the bw auth votes in the metrics repo somewhere. Perhaps he can point you at them. In the past, I've found them very useful for ensuring new bandwidth auth changes produced results in line with older code running on other bw auths.

Was that #2394? If so, the code is in the metrics-tasks repo in task-2394/.

comment:14 Changed 7 years ago by arma

Summary: bwauths learn to recognize Capped=1 in consensus line and treat it differentlybwauths learn to recognize Unmeasured=1 in consensus line and treat it differently

comment:15 Changed 7 years ago by arma

moria1's bwauth is now running with this patch

comment:16 Changed 7 years ago by nickm

Parent ID: #2286

Removing parent because we're about to merge #2286.

We should merge this patch into torflow soon!

comment:17 Changed 7 years ago by arma

moria1's bwauth is still running this patch. The world hasn't ended yet.

comment:18 Changed 7 years ago by nickm

Priority: normalmajor

Can we please now merge this? #2286 is merged .

If we can't, what's blocking this?

comment:19 Changed 7 years ago by nickm

Cc: aagbsn added

comment:20 Changed 7 years ago by aagbsn

Owner: changed from aagbsn to mikeperry
Status: needs_reviewassigned

I can't commit this code because: W access for pytorctl DENIED to aagbsn.

mikeperry?

comment:21 Changed 7 years ago by aagbsn

Resolution: fixed
Status: assignedclosed

Pushed.

Note: See TracTickets for help on using tickets.