Opened 2 years ago

Last modified 21 months ago

#21846 new enhancement

BwAuthority can't be run out of the box without manual work

Reported by: davidwf Owned by:
Priority: Medium Milestone:
Component: Core Tor/Torflow Version:
Severity: Normal Keywords:
Cc: chelseakomlo, mikeperry, teor, AgentSeb, davidwf, tom Actual Points:
Parent ID: Points:
Reviewer: chelseakomlo, tom Sponsor:

Description

Currently the bwauthority is quite out of date with the current python ecosystem and doesn't play nice in a dev environment.

The submitted patch makes the following updates:

1) Removes the cron setup to a separate script (so running setup.sh doesn't alter your crontab)
2) Changes the shell script to use a modern pip instead of peep (peep is deprecated and hash checking is incorporated into pip as of version 8.0 (https://pypi.python.org/pypi/peep/)
3) Updated dependencies as far as possible (Elixer's last version is 0.7.1 and is only compatible with SQLAlchemy <= 0.7.8, obvious future enhancement to move away from Elixir)
4) Added a convenience script to kill all the spun up processes
5) Updated the ReadMe and the .gitignore to reflect these changes.

Child Tickets

TicketTypeStatusOwnerSummary
#20452defectnewmacOS and BSD don't support readlink -f in bwauth setup.sh
#20466defectnewReplacing python path sometimes breaks bwauths
#24089defectnewtomrequirements.txt uses peep format

Attachments (1)

0001-21846-Work-to-make-bwauthenv-nicer-to-devs.patch.gz (4.5 KB) - added by davidwf 2 years ago.
Patch as described

Download all attachments as: .zip

Change History (21)

Changed 2 years ago by davidwf

Patch as described

comment:1 Changed 2 years ago by davidwf

Gzipped patch attached + github branch here: https://github.com/davidfaulkner12/bwauth/tree/nicedev

comment:2 Changed 2 years ago by davidwf

Cc: davidwf added
Status: newneeds_review

comment:3 Changed 2 years ago by chelseakomlo

Reviewer: chelseakomlo, teor

comment:4 Changed 2 years ago by chelseakomlo

I will test functionality, it would be good to have teor or someone who runs a bwauth to give a second opinion.

comment:5 Changed 2 years ago by chelseakomlo

Reviewer: chelseakomlo, teorchelseakomlo, tjr

comment:6 Changed 2 years ago by tom

Cc: tom added
Reviewer: chelseakomlo, tjrchelseakomlo, tom

This seems good, but I'm wondering if it conflicts or subsumes some outstanding work in #20466, #20452, #20454, #20452

comment:7 Changed 2 years ago by chelseakomlo

This seems good to me as well (I tested this on macOS).

If virtualenv is used, #20466 shouldn't be an issue (I believe).

#20454 should also be addressed with this patch.

comment:8 Changed 2 years ago by chelseakomlo

Status: needs_reviewmerge_ready

comment:9 Changed 2 years ago by chelseakomlo

Changing this to merge ready. tom, feel free to change to needs_review if you see anything else that needs revision.

comment:10 Changed 2 years ago by teor

Status: merge_readyneeds_revision

davidwf: thanks for this patch!

In general:

Why did you downgrade from SQLAlchemy 0.7.10 to 0.7.8?
Whichever way you choose to go, please find all instances of SQLAlchemy-0.7.* and make them consistent.

For future patches, it is ok to provide a branch and use multiple commits - in fact, we prefer it!

In particular, it is nice if whitespace changes are in a separate commit to functional changes.

In particular:

In setup.sh and setup_cron.sh:

readlink -f works with gnu readlink, but not with BSD readlink. It is possible to install BSD readlink on Linux, and GNU readlink on BSD or macOS. So we can't depend on uname, we should just try basename if readlink doesn't work here:

+# readlink -f does not work on Mac
+if [ `uname` != "Darwin" ]
+then
+  SCANNER_DIR=$(readlink -f "$SCANNER_DIR")
+fi

Also, let's use the same code in all the scripts to do this, for consistency.

Some OSs only have a non-versioned python (e.g. macOS), so we should fall back to "which python" here:

+PYTHON=$(which python2.7 || which python2.6)

In stop_scan.sh:

This is a dangerous strategy if the process has already died, and its PID is being used by something else. Can we check the process has the right name in a cross-platform way?

kill -9 `head -1 $PIDFILE` && rm $PIDFILE

You may also be better making the parent scanner script Tor's OwningControllerProcess, so tor exits automatically when the script exits. But you'll just have to kill the bwauthority child processes.

Please don't copy environmental variables like this:

+SCANNERS_PER_TOR_COUNT=4
+TOR_COUNT=2
+SCANNER_COUNT=$(($SCANNERS_PER_TOR_COUNT * $TOR_COUNT + 1))

It makes it easy for them to get out of sync with the launch script.
Also, if someone changes the variables, then stops the scanners, some will be left over.
Instead, why not do:

for PIDFILE in ./data/scanner.*/bwauthority.pid

and the same for the tor instances.

comment:11 in reply to:  10 ; Changed 2 years ago by davidwf

Please see changes at 'git@…:davidfaulkner12/bwauth.git', branch 'nicedev'.

Why did you downgrade from SQLAlchemy 0.7.10 to 0.7.8?
Whichever way you choose to go, please find all instances of SQLAlchemy-0.7.* and make them consistent.

Updated -- the PYTHONPATH variables in the shell scripts had no impact (and didn't resolve to anything on my machine at least), so I removed them. I used 0.7.8 because that's the last documented "compatible" version with Elixer.

For future patches, it is ok to provide a branch and use multiple commits - in fact, we prefer it!

Noted! It was actually always in Github, I just through patches were preferred. :-)

readlink -f works with gnu readlink, but not with BSD readlink. It is possible to install BSD readlink on Linux, and GNU readlink on BSD or macOS. So we can't depend on uname, we should just try basename if readlink doesn't work here:

I checked to see if the command fails, and only set the variable if necessary. This should work!

Some OSs only have a non-versioned python (e.g. macOS), so we should fall back to "which python" here:

Done.

In stop_scan.sh: ...

stop_scan.sh is actually just copy and pasted from the first lines of the run_scan.sh. I don't disagree with anything you said, but I'm not confident that I can make those changes in a way that works reliably without digging too deeply past the initial scope of "get this working on a laptop".

comment:12 Changed 2 years ago by chelseakomlo

Status: needs_revisionneeds_review

comment:13 in reply to:  11 Changed 2 years ago by chelseakomlo

Replying to davidwf:

In stop_scan.sh: ...

stop_scan.sh is actually just copy and pasted from the first lines of the run_scan.sh. I don't disagree with anything you said, but I'm not confident that I can make those changes in a way that works reliably without digging too deeply past the initial scope of "get this working on a laptop".

If we don't want to introduce tech debt in multiple places, stop_scan.sh doesn't need to be added as part of this patch, but I agree having this as a convenience would be useful.

comment:14 Changed 2 years ago by teor

What testing do we need to do before we merge this patch?

comment:15 Changed 2 years ago by tom

When I lost the bwauth box that my bwauth was runnign on, I lost the ability to keep working on this. Now that I have a new box I can resume the bwauth queue of patches. Feel free to bug me ~weekly about them.

I'm going to try running a second bwauth (on the same box), see if that causes any abnormalities in the existing one, compare results, and then start testing changes...

comment:16 Changed 2 years ago by tom

Hm, I think we want to solve the following:

  • setup_cron.sh needs to marked executable
  • We should centralize the variables and functionality shared in run_scan and stop_scan
  • I don't know why, but the setup_cron.sh script wipes out any existing cron entries you have. Or at least that is my experience.

I can probably take these as actions to resolve and then we can merge.

comment:17 Changed 22 months ago by teor

Status: needs_reviewneeds_revision

Single tickets that deal with multiple issues are really hard to resolve. Please split up any issues that you'd like to continue working on into their own tickets, and make this ticket the parent.

1) Removes the cron setup to a separate script (so running setup.sh doesn't alter your crontab)

wontfix - In #24088, we want to get rid of setup.sh, it's just too fragile. We need to write good instructions instead.

2) Changes the shell script to use a modern pip instead of peep (peep is deprecated and hash checking is incorporated into pip as of version 8.0 (​https://pypi.python.org/pypi/peep/)

revise - this needs to become a change to the requirements file, see #24089

3) Updated dependencies as far as possible (Elixer's last version is 0.7.1 and is only compatible with SQLAlchemy <= 0.7.8, obvious future enhancement to move away from Elixir)

split off into its own ticket, please

4) Added a convenience script to kill all the spun up processes

split off into its own ticket, please

5) Updated the ReadMe and the .gitignore to reflect these changes.

put these in the branches for the relevant ticket(s), please

comment:18 Changed 22 months ago by teor

Priorities and Severities in torflow are meaningless, setting them all to Medium/Normal.

comment:19 Changed 22 months ago by teor

Owner: aagbsn deleted
Status: needs_revisionassigned

aagbsn was the default owner, unassigning

comment:20 Changed 21 months ago by teor

Status: assignednew

Mark all tickets that are assigned to nobody as "new".

Note: See TracTickets for help on using tickets.