Opened 5 months ago

Closed 5 months ago

Last modified 4 months ago

#29806 closed defect (fixed)

Ignore bandwidth file lines with "vote=0"

Reported by: juga Owned by: teor
Priority: Medium Milestone: Tor: 0.3.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-bwauth, 040-backport, 035-backport, 034-backport, merge-after-0403-alpha, authority-test-completed, sbws-bwauth-test-completed
Cc: Actual Points: 0.5
Parent ID: Points: 1
Reviewer: nickm, teor Sponsor:

Child Tickets

Change History (33)

comment:1 Changed 5 months ago by juga

It probably should be backported to the minimum Tor version dirauths are running.

comment:2 Changed 5 months ago by juga

Keywords: 035-backport added

comment:3 Changed 5 months ago by juga

Owner: set to juga
Status: newassigned

comment:4 Changed 5 months ago by juga

Status: assignedneeds_review

comment:5 Changed 5 months ago by juga

Status: needs_reviewneeds_revision

CI failed

comment:6 Changed 5 months ago by teor

Keywords: 034-backport added

Marking for backport to 0.3.4, some directory authorities are still running 0.3.4:
https://consensus-health.torproject.org/#authorityversions
(In general, we ask directory authorities to be on the two latest stable versions. At the moment, that's 0.3.4 and 0.3.5.)

Juga, please rebase your patch on 0.3.4 before review.
The parsing code changed between 0.3.4 and 0.3.5, so you may need a pull request for 0.3.4, and another pull request for 0.3.5.

comment:7 Changed 5 months ago by juga

Parent ID: #28563

Add parent since this might need to be reviewed with it

comment:8 Changed 5 months ago by juga

Status: needs_revisionneeds_review

comment:9 Changed 5 months ago by nickm

Reviewer: nickm

comment:10 Changed 5 months ago by nickm

Status: needs_reviewneeds_revision

This looks okay to me, but I would like to see a unit test to make sure that we *don't* ignore lines that say "vote=1" or "unmeasured=1" or "unmeasured=0".

comment:11 Changed 5 months ago by teor

Keywords: 040-backport added

comment:12 Changed 5 months ago by juga

Status: needs_revisionneeds_review

Added tests, modified one to remove things that were not needed and pushed in 035 the change teor suggested in 034

comment:13 Changed 5 months ago by nickm

lgtm.

comment:14 Changed 5 months ago by nickm

Status: needs_reviewmerge_ready

comment:15 Changed 5 months ago by teor

There is a changes file.
The spec is in #29813.
The CI passed on both pull requests, but 809 still shows travis as pending.
That seems to be a GitHub bug.

The branches have fixups, so I need to do a squash.

The branches have the same effect, but the code changes are in different files. I was concerned about merge conflicts between 0.3.4 and 0.3.5. I did a merge from the 0.3.4 branch to 0.3.5, and copied over the code changes to the new directory structure.

Here are the pull requests:

comment:16 Changed 5 months ago by teor

Oops, there is an extra argument in dirserv_read_measured_bandwidths() in 0.3.5.
Pushed a commit to 0.3.5 and re-did the master merge.

comment:17 Changed 5 months ago by nickm

Keywords: asn-merge added

comment:18 in reply to:  16 ; Changed 5 months ago by teor

Replying to teor:

Oops, there is an extra argument in dirserv_read_measured_bandwidths() in 0.3.5.
Pushed a commit to 0.3.5 and re-did the master merge.

There is a 4th argument to dirserv_read_measured_bandwidths() in master.

If we backport any more calls to dirserv_read_measured_bandwidths(), we're going to have to be really careful.

So we need to merge:

Edit: add 0.4.0 to the list.

Last edited 5 months ago by teor (previous) (diff)

comment:19 Changed 5 months ago by teor

asn, please don't merge this into 0.4.0 until next week: nickm is doing an alpha.

comment:20 Changed 5 months ago by teor

Reviewer: nickmnickm, teor

comment:21 Changed 5 months ago by teor

Keywords: merge-after-0403-alpha added

Tag these tickets for merge after 0.4.0.3-alpha, to make searches easier.

comment:22 Changed 5 months ago by teor

0.4.0.3-alpha was released on Friday, so these tickets can now be merged.

#29018 and #29806 can be merged by asn, and #29703 can be merged by teor as a backport.

comment:23 Changed 5 months ago by teor

Hi asn,

Here's what you need to do to merge two different pull requests to 0.4.0 and master:
(You don't want to merge to 0.3.4 or 0.3.5. That's my job, in a few months time, after a lot of testing!)

  1. Merge ​​https://github.com/torproject/tor/pull/821 to maint-0.4.0
  2. Merge ​​https://github.com/torproject/tor/pull/822 to master
  3. Merge forward

Here are the exact steps on the command-line:

$ cd ../maint-0.4.0
$ git merge tor-github/pr/821
Auto-merging src/test/test_dir.c
Auto-merging src/feature/dirauth/bwauth.c
Merge made by the 'recursive' strategy.
 changes/ticket29806          |  7 ++++++
 src/feature/dirauth/bwauth.c |  8 ++++++-
 src/test/test_dir.c          | 57 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 71 insertions(+), 1 deletion(-)
 create mode 100644 changes/ticket29806
$ cd ../tor
$ git merge tor-github/pr/822
Merge made by the 'recursive' strategy.
 changes/ticket29806          |  7 ++++++
 src/feature/dirauth/bwauth.c |  8 +++++-
 src/test/test_dir.c          | 59 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 73 insertions(+), 1 deletion(-)
 create mode 100644 changes/ticket29806
$ git-merge-forward.sh
  [+] Fetching origin...success
[+] Handling branch 
release-0.2.9 Handling branch 
  [+] Switching branch to release-0.2.9...success
  [+] Merging branch origin/release-0.2.9...success
  [+] Merging branch maint-0.2.9 into release-0.2.9...success
[+] Handling branch 
maint-0.3.4 Handling branch 
  [+] Switching branch to maint-0.3.4...success
  [+] Merging branch origin/maint-0.3.4...success
  [+] Merging branch maint-0.2.9 into maint-0.3.4...success
[+] Handling branch 
release-0.3.4 Handling branch 
  [+] Switching branch to release-0.3.4...success
  [+] Merging branch origin/release-0.3.4...success
  [+] Merging branch maint-0.3.4 into release-0.3.4...success
[+] Handling branch 
maint-0.3.5 Handling branch 
  [+] Switching branch to maint-0.3.5...success
  [+] Merging branch origin/maint-0.3.5...success
  [+] Merging branch maint-0.3.4 into maint-0.3.5...success
[+] Handling branch 
release-0.3.5 Handling branch 
  [+] Switching branch to release-0.3.5...success
  [+] Merging branch origin/release-0.3.5...success
  [+] Merging branch maint-0.3.5 into release-0.3.5...success
[+] Handling branch 
maint-0.4.0 Handling branch 
  [+] Switching branch to maint-0.4.0...success
  [+] Merging branch origin/maint-0.4.0...success
  [+] Merging branch maint-0.3.5 into maint-0.4.0...success
[+] Handling branch 
release-0.4.0 Handling branch 
  [+] Switching branch to release-0.4.0...success
  [+] Merging branch origin/release-0.4.0...success
  [+] Merging branch maint-0.4.0 into release-0.4.0...success
[+] Handling branch 
master Handling branch 
  [+] Switching branch to master...success
  [+] Merging branch origin/master...success
  [+] Merging branch maint-0.4.0 into master...success

These forward merges were clean merges:

  [+] Merging branch maint-0.4.0 into release-0.4.0...success
  [+] Merging branch maint-0.4.0 into master...success

The rest of the merges did not add a merge commit, because there were no changes.

If you want to see what the merge looks like, I pushed the updated maint-0.4.0, release-0.4.0, and master to my GitHub:
https://github.com/teor2345/tor/branches

I added some instructions to the merge policy.
https://trac.torproject.org/projects/tor/wiki/org/teams/NetworkTeam/MergePolicy?action=diff&version=10

comment:24 Changed 5 months ago by teor

Owner: changed from juga to teor
Status: merge_readyassigned

Oops, the 0.3.5 pull request doesn't work when merged to 0.4.0:
https://ci.appveyor.com/project/teor2345/tor/builds/23353398/job/ocl4y7is4jgfbqyu#L2460
(I thought I tested that? Maybe one of the other CI failures hid it?)

I'll try again.

comment:25 Changed 5 months ago by teor

Status: assignedneeds_revision

comment:26 in reply to:  18 Changed 5 months ago by teor

Status: needs_revisionmerge_ready

Ok, all fixed now.

When I backport, I will merge:

When asn merges to mainline, he needs to merge:

The merge forward to master is a clean merge, we should check it passes CI at:
https://github.com/torproject/tor/pull/822

comment:27 Changed 5 months ago by teor

Keywords: consider-backport-after-0405-alpha consider-backport-after-authority-test consider-backport-after-sbws-bwauth-test added

Backport triage tags

comment:28 Changed 5 months ago by teor

The CI passed on PR 847 and the new PR 822.

comment:29 Changed 5 months ago by asn

Thanks for the instructions teor.

I pushed to 040 and master. I think I also messed up somehow and I also accidentally pushed to release-0.3.4... Perhaps it was a wrong merge-forward from yesterday that left trash? Sorry about that, I hope it doesn't cause you trouble.

I opened #29905 for some improvements to the git scripts that would help me against such PEBCAKs.

Last edited 5 months ago by asn (previous) (diff)

comment:30 Changed 5 months ago by asn

Keywords: asn-merge removed

comment:31 Changed 5 months ago by teor

Here is the status of the backports/merges for this ticket:

  • maint-0.3.4: not merged
  • release-0.3.4: merged tor-github/pr/820 to maint-0.3.4 as ee43bc706c, and merged forward to release-0.3.4 as f8304cc6fb
  • maint-0.3.5: not merged
  • release-0.3.5: not merged
  • maint-0.4.0: merged tor-github/pr/847 as 06951cb3fc
  • release-0.4.0: merged maint-0.4.0 forward as fa9e0963cf
  • master: merged maint-0.4.0 forward as 2790ee3685

We could revert the commit, but that's messy. (We don't want to revert the merge, because then getting the commits back in the release is hard.)

This is a 1-line change, with associated debug log, memory management, and unit tests. So we decided to backport now.

We need to make sure this change is tested on a torflow authority, and a sbws authority, before the next 0.3.4 or 0.3.5 release.

comment:32 Changed 5 months ago by teor

Actual Points: 0.5
Milestone: Tor: 0.4.1.x-finalTor: 0.3.4.x-final
Resolution: fixed
Status: merge_readyclosed

Merged pr/820 to maint-0.3.4 and pr/821 to maint-0.3.5, and merged forward.

comment:33 Changed 4 months ago by teor

Keywords: authority-test-completed sbws-bwauth-test-completed added; consider-backport-after-0405-alpha consider-backport-after-authority-test consider-backport-after-sbws-bwauth-test removed
Parent ID: #28563

Update tags and un-parent, because trac is fussy.

Note: See TracTickets for help on using tickets.