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

Exception occurred when minion return large data. #53230

Closed
GwiYeong opened this issue May 24, 2019 · 6 comments · Fixed by #58283
Closed

Exception occurred when minion return large data. #53230

GwiYeong opened this issue May 24, 2019 · 6 comments · Fixed by #58283
Labels
Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged
Milestone

Comments

@GwiYeong
Copy link
Contributor

Description of Issue/Question

i reinstalled requirements/base.txt with pip -r when i upgrade my salt-master. after then Exception occurred while handling stream: 1048606 exceeds max_str_len(1048576) error is occured.
my previous version of saltstack is v2017.7.4 and upgrading to v2017.7.8.

i search the issue and then i find out that is msgpack version issue.
pip -r requirements/base.txt will install latest version of msgpack (0.6.1 currently). i find this issue. so i installed msgpack 0.5.6 manually after uninstalling latest version of msgpack and the error is gone. the problem is solved.

so how does saltstack solve this problem?
i thinks there is 2 options to fix it.

  1. fix requirements
  2. support current msgpack version

first option is simpler than second, so i can fix it myself and send pr(just update requirements).
but second one i can not fix it (it is so complicated).

So what do you think what should i do for?
could you guys fix it?

i need your opinion.

thanks in advance

Setup

  1. install saltstack 2017.7
  2. make large length file.
cat /dev/urandom | tr -dc 'a-zA-Z0-9' | fold -w 128 | head -n 8126 > ~/long_text_1 
cat /dev/urandom | tr -dc 'a-zA-Z0-9' | fold -w 128 | head -n 8127 > ~/long_text_2

Steps to Reproduce Issue

  1. send commend with salt-api with payload
{
	"client": "local",
	"tgt": ["salt-minion"],
	"tgt_type": "list",
	"fun": "cmd.run",
	"arg": ["cat ~/long_text_1"]
}
  • with msgpack > 0.5.6
  long_text_1 -> Success
  long_text_2 -> Exception occurred while handling stream: 1048646 exceeds 
 max_str_len(1048576) occured
  • with msgpack <= 0.5.6
  long_text_1 -> Success
  long_text_2 -> Success

Versions Report

salt-master & salt-minion (same host)

Salt Version:
           Salt: 2017.7.8-426-g464764a

Dependency Versions:
           cffi: Not Installed
       cherrypy: Not Installed
       dateutil: 2.6.0
      docker-py: Not Installed
          gitdb: 2.0.0
      gitpython: 2.1.1
          ioflo: Not Installed
         Jinja2: 2.9.5
        libgit2: Not Installed
        libnacl: Not Installed
       M2Crypto: Not Installed
           Mako: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.5.6
   mysql-python: Not Installed
      pycparser: Not Installed
       pycrypto: 2.6.1
   pycryptodome: Not Installed
         pygit2: Not Installed
         Python: 2.7.12 (default, Dec 23 2016, 09:50:41)
   python-gnupg: Not Installed
         PyYAML: 3.12
          PyZMQ: 17.1.2
           RAET: Not Installed
          smmap: 2.0.1
        timelib: Not Installed
        Tornado: 4.5.3
            ZMQ: 4.2.5

System Versions:
           dist: centos 6.5 Final
         locale: UTF-8
        machine: x86_64
        release: 2.6.32-696.3.2.el6.x86_64
         system: Linux
        version: CentOS 6.5 Final
@dmurphy18 dmurphy18 added the Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged label May 28, 2019
@dmurphy18 dmurphy18 added this to the Blocked milestone May 28, 2019
@dmurphy18
Copy link
Contributor

@GwiYeong The version of Salt that you are using has CVE-Support Only and I would urge upgrading to a currently supported version, for example: Salt 2018.3.4 or 2019.2.0 for which there are RHEL 6 packages provided.

Note that with pip installations, you will get the latest version of a package unless a specific version is specified, for example: Salt 2019.2.0 salt/requirements/base/txt

:~/devcode/test_checkout_salt/salt/requirements$ cat base.txt
Jinja2

This should be changed to msgpack-python for Packages

msgpack-python>0.3,!=0.5.5

msgpack>=0.5,!=0.5.5
PyYAML<5.1
MarkupSafe
requests>=1.0.0
tornado>=4.2.1,<6.0; python_version < '3'
tornado>=4.2.1,<5.0; python_version >= '3.4'

Required by Tornado to handle threads stuff.

futures>=2.0; python_version < '3.0'

There have been fixes made since 2017.7.x for msgpack issues, hence the benefit of updating to supported versions. Note that Salt 2017.7.8-426-g464764a is not a released version and hence is unsupported. Release versions for Redhat 6 are available from repo.saltstack.com.

Internal testing with latest versions of msgpack, pip installed, have not shown any issues with currently supported software, however if you upgrade to a later version of Salt and still encounter an issue with msgpack v0.6.1, please let us know as soon as possible.

If this resolves the issue, please consider closing the issue.

@GwiYeong
Copy link
Contributor Author

GwiYeong commented Jun 19, 2019

@dmurphy18 ok. i will upgrade my saltstack. thanks!

@hedrickbt
Copy link

hedrickbt commented Sep 10, 2019

We are seeing this error with 2019.2.0

sudo salt --versions-report
Salt Version:
Salt: 2019.2.0

Dependency Versions:
cffi: 1.11.5
cherrypy: 3.5.0
dateutil: 2.8.0
docker-py: Not Installed
gitdb: 0.6.4
gitpython: 1.0.1
ioflo: Not Installed
Jinja2: 2.10.1
libgit2: Not Installed
libnacl: Not Installed
M2Crypto: Not Installed
Mako: 1.0.10
msgpack-pure: Not Installed
msgpack-python: 0.6.1
mysql-python: Not Installed
pycparser: 2.19
pycrypto: 2.6.1
pycryptodome: Not Installed
pygit2: Not Installed
Python: 3.5.2 (default, Nov 12 2018, 13:43:14)
python-gnupg: 0.3.8
PyYAML: 3.13
PyZMQ: 18.0.1
RAET: Not Installed
smmap: 0.9.0
timelib: Not Installed
Tornado: 4.5.3
ZMQ: 4.3.1

System Versions:
dist: Ubuntu 16.04 xenial
locale: UTF-8
machine: x86_64
release: 4.4.0-151-generic
system: Linux
version: Ubuntu 16.04 xenial

In our case it is for bin instead of str:
Sep 10 10:48:24 localhost salt-master[9599]: [ERROR ] Exception occurred while handling stream: 1169124 exceeds max_bin_len(1048576)

@hedrickbt
Copy link

It looks like the issue is that changes were made in msgpack 0.60 to avoid DOS attacks caused by large messages. https://github.com/msgpack/msgpack-python/blob/master/ChangeLog.rst

We are going to try to change the MAX_EVENT_SIZE in the salt master configuration file to see if we can accommodate the size of our messages. A 1MB response default size seems small for something like a highstate.

@gordonm
Copy link

gordonm commented Sep 16, 2019

