plugin: sftp transport

[John A Meinel]

Robey Pointer wrote:
> I wish there were more hours in the day to do all the things I want  to
> do. :)
>
> I updated the SFTP plugin and put it in a bzr branch here:
>     http://www.lag.net/~robey/bzr-plugins/sftp/
>
>
> On 18 Oct 2005, at 14:35, John A Meinel wrote:
>
>> [Robey wrote:]
>>
>>> It requires at least verison 1.4 of paramiko, though 1.5 is probably
>>> better:
>>>      http://www.lag.net/paramiko/
>>>
>>
>> I know there has been some discussion (and I believe it was  decided)
>> about
>> using Twisted, which also provides sftp support. I'm guessing what
>> you've
>> done can probably be adapted for it. But I know one reason I wasn't
>> persuing it heavily, is that I was waiting for Twisted to be  integrated
>> first.
>
>
> A large part of it could probably be salvaged if we switch to  Twisted,
> but I won't hold my breath for that.  Twisted is an event- based
> framework, so it would mean changing a lot of the flow of bzr  to suit
> twisted.  (This is one reason I prefer synchronous code over
> event-based: Simpler is better.)
>

Simpler, but not without loss of functionality. And it looks like we
need to go async for performance reasons when downloading lots of code,
and to enable nice responsive guis to be built. I'm only about 60%
convinced, but it seems to be a general consensus that it is the least
evil thing. :)

>
>>> I implemented the write support, too, but did NOT test it yet
>>
>>
>> Did you see the transport test suite, available in
>>  bzrlib/selftest/testtransport.py?
>
>
> I spent last night's and this morning's train rides playing with  that.
> Very cool.

I'm glad you like it. I'm was hoping it was written such that lots of
transports could be tested. It seemed decent for me. It also makes it
easier as more portions of the API are tested (we need to test the
clone() functionality for instance).

>
> I've added a stub (loopback) SFTP server to the plugin for unit  tests,
> and hooked it up using the TestTransportMixIn.  Two tests  fail, and
> they appear to be due to an impedance mismatch, so I'm not  sure how to
> solve them yet: two tests are expecting a NoSuchFile  exception to be
> raised, but get SFTPTransportError raised instead.
>
> The problem is that paramiko is trying to emulate python's exception
> handling (converting SFTP errors into python exceptions), so it loses
> some information.  There's an SFTP error code for NO_SUCH_FILE which
> could map directly to the bzr NoSuchFile exception, but it's  converted
> into a generic IOError on the way out.

You could always just trap the IOError. That is what the LocalTransport
does. Look for IOError.errno==errno.ENOENT

>
> The solution I'm leaning toward is to have paramiko define its own
> NoSuchFile exception, subclassed from IOError, and let the
> SFTPTransport plugin catch that and translate it to bzr's  NoSuchFile.
> That would require upgrading paramiko but that's  probably not a serious
> issue.
>
> In any case, the tests all seem to be "functionally" passing, aside
> from the exception mismatch.  I set the test to NOT be read-only,  which
> improves my faith that this should work as a writable branch too.
>
>
>>> Read and write locks aren't supported, just like with HTTP.  There's
>>> probably something clever I can do there, assuming we only care about
>>> read/write locking against other bzr-over-sftp instances, but I
>>> haven't put much thought into it.
>>
>>
>> Read locks would be nice, because it would let us cache things  locally,
>> and guarantee that they aren't changing remotely. Write locks would be
>> nice when we go to support "bzr push" to a remote branch. (and when
>> Aaron's changes for remote bound-branches is implemented).
>>
>> I know the SFTP v6 spec supported all sorts of nice things, but I
>> believe
>> everyone is using v3, and I don't know what that does and doesn't
>> implement.
>
>
> This is kind of a general problem, which I believe we should solve  for
> all transports.  Basically there are tricks you can do even  across a
> "dumb" filesystem to do locking -- arch did these and we may  be able to
> steal from them.  As Jan Hudec points out in a different  thread, it
> would be best if we could even support locking via one  transport and
> seeing the lock via another.

We really need put() to at least be semi-atomic, though. So that you
upload the file to a temp file, and then rename it into place. That is
pretty important to me. Otherwise a disconnect in the middle breaks
everything.

>
> As far as the SFTP implemented in the wild, there's no file locking,
> but there is an equivalent to "O_CREAT|O_EXCL" so all hope is not lost.

Yeah. I don't really have a solution. Something like Tom's latest work
on a transactional remote system, where you just have some directories
that keep getting renamed to indicate whether you are locked or not. I
don't think we can change things such that
>
>> It would be nice to have a directory with an __init__.py that also
>> has the
>> test_suite(), so that running "bzr selftest" could also test the sftp
>> plugin.
>
>
> Yeah, I did that now.  I guess it was too optimistic to think I could
> keep it down to one file. :)

Actually, you could put everything into one file. The plugin loader just
looks for a "test_suite" function in the module that it loaded. And you
could also put the test in the same module.

I personally think it is cleaner to break things out, so that you don't
have to see all the test code when you are looking at the implementation.

Also, if you have a standalone "test_sftptransport.py" file, that can
simply be copied into bzrlib/selftest/ and __init__.py updated to look
for it, rather than copying and pasting the test code into another file.

In other words, it lets me write code as though it was part of the core
of bzr, and just have an __init__.py which handles the fact that it is a
plugin.

>
>>> Oh yeah, this is mostly just ported from my last version, except
>>> using the new Transport API (which has been cleaned up a lot --
>>> thanks).
>>
>>
>> I'm not really sure how the interface was cleaned up, perhaps I  moved
>> the
>> LocalTransport calls into a separate class. Or maybe I just missed
>> someone
>> else's work.
>
>
> I just meant compared to my last patch, which was before your  Transport
> work got in.  Using the old "store" interface to implement  new
> transports had a lot of redundancy.  This way is much better.
>
>> I've looked over the code (briefly), and generally it looks good.  I'm a
>> little curious why you have the global SYSTEM_HOST_KEYS loaded into
>> save_host_keys(), but you don't use it.
>
>
> Good point, too much cut-n-paste. :)
>
>> Also, I would think that in _sftp_connect() we would preferentially  use
>> "BZR_HOST_KEYS" over using SYSTEM_HOST_KEYS, since we are BZR, but  this
>> probably isn't a big deal.
>
>
> *shrug* Makes sense to me, so I swapped them.
>
>> I also noticed that you never disconnect from the remote host. Now, I
>> don't know if the disconnect automatically happens when the last
>> reference
>> to self._sftp goes away, which is perfectly fine.
>
>
> Yeah, it's using python __del__ magick so that SFTP file objects can  be
> casually discarded just like python's built-in file objects.  The  same
> applies up the chain to the (paramiko) Transport.
>
>> We might also consider having a weakref dictionary of remote hosts
>> that we
>> are currently connected to, and their connection object. I know that I
>> believe everything will be called through "clone()" but I suppose  it
>> would
>> be possible for a second connection to be attempted. A weakref
>> dictionary
>> lets you use the connection if it exists, but it also doesn't hang  on to
>> it (letting it disconnect) when all real references go away.
>
>
> It's a good idea.  I'll punt on that for now though, until I'm more
> convinced that the core of it is solid.
>
> Thanks for the comments!  Sorry this email got so long.

No problem, I send long emails too. :)

>
> robey

John
=:->

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 253 bytes
Desc: OpenPGP digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20051019/1f6e6de9/attachment.pgp