Fix: serialization error when gossip working #6566

kitsuyui · 2020-12-28T09:32:30Z

Current problem

Currently, Gossip does not respect accept_content when generating a consumer.
If you have multiple Celery workers running and allow pickle with accept_content, you may get a Refusing to deserialize disabled content of type pickle (application/x-python-serialize) error.

This error does not occur when running a single worker, but it does occur when running multiple workers.

What this PR does

Make Gossip respect accept_content
- To do this, Gossip needs to pass accept_content when generating a consumer
Add tests to verify that multiple workers are running and no errors containing Refusing to deserialize disabled content of type are occurring

Additional notes

Some of the existing unit tests use Mock() to replace Gossip processing. This is the case with t/unit/worker/test_consumer.py.
- The Mock() needs to be configured with additional settings to work as intended with this change.
Some of the tests are unstable. I (@kitsuyui) have confirmed that these failures occur without my changes.

The update to Codecov reports sometimes fails with a 500 error
Integration tests sometimes fail due to timeouts

auvipy · 2020-12-28T13:51:46Z

celery/worker/consumer/gossip.py

@@ -176,6 +176,7 @@ def get_consumers(self, channel):
            channel,
            queues=[ev.queue],
            on_message=partial(self.on_message, ev.event_from_message),
+            accept=ev.accept,


hey, thanks for the PR, but this change needs some unit test to get accepted

auvipy · 2021-01-04T20:31:09Z

Description

fix of #5075
Pass accept when Gossip getting consumers.

can you at least add a unit test for this fix?

kitsuyui · 2021-01-05T02:01:30Z

@auvipy
Thanks. I'd tried to do that but it will take more time for me.
Or can you write instead of me?

auvipy · 2021-01-06T05:50:14Z

@auvipy
Thanks. I'd tried to do that but it will take more time for me.
Or can you write instead of me?

you can take your time, I have other priorities todo in celery at the moment

thedrow · 2021-02-24T14:36:35Z

@kitsuyui Is there a chance that you'll complete this PR soon?

kitsuyui · 2021-02-24T15:43:41Z

@thedrow No.
I don't have any clue to solve soon. I've tried but the unit test has become too complicated.

jbvsmo · 2023-10-03T15:12:31Z

Any news on this? As the author said he can't finish is it possible some celery mantainer pick it up, please? cc @auvipy

This is affecting us greatly with no other fix or workaround

auvipy · 2023-10-03T15:19:45Z

We need to verify this is working and not creating any regression. Thanks for the ping

rfqu · 2023-10-13T23:31:31Z

I encountered this issue too and solved by adding
accept_content = ['application/json', 'application/x-python-serialize','application/x-pickle2']

auvipy · 2023-10-14T05:24:13Z

I encountered this issue too and solved by adding accept_content = ['application/json', 'application/x-python-serialize','application/x-pickle2']

hey! great to know that. is it possible for you to send a PR to improve the docs regarding the issue/solution you mentioned?

wlach · 2023-10-21T12:00:22Z

I encountered this issue too and solved by adding accept_content = ['application/json', 'application/x-python-serialize','application/x-pickle2']

Based on what I've seen, I don't see how this could fix the problem since the problem is that gossip doesn't respect these settings. I'm pretty sure this PR is the correct solution based on a frustrating encounter with this issue today.

Is there any chance we could merge this as-is and file a follow-up for a unit test? This is a pretty bad bug: it will affect anyone still using pickle serialization with gossip.

auvipy · 2023-10-22T10:41:08Z

problem is the patch is making the CI red. which means it will create regression

kitsuyui · 2023-10-22T14:19:53Z

Hi, sorry for being vague. Sorry for the long delay.

I have quitted the company that required this patch. However, I now have time to develop it. If there is anything I can do to help, I would like to do so.

I have already found a way to reproduce the problem at #5075 (comment), so I could try again.

The problem was that I was not able to write the necessary tests well, and I was unable to eliminate concerns about regression. I may still need someone's help in this.

auvipy · 2023-10-23T05:12:30Z

thanks for coming back! as you shared in your repo kitsuyui#5 (comment) some changes on these tests would be needed. I currently have a big backlog in celery to clear. so in the mean time if you can try to figure out some sort of test, it would be great

kitsuyui · 2023-10-26T08:58:06Z

@auvipy Hi. I have updated the commits and description of the pull request.
I have added tests. I have also confirmed that the tests fail if I do not make this change.
I have confirmed that the tests pass on my local environment and on the CI of my fork. (kitsuyui#5)
But I am not confident that these tests meet your standards.
Could you please review this?

codecov · 2023-10-26T09:21:20Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (3723009) 87.33% compared to head (53ea3f3) 87.33%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #6566   +/-   ##
=======================================
  Coverage   87.33%   87.33%           
=======================================
  Files         148      148           
  Lines       18515    18515           
  Branches     3163     3163           
=======================================
  Hits        16170    16170           
  Misses       2060     2060           
  Partials      285      285

Flag	Coverage Δ
unittests	`87.30% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
celery/worker/consumer/gossip.py	`96.96% <ø> (ø)`

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

auvipy · 2023-10-27T06:23:55Z

thanks for the tests! the failures seems to be github network related. so going to re start the CI

kitsuyui · 2023-10-31T12:44:48Z

@auvipy Hi, I can't rerun CI because I don't have permissions on the project. Please retry. ☕

Nusnus · 2023-11-06T08:57:52Z

@kitsuyui

@auvipy Hi, I can't rerun CI because I don't have permissions on the project. Please retry. ☕

Please rebase on main.
CI fixes were pushed.

I will make sure the CI will run if it won't automatically.

auvipy

please pull from main branch again

Pass accept when Gossip getting consumers.

If event_serializer, result_serializer, accept_content are set correctly, it works normally. Otherwise, an error is output to the log. Test that.

kitsuyui · 2023-11-08T08:40:47Z

@Nusnus @auvipy Hi, I rebased the branch and pushed it again.

Nusnus · 2023-11-08T09:31:24Z

@Nusnus @auvipy Hi, I rebased the branch and pushed it again.

Thank you!
I'll review it later today

auvipy

looks good to me

kitsuyui · 2023-11-08T13:40:46Z

@auvipy @Nusnus Thank you!

auvipy requested changes Dec 28, 2020

View reviewed changes

auvipy added this to the 5.0.6 milestone Dec 28, 2020

thedrow added Component: Gossip PR Type: Bugfix Status: Needs Test Coverage ✘ labels Dec 31, 2020

thedrow modified the milestones: 5.0.6, 5.1.0 Feb 24, 2021

thedrow added this to Review in progress in Celery 5.1.0 Feb 24, 2021

auvipy closed this Feb 24, 2021

Celery 5.1.0 automation moved this from Review in progress to Done Feb 24, 2021

auvipy reopened this Feb 24, 2021

Celery 5.1.0 automation moved this from Done to In progress Feb 24, 2021

thedrow assigned auvipy Feb 28, 2021

auvipy moved this from In progress to Postponed in Celery 5.1.0 May 4, 2021

auvipy modified the milestones: 5.1.0, 5.2 May 4, 2021

auvipy modified the milestones: 5.2, 5.2.x Oct 31, 2021

thedrow removed this from the 5.2.x milestone Dec 13, 2021

auvipy added this to the 5.3.x milestone Jun 21, 2022

auvipy closed this Feb 6, 2023

auvipy reopened this Feb 6, 2023

auvipy mentioned this pull request Feb 6, 2023

Pickle serialization kills multiple workers #5075

Closed

kitsuyui force-pushed the gossip-get-consumer-prefer-event-receiver-accept branch from 1a18671 to c8f286b Compare October 22, 2023 14:23

kitsuyui mentioned this pull request Oct 22, 2023

Try 6566 kitsuyui/celery#5

Closed

auvipy removed their assignment Oct 23, 2023

kitsuyui force-pushed the gossip-get-consumer-prefer-event-receiver-accept branch from c8f286b to 55574dc Compare October 26, 2023 08:38

auvipy closed this Oct 27, 2023

auvipy reopened this Oct 27, 2023

auvipy requested changes Nov 6, 2023

View reviewed changes

kitsuyui added 3 commits November 8, 2023 17:38

Fix: serialization error when gossip working

0bbda8b

Pass accept when Gossip getting consumers.

Define Receiver mock indirectly mocked by consumer mock

1a939d1

This commit adds tests to serialize based on worker settings.

53ea3f3

If event_serializer, result_serializer, accept_content are set correctly, it works normally. Otherwise, an error is output to the log. Test that.

kitsuyui force-pushed the gossip-get-consumer-prefer-event-receiver-accept branch from 55574dc to 53ea3f3 Compare November 8, 2023 08:38

auvipy removed the Status: Needs Test Coverage ✘ label Nov 8, 2023

auvipy approved these changes Nov 8, 2023

View reviewed changes

auvipy merged commit 4e88881 into celery:main Nov 8, 2023
32 checks passed

kitsuyui deleted the gossip-get-consumer-prefer-event-receiver-accept branch November 8, 2023 17:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: serialization error when gossip working #6566

Fix: serialization error when gossip working #6566

kitsuyui commented Dec 28, 2020 •

edited

Loading

auvipy Dec 28, 2020

auvipy commented Jan 4, 2021

Description

kitsuyui commented Jan 5, 2021

auvipy commented Jan 6, 2021

thedrow commented Feb 24, 2021

kitsuyui commented Feb 24, 2021

jbvsmo commented Oct 3, 2023

auvipy commented Oct 3, 2023

rfqu commented Oct 13, 2023

auvipy commented Oct 14, 2023

wlach commented Oct 21, 2023

auvipy commented Oct 22, 2023

kitsuyui commented Oct 22, 2023

auvipy commented Oct 23, 2023

kitsuyui commented Oct 26, 2023 •

edited

Loading

codecov bot commented Oct 26, 2023 •

edited

Loading

auvipy commented Oct 27, 2023

kitsuyui commented Oct 31, 2023

Nusnus commented Nov 6, 2023

auvipy left a comment

kitsuyui commented Nov 8, 2023

Nusnus commented Nov 8, 2023

auvipy left a comment

kitsuyui commented Nov 8, 2023

Fix: serialization error when gossip working #6566

Fix: serialization error when gossip working #6566

Conversation

kitsuyui commented Dec 28, 2020 • edited Loading

Current problem

What this PR does

Additional notes

auvipy Dec 28, 2020

Choose a reason for hiding this comment

auvipy commented Jan 4, 2021

Description

kitsuyui commented Jan 5, 2021

auvipy commented Jan 6, 2021

thedrow commented Feb 24, 2021

kitsuyui commented Feb 24, 2021

jbvsmo commented Oct 3, 2023

auvipy commented Oct 3, 2023

rfqu commented Oct 13, 2023

auvipy commented Oct 14, 2023

wlach commented Oct 21, 2023

auvipy commented Oct 22, 2023

kitsuyui commented Oct 22, 2023

auvipy commented Oct 23, 2023

kitsuyui commented Oct 26, 2023 • edited Loading

codecov bot commented Oct 26, 2023 • edited Loading

Codecov Report

auvipy commented Oct 27, 2023

kitsuyui commented Oct 31, 2023

Nusnus commented Nov 6, 2023

auvipy left a comment

Choose a reason for hiding this comment

kitsuyui commented Nov 8, 2023

Nusnus commented Nov 8, 2023

auvipy left a comment

Choose a reason for hiding this comment

kitsuyui commented Nov 8, 2023

kitsuyui commented Dec 28, 2020 •

edited

Loading

kitsuyui commented Oct 26, 2023 •

edited

Loading

codecov bot commented Oct 26, 2023 •

edited

Loading