Opened 6 years ago

Closed 3 years ago

#3280 closed enhancement (wontfix)

Make the new Python Torperf write torrc's

Reported by: karsten Owned by: tomb
Priority: Medium Milestone:
Component: Metrics/Torperf Version:
Severity: Keywords:
Cc: Actual Points:
Parent ID: #2565 Points:
Reviewer: Sponsor:

Description

Once #3279 is complete, we should extend the new Torperf to interact with its tor processes, except talking to the control port which should come later. In this ticket, Torperf should create any missing Tor data directories and torrc files under the path given in DataDirectory here. Some logic is necessary to auto-generate data directory names, increment port numbers, etc. Also, there are some torrc config options that don't show up in Torperf's config file, but that need to go into the torrc files, e.g., MaxCircuitDirtiness 1 minute, UseEntryGuards 0, and RunAsDaemon 1. See measurements-HOWTO for details.

Assigning to tomb upon request.

Child Tickets

Change History (6)

comment:1 follow-up: Changed 6 years ago by tomb

  • Status changed from new to needs_review

I would like some general feedback on my approach at this point. Many parts are sketched in. Known bugs are in the BUGS file. The README has some details of what I am hoping to have accomplished by this point.

comment:2 in reply to: ↑ 1 Changed 6 years ago by tomb

I forgot to say:
Until I am confident in my general approach, I am keeping this branch in tomb/torperf.git branch ticket3280

comment:3 Changed 6 years ago by atagar

Hi Tom. Looks like a nice start. I checked your series of commits (via 'git diff 079de66 7159cb1') and here's my two cents...

========================================
README
========================================

'controling' and 'necisary' are misspellings in the header

BUGS: bugs that I know about that could impact normal use, please add
any major bugs you spot to this file.

It would be better if they filed trac tickets for any issues. Especially since they're not likely to have commit access. ;)

========================================
torperf.py
========================================

# as much code as possible is stolen from the arm project

Heh, cute. ;)

I'll be moving arm's utilities to Stem so in a few months you should have a proper library to import from rather than duplicating modules.

# Global Vars are in ALL CAPS

This is a very, very common convention for all languages. No need to comment it.

DEF_WD = os.path.expanduser("~/torperf/wd/")

What is the 'DEF_' prefix for? It seems to be fairly common and my first guess is 'defined', but that wouldn't make sense. A comment explaining the purpose of those globals would be nice.

DEF_LOG_DUMP_FQ_FILENAME = DEF_WD + "log"

Very minor thing but 'log' would be a filename and '/home/user/torperf/wd/log' is a path. I'd call this something like 'LOG_PATH' instead.

"""
Dumps the current torrc configuration into the log
We recomment that you dump conf and keep it with your data so that
you can replicate experiments.
"""

Very minor thing but there's some trailing whitespace after the quotes. There's a bit of a holy war about extra whitespace and my feeling is that lines should have the same indentation level as the code around it. However, you pick neither of the warring whitespace camps and seem to include/drop whitespace randomly. ;)

torperfConfigKeys = list(config.getKeys())
torperfConfigKeys.sort()

You could also do...
torperfConfigKeys = sorted(config.getKeys())

def _dumpConfig():

I'd probably replace this entire function with...

config = util.conf.getConfig("torperf")
dumpLines = Torperf Configuration:?

configKeys = sorted(config.getKeys())

if configKeys:

dumpLines += % (key, config.contents[key?) for key in configKeys]

else:

dumpLines[0] += " None"

util.log.log(util.log.DEBUG, "\n".join(dumpLines))

# --------------------------- end boot stuff -----------------------

I don't know what this comment means.

# TODO - tomb: shouldn't config.update() do this?
confKeys = config.getKeys()
for key in confKeys:

if not key in CONFIG:

CONFIG[key] = config.getValue(key)

No, config.update() won't and there seems to be a misunderstanding here of how the config works.

Every file that uses configuration options has its own dictionary (CONFIG in this case) which tells us two things: which configuration values this file cares about and what the default values are. Like all constant values this should never be edited.

What you're doing here is adding anything that's in configuration file that we loaded but not in CONFIG to the CONFIG dictionary. To use this properly you should...

  • Remove the above code snippet.
  • Add "run.ids" to the CONFIG dictionary at the top of the file since you use it a little later.
  • Don't pass CONFIG into 'util.run.Run'. No other file should care about the default CONFIG values that torperf.py uses internally.

runIds = config.getValue("run.ids", default = "")
runIds = runIds.split(",")

Since you have the default value this could be combined into...
runIds = config.getValue("run.ids", default = "").split(",")

or better you could do...
runIds = config.getIntCSV("run.ids", [])

if you want integer runIds or 'getStrCSV' instead if you want to allow arbitrary strings.

util.log.log(util.log.DEBUG, "Run IDs: " + CONFIGrun.ids?)

This will currently crash if "run.ids" isn't in your config file. This is probably fine since you check it above (by exiting with "at least one run id must be specified in configuration"), but this is kinda funky.

runs = []

Not used, you can delete this. By the way, running pylint with your code will catch a lot of issues. Here's how I run it...
pylint --indent-string=" " --disable=C,R myScript.py | less

runId = runId.lstrip()

This is alright, but I'd probably sanitize the values earlier. Also, are you fine with empty ids? For instance, a config entry like...
run.ids 1,23

run = util.run.Run(runId, CONFIG, socksPort, controlPort)

Besides the CONFIG, passing in the socksPort and controlPort shouldn't be necessary since it's just the "general.startSocksPorts" and "general.startControlPorts" config values. By the way, aren't separate torperf instances supposed to have unique ports? Otherwise running them all at the same time will fail (tor will fail to bind to the port and refuse to start).

========================================
util/run.py
========================================

config = conf.getConfig("torperf")

This is never used in the file

self.CONFIG = CONFIG # DO NOT MODIFY CONFIG, it might explode

Right, because that would be concurrent edits of the 'torperf.py' default config values which were synced to 'config' in an addhoc manner. ;)

Use the 'config' global that you made earlier. It has the same configuration values and has locking to safely handle concurrent modifications.

def getConfig(self, key):

runPrefix = "run." + self.runId + "."
if runPrefix + key in self.CONFIG:

return self.CONFIG[runPrefix + key]

else:

if "general." + key in self.CONFIG:

return self.CONFIG+ key?

else:

return None

If you used the config file instead (ie, the 'config' rather than 'CONFIG' variable) then this would all be a little nicer.

def getConfig(self, key):

prefix = "run.%s." % self.runId
default = config.get("general.%s" % key)
return config.get(prefix + key, default)

def writeTorrc(self):

It's probably overkill but another option is...
https://gitweb.torproject.org/arm.git/blob/HEAD:/src/util/torConfig.py#l910

If you're gonna stick with this then I'd suggest instead going with...
torrcLines = ["DataDirectory %s" % self.dataDirectory,

"SocksPort %s" % str(self.socksPort),
... etc... ]

fh.write("\n".join(torrcLines))

os.chdir(self.dataDirectory)

This doesn't look necessary until, maybe, the download later (I'm not really clear what most of this is doing - commenting would be a good idea). Also, like outputFh and dumpFh this looks like it'll be broken with concurrent runs.

========================================
util/test.py
========================================

This is an empty python script. Why is it here?

I blame trac for any whitespace/line wrapping funkyness in the above. Cheers! -Damian

comment:4 Changed 6 years ago by tomb

  • Status changed from needs_review to accepted

Thank you atagar for those comments! I will implement them.

comment:5 Changed 6 years ago by atagar

I've revised the conf util for stem, expanding its documentation and giving example usage. Hopefully this helps:
https://gitweb.torproject.org/stem.git/blob/HEAD:/stem/util/conf.py#l1

Cheers! -Damian

comment:6 Changed 3 years ago by karsten

  • Resolution set to wontfix
  • Status changed from accepted to closed

There's a new attempt to rewrite Torperf, starting with a design document. Closing all tickets related to the earlier attempt, because they're not relevant anymore.

Note: See TracTickets for help on using tickets.