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

[carbonlink] Python 3 Pickle Compatible Metric Request Parser #340

Merged
merged 5 commits into from
Jun 7, 2020
Merged

[carbonlink] Python 3 Pickle Compatible Metric Request Parser #340

merged 5 commits into from
Jun 7, 2020

Conversation

alikhtag
Copy link
Contributor

Problem

When graphite-web is installed on Python 3.0+ environments graphite-web will send pickle request to carbonlink using the highest pickle protocol (link to code in graphite-web) .

In python 3.0 until 3.7 highest pickle protocol is 4, while in python 3.8 highest is 5.

Since go-carbon's carbonlink was written for python 2.x, when highest protocol available was 2, it is unable to parse carbonlink pickle messages coming from python graphite-web. Even if the protocol is set to 2 when using python 3.x it won't be able to parse it due encoding change from ASCII in python 2.x to Unicode in python 3.x.

Steps to recreate
Run go-carbon normally, but run graphite-web in Python 3.0+ and request a metric that will utilize carbonlink. You will see protocol 4 message request coming *protocol 5 in Python 3.8+). Consequently go-carbon will WARN regarding bad pickle message error and wont serve metric from cache.

# go-carbon
WARN [carbonlink] request parse failed {"peer": "ip_address", "error": "Bad pickle message"} 
# Graphite web
ConnectionRefusedError: [Errno 111] Connection refused

PR Fixes

This PR should allow carbonlink to handle pickle protocols from 2 up to 5 by expanding the carbonlink pickle message parser to Python <= 3.8 pickle messages. More on python pickle protocols can be found here

Note: Will make PR later in graphite-web to make pickle protocol configurable for carbonlink requests, so that if newer python version with higher pickle protocol comes out it will not break carbonlink communication (one can set desired protocol) if go-carbon will not be able to parse it.

@deniszh
Copy link
Member

deniszh commented Jun 6, 2020

Hi @azhiltsov @lomik @bom-d-van !
Could someone review and merge this PR, pretty please?
Much appreciated!

@azhiltsov azhiltsov merged commit 0fdd9e5 into go-graphite:master Jun 7, 2020
@alikhtag
Copy link
Contributor Author

alikhtag commented Jun 8, 2020

Thanks for merge @azhiltsov !

@alikhtag alikhtag deleted the fix/carbonlink-py3-compatibility branch June 22, 2020 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants