[GitHub] [airflow] dstandish commented on a change in pull request #19359: Task should fail immediately when pod is unprocessable

[GitBox <gi...@apache.org>]

dstandish commented on a change in pull request #19359:
URL: https://github.com/apache/airflow/pull/19359#discussion_r741325058



##########
File path: airflow/executors/kubernetes_executor.py
##########
@@ -599,13 +599,14 @@ def sync(self) -> None:
                 try:
                     self.kube_scheduler.run_next(task)
                 except ApiException as e:
-                    if e.reason == "BadRequest":
-                        self.log.error("Request was invalid. Failing task")
+                    if e.reason in ("BadRequest", "Unprocessable Entity"):
+                        self.log.error(f"Pod creation failed with reason {e.reason!r}. Failing task")

Review comment:
       It looks like we could do that:
   
   ```
           - 403 Forbidden will be returned when your request exceeds namespace quota.
           - 422 Unprocessable Entity is returned when your parameters are valid but unsupported
               e.g. limits lower than requests.
           - 400 BadRequest is returned when your parameters are invalid e.g. asking for cpu=100ABC123.
   ```
   
   I guess one reason to use `reason` in the if statement though, is that it's english.  So we can read
   
   ```
   if e.reason in ("BadRequest", "Unprocessable Entity"):
   ```
   
   rather than
   
   ```
   if e.status in (400, 422):
   ```
   
   because the former is more descriptive / self-documenting.  
   
   But I also don't have a strong opinion on this.

##########
File path: airflow/executors/kubernetes_executor.py
##########
@@ -599,13 +599,14 @@ def sync(self) -> None:
                 try:
                     self.kube_scheduler.run_next(task)
                 except ApiException as e:
-                    if e.reason == "BadRequest":
-                        self.log.error("Request was invalid. Failing task")
+                    if e.reason in ("BadRequest", "Unprocessable Entity"):
+                        self.log.error(f"Pod creation failed with reason {e.reason!r}. Failing task")

Review comment:
       Do you mean we should use the status code instead of the reason text?
   
   It looks like we could do that:
   
   ```
           - 403 Forbidden will be returned when your request exceeds namespace quota.
           - 422 Unprocessable Entity is returned when your parameters are valid but unsupported
               e.g. limits lower than requests.
           - 400 BadRequest is returned when your parameters are invalid e.g. asking for cpu=100ABC123.
   ```
   
   I guess one reason to use `reason` in the if statement though, is that it's english.  So we can read
   
   ```
   if e.reason in ("BadRequest", "Unprocessable Entity"):
   ```
   
   rather than
   
   ```
   if e.status in (400, 422):
   ```
   
   because the former is more descriptive / self-documenting.  
   
   But I also don't have a strong opinion on this.




-- 
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