Skip to content

fix msgpack max_buffer_size being to low in ver >= 0.6 and < 1.0#58283

Merged
dwoz merged 8 commits into
saltstack:masterfrom
psyer:fix_msgpack_max_buffer_size
Oct 8, 2020
Merged

fix msgpack max_buffer_size being to low in ver >= 0.6 and < 1.0#58283
dwoz merged 8 commits into
saltstack:masterfrom
psyer:fix_msgpack_max_buffer_size

Conversation

@psyer

@psyer psyer commented Aug 26, 2020

Copy link
Copy Markdown
Contributor

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

What does this PR do?

Fixes error "...exceeds max_str_len(1048576)" when using msgpack ver >= 0.6 and < 1.0.

What issues does this PR fix or reference?

fixes #53230, fixes #54884,
fixes #55180, fixes #55184,
fixes #57026, fixes #57921

Previous Behavior

When trying to unpack a message larger than 1MiB from the salt bus using msgpack, you will get the error mentioned in "What does this PR do?". Then the master will usually hang for long periods or just die.

New Behavior

After patch, the limit will be bumped to 100MiB and allow much larger files and not throw errors.

Merge requirements satisfied?

Commits signed with GPG?

No

@psyer psyer requested a review from a team as a code owner August 26, 2020 01:47
@ghost ghost requested review from dwoz and removed request for a team August 26, 2020 01:47
@psyer psyer force-pushed the fix_msgpack_max_buffer_size branch from ac6136a to b11c9df Compare August 26, 2020 03:44
@psyer

psyer commented Aug 26, 2020

Copy link
Copy Markdown
Contributor Author

Need some insight into my 1 failing test in ci/pre-commit.

Drop six usage and Py2 support...........................................Failed
23:51:34  - hook id: pyupgrade
23:51:34  - exit code: 1

This seems to be the exit code that is failing, but I'm not sure what I'm supposed to do to correct it. Nothing in my code had six or any Py2 related things. Also, the Black test failing is perplexing, as I ran this code through Black and it should have fixed it all in that commit.

@s0undt3ch

Copy link
Copy Markdown
Contributor

https://jenkinsci.saltstack.com/job/pr-pre-commit/job/PR-58283/4/console

If you look at the output, you'll see what black is complaining about.
Also, if you install pre-commit and do a pre-commit install on the root of the repo, these changes happen as you change files and commit them, automatically.

@psyer

psyer commented Aug 27, 2020

Copy link
Copy Markdown
Contributor Author

@s0undt3ch Thank you for help on the pre-commit checks. It should be fixed now. I missed that little section in the contributing docs on this.

@psyer psyer requested a review from s0undt3ch as a code owner October 4, 2020 21:07
@s0undt3ch

Copy link
Copy Markdown
Contributor

Off the bat, 500+ commits?
A rebase is needed?

@psyer

psyer commented Oct 4, 2020

Copy link
Copy Markdown
Contributor Author

Off the bat, 500+ commits?
A rebase is needed?

@s0undt3ch Yep. I did what I was trying not to do, and I even ran a rebase, but must have not done it properly. This is my first time doing this, and the contributing doc did not give me enough info to make me comfortable to doing this. Here's what I did (below), where do I go from here with this mess? Just close the request and resubmit?

git fetch upstream
git rebase upstream/master
git pull
git push

It would not allow me to do a ff merge as i had changes from my fix, so I saw "git pull --rebase origin master" in the contributing doc, and that did not look right on for my fix branch I was on.

@sagetherage sagetherage added Magnesium Mg release after Na prior to Al ZD The issue is related to a Zendesk customer support ticket. labels Oct 6, 2020
@sagetherage sagetherage requested a review from cmcmarrow October 6, 2020 14:47
@twangboy twangboy self-assigned this Oct 6, 2020
psyer and others added 8 commits October 6, 2020 15:03
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
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 twangboy force-pushed the fix_msgpack_max_buffer_size branch from e7d6f6c to 463d5d8 Compare October 6, 2020 21:09
@dwoz dwoz merged commit 959bb98 into saltstack:master Oct 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Magnesium Mg release after Na prior to Al ZD The issue is related to a Zendesk customer support ticket.

Projects

None yet

5 participants