Skip to content

Conversation

@johnhofman
Copy link
Contributor

@johnhofman johnhofman commented Sep 27, 2018

Description

Creating a pod that exceeds a namespace's resource quota throws an ApiException. This change catches the exception and the task is re-queued inside the Executor instead of killing the scheduler.

click 7.0 was recently released but flask-appbuilder 1.11.1 has requirement click==6.7. I have pinned click==6.7 to make the dependencies resolve.

Tests

This adds a single test TestKubernetesExecutor. test_run_next_exception that covers this single scenario. Without the changes this test fails when the ApiException is not caught.

This is the first test case for the KubernetesExecutor, so I needed to add the [kubernetes] section to default_test.cfg so that the KubernetesExecutor can be built without exceptions.

Jira ticket: https://issues.apache.org/jira/browse/AIRFLOW-2966

@johnhofman
Copy link
Contributor Author

This build will fail because the recent release of pynacl needs make to install. Here is a PR to update the airflow-ci image to include make.

@codecov-io
Copy link

codecov-io commented Sep 28, 2018

Codecov Report

Merging #3960 into master will decrease coverage by 13.01%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #3960       +/-   ##
===========================================
- Coverage   76.67%   63.65%   -13.02%     
===========================================
  Files         199      199               
  Lines       16186    20018     +3832     
===========================================
+ Hits        12410    12743      +333     
- Misses       3776     7275     +3499
Impacted Files Coverage Δ
airflow/operators/docker_operator.py 0% <0%> (-97.65%) ⬇️
airflow/operators/slack_operator.py 0% <0%> (-97.37%) ⬇️
airflow/operators/s3_to_hive_operator.py 0% <0%> (-94.02%) ⬇️
airflow/security/kerberos.py 0% <0%> (-71.43%) ⬇️
airflow/operators/latest_only_operator.py 25% <0%> (-65%) ⬇️
airflow/hooks/S3_hook.py 54.06% <0%> (-40.27%) ⬇️
airflow/example_dags/example_trigger_target_dag.py 55% <0%> (-36.67%) ⬇️
...le_dags/example_passing_params_via_test_command.py 65.21% <0%> (-34.79%) ⬇️
airflow/example_dags/example_subdag_operator.py 65.38% <0%> (-34.62%) ⬇️
airflow/www_rbac/app.py 64.38% <0%> (-32.68%) ⬇️
... and 32 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e703d6b...8d2dbda. Read the comment docs.

@johnhofman
Copy link
Contributor Author

Travis passes in my environment, but the incubator-airflow build times-out on an unrelated test. Restarting the build may pass, since this seems to be a symptom of a flaky kubernetes test environment.

@Fokko
Copy link
Contributor

Fokko commented Oct 12, 2018

@johnhofman Can you rebase onto master?

Creating a pod that exceeds a namespace's resource quota throws an
ApiException. This change catches the exception and the task is
re-queued inside the Executor instead of killing the scheduler.
@johnhofman
Copy link
Contributor Author

@Fokko I have rebased. Is this failing test something I need to look into?

@Fokko
Copy link
Contributor

Fokko commented Nov 5, 2018

Sorry @johnhofman, I was a bit busy. I've restarted the failing test, let's see if it passes now. The Kubernetes tests can sometimes be a bit flaky.

@ron819
Copy link
Contributor

ron819 commented Nov 15, 2018

@Fokko tests passed

Copy link
Contributor

@Fokko Fokko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @johnhofman LGTM

@Fokko Fokko merged commit 03de9ee into apache:master Nov 18, 2018
Fokko pushed a commit to Fokko/incubator-airflow that referenced this pull request Nov 18, 2018
@Fokko
Copy link
Contributor

Fokko commented Nov 18, 2018

@johnhofman I had to revert the commit again. The CI wasn't happy. Please take a look and open a new PR: https://travis-ci.org/apache/incubator-airflow/jobs/456661023
Please tag me when I can take a look.

tmiller-msft pushed a commit to cse-airflow/incubator-airflow that referenced this pull request Nov 27, 2018
…3960)

Creating a pod that exceeds a namespace's resource quota throws an
ApiException. This change catches the exception and the task is
re-queued inside the Executor instead of killing the scheduler.
tmiller-msft pushed a commit to cse-airflow/incubator-airflow that referenced this pull request Nov 27, 2018
elizabethhalper pushed a commit to cse-airflow/incubator-airflow that referenced this pull request Dec 7, 2018
…3960)

Creating a pod that exceeds a namespace's resource quota throws an
ApiException. This change catches the exception and the task is
re-queued inside the Executor instead of killing the scheduler.
elizabethhalper pushed a commit to cse-airflow/incubator-airflow that referenced this pull request Dec 7, 2018
aliceabe pushed a commit to aliceabe/incubator-airflow that referenced this pull request Jan 3, 2019
…3960)

Creating a pod that exceeds a namespace's resource quota throws an
ApiException. This change catches the exception and the task is
re-queued inside the Executor instead of killing the scheduler.
aliceabe pushed a commit to aliceabe/incubator-airflow that referenced this pull request Jan 3, 2019
wmorris75 pushed a commit to modmed-external/incubator-airflow that referenced this pull request Jul 29, 2019
…3960)

Creating a pod that exceeds a namespace's resource quota throws an
ApiException. This change catches the exception and the task is
re-queued inside the Executor instead of killing the scheduler.
wmorris75 pushed a commit to modmed-external/incubator-airflow that referenced this pull request Jul 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants