bzr shouldn't use ftp append when writing a whole file in one go

The high level transport API does reflect it; however the generic thunk
code, which is all that the ftp transport implements, uses append.

If there is a better facility, just hooking it in should be sufficient.

-Rob

2009/12/1 David Muir <email address hidden>:
> Not sure if this is bug or a limitation of Bazaar's ftp capabilities, but I
> just tried pushing a branch via ftp, and it crapped out saying:
>
> dmuir@dmuir-laptop:~/path/to/local/branch$ bzr push
> ftp://example.com/repos/branch
> FTP temporary error: 451
> /repos/branch/.bzr/repository/upload/b7heyf38dq944kqzfswj.pack:
> Append/Restart not permitted, try again. Retrying.
>
> FTP temporary error: 451
> /repos/branch/.bzr/repository/upload/b7heyf38dq944kqzfswj.pack:
> Append/Restart not permitted, try again. Retrying.
> FTP temporary error: 451
> /repos/branch/.bzr/repository/upload/b7heyf38dq944kqzfswj.pack:
> Append/Restart not permitted, try again. Retrying.
> bzr: ERROR: Transport error: FTP temporary error during APPEND
> /repos/branch/.bzr/repository/upload/b7heyf38dq944kqzfswj.pack.Aborting. 451
> /repos/branch/.bzr/repository/upload/b7heyf38dq944kqzfswj.pack:
> Append/Restart not permitted, try again
>
>
> Same thing happens when I try again.
>
> Any help would be much appreciated.

Hi,

This is <https://bugs.edge.launchpad.net/bzr/+bug/409615>. If you are
interested in fixing it, we can help you do a patch. It won't be two
lines but I don't expect it will be a huge change. If you just want a
workaround, I would suggest using sftp (or bzr+ssh) instead.

It might help if you post to that bug what ftp server software you're
using, if you know.

--
Martin <http://launchpad.net/~mbp/>

Just ran into this bug last night.
The server is running ProFTPD 1.3.2a Server.

2009/12/1 David Muir <email address hidden>:
> I can give it a shot, but I may need a bit of hand-holding :-)
>
> Where should I start looking in regards to fixing this bug?

So the way I think i would tackle it is: look (by setting -Derror or
in .bzr.log) at the traceback when this error occurs. This will tell
you some code that's trying to append to a file on the ftp server.
The belief is that this code doesn't really need to append to an
existing file (since current formats don't require that behaviour) and
it can easily be changed to just write a file. Post the traceback to
the bug report so others can help.

You may not actually straightforwardly get a traceback, in which case
you probably need to insert a

  import pdb;pdb.set_trace()

line to pop the debugger at that point. You could just put this into
ftp.py/ append. Then use 'bt' to get a traceback.

The code that's originally doing the append should be changed to use
open_write_stream().

hope that helps, don't hesitate to ask,

--
Martin <http://launchpad.net/~mbp/>

Martin Pool wrote:
> 2009/12/1 David Muir <email address hidden>:
>
>> I can give it a shot, but I may need a bit of hand-holding :-)
>>
>> Where should I start looking in regards to fixing this bug?
>>
>
> So the way I think i would tackle it is: look (by setting -Derror or
> in .bzr.log) at the traceback when this error occurs. This will tell
> you some code that's trying to append to a file on the ftp server.
> The belief is that this code doesn't really need to append to an
> existing file (since current formats don't require that behaviour) and
> it can easily be changed to just write a file. Post the traceback to
> the bug report so others can help.
>
> You may not actually straightforwardly get a traceback, in which case
> you probably need to insert a
>
> import pdb;pdb.set_trace()
>
> line to pop the debugger at that point. You could just put this into
> ftp.py/ append. Then use 'bt' to get a traceback.
>
> The code that's originally doing the append should be changed to use
> open_write_stream().
>
> hope that helps, don't hesitate to ask,
>
>

Got the traceback:

Traceback (most recent call last):
  File "/usr/lib/python2.6/dist-packages/bzrlib/commands.py", line 842,
in exception_to_return_code
    return the_callable(*args, **kwargs)
  File "/usr/lib/python2.6/dist-packages/bzrlib/commands.py", line 1037,
in run_bzr
    ret = run(*run_argv)
  File "/usr/lib/python2.6/dist-packages/bzrlib/commands.py", line 654,
in run_argv_aliases
    return self.run(**all_cmd_args)
  File "/usr/lib/python2.6/dist-packages/bzrlib/builtins.py", line 1156,
in run
    use_existing_dir=use_existing_dir)
  File "/usr/lib/python2.6/dist-packages/bzrlib/push.py", line 128, in
_show_push_branch
    remember, create_prefix)
  File "/usr/lib/python2.6/dist-packages/bzrlib/bzrdir.py", line 1263,
in push_branch
    repository_to.fetch(source.repository, revision_id=revision_id)
  File "/usr/lib/python2.6/dist-packages/bzrlib/repository.py", line
1695, in fetch
    find_ghosts=find_ghosts, fetch_spec=fetch_spec)
  File "/usr/lib/python2.6/dist-packages/bzrlib/decorators.py", line
192, in write_locked
    result = unbound(self, *args, **kwargs)
  File "/usr/lib/python2.6/dist-packages/bzrlib/repository.py", line
3413, in fetch
    pb=pb, find_ghosts=find_ghosts)
  File "/usr/lib/python2.6/dist-packages/bzrlib/fetch.py", line 81, in
__init__
    self.__fetch()
  File "/usr/lib/python2.6/dist-packages/bzrlib/fetch.py", line 107, in
__fetch
    self._fetch_everything_for_search(search)
  File "/usr/lib/python2.6/dist-packages/bzrlib/fetch.py", line 135, in
_fetch_everything_for_search
    stream, from_format, [])
  File "/usr/lib/python2.6/dist-packages/bzrlib/repository.py", line
4234, in insert_stream
    self.target_repo.start_write_group()
  File "/usr/lib/python2.6/dist-packages/bzrlib/repository.py", line
1770, in start_write_group
    self._start_write_group()
  File "/usr/lib/python2.6/dist-packages/bzrlib/repofmt/pack_repo.py",
line 2265, in _start_write_group
    self._pack_collection._start_write_group()
  File "/usr/lib/python2.6/dist-packages/bzrlib/repofmt/pack_repo.py",
line 2021, in _start_write_group
    file_mode...

2009/12/1 David Muir <email address hidden>:
> I'm a little confused by terminology... and Python in general...
> IIUC, the transport assumes append based file streams (because of previous
> limitations?). So even though the method is called append_file, having it
> put the file instead is ok?

What's supposed to be happening here is that we are streaming out from
the groupcompress level, which is going to write one long file in a
series of chunks. The base class implementation in transport.py and
AppendBasedFileStream changes them into a sequence of
transport.append_bytes calls. However, since some servers don't
support append, we need to make sure that by the time it gets to the
ftp level we just open the file once and write one long stream.

By the way for openftpd I think there is a configuration option to
allow appending. But patches to avoid needing to set it would still
be great.

--
Martin <http://launchpad.net/~mbp/>

Martin Pool wrote:
> 2009/12/1 David Muir <email address hidden>:
>
>> I'm a little confused by terminology... and Python in general...
>> IIUC, the transport assumes append based file streams (because of previous
>> limitations?). So even though the method is called append_file, having it
>> put the file instead is ok?
>>
>
> What's supposed to be happening here is that we are streaming out from
> the groupcompress level, which is going to write one long file in a
> series of chunks. The base class implementation in transport.py and
> AppendBasedFileStream changes them into a sequence of
> transport.append_bytes calls. However, since some servers don't
> support append, we need to make sure that by the time it gets to the
> ftp level we just open the file once and write one long stream.
>
> By the way for openftpd I think there is a configuration option to
> allow appending. But patches to avoid needing to set it would still
> be great.
>
>
Ok, I was able to get it working by skipping the if self._has_append
part, and just using self._fallback_append()
Probably not the ideal solution, but it worked :-)

Let me know if I'm on the right track and how it can be improved.

=== modified file 'bzrlib/transport/ftp/__init__.py'
--- bzrlib/transport/ftp/__init__.py 2009-10-06 08:24:14 +0000
+++ bzrlib/transport/ftp/__init__.py 2009-12-02 01:48:45 +0000
@@ -397,12 +397,12 @@
             result = ftp.size(abspath)
         else:
             result = 0
-
- if self._has_append:
- mutter("FTP appe to %s", abspath)
- self._try_append(relpath, text, mode)
- else:
- self._fallback_append(relpath, text, mode)
+ #is this if/else required anymore?
+ #if self._has_append:
+ # mutter("FTP appe to %s", abspath)
+ # self._try_append(relpath, text, mode)
+ #else:
+ self._fallback_append(relpath, text, mode)

         return result

Definitely getting warm.

However, this specific patch will make things slower than they need to be, because _fallback_append reads the whole file and then writes it back with the data appended. This could be awfully slow.

The smaller correct patch along these lines would be to update the code around line 426 that falls back to doing this, so that it also detects the response sent by your server.

A much better approach and eventually much faster would be to override open_write_stream for the FTPTransport class, so that it returns an object that when started opens a stream, then it sends each block across the connection as it is written in. You can base this on the code in put_file - in fact put_file could eventually be rewritten to stand on top of this.

That would be a big improvement for ftp users. I'm happy to look at an updated patch or to talk to you on irc.

>>>>> "DM" == David Muir <email address hidden> writes:

<snip/>

    DM> Let me know if I'm on the right track and how it can be improved.

AIUI, you are. Have a look at _try_append and may be add 451 to
the reply codes that should be considered.

    Vincent

This bug also affects me

bzr --version
Bazaar (bzr) 2.3b1
  Python interpreter: /usr/bin/python 2.6.5
  Python standard library: /usr/lib/python2.6
  Platform: Linux-2.6.32-25-generic-x86_64-with-Ubuntu-10.04-lucid
  bzrlib: /usr/lib/python2.6/dist-packages/bzrlib

The error that return the ftp server is 451
and the bzrlib ftp layer cannot detect it

the file attached is my .bzr-log (i removed ftp address and password and my home address for security....)

the relevant lines are

9.816 FTP stat: /htdocs/.bzr/repository
10.045 FTP get: /htdocs/.bzr/repository/pack-names
10.333 FTP rm: /htdocs/.bzr/repository/no-working-trees
10.393 FTP get: /htdocs/.bzr/repository/pack-names
10.693 Using fetch logic to copy between CHKInventoryRepository('file://~/www-data/.bzr/repository/')(RepositoryFormat2a()) and CHKInventoryRepository('ftp://ftp.ciccio.com/htdocs/.bzr/repository/')(RepositoryFormat2a())
10.695 FTP put: /htdocs/.bzr/repository/upload/z1lkk39vtt2ohonyndb1.pack
11.164 FTP has check: z1lkk39vtt2ohonyndb1.pack => /htdocs/.bzr/repository/upload/z1lkk39vtt2ohonyndb1.pack
11.221 FTP has: /htdocs/.bzr/repository/upload/z1lkk39vtt2ohonyndb1.pack
11.278 FTP appe to /htdocs/.bzr/repository/upload/z1lkk39vtt2ohonyndb1.pack
11.279 FTP appe (try 0) to /htdocs/.bzr/repository/upload/z1lkk39vtt2ohonyndb1.pack
[ 4808] 2010-10-12 06:33:27.985 WARNING: FTP temporary error: 451 /htdocs/.bzr/repository/upload/z1lkk39vtt2ohonyndb1.pack: Append/Restart not permitted, try again. Retrying.
11.452 Constructing FTP instance against ('ftp.ciccio.com', None, '++++++++', '********', False)
11.863 FTP appe (try 1) to /htdocs/.bzr/repository/upload/z1lkk39vtt2ohonyndb1.pack
[ 4808] 2010-10-12 06:33:28.569 WARNING: FTP temporary error: 451 /htdocs/.bzr/repository/upload/z1lkk39vtt2ohonyndb1.pack: Append/Restart not permitted, try again. Retrying.
12.036 Constructing FTP instance against ('ftp.ciccio.com', None, '++++++++', '********', False)
12.449 FTP appe (try 2) to /htdocs/.bzr/repository/upload/z1lkk39vtt2ohonyndb1.pack
[ 4808] 2010-10-12 06:33:29.156 WARNING: FTP temporary error: 451 /htdocs/.bzr/repository/upload/z1lkk39vtt2ohonyndb1.pack: Append/Restart not permitted, try again. Retrying.
12.623 Constructing FTP instance against ('ftp.ciccio.com', None, '++++++++', '********', False)
13.048 FTP appe (try 3) to /htdocs/.bzr/repository/upload/z1lkk39vtt2ohonyndb1.pack
13.339 Traceback (most recent call last):
  File "/usr/lib/python2.6/dist-packages/bzrlib/commands.py", line 912, in exception_to_return_code
    return the_callable(*args, **kwargs)
  File "/usr/lib/python2.6/dist-packages/bzrlib/commands.py", line 1112, in run_bzr
    ret = run(*run_argv)
  File "/usr/lib/python2.6/dist-packages/bzrlib/commands.py", line 690, in run_argv_aliases
    return self.run(**all_cmd_args)
  File "/usr/lib/python2.6/dist-packages/bzrlib/commands.py", line 705, in run
    return self._operation.run_simple(*args, **kwargs)
  File "/usr/lib/python2.6/dist-packages/bzrlib/cleanup.py", line 135, in run_simple
    self.cleanups, self.func, *args, **kwargs)
  File "/usr/lib/python2.6/dist-packages/bzrlib/cleanup.py", line 165, in _do_with_cleanups
    result = func(...

This bug also affects me

bzr --version
Bazaar (bzr) 2.3b1
  Python interpreter: /usr/bin/python 2.6.5
  Python standard library: /usr/lib/python2.6
  Platform: Linux-2.6.32-25-generic-x86_64-with-Ubuntu-10.04-lucid
  bzrlib: /usr/lib/python2.6/dist-packages/bzrlib

The error that return the ftp server is 451
and the bzrlib ftp layer cannot detect it

the file attached is my .bzr-log (i removed ftp address and password and my home address for security....)

the relevant lines are

9.816 FTP stat: /htdocs/.bzr/repository
10.045 FTP get: /htdocs/.bzr/repository/pack-names
10.333 FTP rm: /htdocs/.bzr/repository/no-working-trees
10.393 FTP get: /htdocs/.bzr/repository/pack-names
10.693 Using fetch logic to copy between CHKInventoryRepository('file://~/www-data/.bzr/repository/')(RepositoryFormat2a()) and CHKInventoryRepository('ftp://ftp.ciccio.com/htdocs/.bzr/repository/')(RepositoryFormat2a())
10.695 FTP put: /htdocs/.bzr/repository/upload/z1lkk39vtt2ohonyndb1.pack
11.164 FTP has check: z1lkk39vtt2ohonyndb1.pack => /htdocs/.bzr/repository/upload/z1lkk39vtt2ohonyndb1.pack
11.221 FTP has: /htdocs/.bzr/repository/upload/z1lkk39vtt2ohonyndb1.pack
11.278 FTP appe to /htdocs/.bzr/repository/upload/z1lkk39vtt2ohonyndb1.pack
11.279 FTP appe (try 0) to /htdocs/.bzr/repository/upload/z1lkk39vtt2ohonyndb1.pack
[ 4808] 2010-10-12 06:33:27.985 WARNING: FTP temporary error: 451 /htdocs/.bzr/repository/upload/z1lkk39vtt2ohonyndb1.pack: Append/Restart not permitted, try again. Retrying.
11.452 Constructing FTP instance against ('ftp.ciccio.com', None, '++++++++', '********', False)
11.863 FTP appe (try 1) to /htdocs/.bzr/repository/upload/z1lkk39vtt2ohonyndb1.pack
[ 4808] 2010-10-12 06:33:28.569 WARNING: FTP temporary error: 451 /htdocs/.bzr/repository/upload/z1lkk39vtt2ohonyndb1.pack: Append/Restart not permitted, try again. Retrying.
12.036 Constructing FTP instance against ('ftp.ciccio.com', None, '++++++++', '********', False)
12.449 FTP appe (try 2) to /htdocs/.bzr/repository/upload/z1lkk39vtt2ohonyndb1.pack
[ 4808] 2010-10-12 06:33:29.156 WARNING: FTP temporary error: 451 /htdocs/.bzr/repository/upload/z1lkk39vtt2ohonyndb1.pack: Append/Restart not permitted, try again. Retrying.
12.623 Constructing FTP instance against ('ftp.ciccio.com', None, '++++++++', '********', False)
13.048 FTP appe (try 3) to /htdocs/.bzr/repository/upload/z1lkk39vtt2ohonyndb1.pack
13.339 Traceback (most recent call last):
  File "/usr/lib/python2.6/dist-packages/bzrlib/commands.py", line 912, in exception_to_return_code
    return the_callable(*args, **kwargs)
  File "/usr/lib/python2.6/dist-packages/bzrlib/commands.py", line 1112, in run_bzr
    ret = run(*run_argv)
  File "/usr/lib/python2.6/dist-packages/bzrlib/commands.py", line 690, in run_argv_aliases
    return self.run(**all_cmd_args)
  File "/usr/lib/python2.6/dist-packages/bzrlib/commands.py", line 705, in run
    return self._operation.run_simple(*args, **kwargs)
  File "/usr/lib/python2.6/dist-packages/bzrlib/cleanup.py", line 135, in run_simple
    self.cleanups, self.func, *args, **kwargs)
  File "/usr/lib/python2.6/dist-packages/bzrlib/cleanup.py", line 165, in _do_with_cleanups
    result = func(...