thc1006 opened a new pull request, #71744:
URL: https://github.com/apache/airflow/pull/71744

   closes: #71743
   
   ## Why the name alone is not enough
   
   `_persist_pod_identity_to_task_state_store` keep only name and namespace, 
and on retry `_get_pod_from_task_state_store` do `self.hook.get_pod(name, 
namespace)`. In Kubernetes a name is only a slot, after a pod is delete another 
pod can take the same name. So when the pod name can repeat, 
`random_name_suffix=False` or a fixed name from `pod_template_file` / 
`full_pod_spec`, the retry can reattach to a pod of another dag run. The real 
cluster log is in #71743.
   
   The old `find_pod()` path do not have this problem, its label selector carry 
run_id.
   
   ## What this change do
   
   Put the uid back into the persisted identity, and compare it after the pod 
is read back. If the uid is not the same, log and return None so it fall to the 
label search, the same way the 404 branch just above it already do.
   
   This is the first of the two option @kaxil write in the review of #69914:
   
   > I'd either compare the uid and fall back to label search on mismatch, or 
drop `uid` from the stored payload so the schema doesn't imply a check that 
isn't performed.
   
   The second one is take at that time, because a pod name normally carry a 
random suffix. That is true for the default, but `random_name_suffix=False` is 
a public parameter and then the name repeat.
   
   ## Identity persisted by 10.20 and 10.21
   
   Those rows have no uid so there is nothing to compare. The check is skip for 
them and the behaviour stay the same as today. Every identity write from this 
version carry the uid.
   
   ## Test
   
   Two new test in `TestKubernetesPodOperatorDurableExecution`:
   
   - `test_durable_reconnect_skips_pod_whose_uid_no_longer_matches`
   - `test_durable_reconnect_accepts_identity_persisted_without_uid`
   
   The existing durable test now carry a uid too, because the persisted payload 
have one more key.
   
   ```
   22 passed, 235 deselected
   ```
   
   The whole `test_pod.py` is 245 passed / 2 failed / 10 errors here, and the 
same 2 failed and 10 errors are already there on `main` without my change 
(`TestKubernetesPodOperatorAsync` and `TestSuppress`, they want things my local 
env do not have).
   
   ---
   
   ##### Was generative AI tooling used to co-author this PR?
   
   - [x] Yes - Claude Code
   
   This PR was written in part with the assistance of generative AI. I have 
reviewed and tested every change myself.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to