Increase SFTP chunk size to increase the SFTP throughput in both directions #664
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
psf-chronographer
bot
added
the
bot:chronographer:provided
Merged
|
Congratulations! One of the builds has completed. 🍾 You can install the built RPMs by following these steps:
Please note that the RPMs should be used only in a testing environment. |
webknjaz
reviewed
Dec 20, 2024
webknjaz
reviewed
Dec 20, 2024
|
The |
|
|
@Jakuje When trying to build a wheel/sdist with the combined changes in PR #638 and this one, I get the following error: Any ideas? |
|
@Jakuje You're right. I might have missed something while applying the patches. The PR works out great. I've verified it to significantly improve SFTP performance compared to the existing one. |
NilashishC
approved these changes
Jan 23, 2025
@Jakuje py3.10 too. One of the differences is that the rerun disables the coverage plugin. And the first run seems to be crashing in one of the I've restarted the CI to see if that happens again. But will probably have to look into what deps need to be pinned or upgraded. |
Urgh.. Need to upgrade that too. |
|
@NilashishC adding merge commits to topic branches causes foxtrot merges and inconveniences for contributors, it's best to select the rebase mode or let the contributor handle such updates unless they stated that it's okay to mess with their branch. Let's not be careless like this. |
|
@Jakuje this can be rebased now. |
Jakuje
force-pushed
the
sftp-chunk
branch
2 times, most recently
from
f20bf60 to
784c58a
Compare
|
The Fedora testing farm is broken too? From the short review, I do not see that it could be something I should have broken ... |
|
@Jakuje yeah, it was. I've just made a few more CI fixes and I think all the CIs should be functional again. Let's rebase once more. |
Signed-off-by: Jakub Jelen <jjelen@redhat.com>
Signed-off-by: Jakub Jelen <jjelen@redhat.com>
Signed-off-by: Jakub Jelen <jjelen@redhat.com>
|
|
@Jakuje looks like the tests still SEGFAULT flakily? https://github.com/ansible/pylibssh/actions/runs/13926542675/job/38972875024?pr=664#step:16:293 |
webknjaz
reviewed
Mar 19, 2025
| " does not match number of bytes [%s] written to local file [%s]" | ||
| " due to error [%s]" | ||
| % (file_data, remote_file, bytes_written, local_file, self._get_sftp_error_str())) | ||
| try: |
There was a problem hiding this comment.
I think it'd be better to convert the try-finally into a CM.
Something like
@contextlib.contextmanager
def allocated_buffer(file_size):
buffer_size = min(SFTP_MAX_CHUNK, file_size)
read_buffer = <char *>PyMem_Malloc(buffer_size)
if read_buffer is NULL:
raise LibsshSFTPException("Memory allocation error")
try:
yield read_buffer
finally:
if read_buffer is not NULL:
PyMem_Free(read_buffer)And then with open(local_file, 'wb') as f, allocated_buffer(file_size) as read_buffer: would allow you to dedent this entire block back and avoid excessive levels of nesting.
There was a problem hiding this comment.
Thanks for the suggestion. I think this is a good idea. But I was not able to make it working from cpython for some reason. Probably I was doing something wrong as when I do this, all the tests using the get() method start crashing (without any good trace to see where and why).
There was a problem hiding this comment.
Got it. Could you at least post those logs here for history?
There was a problem hiding this comment.
Can it be cython/cython#5963? It hasn't made it into any release AFAICS.
There was a problem hiding this comment.
Could be. The diff I used:
diff --git a/src/pylibsshext/sftp.pyx b/src/pylibsshext/sftp.pyx
index 7528eba..11f9306 100644
--- a/src/pylibsshext/sftp.pyx
+++ b/src/pylibsshext/sftp.pyx
@@ -15,6 +15,7 @@
# License along with this library; if not, see file LICENSE.rst in this
# repository.
+from contextlib import contextmanager
from posix.fcntl cimport O_CREAT, O_RDONLY, O_TRUNC, O_WRONLY
from cpython.bytes cimport PyBytes_AS_STRING
@@ -27,6 +28,19 @@ from pylibsshext.session cimport get_libssh_session
SFTP_MAX_CHUNK = 32_768 # 32kB
+@contextmanager
+def allocated_buffer(buffer_size):
+ read_buffer = <char *>PyMem_Malloc(buffer_size)
+ if read_buffer is NULL:
+ raise LibsshSFTPException("Memory allocation error")
+
+ try:
+ yield read_buffer
+ finally:
+ if read_buffer is not NULL:
+ PyMem_Free(read_buffer)
+
+
MSG_MAP = {
sftp.SSH_FX_OK: "No error",
sftp.SSH_FX_EOF: "End-of-file encountered",
@@ -85,7 +99,6 @@ cdef class SFTP:
def get(self, remote_file, local_file):
cdef sftp.sftp_file rf
- cdef char *read_buffer = NULL
cdef sftp.sftp_attributes attrs
remote_file_b = remote_file
@@ -102,32 +115,25 @@ cdef class SFTP:
if rf is NULL:
raise LibsshSFTPException("Opening remote file [%s] for read failed with error [%s]" % (remote_file, self._get_sftp_error_str()))
- try:
- with open(local_file, 'wb') as f:
- buffer_size = min(SFTP_MAX_CHUNK, file_size)
- read_buffer = <char *>PyMem_Malloc(buffer_size)
- if read_buffer is NULL:
- raise LibsshSFTPException("Memory allocation error")
-
- while True:
- file_data = sftp.sftp_read(rf, <void *>read_buffer, sizeof(char) * buffer_size)
- if file_data == 0:
- break
- elif file_data < 0:
- sftp.sftp_close(rf)
- raise LibsshSFTPException("Reading data from remote file [%s] failed with error [%s]"
- % (remote_file, self._get_sftp_error_str()))
-
- bytes_written = f.write(read_buffer[:file_data])
- if bytes_written and file_data != bytes_written:
- sftp.sftp_close(rf)
- raise LibsshSFTPException("Number of bytes [%s] read from remote file [%s]"
- " does not match number of bytes [%s] written to local file [%s]"
- " due to error [%s]"
- % (file_data, remote_file, bytes_written, local_file, self._get_sftp_error_str()))
- finally:
- if read_buffer is not NULL:
- PyMem_Free(read_buffer)
+ buffer_size = min(SFTP_MAX_CHUNK, file_size)
+ with open(local_file, 'wb') as f, allocated_buffer(buffer_size) as read_buffer:
+
+ while True:
+ read_length = sftp.sftp_read(rf, <void *>read_buffer, sizeof(char) * buffer_size)
+ if read_length == 0:
+ break
+ elif read_length < 0:
+ sftp.sftp_close(rf)
+ raise LibsshSFTPException("Reading data from remote file [%s] failed with error [%s]"
+ % (remote_file, self._get_sftp_error_str()))
+
+ bytes_written = f.write(read_buffer[:read_length])
+ if bytes_written and read_length != bytes_written:
+ sftp.sftp_close(rf)
+ raise LibsshSFTPException("Number of bytes [%s] read from remote file [%s]"
+ " does not match number of bytes [%s] written to local file [%s]"
+ " due to error [%s]"
+ % (read_length, remote_file, bytes_written, local_file, self._get_sftp_error_str()))
sftp.sftp_close(rf)
def close(self):
And the test results (for only one failing test -- the other get tests fail the same way):
$ tox -e just-pytest -- -s tests/unit/sftp_test.py::test_get_existing[large-payload]
.pkg: _optional_hooks> python /usr/lib/python3.13/site-packages/pyproject_api/_backend.py True pep517_backend.hooks
.pkg: get_requires_for_build_sdist> python /usr/lib/python3.13/site-packages/pyproject_api/_backend.py True pep517_backend.hooks
.pkg: get_requires_for_build_wheel> python /usr/lib/python3.13/site-packages/pyproject_api/_backend.py True pep517_backend.hooks
.pkg: prepare_metadata_for_build_wheel> python /usr/lib/python3.13/site-packages/pyproject_api/_backend.py True pep517_backend.hooks
.pkg: build_sdist> python /usr/lib/python3.13/site-packages/pyproject_api/_backend.py True pep517_backend.hooks
just-pytest: install_package> python -I -m pip install --force-reinstall --no-deps /home/jjelen/devel/pylibssh/.tox/.tmp/package/45/ansible_pylibssh-1.2.3.dev140+g2cbd1b7.tar.gz
just-pytest: commands[0]> .tox/just-pytest/bin/python -m pytest --color=yes --no-cov -s 'tests/unit/sftp_test.py::test_get_existing[large-payload]'
==================================================================================================================================================== test session starts ====================================================================================================================================================
platform linux -- Python 3.13.2, pytest-8.3.5, pluggy-1.5.0
cachedir: .tox/just-pytest/.pytest_cache
rootdir: /home/jjelen/devel/pylibssh
configfile: pytest.ini
plugins: cov-6.0.0, xdist-3.6.1, forked-1.6.0
16 workers [1 item]
debug2: load_server_config: filename /dev/null
debug2: load_server_config: done config len = 1
debug2: parse_server_config_depth: config /dev/null len 1
debug1: sshd version OpenSSH_9.9, OpenSSL 3.2.4 11 Feb 2025
debug1: private host key #0: ssh-ed25519 SHA256:Xiy9ToCvuDrDWWfaaDLnzChYEmwbXWrJeDzDvxGrF5A
debug1: setgroups() failed: Operation not permitted
debug1: rexec_argv[1]='-D'
debug1: rexec_argv[2]='-f'
debug1: rexec_argv[3]='/dev/null'
debug1: rexec_argv[4]='-o'
debug1: rexec_argv[5]='LogLevel=DEBUG3'
debug1: rexec_argv[6]='-o'
debug1: rexec_argv[7]='HostKey=/tmp/pytest-of-jjelen/pytest-14/popen-gw0/test_get_existing_large_payloa0/sshd/ssh_host_rsa_key'
debug1: rexec_argv[8]='-o'
debug1: rexec_argv[9]='PidFile=/tmp/pytest-of-jjelen/pytest-14/popen-gw0/test_get_existing_large_payloa0/sshd/sshd.pid'
debug1: rexec_argv[10]='-o'
debug1: rexec_argv[11]='UsePAM=yes'
debug1: rexec_argv[12]='-o'
debug1: rexec_argv[13]='PasswordAuthentication=no'
debug1: rexec_argv[14]='-o'
debug1: rexec_argv[15]='ChallengeResponseAuthentication=no'
debug1: rexec_argv[16]='-o'
debug1: rexec_argv[17]='GSSAPIAuthentication=no'
debug1: rexec_argv[18]='-o'
debug1: rexec_argv[19]='StrictModes=no'
debug1: rexec_argv[20]='-o'
debug1: rexec_argv[21]='PermitEmptyPasswords=yes'
debug1: rexec_argv[22]='-o'
debug1: rexec_argv[23]='PermitRootLogin=yes'
debug1: rexec_argv[24]='-o'
debug1: rexec_argv[25]='Protocol=2'
debug1: rexec_argv[26]='-o'
debug1: rexec_argv[27]='HostbasedAuthentication=no'
debug1: rexec_argv[28]='-o'
debug1: rexec_argv[29]='IgnoreUserKnownHosts=yes'
debug1: rexec_argv[30]='-o'
debug1: rexec_argv[31]='Port=37369'
debug1: rexec_argv[32]='-o'
debug1: rexec_argv[33]='ListenAddress=127.0.0.1'
debug1: rexec_argv[34]='-o'
debug1: rexec_argv[35]='AuthorizedKeysFile=/tmp/pytest-of-jjelen/pytest-14/popen-gw0/test_get_existing_large_payloa0/sshd/authorized_keys'
debug1: rexec_argv[36]='-o'
debug1: rexec_argv[37]='AcceptEnv=LANG LC_*'
debug1: rexec_argv[38]='-o'
debug1: rexec_argv[39]='Subsystem=sftp internal-sftp'
debug3: using /usr/libexec/openssh/sshd-session for re-exec
debug1: sshd version OpenSSH_9.9, OpenSSL 3.2.4 11 Feb 2025
debug3: recv_rexec_state: entering fd = 5
debug3: ssh_msg_recv entering
debug2: parse_hostkeys: privkey 0: ssh-ed25519
debug2: parse_hostkeys: pubkey 0: ssh-ed25519
debug3: recv_rexec_state: done
debug2: parse_server_config_depth: config rexec len 1
Warning: Permanently added '[127.0.0.1]:37369' (ED25519) to the list of known hosts.
debug1: sshd version OpenSSH_9.9, OpenSSL 3.2.4 11 Feb 2025
debug3: recv_rexec_state: entering fd = 5
debug3: ssh_msg_recv entering
debug2: parse_hostkeys: privkey 0: ssh-ed25519
debug2: parse_hostkeys: pubkey 0: ssh-ed25519
debug3: recv_rexec_state: done
debug2: parse_server_config_depth: config rexec len 1
Fatal Python error: Segmentation fault
Thread 0x00007fe86dba96c0 (most recent call first):
File "/home/jjelen/devel/pylibssh/.tox/just-pytest/lib/python3.13/site-packages/execnet/gateway_base.py", line 534 in read
File "/home/jjelen/devel/pylibssh/.tox/just-pytest/lib/python3.13/site-packages/execnet/gateway_base.py", line 567 in from_io
File "/home/jjelen/devel/pylibssh/.tox/just-pytest/lib/python3.13/site-packages/execnet/gateway_base.py", line 1160 in _thread_receiver
File "/home/jjelen/devel/pylibssh/.tox/just-pytest/lib/python3.13/site-packages/execnet/gateway_base.py", line 341 in run
File "/home/jjelen/devel/pylibssh/.tox/just-pytest/lib/python3.13/site-packages/execnet/gateway_base.py", line 411 in _perform_spawn
Current thread 0x00007fe87cc5ab80 (most recent call first):
File "/home/jjelen/devel/pylibssh/tests/unit/sftp_test.py", line 100 in test_get_existing
File "/home/jjelen/devel/pylibssh/.tox/just-pytest/lib/python3.13/site-packages/_pytest/python.py", line 159 in pytest_pyfunc_call
File "/home/jjelen/devel/pylibssh/.tox/just-pytest/lib/python3.13/site-packages/pluggy/_callers.py", line 103 in _multicall
File "/home/jjelen/devel/pylibssh/.tox/just-pytest/lib/python3.13/site-packages/pluggy/_manager.py", line 120 in _hookexec
File "/home/jjelen/devel/pylibssh/.tox/just-pytest/lib/python3.13/site-packages/pluggy/_hooks.py", line 513 in __call__
File "/home/jjelen/devel/pylibssh/.tox/just-pytest/lib/python3.13/site-packages/_pytest/python.py", line 1627 in runtest
File "/home/jjelen/devel/pylibssh/.tox/just-pytest/lib/python3.13/site-packages/_pytest/runner.py", line 174 in pytest_runtest_call
File "/home/jjelen/devel/pylibssh/.tox/just-pytest/lib/python3.13/site-packages/pluggy/_callers.py", line 103 in _multicall
File "/home/jjelen/devel/pylibssh/.tox/just-pytest/lib/python3.13/site-packages/pluggy/_manager.py", line 120 in _hookexec
File "/home/jjelen/devel/pylibssh/.tox/just-pytest/lib/python3.13/site-packages/pluggy/_hooks.py", line 513 in __call__
File "/home/jjelen/devel/pylibssh/.tox/just-pytest/lib/python3.13/site-packages/_pytest/runner.py", line 242 in <lambda>
File "/home/jjelen/devel/pylibssh/.tox/just-pytest/lib/python3.13/site-packages/_pytest/runner.py", line 341 in from_call
File "/home/jjelen/devel/pylibssh/.tox/just-pytest/lib/python3.13/site-packages/_pytest/runner.py", line 241 in call_and_report
File "/home/jjelen/devel/pylibssh/.tox/just-pytest/lib/python3.13/site-packages/_pytest/runner.py", line 132 in runtestprotocol
File "/home/jjelen/devel/pylibssh/.tox/just-pytest/lib/python3.13/site-packages/_pytest/runner.py", line 113 in pytest_runtest_protocol
File "/home/jjelen/devel/pylibssh/.tox/just-pytest/lib/python3.13/site-packages/pluggy/_callers.py", line 103 in _multicall
File "/home/jjelen/devel/pylibssh/.tox/just-pytest/lib/python3.13/site-packages/pluggy/_manager.py", line 120 in _hookexec
File "/home/jjelen/devel/pylibssh/.tox/just-pytest/lib/python3.13/site-packages/pluggy/_hooks.py", line 513 in __call__
File "/home/jjelen/devel/pylibssh/.tox/just-pytest/lib/python3.13/site-packages/xdist/remote.py", line 195 in run_one_test
File "/home/jjelen/devel/pylibssh/.tox/just-pytest/lib/python3.13/site-packages/xdist/remote.py", line 174 in pytest_runtestloop
File "/home/jjelen/devel/pylibssh/.tox/just-pytest/lib/python3.13/site-packages/pluggy/_callers.py", line 103 in _multicall
File "/home/jjelen/devel/pylibssh/.tox/just-pytest/lib/python3.13/site-packages/pluggy/_manager.py", line 120 in _hookexec
File "/home/jjelen/devel/pylibssh/.tox/just-pytest/lib/python3.13/site-packages/pluggy/_hooks.py", line 513 in __call__
File "/home/jjelen/devel/pylibssh/.tox/just-pytest/lib/python3.13/site-packages/_pytest/main.py", line 337 in _main
File "/home/jjelen/devel/pylibssh/.tox/just-pytest/lib/python3.13/site-packages/_pytest/main.py", line 283 in wrap_session
File "/home/jjelen/devel/pylibssh/.tox/just-pytest/lib/python3.13/site-packages/_pytest/main.py", line 330 in pytest_cmdline_main
File "/home/jjelen/devel/pylibssh/.tox/just-pytest/lib/python3.13/site-packages/pluggy/_callers.py", line 103 in _multicall
File "/home/jjelen/devel/pylibssh/.tox/just-pytest/lib/python3.13/site-packages/pluggy/_manager.py", line 120 in _hookexec
File "/home/jjelen/devel/pylibssh/.tox/just-pytest/lib/python3.13/site-packages/pluggy/_hooks.py", line 513 in __call__
File "/home/jjelen/devel/pylibssh/.tox/just-pytest/lib/python3.13/site-packages/xdist/remote.py", line 393 in <module>
File "/home/jjelen/devel/pylibssh/.tox/just-pytest/lib/python3.13/site-packages/execnet/gateway_base.py", line 1291 in executetask
File "/home/jjelen/devel/pylibssh/.tox/just-pytest/lib/python3.13/site-packages/execnet/gateway_base.py", line 341 in run
File "/home/jjelen/devel/pylibssh/.tox/just-pytest/lib/python3.13/site-packages/execnet/gateway_base.py", line 411 in _perform_spawn
File "/home/jjelen/devel/pylibssh/.tox/just-pytest/lib/python3.13/site-packages/execnet/gateway_base.py", line 389 in integrate_as_primary_thread
File "/home/jjelen/devel/pylibssh/.tox/just-pytest/lib/python3.13/site-packages/execnet/gateway_base.py", line 1273 in serve
File "/home/jjelen/devel/pylibssh/.tox/just-pytest/lib/python3.13/site-packages/execnet/gateway_base.py", line 1806 in serve
File "<string>", line 8 in <module>
File "<string>", line 1 in <module>
Extension modules: pylibsshext._libssh_version, pylibsshext.errors, pylibsshext.channel, pylibsshext.scp, pylibsshext.sftp, pylibsshext.session (total: 6)
[gw0] node down: Not properly terminated
F
replacing crashed worker gw0
collecting: 16/17 workers
========================================================================================================================================================= FAILURES ==========================================================================================================================================================
__________________________________________________________________________________________________________________________________________________ tests/unit/sftp_test.py __________________________________________________________________________________________________________________________________________________
[gw0] linux -- Python 3.13.2 /home/jjelen/devel/pylibssh/.tox/just-pytest/bin/python
worker 'gw0' crashed while running 'tests/unit/sftp_test.py::test_get_existing[large-payload]'
--------------------------------------------------------------------------------------------------------------------- generated xml file: /home/jjelen/devel/pylibssh/.test-results/pytest/results.xml ----------------------------------------------------------------------------------------------------------------------
=================================================================================================================================================== slowest 10 durations ====================================================================================================================================================
3.54s setup tests/unit/sftp_test.py::test_get_existing[large-payload]
(1 durations < 0.005s hidden. Use -vv to show these durations.)
================================================================================================================================================== short test summary info ==================================================================================================================================================
FAILED tests/unit/sftp_test.py::test_get_existing[large-payload]
===================================================================================================================================================== 1 failed in 4.65s =====================================================================================================================================================
just-pytest: exit 1 (4.83 seconds) /home/jjelen/devel/pylibssh> .tox/just-pytest/bin/python -m pytest --color=yes --no-cov -s 'tests/unit/sftp_test.py::test_get_existing[large-payload]' pid=1424782
.pkg: _exit> python /usr/lib/python3.13/site-packages/pyproject_api/_backend.py True pep517_backend.hooks
just-pytest: FAIL code 1 (11.10=setup[6.27]+cmd[4.83] seconds)
evaluation failed :( (11.14 seconds)
The segfault is a known issue, which should be fixed with #658, which I just rebased on top of master. Will see how it will go. I think the failure is likely related to the segfault, which might have brought the server down for other proces or something async. Rerun with max verbosity is green. |
|
#658 seem to bring new failures so it's not fully ready, but I've restarted the failing jobs here to see how flaky it is. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
🚀 New features to boost your workflow:
|
|
I'm going to force-merge this, since checks that fail are known to be unrelated. |
github-project-automation
bot
moved this from 🧐 @webknjaz's review queue 📋
to 🌈 Done 🦄
in 📅 Procrastinating in public
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
SUMMARY
This is built on top of #638 so it depends on that change
Previously, the 1024b chunk was used, which resulted in large traffic overhead as each of the chunk needs to be wrapped in SFTP packet, as well as large overhead on high-latency links as the SFTP is now synchronous, waiting for every packet confirmation.
The libssh documentation recommends to use 16kB chunks, but using 32kB chunks should be safe as it still fits to the recommended limits in the SFTP specification. Using large reads could cause issues such as curl/curl#11804
ISSUE TYPE