Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement assynchronous SFTP file transfer #641

Draft
wants to merge 7 commits into
base: devel
Choose a base branch
from

Conversation

Jakuje
Copy link
Contributor

@Jakuje Jakuje commented Sep 11, 2024

SUMMARY

The SFTP transfers done the way they work now are very slow especially on connections with large latency because each chunk (now 1024B) needs to travel all the way to remote as well as the confirmation (or request + data for the other transfer), before another chunk is sent. This can be improved by two things:

  • sending larger chunks (up to the size reported by SFTP limits extension)
  • sending more requests while waiting for the confirmation/data

This depends on #636 (libssh 0.11), but also on other platforms having the updated libssh. It will likely need some conditional compilation based on the libssh version, but I did not find a good solution for cython after it was deprecated (cython/cython#4310) so any help/suggestions welcomed.

This PR also reproduces an issue with the new API that it currently hangs waiting for the second message, while it already happily sits in the queue, which is being worked on upstream.

Therefore marking as a draft to collect initial inputs.

ISSUE TYPE
  • Feature Pull Request

@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Sep 11, 2024
@webknjaz
Copy link
Member

I applied some CI fixes and now this PR needs to be rebased to pick them up. I'm clicking the rebase button now.

@webknjaz webknjaz marked this pull request as ready for review September 12, 2024 13:10
@webknjaz webknjaz marked this pull request as draft September 12, 2024 13:10
@webknjaz webknjaz marked this pull request as draft September 12, 2024 13:10
@webknjaz webknjaz force-pushed the async-sftp branch 2 times, most recently from 3c22bba to fd42bb2 Compare September 12, 2024 13:35
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
2 Security Hotspots

See analysis details on SonarCloud

Signed-off-by: Jakub Jelen <[email protected]>
src/pylibsshext/sftp.pyx Outdated Show resolved Hide resolved
src/pylibsshext/sftp.pyx Outdated Show resolved Hide resolved
src/pylibsshext/sftp.pyx Outdated Show resolved Hide resolved
src/pylibsshext/sftp.pyx Outdated Show resolved Hide resolved
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
2 Security Hotspots

See analysis details on SonarCloud

Copy link

Congratulations! One of the builds has completed. 🍾

You can install the built RPMs by following these steps:

  • sudo yum install -y dnf-plugins-core on RHEL 8
  • sudo dnf install -y dnf-plugins-core on Fedora
  • dnf copr enable packit/ansible-pylibssh-641
  • And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.

@Jakuje
Copy link
Contributor Author

Jakuje commented Oct 24, 2024

The testing farm build are failing because the libssh 0.11 did not make it to the stable fedora versions.

@Jakuje Jakuje mentioned this pull request Nov 19, 2024
@NilashishC
Copy link
Contributor

Looks like v0.11.0 should be available now in F41? https://koji.fedoraproject.org/koji/buildinfo?buildID=2524005

@Jakuje
Copy link
Contributor Author

Jakuje commented Jan 13, 2025

Looks like v0.11.0 should be available now in F41? https://koji.fedoraproject.org/koji/buildinfo?buildID=2524005

Correct, but not in RHEL9 and other target architectures, which means this one still need some work to keep this code working also with the older libssh. Either with some conditional compilation or some other tricks...

@NilashishC
Copy link
Contributor

@Jakuje So this is what I understood. The async SFTP is only available in libssh >=0.11.0, and this PR updates the existing SFTP implementation in pylibssh to use the libssh async SFTP API. This means that if this PR is merged, the minimum required libssh version for pylibssh SFTP to work will be >=0.11.0.

Now, the challenge is that libssh >=0.11.0 is not yet available on all platforms (and target arch). Furthermore, it isn't clear to me if libssh >=0.11.0 will be made available on other RHEL versions (+target arch) that AAP currently supports OR if the async SFTP support will be backported to older libssh version(s), which would also require a bump in the min libssh version required by this binding.

An alternative that I can think of is inspect the libssh version and invoke the async implementation is we're running a libssh version that supports the async SFTP API and fallback to the existing implementation if not. That way, older pylibssh SFTP would still work but it'll be terribly slow like it has been so far.

Please let me know if what I said above is correct here.

Copy link

@Jakuje
Copy link
Contributor Author

Jakuje commented Jan 17, 2025

All correct. We do not plan to backport this API to older RHELs so if there is a way to implement some fallback, that would be preferred. Hints or pointers welcomed if you know how to approach. I am keeping this as a draft as this is started more like a proof of concept that the new API does what it is supposed to do.

To increase the SFTP throughput everywhere, the #664 should get you basically to the same performance for the most common read sizes (up until 32kB. But as you can see in #638, the large (>1kB) reads over SFTP were not working at all so right now, I would rather focus on these PRs. I think the changes there are ready to be merged (or at least reviewed).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot:chronographer:provided There is a change note present in this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants