Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[AIRFLOW-2568] Azure Container Instances operator #4121

Merged
merged 1 commit into from
Dec 26, 2018

Conversation

omusavi
Copy link
Contributor

@omusavi omusavi commented Nov 1, 2018

Add an operator to create a Docker container in Azure Container
Instances. Azure Container Instances hosts a container and abstracts
away the infrastructure around orchestration of a container service.

Operator supports creating an ACI container and pull an image from Azure
Container Registry or public Docker registries.

This PR takes the great work @NielsZeilemaker did with #3467 and fixes it to work
with the latest Azure SDK as well as adding an example DAG and some
minor correlating fixes.

Make sure you have checked all steps below.

Jira

  • My PR addresses the following Airflow Jira issues and references them in the PR title. For example, "[AIRFLOW-XXX] My Airflow PR"

Description

  • Here are some details about my PR, including screenshots of any UI changes:
    An Azure Container Instances operator which allows you to run/monitor containers on ACI

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:

Commits

  • My commits all reference Jira issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters (not including Jira issue reference)
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"

Documentation

  • In case of new functionality, my PR adds documentation that describes how to use it.
    • When adding new operators/hooks/sensors, the autoclass documentation generation needs to be added.

Code Quality

  • Passes flake8

@kaxil kaxil requested a review from Fokko November 2, 2018 11:33
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.

Some comments

@omusavi
Copy link
Contributor Author

omusavi commented Nov 7, 2018

Updated PR, thanks for the comments @Fokko! Let me know if there is anything else I can fix

@codecov-io
Copy link

codecov-io commented Nov 7, 2018

Codecov Report

Merging #4121 into master will increase coverage by 61.98%.
The diff coverage is 0%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #4121       +/-   ##
===========================================
+ Coverage   16.13%   78.11%   +61.98%     
===========================================
  Files         202      202               
  Lines       16483    16484        +1     
===========================================
+ Hits         2660    12877    +10217     
+ Misses      13823     3607    -10216
Impacted Files Coverage Δ
airflow/models/connection.py 88.6% <ø> (+65.82%) ⬆️
airflow/utils/db.py 32.28% <0%> (+17.2%) ⬆️
airflow/plugins_manager.py 93.1% <0%> (+1.14%) ⬆️
airflow/executors/dask_executor.py 2% <0%> (+2%) ⬆️
airflow/exceptions.py 100% <0%> (+2.85%) ⬆️
airflow/utils/operator_resources.py 86.95% <0%> (+4.34%) ⬆️
airflow/executors/__init__.py 63.46% <0%> (+5.76%) ⬆️
airflow/utils/module_loading.py 100% <0%> (+10.52%) ⬆️
airflow/utils/decorators.py 91.66% <0%> (+14.58%) ⬆️
airflow/settings.py 80.41% <0%> (+14.68%) ⬆️
... and 163 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 f211390...0b6dea0. Read the comment docs.

@omusavi omusavi force-pushed the aci/squashed branch 2 times, most recently from a74f41e to c65dc8b Compare November 14, 2018 02:38
@omusavi
Copy link
Contributor Author

omusavi commented Nov 14, 2018

Removed extra line and rebased onto master so it is all up to date. Thanks @Fokko


def __init__(self, conn_id='azure_registry'):
self.conn_id = conn_id
self.connection = self.get_conn()
Copy link
Contributor

Choose a reason for hiding this comment

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

Unused.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like self.connection gets used inside azure_container_instances_operator.py when setting up the registry hook. Is there something else I am missing here? Design is a bit confusing admittedly as this was code I picked up from an existing PR so if there is some best practice here to change the code please let me know

airflow/models.py Outdated Show resolved Hide resolved
docs/integration.rst Outdated Show resolved Hide resolved
@omusavi
Copy link
Contributor Author

omusavi commented Nov 19, 2018

Apologies for the bad iteration after splitting out the files @Fokko . Everything should be up to date now and I have verified the docs are updated and verified the scenario works end-to-end. Let me know if there is anything else (particularly with the one remaining unresolved comment)

@omusavi
Copy link
Contributor Author

omusavi commented Nov 26, 2018

Noticed there were some merge conflicts in integration.rst since last week so I fixed that and pushed again

@omusavi
Copy link
Contributor Author

omusavi commented Nov 26, 2018

Flaky tests caused a build failure, could somebody with write access restart the job that failed? It passed in our forked repo so doesn't look like a code failure:
https://travis-ci.org/cse-airflow/incubator-airflow/builds/459789200

Otherwise I can close and re-open PR or rebase again once there are new commits

@omusavi
Copy link
Contributor Author

omusavi commented Nov 27, 2018

Rebased again to get a new build. @Fokko, please take a look when you get a chance, hope it is ready to go. Thanks!

@feluelle
Copy link
Member

feluelle commented Dec 4, 2018

Could you please add tests for the hook, too? :)

@omusavi omusavi force-pushed the aci/squashed branch 3 times, most recently from 3c0e3a0 to 5aa8a7a Compare December 20, 2018 21:27
Add an operator to create a Docker container in Azure Container
Instances. Azure Container Instances hosts a container and abstracts
away the infrastructure around orchestration of a container service.

Operator supports creating an ACI container and pull an image from Azure
Container Registry or public Docker registries.
@omusavi
Copy link
Contributor Author

omusavi commented Dec 21, 2018

Thanks to all the previous commenters! The PR has now been updated to include full unit testing for all the hooks. I have also rebased against the latest master and pulled in the connections.py changes.

Thanks @Fokko and @feluelle for the comments, please review when you get a chance. Happy holidays!

@Fokko Fokko merged commit 01880dc into apache:master Dec 26, 2018
aliceabe pushed a commit to aliceabe/incubator-airflow that referenced this pull request Jan 3, 2019
Add an operator to create a Docker container in Azure Container
Instances. Azure Container Instances hosts a container and abstracts
away the infrastructure around orchestration of a container service.

Operator supports creating an ACI container and pull an image from Azure
Container Registry or public Docker registries.
ashb pushed a commit that referenced this pull request Mar 21, 2019
Add an operator to create a Docker container in Azure Container
Instances. Azure Container Instances hosts a container and abstracts
away the infrastructure around orchestration of a container service.

Operator supports creating an ACI container and pull an image from Azure
Container Registry or public Docker registries.
ashb pushed a commit that referenced this pull request Mar 22, 2019
Add an operator to create a Docker container in Azure Container
Instances. Azure Container Instances hosts a container and abstracts
away the infrastructure around orchestration of a container service.

Operator supports creating an ACI container and pull an image from Azure
Container Registry or public Docker registries.
wmorris75 pushed a commit to modmed/incubator-airflow that referenced this pull request Jul 29, 2019
Add an operator to create a Docker container in Azure Container
Instances. Azure Container Instances hosts a container and abstracts
away the infrastructure around orchestration of a container service.

Operator supports creating an ACI container and pull an image from Azure
Container Registry or public Docker registries.
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.

5 participants