Repository navigation
Too many requests #47950
Description
When running airflow DatabricksRunNowOperator, if using managed identity, and if there is multiple tasks running in parallel, the code below can quickly exhaust the allowed metadata server as explained here https://learn.microsoft.com/en-us/azure/virtual-machines/instance-metadata-service?tabs=windows#rate-limiting
In general, requests to IMDS are limited to 5 requests per second (on a per VM basis). Requests exceeding this threshold will be rejected with 429 responses.
I don't understand why this check was put here 3 years ago, but I feel this is unnecessary.
I propose we remove this check, or make it optional. Can I open a PR for this?
Activity
boring-cyborg commentedon Mar 19, 2025
Thanks for opening your first issue here! Be sure to follow the issue template! If you are willing to raise PR to address this issue please do so, no need to wait for approval.
DjVinnii commentedon Mar 19, 2025
It looks like this check is done to validate if the metadata service is available before actually issuing a token. In my opinion this a good thing to validate, however there might be a better way to check this and not get rate limited. Otherwise a retry mechanism (with exponential backoff) could be useful, especially when kicking of a higher number of tasks.
EvertonSA commentedon Mar 19, 2025
there is no need to check for the metadata to be available, if its not available it will fail anyway
EvertonSA commentedon Mar 19, 2025
Out of all integrations available for airflow and Microsoft products this is the only one that checks the metadata availability before issuing the token. The code right now hammers the metadata api even if the token is valid.
rxxj25 commentedon Mar 30, 2025
HI,can I work on this?
hakunamatata-sb commentedon May 28, 2025
@rxxj25 are you working on this ?, would also like to contribute..
@EvertonSA if noone is working on this can you assign this to me ?
EvertonSA commentedon May 29, 2025
hi @hakunamatata-sb , i cant assign issues, please feel free to work on it. the draft is here #47956
xchwan commentedon Sep 12, 2025
None work on it, so I make a PR to handle it. I think remove the check function is not a good way. So, I add exponential back-off. Once it get 429 it will delay and retry
xchwan commentedon Sep 19, 2025
@EvertonSA Could you help me check whether this change working? I'm not sure how to reproduce your case and I don't have databricks service. It will call service API in almost same time If you have multiple test running. Once the first task call, the second will get 429. However, it will wait and call again since exponential back-off due to this change.
EvertonSA commentedon Sep 19, 2025
@xchwan it's nearly impossible to me to test if your change is working as the condition only happens on very specific situations of high load.
Are you sure? I doubt that is that simple colleague. You need way more load than that, approximately 5RPS.
I express my opinion that this check is non sense and yet you said "remove the check function is not a good way". I could help you test if the check was removed. I can't help you test this current implementation.
xchwan commentedon Sep 19, 2025
Thanks your reply. But I'm wondering that the check function is called at _get_token if statement and it do _get_aad_token function after check. Does _get_aad_token not have this limit? I mean _get_add_token may possible get 429 even thought remove check function in the case you mention. You still call too much API to Databricks, right?
EvertonSA commentedon Sep 19, 2025
Hi you can see the implementation here https://github.com/apache/airflow/blob/e66b237238c6eef1826c001e584a1992b3d879e7/providers/databricks/src/airflow/providers/databricks/hooks/databricks_base.py#L297C9-L297C23 it caches the auth token so it does not instantiate ManagedIdentityCredential.
@xchwan I can't test your change.
xchwan commentedon Sep 19, 2025
I also add cache for the check, you can find
if self._metadata_cache and time.time() < self._metadata_expiry:in PR. You said it cache can work means it already get info. So I guess it possible to handle.It is ok that you can't test. I just want to know you opinion.
EvertonSA commentedon Sep 19, 2025
Let me explain before early conclusions are taken: for me, there is no point of checking if API Internals are available before calling the API.
Lets say for example you are using google paid search. google search uses multiple internals to work (dns, auth, cache, etc etc). It is nonsense to test if the google search internals are available before calling the google search. Or imagine you are using a payment gateway configured with bank1 and bank2 and in your code you check the availability of bank1 and bank2 before calling the payment gateway. It seems like a claptrap.
I have opened this issue with the intention of removing this check. I believe it is Microsoft ManagedIdentityCredential responsibility to deal with unhealthy metadata API, not Airflow, not us. Checking this metadata API does not add value and does not solve the problem as already vastly researched in classic TOCTOU bugs. I support encapsulation in this case by hiding implementation details behind a stable interface (ManagedIdentityCredential, backed by trillion dollar company Microsoft), making us, airflow users/developers merely consumers of a abstract interface instead of having to deal with internals. In addition, Microsoft is free to change it's internals. I can also bring dependency inversion principles, separation of concerns, and design patterns to this as well, but I'm no expert.
If you need to report to the community if this is working or not, just triggering a simple databricks is sufficient. The effort and money spent trying to replicate this 429 depends on so many runs that will spend too much money from the company I work for.
EvertonSA commentedon Sep 19, 2025
also, I think it's ok to close this issue. I'm sure 429 won't happen now.