stop respecting the pre-v1.3.0 configuration files (improve error message and tests)

[zooko]

As documented in configuration.rst​, Tahoe-LAFS before release v1.3.0 (2009-02-13) had a different way to control configuration. Until now if that older way is present then it overrides​ the newer way. Let's remove any detection of the old way and just leave docs (in trunk/docs/historical) for anybody who needs to upgrade. (There are probably zero users of Tahoe-LAFS < 1.3.)

The advantage is to simplify the docs and the configuration code and reduce the number of ways that things can be configured.

[zooko]

Replying to zooko:

(There are probably zero users of Tahoe-LAFS < 1.3.)

There might actually be someone who still has config files that were generated by Tahoe-LAFS < 1.3. Certainly I don't. I don't think Brian does. Anyone else?

In any case, I'm writing this patch and I'm making it emit a warning message if any old-style configuration files are detected.

[davidsarah]

Replying to zooko:

In any case, I'm writing this patch and I'm making it emit a warning message if any old-style configuration files are detected.

Another option would be to print the warning but still use the old settings for one major release, and then stop using the old settings in the next major release.

OTOH, I'm fine with going straight to ignoring these files, but in that case we should probably announce the change on tahoe-dev.

[zooko]

Done: ​http://tahoe-lafs.org/pipermail/tahoe-dev/2011-April/006243.html

[zooko]

By the way I like the principle of warning in a major release before ignoring in a later major release, but in this case I don't think it is worth it since I doubt there are any users with old-style configuration files, and the failure mode if there are is pretty safe.

[zooko]

There's one issue I've found: foolscap (v0.6.1) can't accept a set of log gatherer furls through its Python API—it can accept at most one that way and it can accept any number by reading a file and finding one furl per line in that file. I've opened ​foolscap ticket 176 to request that a future version of foolscap accept any number of log gatherer furls through its Python API.

In the meantime we could either:

1. remove the ability to have multiple log gatherers from Tahoe-LAFS, which would be a regression (albeit this is probably a feature that nobody currently uses), or

2. we could preserve the file $BASEDIR/log_gatherer.furl for another major release (unioning with the contents of the singleton log_gatherer.furl key in $BASEDIR/tahoe.cfg), or

3. we could extend the tahoe.cfg key to accept multiple furls (whitespace separated), treat $BASEDIR/log_gatherer.furl like all the other old-style configuration files by warning about its existence and ignoring its contents, and use a different filename such as $BASEDIR/foolscap/log_gatherer_furls.txt to transmit the set of furls from tahoe.cfg to foolscap.

The advantage of approach 3 is that the user configures log gatherer furls just like she configures everything else: in $BASEDIR/tahoe.cfg. $BASEDIR/foolscap/log_gatherer_furls.txt is documented as being "internal use only" and not for users to read or edit. (We might rm it after letting foolscap read it just to drive the point home.)

Someday when all users have upgraded to a version of foolscap that provides ​foolscap ticket 176, then we could stop using the temporary file hack to communicate the set of furls from tahoe.cfg into foolscap.

So, I'm currently implementing approach 3, but I'll listen if anybody has a strong opinion to the contrary.

[warner]

I'd like some sort of warning in a release or two, but Zooko says he's writing code to detect-and-complain-about the old files, which I'm happy with as long as we have a plan to remove it eventually (probably around the 2.0 timeframe) (so we don't accumulate old cruft forever). Note that it'd probably be sufficient to do a fatal complaint about the lack of a tahoe.cfg, because the nodes that were configured with individual files probably won't have one, and that sort of complaint could be kept around forever.

I'm ok with regressing on multiple-log-gatherers until Foolscap has an API to handle that. I'd prefer the whitespace-separated tahoe.cfg key over having a separate file with a funny name.

(note that part of the reason for having discrete files for things like log-gatherers was to make it easy to set up or modify a whole bunch of Allmydata servers with a batch of scp commands: just stuff the log-gatherer.furl file into all of them and then bounce them all. To do that with tahoe.cfg requires editing files, so needs more complex tooling. But I don't think this is an important feature these days, and I prefer the simplicity of a single config file).

[zooko]

Just to be clear, are you also okay with my plan 3 which supports multiple log gatherers in Tahoe-LAFS by putting them into a file to give to foolscap?

[warner]

eh, yeah, if you want to do that, I'm ok with it, but it feels a touch complex. I'd be just as happy with the simpler plan 1.

[zooko]

Replying to warner:

(note that part of the reason for having discrete files for things like log-gatherers was to make it easy to set up or modify a whole bunch of Allmydata servers with a batch of scp commands: just stuff the log-gatherer.furl file into all of them and then bounce them all. To do that with tahoe.cfg requires editing files, so needs more complex tooling. But I don't think this is an important feature these days, and I prefer the simplicity of a single config file).

I see. Yes, I think we can push this complexity off because nowadays sysadmins and their tools like puppet are getting good at editing config files in place.

[zooko]

Here are all the old-style config files that I've found (from configuration.rst):

• BASEDIR/nickname
• BASEDIR/webport
• BASEDIR/client.port
• BASEDIR/introducer.port
• BASEDIR/advertised_ip_addresses
• BASEDIR/log_gatherer.furl
• BASEDIR/keepalive_timeout
• BASEDIR/disconnect_timeout
• BASEDIR/introducer.furl
• BASEDIR/helper.furl
• BASEDIR/key_generator.furl
• BASEDIR/stats_gatherer.furl
• BASEDIR/no_storage
• BASEDIR/readonly_storage
• BASEDIR/sizelimit
• BASEDIR/debug_discard_storage
• BASEDIR/run_helper

Here is the NEWS file entry from the Tahoe-LAFS v1.3.0 release which announced the new config file format: NEWS.

[zooko]

Replying to warner:

eh, yeah, if you want to do that, I'm ok with it, but it feels a touch complex. I'd be just as happy with the simpler plan 1.

Okay, I would still prefer plan 3 (simple for users, more complicated for implementors, allows multiple log gatherers), but only if someone else does the work of implementing it. :-) Since I'm doing the work, I've changed my mind to plan 1 (simple for users, simple for implementors, doesn't allow multiple log gatherers).

[davidsarah]

Replying to warner:

eh, yeah, if you want to do that, I'm ok with it, but it feels a touch complex. I'd be just as happy with the simpler plan 1.

I'd also be just as happy with plan 1.

[davidsarah]

Incidentally, the old-file-detection code doesn't need to be particularly complex (it just needs a list of old filenames, not separate code for each), so I don't think will be a problem to keep it until the next major significant compatibility break, say 2.0.

[zooko]

Okay I have this patch almost finished -- I'm just writing the source:NEWS and patch description, and I was writing "This will fail safe and fail loud if an old-style config is found", and then I started to wonder if we shouldn't ensure that it fails safe by stopping the node if it detects an old-style config file. If the node goes ahead and starts up and runs, then it will (after emitting a warning) be operating with different values than it was using in Tahoe-LAFS v1.8.2. I'm looking at the list of old-style config files wondering if it would be unsafe for the user if their node goes ahead and switches to the tahoe.cfg value from the old-style value for any of them. I don't like to wonder about things like that. (Even though there are few or no users who have any old-style config files left.)

So I'm going to go back and change this patch to make the node emit a warning and then stop itself, if it detects an old-style config file.

[davidsarah]

Replying to zooko:

So I'm going to go back and change this patch to make the node emit a warning and then stop itself, if it detects an old-style config file.

+1.

[zooko]

Thanks for the design review!

[zooko]

Planning to work on this and #1363 on the car ride home tomorrow (about ten hours, with one co-driver and two children in the car). In order to make the deadline for new-feature patches for v1.9, which is tomorrow.

[zooko]

attachment:reject-old-style-config-files.darcs.patch​ has three patches in it: the one that actually rejects old-style config files, a tidy-up patch that it depends on, and a whitespace-cleanup patch that it does not depend on. Please review! :-)

[davidsarah]

In the changes to docs/configuration.rst:

• "an old-style file file" -> "an old-style file"
• "ticket #1423" should be a link
• the change starting "These values control the tradeoff" should be in a separate patch. Also that text isn't quite right: "assuming there are at least N servers, up to N-k servers can be offline without losing the file" implicitly assumes that the shares are all stored on different servers.
• "There are two kinds of gatherers. Both produce a FURL which needs to be placed in the NODEDIR/tahoe.cfg file..." -- say which fields of the file.

Otherwise +1.

[zooko@…]

In e5c4e83f4cfe3769:

(The changeset message doesn't reference this ticket)

[davidsarah]

T_X discovered the following regression: Introducer nodes create a file named introducer.furl in their base directory. On the second and subsequent runs after the introducer.furl file has already been created, the introducer will fail to start, because it will see that file and think that it is an old config file.

Our tests failed to detect this for two reasons:

• we don't have any tests that start an introducer twice;
• the "no noise" check when starting an introducer in test_runner.RunNode.test_introducer was switched off, presumably because it was causing too many false positives.

[david-sarah@…]

In e74387f4f15e6839:

test_runner.py: modify RunNode?.test_introducer to test that starting an introducer works when the introducer.furl file already exists. refs #1385

[davidsarah]

Replying to davidsarah:

Our tests failed to detect this for two reasons:

• we don't have any tests that start an introducer twice;
• the "no noise" check when starting an introducer in test_runner.RunNode.test_introducer was switched off, presumably because it was causing too many false positives.

Actually test_runner.RunNode.test_introducer did start and then restart an introducer, but it deleted the introducer.furl in-between. I've changed it to use the mtime of introducer.furl to detect when it has been rewritten.

The "no noise" check being switched off was a red herring; the test would have failed anyway if it hadn't been deleting the introducer.furl.

[davidsarah]

Hmm, maybe there are filesystems on which mtime is too coarse for the new test to work without hanging (because the new mtime of introducer.furl might be the same as the old one). Can anyone think of a better way to tell when the introducer process has restarted?

[david-sarah@…]

In 2d16a16ee3d99482:

Fix the bug that prevents an introducer from starting when introducer.furl already exists. Also remove some dead code that used to read old config files, and rename 'warn_about_old_config_files' to reflect that it's not a warning. refs #1385

[davidsarah]

The regression seems to be fixed, but there's still the issue in comment:32 about the new test. Also the way in which the existence of old config files is reported is quite ugly, with an unnecessary traceback (which initially made me miss the message at the top):

STARTING '/home/davidsarah/tahoe/sparkly/intro'
u"Found pre-Tahoe-LAFS-v1.3 configuration file: '/home/davidsarah/tahoe/sparkly/intro/introducer.furl'. See docs/historical/configuration.rst."
Traceback (most recent call last):
  File "/usr/lib/python2.6/dist-packages/twisted/application/app.py", line 626, in run
    runApp(config)
  File "/usr/lib/python2.6/dist-packages/twisted/scripts/twistd.py", line 23, in runApp
    _SomeApplicationRunner(config).run()
  File "/usr/lib/python2.6/dist-packages/twisted/application/app.py", line 374, in run
    self.application = self.createOrGetApplication()
  File "/usr/lib/python2.6/dist-packages/twisted/application/app.py", line 439, in createOrGetApplication
    application = getApplication(self.config, passphrase)
--- <exception caught here> ---
  File "/usr/lib/python2.6/dist-packages/twisted/application/app.py", line 450, in getApplication
    application = service.loadApplication(filename, style, passphrase)
  File "/usr/lib/python2.6/dist-packages/twisted/application/service.py", line 390, in loadApplication
    application = sob.loadValueFromFile(filename, 'application', passphrase)
  File "/usr/lib/python2.6/dist-packages/twisted/persisted/sob.py", line 210, in loadValueFromFile
    exec fileObj in d, d
  File "tahoe-introducer.tac", line 10, in <module>
    c = introducer.IntroducerNode()
  File "/home/davidsarah/tahoe/sparkly/src/allmydata/introducer/server.py", line 19, in __init__
    node.Node.__init__(self, basedir)
  File "/home/davidsarah/tahoe/sparkly/src/allmydata/node.py", line 67, in __init__
    self.read_config()
  File "/home/davidsarah/tahoe/sparkly/src/allmydata/node.py", line 114, in read_config
    self.error_about_old_config_files()
  File "/home/davidsarah/tahoe/sparkly/src/allmydata/node.py", line 134, in error_about_old_config_files
    raise OldConfigError(oldfnames)
allmydata.node.OldConfigError: set([u'/home/davidsarah/tahoe/sparkly/intro/introducer.furl'])

Failed to load application: set([u'/home/davidsarah/tahoe/sparkly/intro/introducer.furl'])

[david-sarah@…]

In f45bfeb3df62df17:

(The changeset message doesn't reference this ticket)

[davidsarah]

test_runner.py: fix RunNode?.test_introducer to not rely on the mtime of introducer.furl to detect when the node has restarted. Instead we detect when node.url has been written. refs #1385

[davidsarah]

Further improve error message about old config files. refs #1385

[davidsarah]

With improve-old-config-error-message.darcs.patch​, the error looks like:

STARTING '/home/davidsarah/.tahoe'
Traceback (most recent call last):
  File "/usr/lib/python2.6/dist-packages/twisted/application/app.py", line 626, in run
    runApp(config)
  File "/usr/lib/python2.6/dist-packages/twisted/scripts/twistd.py", line 23, in runApp
    _SomeApplicationRunner(config).run()
  File "/usr/lib/python2.6/dist-packages/twisted/application/app.py", line 374, in run
    self.application = self.createOrGetApplication()
  File "/usr/lib/python2.6/dist-packages/twisted/application/app.py", line 439, in createOrGetApplication
    application = getApplication(self.config, passphrase)
--- <exception caught here> ---
  File "/usr/lib/python2.6/dist-packages/twisted/application/app.py", line 450, in getApplication
    application = service.loadApplication(filename, style, passphrase)
  File "/usr/lib/python2.6/dist-packages/twisted/application/service.py", line 390, in loadApplication
    application = sob.loadValueFromFile(filename, 'application', passphrase)
  File "/usr/lib/python2.6/dist-packages/twisted/persisted/sob.py", line 210, in loadValueFromFile
    exec fileObj in d, d
  File "tahoe-client.tac", line 10, in <module>
    c = client.Client()
  File "/home/davidsarah/tahoe/sparkly/src/allmydata/client.py", line 134, in __init__
    node.Node.__init__(self, basedir)
  File "/home/davidsarah/tahoe/sparkly/src/allmydata/node.py", line 73, in __init__
    self.read_config()
  File "/home/davidsarah/tahoe/sparkly/src/allmydata/node.py", line 120, in read_config
    self.error_about_old_config_files()
  File "/home/davidsarah/tahoe/sparkly/src/allmydata/node.py", line 141, in error_about_old_config_files
    raise e
allmydata.node.OldConfigError: Found pre-Tahoe-LAFS-v1.3 configuration file(s):
'/home/davidsarah/.tahoe/webport'
See docs/historical/configuration.rst.

Failed to load application: Found pre-Tahoe-LAFS-v1.3 configuration file(s):
'/home/davidsarah/.tahoe/webport'
See docs/historical/configuration.rst.

The stack trace is still there, but at least it prints a sensible message at the end.

[zooko]

+1 on 2d16a16ee3d99482, f45bfeb3df62df17, fix-introducer-test.darcs.patch​, and improve-old-config-error-message.darcs.patch​. Thanks for the nice usability and testing improvements!

[david-sarah@…]

In 80300ea7a3c582ea:

Further improve error message about old config files. refs #1385

[david-sarah@…]

In 521754b5062cfadd:

test_runner.py: fix RunNode?.test_introducer to not rely on the mtime of introducer.furl to detect when the node has restarted. Instead we detect when node.url has been written. refs #1385

[david-sarah@…]

In b6cfbbeb234cd8c9:

test_client.py: repair Basic.test_error_on_old_config_files. refs #1385

[david-sarah@…]

In b9eb0235ea38ce37:

test_runner.py: fix a bug in CreateNode?.do_create introduced in changeset 521754b5062cfadd when the tahoe.cfg file has been written with CRLF line endings. refs #1385