presence of MDMF in aliases break the CLI < v1.9.0

[zooko]

I created some MDMF dirs and put them into ~/.tahoe/private/aliases with a recent trunk snapshot (very likely it behaves the same as 1.9.0 does in this respect). Later I switched to Tahoe-LAFS v.1.8.3, and the CLI stopped working. Moving the aliases file aside fixes it, and putting the aliases file back breaks it again

$ tahoe mkdir
Traceback (most recent call last):
  File "/home/zooko/playground/tahoe-lafs/1.8.3/support/bin/tahoe", line 9, in <module>
    load_entry_point('allmydata-tahoe==1.8.3', 'console_scripts', 'tahoe')()
  File "/home/zooko/playground/tahoe-lafs/1.8.3/src/allmydata/scripts/runner.py", line 113, in run
    rc = runner(sys.argv[1:], install_node_control=install_node_control)
  File "/home/zooko/playground/tahoe-lafs/1.8.3/src/allmydata/scripts/runner.py", line 67, in runner
    config.parseOptions(argv)
  File "/usr/local/lib/python2.7/dist-packages/Twisted-11.0.0-py2.7-linux-x86_64.egg/twisted/python/usage.py", line 231, in parseOptions
    self.subOptions.parseOptions(rest)
  File "/usr/local/lib/python2.7/dist-packages/Twisted-11.0.0-py2.7-linux-x86_64.egg/twisted/python/usage.py", line 241, in parseOptions
    self.postOptions()
  File "/home/zooko/playground/tahoe-lafs/1.8.3/src/allmydata/scripts/cli.py", line 46, in postOptions
    aliases = get_aliases(self['node-directory'])
  File "/home/zooko/playground/tahoe-lafs/1.8.3/src/allmydata/scripts/common.py", line 105, in get_aliases
    aliases[name] = uri.from_string_dirnode(cap).to_string()
  File "/home/zooko/playground/tahoe-lafs/1.8.3/src/allmydata/uri.py", line 692, in from_string_dirnode
    assert IDirnodeURI.providedBy(u)
AssertionError
$ mv ~/.tahoe/private/aliases ~/.tahoe/private/1.9-aliases
$ tahoe mkdir
URI:DIR2:CENSORED
$ tahoe --version
allmydata-tahoe: 1.8.3,
foolscap: 0.6.1,
pycryptopp: 0.5.29-r774,
zfec: 1.4.5,
Twisted: 11.0.0,
Nevow: 0.10.0,
zope.interface: unknown,
python: 2.7.1+,
platform: Linux-Ubuntu_11.04-x86_64-64bit_ELF,
pyOpenSSL: 0.10,
simplejson: 2.1.2,
pycrypto: 2.4.1,
pyasn1: unknown,
mock: 0.8.0beta3,
sqlite3: 2.6.0 [sqlite 3.7.4],
setuptools: 0.6c16dev3

[zooko]

This is a bit disappointing because we did quite a lot of work attempting to make forward-compatibility for caps, i.e. that the introduction of new types of caps would not break old installations of the software. If your software looks in a Tahoe-LAFS directory and finds therein a cap of a new type that your software doesn't know how to decode it will handle that situation relatively gracefully. However, I guess we didn't think about the possibility of a new, unknown type being in your private local state (~/.tahoe). That's what happens when a user downgrades—switches to using an older version of the software.

