'tahoe cp' copying to a mutable file should replace the contents

[davidsarah]

When tahoe cp copies onto an existing mutable file (in a writable directory), it replaces the directory entry. This is not consistent with SFTP, which would replace the contents of the file. Replacing the contents is more similar to a conventional filesystem, and I think more likely to be expected by users who are using mutable files.

Notice that TahoeFileTarget in src/allmydata/scripts/tahoe_cp.py@4768#L184 never uses the mutable variable passed into its constructor.

Although the comment there mentions #835, this is not the same bug. #835 is about adding options to allow specifying whether directories are copied as mutable or immuutable.

[davidsarah]

There's also a case for making tahoe cp pay attention to the no-write flag introduced in #1063. It's only a fairly weak case though, because that flag was added for the benefit of clients who are expecting permissions semantics as close as possible to a POSIX filesystem (e.g. clients using sshfs), and users of tahoe cp are probably not expecting that.

[gdt]

I would expect copy with an existing target to write a new file in the same directory with a temporary name and then call the rename() system call. rename guarantees that either the old target or the new will still exist, even during a system crash (obviously those are local semantics). However, I see that NetBSD cp opens the file for writing. mv behaves as I expect, and I am pretty sure rsync does too.

[kevan]

I've been working on this, and I hope to have it fixed by 1.9.

[kevan]

In the weekly phonecall of two weeks ago (I think), I mentioned that I thought this was a bug in the webapi. That is wrong -- I misunderstood how tahoe cp assigns sources to targets.

I think that the fact that tahoe cp will replace a mutable file with an immutable file is pretty unintuitive, and that's what it does now. Replacing the old contents with new contents also helps tahoe cp preserve use cases that rely on mutable files in directories having consistent caps after updates.

How do we want to behave when tahoe cp's authority is inadequate to replace the contents of the target? I like failure in that case; I don't like the idea of falling back to directory modification from content replacement for mutable files, since they're pretty different behaviors.

I've attached a patch. I'll set 'review-needed' once we've decided what to do when the target mutable file isn't writable and I've written a test for that.

[davidsarah]

Replying to kevan:

How do we want to behave when tahoe cp's authority is inadequate to replace the contents of the target? I like failure in that case; I don't like the idea of falling back to directory modification from content replacement for mutable files, since they're pretty different behaviors.

+1, that case should be an error.

I've attached a patch. I'll set 'review-needed' once we've decided what to do when the target mutable file isn't writable and I've written a test for that.

I'll review what you have so far.

[davidsarah]

Excellent tests! Very comprehensive. Now this just needs the test for when the target file isn't writeable.

[kevan]

Thanks for the review, davidsarah. I've attached another patch with a new test.

The error message that we test for is a webapi error message. That seems a bit lazy -- it didn't require any code changes, for example -- and it's not the most informative error message I've ever seen. The only way I've thought of to do better is to teach tahoe cp to inspect the error response from the webapi if it gets one so it can append an explanation to the output, and I'm not sure I like the idea of coupling tahoe cp to the exception messages printed by the webapi in that way.

(another initially attractive option is a sanity check performed on tahoe cp's targetmap before copying starts. Based on this, we could either abort the copy operation or print a warning if we found a readonly mutable target. All of that would rely upon JSON gathered from a different HTTP request than the request(s) actually modifying data, though, so any check would be inherently unreliable. Given that, I don't think I'd want to stop the copy operation if a readonly target was found, since I wouldn't want users to get the idea that we could do that reliably and I wouldn't want to annoy users if that fact got in their way, but I'm not as against printing a warning and then continuing, though at that point we might as well just figure out a way to write a better error message after the fact, since that solves the problem in more cases)

[kevan]

added error test

[davidsarah]

Typo in comment: "# Store the URIs for later user." -> "later use".

When running the tests I get:

FAIL]: allmydata.test.test_cli.Cp.test_cp_overwrite_readonly_mutable_file

Traceback (most recent call last):
  File "/home/davidsarah/tahoe/trunk/src/allmydata/test/test_cli.py", line 2150, in _check_error_message
    self.failUnlessIn("need write capability to publish", err)
twisted.trial.unittest.FailTest: 'need write capability to publish' not in 'Error during PUT: 400 Bad Request\nPUT to a mutable file: replace or update requested with read-only cap\n'

The 400 response has a reasonable error message -- is it just the test that needs changing, or did you get a different message?

[davidsarah]

Replying to kevan:

(another initially attractive option is a sanity check performed on tahoe cp's targetmap before copying starts. Based on this, we could either abort the copy operation or print a warning if we found a readonly mutable target. All of that would rely upon JSON gathered from a different HTTP request than the request(s) actually modifying data, though, so any check would be inherently unreliable.

Yes, that's a general problem with trying to make distributed operations have all-or-nothing semantics. Anyway, even if this could be improved, we don't need to do so for 1.9.

[kevan]

I got a different error message:

Error during PUT: 500 Internal Server Error
[...]
exceptions.AssertionError: need write capability to publish

Are you testing with my #393 patches applied? It looks like the message you see is something that I added in #393, but which isn't in trunk yet. I like the #393 400 more than the 500 error, though, and we'll have to change the test anyway once #393 is landed. I could change it in my patch now and we could land #1304 once #393 is landed so there isn't a spurious test failure. Alternatively, we could leave the patch as-is, land it now, and then change the test once #393 is landed.

Thanks for spotting the typo; I'll upload a fresh patch.

[kevan]

fixed typo

[davidsarah]

Replying to kevan:

I got a different error message:

Error during PUT: 500 Internal Server Error
[...]
exceptions.AssertionError: need write capability to publish

Are you testing with my #393 patches applied?

Yes, that was the problem.

Alternatively, we could leave the patch as-is, land it now, and then change the test once #393 is landed.

+1.

[davidsarah]

Fixed in 448278e807e489e4.

[davidsarah]

This seems to have caused a test failure on Windows: ​http://tahoe-lafs.org/buildbot/builders/FreeStorm%20WinXP-x86%20py2.6/builds/539/steps/test/logs/stdio

It seems that a command line URI argument is being passed as Unicode when it should be a str. I can't immediately see why it only fails on Windows.

[davidsarah]

Oh, I see. simplejson.loads doesn't consistently return the same type, it returns either unicode or str depending on its version and the phase of the moon. For example,

self.rw_uri = data["rw_uri"]

should be

self.rw_uri = to_str(data["rw_uri"])

where to_str is defined in src/allmydata/util/encodingutil.py​ (similarly for all the URIs read from JSON).

[david-sarah@…]

In 16fd14a78ad86536:

test_cli.py: use to_str on fields loaded using simplejson.loads in new tests. refs #1304