@hedrickbt We have a similar issue, the return data for large orchestration states wind up being around 2-3 MB and are supposed to be returned to a client calling salt-api. In our case the exception is in salt.transport.ipc, so max_event_size has no effect there (I tested this just to be sure, I'm sure you will have found the same).

For msgpack >=0.6.1, I believe the fix would be to pass max_buffer_size to msgpack.Unpacker() with a configurable value. For now I'll try to patch the master with a hardcoded value and test again.

@johnj
Copy link
Contributor

johnj commented Oct 17, 2019

I had a similar issue and I just ended up pinning msgpack to 0.5.6 in our pex single-binary build for now.

https://github.com/johnj/salt-pex

oeuftete pushed a commit to oeuftete/salt that referenced this issue Oct 6, 2020
salt 3000 introduced a dependency on Python msgpack >= 0.6. In msgpack 0.6 their
developers changed the max length of a string that could be unpacked from 2GiB
to 1MiB. They then proceeded to change it to 100MiB in msgpack 1.0. Now when
using msgpack >=0.6 and < 1.0, any message that is unpacked larger than 1MiB
will throw an error. For example a file state which tries to send a file larger
than 1MiB will get the following error:

Unable to manage file: 1048688 exceeds max_str_len(1048576)

This error could send the master into a tailspin where subsequent file
operations take a long time or just time out.

This patch sets the keyword argument max_buffer_size for the Unpacker to 100MiB
which is the msgpack 1.0 default now. It will allow people to use msgpack
versions >.6 and < 1.0 without this crazy low limit. It should be discussed if
we go back to the 2GiB size from < 0.6 days (my vote), or just anything larger
than 100MB. This code does a version check, and will only set the keyword on
msgpack > = 0.6 and < 1.0.

The test checks that we have a buffer size of at least 100MiB in case the Python
msgpack devs decide to lower this value again, and break things. We want enforce
a decent minimum.

fixes saltstack#53230, fixes saltstack#54884,
fixes saltstack#55180, fixes saltstack#55184,
fixes saltstack#57026, fixes saltstack#57921
twangboy pushed a commit to psyer/salt that referenced this issue Oct 6, 2020
salt 3000 introduced a dependency on Python msgpack >= 0.6. In msgpack 0.6 their
developers changed the max length of a string that could be unpacked from 2GiB
to 1MiB. They then proceeded to change it to 100MiB in msgpack 1.0. Now when
using msgpack >=0.6 and < 1.0, any message that is unpacked larger than 1MiB
will throw an error. For example a file state which tries to send a file larger
than 1MiB will get the following error:

Unable to manage file: 1048688 exceeds max_str_len(1048576)

This error could send the master into a tailspin where subsequent file
operations take a long time or just time out.

This patch sets the keyword argument max_buffer_size for the Unpacker to 100MiB
which is the msgpack 1.0 default now. It will allow people to use msgpack
versions >.6 and < 1.0 without this crazy low limit. It should be discussed if
we go back to the 2GiB size from < 0.6 days (my vote), or just anything larger
than 100MB. This code does a version check, and will only set the keyword on
msgpack > = 0.6 and < 1.0.

The test checks that we have a buffer size of at least 100MiB in case the Python
msgpack devs decide to lower this value again, and break things. We want enforce
a decent minimum.

fixes saltstack#53230, fixes saltstack#54884,
fixes saltstack#55180, fixes saltstack#55184,
fixes saltstack#57026, fixes saltstack#57921
twangboy pushed a commit to psyer/salt that referenced this issue Oct 6, 2020
salt 3000 introduced a dependency on Python msgpack >= 0.6. In msgpack 0.6 their
developers changed the max length of a string that could be unpacked from 2GiB
to 1MiB. They then proceeded to change it to 100MiB in msgpack 1.0. Now when
using msgpack >=0.6 and < 1.0, any message that is unpacked larger than 1MiB
will throw an error. For example a file state which tries to send a file larger
than 1MiB will get the following error:

Unable to manage file: 1048688 exceeds max_str_len(1048576)

This error could send the master into a tailspin where subsequent file
operations take a long time or just time out.

This patch sets the keyword argument max_buffer_size for the Unpacker to 100MiB
which is the msgpack 1.0 default now. It will allow people to use msgpack
versions >.6 and < 1.0 without this crazy low limit. It should be discussed if
we go back to the 2GiB size from < 0.6 days (my vote), or just anything larger
than 100MB. This code does a version check, and will only set the keyword on
msgpack > = 0.6 and < 1.0.

The test checks that we have a buffer size of at least 100MiB in case the Python
msgpack devs decide to lower this value again, and break things. We want enforce
a decent minimum.

fixes saltstack#53230, fixes saltstack#54884,
fixes saltstack#55180, fixes saltstack#55184,
fixes saltstack#57026, fixes saltstack#57921
dwoz pushed a commit that referenced this issue Oct 8, 2020
salt 3000 introduced a dependency on Python msgpack >= 0.6. In msgpack 0.6 their
developers changed the max length of a string that could be unpacked from 2GiB
to 1MiB. They then proceeded to change it to 100MiB in msgpack 1.0. Now when
using msgpack >=0.6 and < 1.0, any message that is unpacked larger than 1MiB
will throw an error. For example a file state which tries to send a file larger
than 1MiB will get the following error:

Unable to manage file: 1048688 exceeds max_str_len(1048576)

This error could send the master into a tailspin where subsequent file
operations take a long time or just time out.

This patch sets the keyword argument max_buffer_size for the Unpacker to 100MiB
which is the msgpack 1.0 default now. It will allow people to use msgpack
versions >.6 and < 1.0 without this crazy low limit. It should be discussed if
we go back to the 2GiB size from < 0.6 days (my vote), or just anything larger
than 100MB. This code does a version check, and will only set the keyword on
msgpack > = 0.6 and < 1.0.

The test checks that we have a buffer size of at least 100MiB in case the Python
msgpack devs decide to lower this value again, and break things. We want enforce
a decent minimum.

fixes #53230, fixes #54884,
fixes #55180, fixes #55184,
fixes #57026, fixes #57921
dwoz pushed a commit that referenced this issue Oct 8, 2020
salt 3000 introduced a dependency on Python msgpack >= 0.6. In msgpack 0.6 their
developers changed the max length of a string that could be unpacked from 2GiB
to 1MiB. They then proceeded to change it to 100MiB in msgpack 1.0. Now when
using msgpack >=0.6 and < 1.0, any message that is unpacked larger than 1MiB
will throw an error. For example a file state which tries to send a file larger
than 1MiB will get the following error:

Unable to manage file: 1048688 exceeds max_str_len(1048576)

This error could send the master into a tailspin where subsequent file
operations take a long time or just time out.

This patch sets the keyword argument max_buffer_size for the Unpacker to 100MiB
which is the msgpack 1.0 default now. It will allow people to use msgpack
versions >.6 and < 1.0 without this crazy low limit. It should be discussed if
we go back to the 2GiB size from < 0.6 days (my vote), or just anything larger
than 100MB. This code does a version check, and will only set the keyword on
msgpack > = 0.6 and < 1.0.

The test checks that we have a buffer size of at least 100MiB in case the Python
msgpack devs decide to lower this value again, and break things. We want enforce
a decent minimum.

fixes #53230, fixes #54884,
fixes #55180, fixes #55184,
fixes #57026, fixes #57921
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants