[AIRFLOW-1746] Add a Nomad operator to trigger job from Airflow #2708

etrabelsi · 2017-10-19T18:22:37Z

Add a new operator to run a job on nomad.

Description

Nomad allows declarative job file for scheduling virtualized, containerized, and standalone applications. For now it make sense to run only batch jobs because I wait for termination, and services doesn't terminate (if its seems essential its small code change).

Tests

[] Written tests for hooks

JIRA

My PR addresses the following Airflow JIRA1746 issues and references them in the PR title. For example, "[AIRFLOW-XXX] My Airflow PR"
https://issues.apache.org/jira/browse/AIRFLOW-1746

…omad and wait for success in semiliar way to boto-core waiter

Acehaidrey · 2017-10-20T01:22:07Z

You'll need to also provide tests. You will also want to create a JIRA and tie this to that. It's just the procedure to get your PR reviewed.
But this is interesting operator! Cool work.

etrabelsi · 2017-12-17T09:48:56Z

@Acehaidrey

should i use connection_id instead of expecting to get ip and port ? or should it be implentation detail of the person who uses it?
where do i add dependency of the python-nomad == 0.5.0?
examples needed to be added?
documentation ?

etrabelsi · 2018-03-21T11:07:25Z

@Acehaidrey

Acehaidrey · 2018-03-21T18:50:14Z

Hi @etrabelsi I am so sorry for missing your comments in December, I never saw them. Let me respond in a bit

christopherRiddersater · 2018-04-06T07:33:48Z

@Acehaidrey
Have you had any time to look into this regarding the questions from @etrabelsi?
It would be great to have this feature.

Fokko · 2018-04-09T07:43:42Z

Could you extend the tests a bit? The current test is really minimal. At least I would expect mocking some of the response objects.

etrabelsi · 2018-04-09T07:46:33Z

@Fokko
I have no problem, let me know what you want me to test more and i will add it.
Regarding my questions:

should i use connection_id instead of expecting to get ip and port ? or should it be implentation detail of the person who uses it?
where do i add dependency of the python-nomad == 0.5.0?

Fokko · 2018-04-09T08:12:47Z

Yes, good point. Your current code isn't fully in line with the design of Airflow. You should get the ip and port form the connection. Like:

conn = self.get_connection(self.redis_conn_id)
self.host = conn.host
self.port = int(conn.port)

https://github.com/apache/incubator-airflow/blob/master/airflow/contrib/hooks/redis_hook.py

The dependency should be added to the setup.py in the root.

I would like to see mocking these calls: self.nomad_client.job.get_allocations(job_id). These calls should be mocked so you get an example response, and some edge cases. What will happen if the job doesn't exists, or if the connection to Nomad times out.

Hope this helps, if there are any further questions, please let me know.

Cheers, Fokko

Acehaidrey · 2018-04-10T02:26:46Z

Hi sorry for late responses. Let me know if Fokko's comments make sense. I can help you in terms of connection and where to put that dependency if you mock a test.

etrabelsi · 2018-04-10T07:29:47Z

@Fokko regarding your statements:

"I would like to see mocking these calls: self.nomad_client.job.get_allocations(job_id)"

This is python-nomad call which is tested https://github.com/jrxFive/python-nomad/blob/6352f513d873710a51be62f24a59c5be66f28ef2/tests/test_job.py
any other test?

@Acehaidrey
I wrote the change and will add it :)

- Installion of python-nomad dependency

ali5h · 2018-05-07T10:12:50Z

Nomad looks more like an airflow executer instead of operator

etrabelsi · 2018-05-07T15:23:33Z

@Fokko regarding your statements:
"I would like to see mocking these calls: self.nomad_client.job.get_allocations(job_id)"

This is python-nomad call which is tested https://github.com/jrxFive/python-nomad/blob/6352f513d873710a51be62f24a59c5be66f28ef2/tests/test_job.py
any other test?

@Fokko @Acehaidrey I see that the tests fail on "The job exceeded the maximum time limit for jobs, and has been terminated." what should i do ?

@shariat why the fact that it can be used as executor contradicting the usability of an operator? (there are similar example for operators jenkins operator,ecs operator.

ali5h · 2018-05-07T16:18:27Z

You certainly can. Imho, u probably will uncover much more potential by using it as an executer

eyaltrabelsi · 2018-05-20T15:48:16Z

@Fokko @Acehaidrey

Dannnir · 2018-06-11T12:48:34Z

@etrabelsi @Fokko @Acehaidrey we really need this feature, any chance to release it soon?

Fokko · 2018-06-11T18:40:56Z

README.md

@@ -240,7 +240,7 @@ Currently **officially** using Airflow:
 1. [Zenly](https://zen.ly) [[@cerisier](https://github.com/cerisier) & [@jbdalido](https://github.com/jbdalido)]
 1. [Zymergen](https://www.zymergen.com/)
 1. [99](https://99taxis.com) [[@fbenevides](https://github.com/fbenevides), [@gustavoamigo](https://github.com/gustavoamigo) & [@mmmaia](https://github.com/mmmaia)]
-
+1. [Yotpo](https://www.yotpo.com) [[@etrabelsi](https://github.com/etrabelsi)]


The list is alphabetical

Fokko · 2018-06-11T18:43:15Z

airflow/contrib/hooks/nomad_hook.py

+        self.connection = self.get_connection(nomad_conn_id)
+        self.nomad_client = self.get_nomad_client(*args, **kwargs)
+
+    def get_nomad_client(self, *args, **kwargs):


This one should override get_conn, please refer to other hooks, for example: https://github.com/apache/incubator-airflow/blob/master/airflow/hooks/http_hook.py

@Fokko what do you mean get_nomad_client returns a client and not a connection do I need get_conn anyway ? thanks

But the client represents a connection, right?

@Fokko not an airflow connection. its more like client/resource of boto3 or jdbc for example

From what I understand from a quick check, the get_conn seems to be intended to be the convention on a hook for retrieving a connection for an external resource, and it typically uses an airflow connection to supply the connection details. It appears that the proposed function here does that, it's just named get_nomad_client instead.

In this particular case, it seems that simply renaming get_nomad_client to get_conn would be sufficient. Does that sound right?

Fokko · 2018-06-11T18:43:57Z

airflow/contrib/operators/batch_nomad_operator.py

+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+"""


Please put this comment in the docs of the Operator itself, for example: https://github.com/apache/incubator-airflow/blob/master/airflow/hooks/http_hook.py#L29

Fokko · 2018-06-11T18:44:06Z

airflow/contrib/hooks/nomad_hook.py

+# limitations under the License.
+#
+
+""" Hook to manage connection to nomad server


Please put this comment in the docs of the Hook itself, for example: https://github.com/apache/incubator-airflow/blob/master/airflow/hooks/http_hook.py#L29

Fokko · 2018-06-11T18:45:11Z

setup.py

@@ -280,6 +281,7 @@ def do_setup():
            'devel_hadoop': devel_hadoop,
            'doc': doc,
            'docker': docker,
+


Remove this newline

potiuk · 2019-09-11T23:16:44Z

Interesting why it has been closed even if you commented it :)

potiuk · 2019-09-11T23:18:17Z

A number of other PRs were un-staled after comments, so this one is strange :D

yishan-lin · 2019-10-08T18:16:14Z

@djenriquez Apologies for the delay. As of now, it seems more likely that we may architect some parts of a potential Airflow integration differently. We'd likely not shepherd this exact PR through once we get started but will need to scope it out first. :)

yishan-lin · 2019-10-10T18:04:30Z

In the meantime, we would love people's +1s on this ticket to the Apache Airflow Committee, so we can optimize for building and providing users with a first-class integration, rather than the incomplete experience of a lagging fork.

https://issues.apache.org/jira/browse/AIRFLOW-5633

stale · 2019-11-24T18:44:50Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

shantanugadgil · 2019-11-25T02:56:47Z

I have added "+1" on this issue, though I see the "stale-bot" marking this as "stale".

Is this Github issue and JIRA ticket (https://issues.apache.org/jira/browse/AIRFLOW-5633) still on the radar? :wonder:

shantanugadgil · 2019-11-25T03:00:08Z

Should we login/signup on the Apache JIRA and then click "Vote for this issue"? (I have done that as well). Hope this gets implemented soon! :)

KarimZaoui · 2019-12-10T12:11:14Z

@yiheng Any updates on this?

LanceNorskog · 2019-12-20T21:57:43Z

I have not used this PR, but based on experience with other Airflow user-contributed code: Please, please pull back all logs from executed Nomad jobs into the Airflow task log. If you already are, that's great, you're already doing better than the GCP control tasks :)

Cheers!

stale · 2020-02-03T22:04:46Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

shantanugadgil · 2020-02-18T12:07:59Z

just checking to see if posting a comment revives this issue.

yishan-lin · 2020-03-10T18:32:16Z

This issue got closed by stalebot, but a first-class Airflow integration is on the roadmap for us internally. We're working on some other integrations first (e.g Spinnaker) before we'll get there but still in plans!

potiuk · 2020-03-10T20:37:59Z

Looking forward to it! If you need any help with that - please let us know. Happy to help!

retarpt · 2021-05-25T20:08:39Z

Any updates on this?

sblanton · 2021-06-16T16:06:01Z

Very excited about this, Stalebot!

retarpt · 2021-06-16T17:04:30Z

🤕 🤞

Oloremo · 2021-06-25T18:28:53Z

@yishan-lin any updates on this?

leonardobsjr · 2021-07-03T00:29:24Z

Please revive this :|

ashb · 2021-07-08T10:29:44Z

@leonardoam We'll happily review and accept such a PR, so "well volunteered?" 😁

sblanton · 2021-07-08T14:49:37Z

What are the next steps to get this "official"...I'll see what I can do. I know nothing about the process and procedures. What I do know is that Airflow crashed not 15 minutes ago due to exceeding available resources with that Docker Compose garbage. This would not have happened with Nomad managing resources. This makes AIRFLOW LOOK BAD and management are never going to look at the details. "Airflow crashed again" is what management hears and they think "Let's use something that doesn't crash"

sblanton · 2021-07-08T14:56:54Z

And isn't this issue already a PR? I don't see any response... who are the reviewers? What needs to be done?

potiuk · 2021-07-08T15:07:56Z

The issue has been closed due to inactivity of the original creator @sblanton. We need someone who "owns" the PR and leads it to successful completion.

The process is described here https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst (and you have even https://github.com/apache/airflow/blob/main/CONTRIBUTORS_QUICK_START.rst ). Airflow is a community managed project so you are free to take overy and lead it to completion. I think it could be a nice contribution (though you might mistake Nomad operator with Nomad managing resources for Airflow). Those are two different things and I doubt either would solve the deployment problem you have.

BTW. The officially released and community supported solution for deploying Airflow is Helm Chart. https://airflow.apache.org/docs/helm-chart/stable/index.html and there you can manage your resources as you wish.

The docker compose we publish here: https://airflow.apache.org/docs/apache-airflow/stable/start/docker.html is a quick-start for development only . However there is an open issue to add more production-ready examples (and maybe even a generator for docker-compose) #16031. Not sure if you would like to work on it but maybe that's a good idea? I can assign you the issue if you want.

sblanton · 2021-07-11T22:53:15Z

Excellent... thank you for the tips @potiuk . I'll review.

shantanugadgil · 2022-03-15T04:00:35Z

Is this still planned to be merged?

- feat(batch_nomad_operator) alllow to run a batch nomad job within n…

a3cf926

…omad and wait for success in semiliar way to boto-core waiter

etrabelsi changed the title ~~Nomad Operator~~ AIRFLOW-1746 Add a Nomad operator to trigger job from Airflow Oct 21, 2017

- feat(nomad_hook) adding nomad_hook

1c253af

etrabelsi force-pushed the master branch from 4af0c17 to 1c253af Compare December 17, 2017 09:57

- NomadHook to support Hook conventions

f691c7d

- Installion of python-nomad dependency

etrabelsi force-pushed the master branch 7 times, most recently from 6b9b1b0 to 1e29353 Compare May 1, 2018 06:21

Merge branch 'master' of https://github.com/apache/incubator-airflow

42444ea

etrabelsi force-pushed the master branch from 1e29353 to 42444ea Compare May 7, 2018 12:21

Fokko requested changes Jun 11, 2018

View reviewed changes

stale bot removed the stale Stale PRs per the .github/workflows/stale.yml policy file label Sep 11, 2019

kaxil force-pushed the master branch from 558f9f1 to d719e1f Compare October 2, 2019 15:31

stale bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Nov 24, 2019

stale bot removed the stale Stale PRs per the .github/workflows/stale.yml policy file label Nov 25, 2019

stale bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Feb 3, 2020

stale bot closed this Feb 10, 2020

[AIRFLOW-1746] Add a Nomad operator to trigger job from Airflow #2708

[AIRFLOW-1746] Add a Nomad operator to trigger job from Airflow #2708

Conversation

etrabelsi commented Oct 19, 2017 • edited

Description

Tests

JIRA

Acehaidrey commented Oct 20, 2017 • edited

etrabelsi commented Dec 17, 2017

etrabelsi commented Mar 21, 2018

Acehaidrey commented Mar 21, 2018

christopherRiddersater commented Apr 6, 2018

Fokko commented Apr 9, 2018

etrabelsi commented Apr 9, 2018

Fokko commented Apr 9, 2018

Acehaidrey commented Apr 10, 2018

etrabelsi commented Apr 10, 2018 • edited

ali5h commented May 7, 2018 • edited

etrabelsi commented May 7, 2018

ali5h commented May 7, 2018

eyaltrabelsi commented May 20, 2018

Dannnir commented Jun 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ndmar Sep 5, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

potiuk commented Sep 11, 2019

potiuk commented Sep 11, 2019

yishan-lin commented Oct 8, 2019

yishan-lin commented Oct 10, 2019

stale bot commented Nov 24, 2019

shantanugadgil commented Nov 25, 2019

shantanugadgil commented Nov 25, 2019

KarimZaoui commented Dec 10, 2019

LanceNorskog commented Dec 20, 2019

stale bot commented Feb 3, 2020

shantanugadgil commented Feb 18, 2020

yishan-lin commented Mar 10, 2020

potiuk commented Mar 10, 2020

retarpt commented May 25, 2021

sblanton commented Jun 16, 2021

retarpt commented Jun 16, 2021

Oloremo commented Jun 25, 2021

leonardobsjr commented Jul 3, 2021

ashb commented Jul 8, 2021

sblanton commented Jul 8, 2021

sblanton commented Jul 8, 2021

potiuk commented Jul 8, 2021

sblanton commented Jul 11, 2021 • edited

shantanugadgil commented Mar 15, 2022

etrabelsi commented Oct 19, 2017 •

edited

Acehaidrey commented Oct 20, 2017 •

edited

etrabelsi commented Apr 10, 2018 •

edited

ali5h commented May 7, 2018 •

edited

ndmar Sep 5, 2018 •

edited

sblanton commented Jul 11, 2021 •

edited