Opened 5 months ago

Closed 5 months ago

#28588 closed defect (fixed)

SBWS 'bw_torflow_scale' does not appear to honor relay MaxAdvertisedBandwidth

Reported by: starlight Owned by:
Priority: Medium Milestone: sbws: 1.0.x-final
Component: Core Tor/sbws Version: sbws: 1.0.0
Severity: Normal Keywords: sbws-1.0-must-moved-20181128
Cc: juga Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Examining the current 1.0.2 branch it does not seem bw_torflow_scale() honors MaxAdvertisedBandwidth. I'm having trouble following the path the data element follows through the (not simple) code. Is clearly visible with comments in bw_sbws_scale() expressed as min(l.desc_bw_avg, l.bw * scale_constant / m) where desc_bw_avg is the max advertised value but cannot determine this value is consumed by bw_torflow_scale() or anywhere else.

Child Tickets

TicketStatusOwnerSummaryComponent
#28596closedUse the latest descriptor bandwidths for torflow weightingCore Tor/sbws
#28598closedShould torflow scaling use the consensus bandwidth when it is measured?Core Tor/sbws

Change History (19)

comment:1 Changed 5 months ago by starlight

Summary: SBWS 'bw_torflow_scale' does not appear to honor relayMaxAdvertisedBandwidthSBWS 'bw_torflow_scale' does not appear to honor relay MaxAdvertisedBandwidth

comment:2 in reply to:  description ; Changed 5 months ago by juga

Replying to starlight:

Examining the current 1.0.2 branch it does not seem bw_torflow_scale() honors MaxAdvertisedBandwidth.

If i understood it correctly, torflow does not use the descriptor average-bandwidth, but observed-bandwidth. Is that correct?.

I'm having trouble following the path the data element follows through the (not simple) code.

There's #28282 to refactor that.

Is clearly visible with comments in bw_sbws_scale() expressed as min(l.desc_bw_avg, l.bw * scale_constant / m) where desc_bw_avg is the max advertised value but cannot determine this value is consumed by bw_torflow_scale() or anywhere else.

No, it's not consumed anywhere else.

BTW, one comparison we didn't made was to compare torflow's scaling with the linear scaling using the observed bandwidth.

comment:3 in reply to:  2 ; Changed 5 months ago by teor

Milestone: sbws 1.0 (MVP must)
Version: sbws: unspecifiedsbws: 1.0.0

This bug needs to be fixed before we deploy any more bwauths.

Replying to juga:

Replying to starlight:

Examining the current 1.0.2 branch it does not seem bw_torflow_scale() honors MaxAdvertisedBandwidth.

If i understood it correctly, torflow does not use the descriptor average-bandwidth, but observed-bandwidth. Is that correct?.

pytorctl's bw_observed is the minimum of all the bandwidths on the bandwidth line.
It is called bw_observed, but that is a confusing name.

      elif kw == "bandwidth":
        bws = map(int, g)
        bw_observed = min(bws)
        rate_limited = False
        if bws[0] < bws[1]:
          rate_limited = True

https://gitweb.torproject.org/pytorctl.git/tree/TorCtl.py#n459

But sbws' bw_observed is just stem's observed_bandwidth:

    @property
    def relay_observed_bandwidth(self):
        return self._relay.observed_bandwidth

https://github.com/torproject/sbws/blob/5a88f749e15764f08bf5b40b46db2a655d12d2b5/sbws/lib/resultdump.py#L229

When sbws should be using min(average_bandwidth, burst_bandwidth, observed_bandwidth):

average_bandwidth (int) -- * average rate it's willing to relay in bytes/s
burst_bandwidth (int) -- * burst rate it's willing to relay in bytes/s
observed_bandwidth (int) -- * estimated capacity based on usage in bytes/s

https://stem.torproject.org/api/descriptor/server_descriptor.html#stem.descriptor.server_descriptor.ServerDescriptor

The MaxAdvertisedBandwidth check was implemented in sbws scaling in:
https://github.com/torproject/sbws/issues/155
https://github.com/torproject/sbws/pull/191
https://trac.torproject.org/projects/tor/ticket/8494

Then removed in commit 514671e:
https://github.com/torproject/sbws/commit/514671efb4ccc4f9b63dad0f5373891f7f27b990

The MaxAdvertisedBandwidth check was never implemented in torflow scaling.

There are no unit tests for MaxAdvertisedBandwidth.
And the integration test didn't catch this bug:
https://github.com/torproject/sbws/blob/master/tests/integration/core/test_scanner.py#L21

comment:4 in reply to:  3 Changed 5 months ago by juga

