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

upgrade celery 5.2.3 #19703

Merged
merged 3 commits into from Jan 3, 2022
Merged

upgrade celery 5.2.3 #19703

merged 3 commits into from Jan 3, 2022

Conversation

auvipy
Copy link
Contributor

@auvipy auvipy commented Nov 19, 2021

this update is very much needed for several reason

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

@auvipy
Copy link
Contributor Author

auvipy commented Jan 2, 2022

let me know what else needs to be updated

@auvipy auvipy changed the title try celery 5.2.1 upgrde celery 5.2.3 Jan 2, 2022
@potiuk
Copy link
Member

potiuk commented Jan 2, 2022

That should be enough @auvipy !

Looks good all tests passed. Anyone knows any potential problem involved with it ?
When we merge it, we will have to refresh the constraints as this is conflicting with old ones, but I am happy to merge it it any other committer confirms it is fine to merge.

@potiuk potiuk changed the title upgrde celery 5.2.3 upgrade celery 5.2.3 Jan 2, 2022
@auvipy
Copy link
Contributor Author

auvipy commented Jan 3, 2022

actually it contains several memory leaks/usage issues fixes in amqp & redis & 1 moderate but very old security issue.

setup.py Outdated
'celery~=5.1,>=5.1.2',
'celery~=5.2,>=5.2.3',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this not continue to allow Celery 5.1.x by declaring celery>=5.1 instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we usually only support the latest stable version in celery & https://github.com/celery/celery/releases/tag/v5.2.2 is not back ported.

Copy link
Member

@potiuk potiuk Jan 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IF this one contains some security issue and memory leaks, then I am ok with >=5.2.3.

But I thought a bit and we should really remove the upper limit - this is our default behaviour and our constraint mechanism will keep it in check but we will avoid any need for future changes like that for 5.3 and above.

setup.py Outdated
@@ -227,7 +227,7 @@ def write_version(filename: str = os.path.join(*[my_dir, "airflow", "git_version
'cassandra-driver>=3.13.0,<4',
]
celery = [
'celery~=5.1,>=5.1.2',
'celery~=5.2,>=5.2.3',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'celery~=5.2,>=5.2.3',
'celery>=5.2.3',

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@potiuk
Copy link
Member

potiuk commented Jan 3, 2022

The error is something I will have to fix - nothing to worry about (aftermath of #20624 - which I was not able to quickly test).

@potiuk
Copy link
Member

potiuk commented Jan 3, 2022

I will upgrade constratints and merge this one.

@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Jan 3, 2022
@github-actions
Copy link

github-actions bot commented Jan 3, 2022

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

@potiuk potiuk merged commit e1fbfc6 into apache:main Jan 3, 2022
@potiuk
Copy link
Member

potiuk commented Jan 3, 2022

Actually we were already at 5.2.3 so it was not needed to upgrade constraints before merge

@auvipy auvipy deleted the patch-1 branch January 3, 2022 15:29
@auvipy
Copy link
Contributor Author

auvipy commented Jan 4, 2022

thanks. btw I have triage access to github, can I use https://issues.apache.org/jira/projects/AIRFLOW/issues/AIRFLOW-4470?filter=allopenissues as well? any instructions? I wish to contribute to good first issues

@potiuk
Copy link
Member

potiuk commented Jan 4, 2022

The Issues is JIRA are not used any more - look at the https://github.com/apache/airflow/contribute instead.

The instrructions for contribution are thre too (top right CONTRIBUTING.rst) link :).

Also you might take a look at a contribution workshop we held last year; https://youtu.be/kvccZizzfTk

@auvipy
Copy link
Contributor Author

auvipy commented Jan 4, 2022

ok though signed up! will check the video

@eladkal
Copy link
Contributor

eladkal commented Jan 4, 2022

I wish to contribute to good first issues

You can find good first issues with this link

@christmoore
Copy link

christmoore commented Jan 21, 2022

FYI this version increase includes some much needed fixes regarding memory leaks in Celery w/ Redis brokers. We were experiencing our airflow workers failing from memory leaks due to a problem with health checks - after some further investigation it seems due to celery/celery#4843.

Pretty good description of the issue in celery/celery#4843 (comment)

For our side, a fix was made in Kombo 5.2.3 and included in Celery 5.2.3 which resolve the memory leak issue:
celery/kombu#1476 to resolve the leaking Transport objects that stayed in mem from closed connections

A change was also included to shrink the overall memory usage of Transport and Connection objects in case of further memory leaks that were missed. celery/kombu#1470

For an overall review of what was changed to resolve the memory leaks, please see celery/celery#4843 (comment)

I'm not sure regarding your release cadence, but this includes some pretty big fixes.

@potiuk
Copy link
Member

potiuk commented Jan 21, 2022

For our side, a fix was made in Kombo 5.2.3 and included in Celery 5.2.3 which resolve the memory leak issue:

Consider it done!

As mentioned before - our automated uggrade process already picked it up and I am also just about to cherry-pick and refresh constraints and docker images for 2.2 branch for the upcoming 2.2.4 release (weeks rather than months).

@auvipy
Copy link
Contributor Author

auvipy commented Jan 22, 2022

really happy to know this!

@auvipy
Copy link
Contributor Author

auvipy commented Jan 22, 2022

I'm not sure regarding your release cadence, but this includes some pretty big fixes.

can you please try celery/py-amqp#368 and linked PR? I will be very grateful

@potiuk potiuk added this to the Airflow 2.2.4 milestone Jan 22, 2022
potiuk pushed a commit that referenced this pull request Jan 22, 2022
(cherry picked from commit e1fbfc6)
@jedcunningham jedcunningham added the type:bug-fix Changelog: Bug Fixes label Jan 25, 2022
jedcunningham pushed a commit that referenced this pull request Jan 27, 2022
(cherry picked from commit e1fbfc6)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
full tests needed We need to run full set of tests for this PR to merge type:bug-fix Changelog: Bug Fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants