Re: [PR] Bugfix/fix spark k8s pod duplicate issue [airflow]

["SameerMesiah97 (via GitHub)" <gi...@apache.org>]

SameerMesiah97 commented on PR #61110:
URL: https://github.com/apache/airflow/pull/61110#issuecomment-3831664445

   @Nataneljpwd
   
   I still have two blocking concerns here:
   
   1) I don’t think the quorum-read argument holds. Not setting `resourceVersion` may require a quorum read , but that only guarantees freshness of what the API server returns. It doesn’t mean we’re reading directly from etcd, and more importantly it doesn’t enforce any stronger invariants (like uniqueness or lifecycle ordering).
   
   2) Even if the API response reflects the latest etcd state, Kubernetes does not guarantee that there can’t be multiple pods with the same labels in `Pending` or `Running` at the same time. I understand the SparkApplication controller intends there to be a single driver, but that guarantee is eventual. During restarts or reconciliation, overlapping driver pods are possible and expected. Your filter for deletion_timestamp partially mitigates against this by exlcuding terminating pods but I suspect there can still be windows where the replacement pod is created before the deletion for the existing pod is triggered. 
   
   Because of that, removing the existing phase-based prioritization is risky. I see that you are now prioritizing 'Succeeded' pods but since you have removed the ordering for `Running` and `Pending` pods, this does not mitigate the concerns I explained above.
   
   I would advise you to implement something like this:
   
   ```
   pod = max(
       pod_list,
       key=lambda p: (
       p.metadata.deletion_timestamp is None,
       p.status.phase == PodPhase.RUNNING,
       p.status.phase == PodPhase.SUCCEEDED,
       p.metadata.creation_timestamp or datetime.min.replace(tzinfo=timezone.utc),
       p.metadata.name or "",
        ),
   )
   ```
   This handles the edge cases revealed by this PR without accidentally returning multiple pods. I would advise removing the field selector as well because we are already filtering in the lambda expression. 


-- 
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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org