Replying to teor:

pytorctl's bw_observed is the minimum of all the bandwidths on the bandwidth line.
It is called bw_observed, but that is a confusing name.

      elif kw == "bandwidth":
        bws = map(int, g)
        bw_observed = min(bws)
        rate_limited = False
        if bws[0] < bws[1]:
          rate_limited = True

https://gitweb.torproject.org/pytorctl.git/tree/TorCtl.py#n459

Auch... Thanks for spotting that.

comment:5 Changed 5 months ago by teor

We should also remove the comment about MaxAdvertisedBandwidth in the sbws scaling section.

comment:6 Changed 5 months ago by juga

As far as i understand, torflow tracks new consensuses, instead sbws gets it at some intervals.
For this reason, to obtain the observed bandwidth from all the results of a relay, we implemented to obtain both from the mean of those results or the last. By default, it's currently using the mean.
I think it should probably default to the last.
Since we need to also get the average bandwidth and the burst bandwidth, should these values be obtained from the last or the mean?

comment:7 in reply to:  5 ; Changed 5 months ago by juga

Replying to teor:

We should also remove the comment about MaxAdvertisedBandwidth in the sbws scaling section.

Shouldn't we also use the minimum of all the values in the descriptor instead of of the descriptor bandwidth average?.
I think that could make the linear method look closer to the torflow's one.

comment:8 in reply to:  7 Changed 5 months ago by teor

Replying to juga:

As far as i understand, torflow tracks new consensuses, instead sbws gets it at some intervals.
For this reason, to obtain the observed bandwidth from all the results of a relay, we implemented to obtain both from the mean of those results or the last. By default, it's currently using the mean.
I think it should probably default to the last.

Yes, torflow uses the last, so we should use the last. See #28596.

We can change this decision in 1.1 if we think #27788 and #27789 are good ideas.

Since we need to also get the average bandwidth and the burst bandwidth, should these values be obtained from the last or the mean?

Torflow uses the latest descriptor, so sbws should too.

Replying to juga:

Replying to teor:

We should also remove the comment about MaxAdvertisedBandwidth in the sbws scaling section.

We should put this comment where we calculate the minimum.

Shouldn't we also use the minimum of all the values in the descriptor instead of of the descriptor bandwidth average?.

Yes, we should do what torflow does: take the minimum of all the values on the bandwidth line in the descriptor.

I think that could make the linear method look closer to the torflow's one.

The linear method is not important for sbws 1.0, so you can fix it if you want, or leave it.

comment:9 Changed 5 months ago by juga

I've the branch https://github.com/torproject/sbws/compare/master...juga0:bug28588, but now i wonder whether i should solve #28600, or just leave it for some future refactoring.

comment:10 in reply to:  9 Changed 5 months ago by teor

Status: newneeds_review

Replying to juga:

I've the branch https://github.com/torproject/sbws/compare/master...juga0:bug28588

Setting this ticket to needs_review.
Let's ask at the meeting if anyone wants to review it.

but now i wonder whether i should solve #28600, or just leave it for some future refactoring.

I will comment on that ticket.

comment:11 Changed 5 months ago by juga

If i refactor, then i'll need to modify the branch and this ticket should be in needs_revision

comment:12 Changed 5 months ago by nickm

Hi! I offered to take a look at the branch at Monday's meeting. I've had a look over the python and I don't see anything objectionable there.

comment:13 in reply to:  11 Changed 5 months ago by teor

Status: needs_reviewmerge_ready

Replying to juga:

If i refactor, then i'll need to modify the branch and this ticket should be in needs_revision

I still think the refactor is a bad idea, see:
https://trac.torproject.org/projects/tor/ticket/28600#comment:5

I think we should merge this branch as it is.

comment:14 Changed 5 months ago by teor

Keywords: sbws-1.0-must-moved-20181128 added
Milestone: sbws 1.0 (MVP must)sbws 1.0.3

Moving all sbws 1.0 must bugs to 1.0.3.

comment:15 Changed 5 months ago by teor

Milestone: sbws 1.0.3sbws 1.0.x

Milestone renamed

comment:16 Changed 5 months ago by teor

Milestone: sbws 1.0.xsbws: 1.0.x

Milestone renamed

comment:17 Changed 5 months ago by teor

Milestone: sbws: 1.0.xsbws: 1.0.x-final

Milestone renamed

comment:18 Changed 5 months ago by juga

merged, it fixes also #28596, but can close because of #28598

comment:19 Changed 5 months ago by juga

Resolution: fixed
Status: merge_readyclosed

#28598 is now merged too

Note: See TracTickets for help on using tickets.