fix: Chart cache-warmup task fails on Superset 4.0 #28706
Conversation
rmasters
changed the title
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
dosubot
bot
added
api
|
@gooroodev please review |
Summary of Changes
Issues, Bugs, or Typos
Proposed Code ImprovementsImprovement 1: Error Handling in
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #28706 +/- ##
===========================================
+ Coverage 60.48% 83.65% +23.16%
===========================================
Files 1931 528 -1403
Lines 76236 38181 -38055
Branches 8568 0 -8568
===========================================
- Hits 46114 31941 -14173
+ Misses 28017 6240 -21777
+ Partials 2105 0 -2105
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
john-bodley
assigned betodealmeida and dpgaspar and unassigned betodealmeida and dpgaspar
|
Is there anything I can do to help get this merged? I'm also interested in working cache warmups |
Feel free to give it a code review, and/or answer any of the open questions on the thread. Still hoping to get an opinion from @betodealmeida and/or @dpgaspar , but I'll add @john-bodley to the mix as well. |
|
I also want to use cache-warmup in Superset 4.0.2. When is it planned to include this fix? Thank you! |
|
Thanks for providing solution, however this wouldn't work if someone is using superset-websockets as in the headers there are two "Set-Cookie"'s first one is an async-token and second session token, your code will get the first one which is wrong and you'll get 404 UNATHORIZED |
I installed the version 4.0.2, and applied the fix suggested by @rmasters in the cache.py file. Before the fix, I was getting the 405 error. |
|
@rmasters this needs a rebase, and I don't have permissions to push changes to your branch to do it. Can you rebase, and optionally make that |
|
@cmartinezdnn Have you tested that condition?? |
|
Looks like some CI checks aren't running for whatever reason. Might be in need of the aforementioned rebase, but I'll close and reopen this to see if that kick-starts it. |
|
You think that this could be merged soon? |
Hi, can you please deliberate on what did you change so it will work for you? |
|
Yeah... any traction on this would be amazing |
|
Apologies for the radio silence; I don't seem to be getting github notifications through to my email... Many thanks for the reviews. I'll rebase and pop that refactor to utils in shortly.
I'm not familiar with the websocket server, but am I right in understanding that it should not pick up the async/websocket-server cookie, or does it proxy requests at all? It doesn't appear that the FAB session cookie name can be changed from Edit: I've popped a change in that parses out all the set-cookie headers and picks the one named |
- Updates fetch_url task to use /api/v1/chart/warm_up_cache/ - Adds CSRF fetcher to use this endpoint
rmasters
force-pushed
the
fix/http-405-warmup-cache
branch
from
f9f3196
to
47d9fea
Compare
This could fail when other cookies are set, such as the websockets server async-token cookie.
|
Hey @villebro, I've rebased onto master, split out the csrf-token fetcher and made a fix towards the session cookie grabber, if you can trigger the CI for me please 🚀 |
|
@rmasters thanks for the rebase. Any changes we need to add in terms of ingress annotation for the chart? |
|
I've started CI and will try to review this tomorrow (PST) |
|
Waiting for flaky tests to rerun, will try to merge as soon as CI passes. |
|
One last flaky test to go.. 🤦 |
sadpandajoe
added
the
v4.1
(cherry picked from commit 0744abe)
|
Hi in which version this PR is going to be released? |
SUMMARY
Fixes #28705
We found our chart cache warm-up task (as in the example in the Kubernetes docs) was failing with this error:
On investigation I found that:
cache-warmuptask callsPUT /superset/warm_up_caches/(via thefetch_urltask)/superset/warm_up_caches/(Superset.warm_up_caches) exists but is a GET request/api/v1/chart/warm_up_caches(ChartRestApi.warm_up_cache) (scheduled for deletion in 4.0 but I couldn't find discussions around that)This PR changes the task to call the newer endpoint (accidentally reverted in 56069b0).
I also ran into CSRF issues (similar to discussion in #27160), so added a method to fetch this (thanks to Will Gan on slack for the hint). Perhaps this can be placed more centrally? This appears to work on installations with CSRF disabled (I've tested with
WTF_CSRF_ENABLED = False).BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
class CeleryConfig: broker_url = f"redis://{REDIS_HOST}:{REDIS_PORT}/{REDIS_CELERY_DB}" - imports = ("superset.sql_lab",) + imports = ( + "superset.sql_lab", + "superset.tasks.cache", + ) result_backend = f"redis://{REDIS_HOST}:{REDIS_PORT}/{REDIS_RESULTS_DB}" worker_prefetch_multiplier = 1 task_acks_late = False @@ -87,6 +90,13 @@ class CeleryConfig: "task": "reports.prune_log", "schedule": crontab(minute=10, hour=0), }, + "cache-warmup-dummy": { + "task": "cache-warmup", + "schedule": crontab(minute="*/1", hour="*"), + "kwargs": { + "strategy_name": "dummy", + }, + }, }A clearer example on my clean install (based on 4.0.0):
ADDITIONAL INFORMATION