Looking at the code mentioned in the stack trace, it looks like all CLI commands parse all of the aliases at startup (even if the CLI command isn't going to use the aliases, as in my case of tahoe mkdir), by calling from_string_dirnode on each one. from_string_dirnode calls from_string and then asserts that the result object is of a (known) directory cap type. If the CLI code had called from_string instead of from_string_dirnode it would not have this problem.

[zooko]

Now what about backward-compatibility? Could we release a Tahoe-LAFS v1.9.1 so that if someone used Tahoe-LAFS v1.9.1 to create an MDMF alias and then switched back to using Tahoe-LAFS v1.6 (from Ubuntu Lucid), their CLI would continue to work? (Not, of course, that it would be able to use one of the MDMF aliases if they specified an MDMF alias on the command-line -- only that it would continue to be able to use other non-MDMF aliases, as well as to do things that require no alias such as tahoe mkdir.)

Hm... not really. I created those MDMF caps with tahoe mkdir or the WUI, and then I inserted them manually into ~/.tahoe/private/aliases using vi. There's not much the software could do to help with this. When we do eventually switch tahoe create-alias over to creating MDMF in addition to SDMF directories [*], we could at that time make it put the resulting MDMF caps into ~/.tahoe/private/aliases-mdmf instead of into ~/.tahoe/private/aliases. That would protect a user of that future version of Tahoe-LAFS from making their aliases file poisonous to their old Tahoe-LAFS v1.6 installation when they run tahoe create-alias.

[*] Aside: I really hope we eventually transition everything to all-MDMF-all-the-time and deprecate SDMF because two kinds of mutable things is a lot more complexity than one kind of mutable thing.

[zooko]

In sum:

I'm sure that we should change trunk to have forward-compatibility as described in comment:1.

If we're going to release a Tahoe-LAFS v1.9.1 anyway for other reasons 1.9.1, we might as well include this forward-compatibility feature in 1.9.1.

I don't think we should go to all the effort of releasing a 1.8.4 just for this. People downgrading their Tahoe-LAFS back to 1.8 will hopefully be very rare by the time more people start manually inserting MDMF caps into their aliases file (especially if we fix any regressions in 1.9).

I'm undecided about whether, when we make tahoe create-alias produce MDMF caps (at least optionally), we should have it put them into a separate alias file in order to avoid breaking old versions (< 1.9.1) that use the alias file.

[davidsarah]

For future reference, this should not have been an assert. Assert statements should be used only for consistency checks that would indicate a bug if they ever fail, not for validating input. That is, if an assertion failure can actually happen as a result of bad input, then an assert was the wrong way of expressing that constraint (because the resulting error reporting is usually ugly and provides insufficient information, and because asserts are ​sometimes switched off).

(Also note that even for consistency checks, we prefer the functions in src/allmydata/util/assertutil.py​.)

[warner]

I'm -1 on a 1.8.4, and +1 on including a forward-looking fix in a 1.9.1 (if it happens). I'm -1 on having a separate alias file: I think that's too much complexity (and long-term burden). I think folks are unlikely to downgrade their local install very much.

Do we have a patch?

davidsarah: I guess we need some other sort of cheap-to-write exception for code to express "Sorry, I can only handle a limited range of things, and you've just exceeded my abilities". I agree "assert" isn't the best choice for that, but I'd also like to avoid needing a huge amount of code (and tests) to express such things. Not sure what the best approach is.

[warner]

I consider this a nice-to-have for 1.9.1, but not a dependency. As RM, I'll release 1.9.1 without it (the other regressions are more important to get out). So somebody write a patch!

[davidsarah]

I will fix the issue in comment:1.

[warner]

Sounds good: the CLI code should refrain from asserting IDirectoryNode-ness during the initial load of the aliases file, but we should add a new IDirectoryNode-ness check on the particular alias that gets used in a directory-like context (to avoid sending an invalid URL to the WAPI).

We could talk about deferring this judgement to the node itself (sending any old thing in the URL, and letting the node decide whether it's a directory URI or not), which would tolerate version skew between the CLI and the node better, possibly at the expense of worse error messages.

If davidsarah doesn't fix something by this weekend, I'll give it a shot.

[warner]

patch to only check dirnode-ness of the aliases that get used

[warner]

Ok, that new version of the patch includes the necessary tests. Handing it off to Daira (or Zooko) for review.

[warner]

Timed out, landed it in 7edae210c67d04e0. Not sure why the bot didn't automatically close this ticket. Daira,Zooko, please do an after-the-fact review when you get a chance.

[daira]

Reviewing.

[daira]

I changed the commented-out #ga1(u"future:stuff") in test_alias_tolerance to self.failUnlessRaises(AssertionError, ga1, u"future:stuff"): d24f931accc94bf2

Otherwise +1.

[warner]

Ah, I was wondering whether that'd be ok or not. If someone runs the tests with -O, then assertions would be turned off, and that check would fail. So I've tended towards either creating a new exception type for things that we care deeply about, or deciding that we don't care deeply and not exercising AssertionErrors. If you're cool with having the tests depend on not--O, then I am too.

[daira]

My patch also replaced assert with _assert from allmydata.util.assertutil as necessary :-)

[daira]

I just checked with bin/tahoe @python -O @tahoe debug trial --rterror. There are three failing/erroring tests, but they're not related to this change. I'll open another bug for them.