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-5221] add host_aliases to KubernetesPodOperator #7365

Closed

Conversation

kuromt
Copy link

@kuromt kuromt commented Feb 5, 2020

add host_aliases paramter to KubernetesPodOperator.
The parameter is set to pod.spec.hostAliases.
Please see detail in the kubernetes document.


Issue link: AIRFLOW-5221

Make sure to mark the boxes below before creating PR: [x]

  • Description above provides context of the change
  • Commit message/PR title starts with [AIRFLOW-NNNN]. AIRFLOW-NNNN = JIRA ID*
  • Unit tests coverage for changes (not needed for documentation changes)
  • Commits follow "How to write a good git commit message"
  • Relevant documentation is updated including usage instructions.
  • I will engage committers as explained in Contribution Workflow Example.

* For document-only changes commit message can start with [AIRFLOW-XXXX].


In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.
Read the Pull Request Guidelines for more information.

@boring-cyborg boring-cyborg bot added the k8s label Feb 5, 2020
@boring-cyborg
Copy link

boring-cyborg bot commented Feb 5, 2020

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst)
Here are some useful points:

  • Pay attention to the quality of your code (flake8, pylint and type annotations). Our pre-commits will help you with that.
  • In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example DAG that shows how users should use it.
  • Consider using Breeze environment for testing locally, it’s a heavy docker but it ships with a working Airflow and a lot of integrations.
  • Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
    Apache Airflow is a community-driven project and together we are making it better 🚀.
    In case of doubts contact the developers at:
    Mailing List: dev@airflow.apache.org
    Slack: https://apache-airflow-slack.herokuapp.com/

@kuromt kuromt requested a review from mik-laj February 7, 2020 01:30
@kuromt kuromt force-pushed the hostaliases-kubernetespodoperator branch from 5620a8d to 9522d77 Compare February 7, 2020 15:46
@codecov-io
Copy link

codecov-io commented Feb 7, 2020

Codecov Report

Merging #7365 into master will decrease coverage by 27.24%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #7365       +/-   ##
===========================================
- Coverage   87.17%   59.93%   -27.25%     
===========================================
  Files         933      933               
  Lines       45290    45292        +2     
===========================================
- Hits        39482    27145    -12337     
- Misses       5808    18147    +12339     
Impacted Files Coverage Δ
airflow/kubernetes/pod_generator.py 96.53% <100.00%> (+0.01%) ⬆️
...viders/cncf/kubernetes/operators/kubernetes_pod.py 70.00% <100.00%> (-24.95%) ⬇️
airflow/providers/amazon/aws/hooks/kinesis.py 0.00% <0.00%> (-100.00%) ⬇️
airflow/providers/apache/livy/sensors/livy.py 0.00% <0.00%> (-100.00%) ⬇️
airflow/providers/amazon/aws/sensors/redshift.py 0.00% <0.00%> (-100.00%) ⬇️
airflow/providers/postgres/operators/postgres.py 0.00% <0.00%> (-100.00%) ⬇️
airflow/providers/microsoft/azure/operators/adx.py 0.00% <0.00%> (-100.00%) ⬇️
...irflow/providers/amazon/aws/hooks/batch_waiters.py 0.00% <0.00%> (-100.00%) ⬇️
...ow/providers/amazon/aws/sensors/cloud_formation.py 0.00% <0.00%> (-100.00%) ⬇️
...w/providers/apache/hive/operators/mysql_to_hive.py 0.00% <0.00%> (-100.00%) ⬇️
... and 313 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 6018532...22b02e8. Read the comment docs.

@kuromt kuromt force-pushed the hostaliases-kubernetespodoperator branch from 9522d77 to fd68dc8 Compare February 8, 2020 12:58
@kuromt kuromt requested a review from ashb February 9, 2020 03:02
@kuromt kuromt force-pushed the hostaliases-kubernetespodoperator branch from f6e6209 to e9ad153 Compare February 12, 2020 06:19
@ashb ashb requested a review from dimberman February 13, 2020 11:12
Copy link
Member

@ashb ashb left a comment

Choose a reason for hiding this comment

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

Code okay.

@dimberman Do we want to carry on down this route or are we close to having your re-work of the entire kube executor config?

airflow/kubernetes/pod_generator.py Show resolved Hide resolved
@kuromt
Copy link
Author

kuromt commented Feb 14, 2020

No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.
Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#build-times-out-because-no-output-was-received
The build has been terminated

@ashb I merged your request, but It seems that travis has a problem.
Could you retry the building job?

@dimberman
Copy link
Contributor

@ashb as the config rewrite is down the road I'm good to merge this.

@kuromt
Copy link
Author

kuromt commented Mar 25, 2020

@ashb as the config rewrite is down the road I'm good to merge this.

Thanks!

pod_generator.py is updated after my PR, so If I needed to rebase and commit again to merge this PR, please let me know.

@dimberman
Copy link
Contributor

tests/runtime/kubernetes/test_kubernetes_executor.py .F

____ TestKubernetesExecutor.test_integration_run_dag_with_scheduler_failure ____

self = <tests.runtime.kubernetes.test_kubernetes_executor.TestKubernetesExecutor testMethod=test_integration_run_dag_with_scheduler_failure>

    def test_integration_run_dag_with_scheduler_failure(self):

        host = KUBERNETES_HOST

        dag_id = 'example_kubernetes_executor_config'

    

        result_json = self.start_dag(dag_id=dag_id, host=host)

    

        self.assertGreater(len(result_json['items']), 0)

    

        execution_date = result_json['items'][0]['execution_date']

        print("Found the job with execution date {}".format(execution_date))

    

        self._delete_airflow_pod()

    

        time.sleep(10)  # give time for pod to restart

    

        # Wait some time for the operator to complete

        self.monitor_task(host=host,

                          execution_date=execution_date,

                          dag_id=dag_id,

                          task_id='start_task',

                          expected_final_state='success', timeout=200)

    

        self.monitor_task(host=host,

                          execution_date=execution_date,

                          dag_id=dag_id,

                          task_id='other_namespace_task',

>                         expected_final_state='success', timeout=200)

tests/runtime/kubernetes/test_kubernetes_executor.py:231: 

_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

tests/runtime/kubernetes/test_kubernetes_executor.py:107: in monitor_task

    self.assertEqual(state, expected_final_state)

E   AssertionError: '' != 'success'

E   + success

@kuromt
Copy link
Author

kuromt commented Mar 30, 2020

Thanks for your pointing.
Aftrer I rebased current apache/master to this PR in local branch, tests are passed.

Test failed replicates in my environemnt.
I'll debug.

@kuromt
Copy link
Author

kuromt commented Mar 30, 2020

After extending timeout to 200, the test passed.

pytest tests/runtime/kubernetes/test_kubernetes_executor.py 
==================================================================== test session starts ====================================================================
platform linux -- Python 3.6.10, pytest-5.4.1, py-1.8.1, pluggy-0.13.1 -- /usr/local/bin/python
cachedir: .pytest_cache
rootdir: /opt/airflow, inifile: pytest.ini
plugins: celery-4.4.2, requests-mock-1.7.0, instafail-0.4.1.post0, cov-2.8.1, flaky-3.6.1
collected 2 items                                                                                                                                           

tests/runtime/kubernetes/test_kubernetes_executor.py::TestKubernetesExecutor::test_integration_run_dag PASSED                                         [ 50%]
tests/runtime/kubernetes/test_kubernetes_executor.py::TestKubernetesExecutor::test_integration_run_dag_with_scheduler_failure PASSED                  [100%]

=============================================================== 2 passed in 613.74s (0:10:13) ===============================================================

@kuromt kuromt force-pushed the hostaliases-kubernetespodoperator branch 2 times, most recently from bb30a62 to 22b02e8 Compare March 30, 2020 13:44
@kuromt
Copy link
Author

kuromt commented Apr 2, 2020

The latest test failed due to timeout, but this fail is due to timing problem.
Should I kick off test again?

@jeffolsi
Copy link

this is very much needed. what is the status?

@dimberman
Copy link
Contributor

Hi @kuromt could you please rebase with master? When those tests pass I'm good to merge

@kuromt kuromt force-pushed the hostaliases-kubernetespodoperator branch from 23dd78e to 2101c9b Compare May 16, 2020 07:41
@kuromt
Copy link
Author

kuromt commented May 16, 2020

@dimberman

Hi @kuromt could you please rebase with master? When those tests pass I'm good to merge

I rebase with master and pused to my branch.
But an integration test was canceled with ##[error]The operation was canceled. message.

I don't have permission to retry this test. Can you retry this test?
https://github.com/apache/airflow/pull/7365/checks?check_run_id=680520307

@kuromt kuromt force-pushed the hostaliases-kubernetespodoperator branch 7 times, most recently from 603555f to dbfd281 Compare May 21, 2020 14:18
@kuromt kuromt force-pushed the hostaliases-kubernetespodoperator branch 5 times, most recently from 8257889 to 6a1aafd Compare May 28, 2020 12:42
@kuromt kuromt force-pushed the hostaliases-kubernetespodoperator branch from 6a1aafd to 425f416 Compare June 4, 2020 12:03
@kuromt kuromt force-pushed the hostaliases-kubernetespodoperator branch from 425f416 to 976be5a Compare June 17, 2020 12:48
@Ziliboba
Copy link

Ziliboba commented Jul 19, 2020

Hello, could you advice - will these changes get merged any time soon? This functionality would be helpful.

@kuromt kuromt force-pushed the hostaliases-kubernetespodoperator branch from 976be5a to 9229fcd Compare July 20, 2020 00:37
@mik-laj
Copy link
Member

mik-laj commented Jul 20, 2020

I'm not a Kubernetes expert, but can't this option be configured with pod or pod_template_file parameter?

    :param pod: The fully specified pod. Mutually exclusive with `path_or_string`
    :type pod: Optional[kubernetes.client.models.V1Pod]
    :param pod_template_file: Path to YAML file. Mutually exclusive with `pod`
    :type pod_template_file: Optional[str]

@kuromt kuromt force-pushed the hostaliases-kubernetespodoperator branch from 9229fcd to c8763bf Compare July 20, 2020 11:33
@kuromt
Copy link
Author

kuromt commented Jul 20, 2020

@mik-laj pod_template_file and pod are solutions for host_aliases.
But host_aliases is useful because pod_template_file and pod parameters requires rest of whole complicate parameters of Pod and decreases flexibility.

@kuromt
Copy link
Author

kuromt commented Jul 20, 2020

Today I run CI test but some tests failed.

I fix my bug and run CI test again.

@mik-laj
Copy link
Member

mik-laj commented Jul 20, 2020

@kuromt Our mistake was to add a large number of parameters to this operator. This makes it very difficult to maintain. We are now trying to change the approach and recommend using the above-mentioned parameters. We know that changing habits can be problematic for many users and there were even command that would facilitate migrations, but it is not finished yet.
#8009
Would you like to help develop this command? In my opinion, this is a better solution and more forward-looking. The use of the Kubernetes API Object means we have better documentation as we can refer to the official documentation and it also makes it easier to use this operator as you can easily copy code snippets from the internet. You too always have access to the latest Kubernetes feature.

If we do not take steps in this direction, we will be sad all the time because we will be missing new Kubernetes features in Airfllow. We as project maintainers are also bored as we add/review every new feature the user may need. I hope that the presented description of the situation will allow you to better understand the problem that we have.

Do you have any other idea to solve this problem? I am open to discussion.

CC: @dimberman @kaxil

@kuromt
Copy link
Author

kuromt commented Jul 20, 2020

@mik-laj Thanks.
I agree that maintaing many parameters in KubernetesPodOperator is not a good way.

Let me confirm that the command is helper tool to generate API Object from existing dag files for pod or pod_template_file parameter, right?
I am interested in the idea.
To help developing the command, should I create new PR?

@kuromt kuromt force-pushed the hostaliases-kubernetespodoperator branch from c8763bf to 9285196 Compare July 20, 2020 12:40
@mik-laj
Copy link
Member

mik-laj commented Jul 20, 2020

@kuromt The new PR is a good idea.

@mik-laj
Copy link
Member

mik-laj commented Jul 20, 2020

@kuromt Should we close this PR or do you want to keep working on it?

@kuromt
Copy link
Author

kuromt commented Jul 22, 2020

@mik-laj Thanks, I'd like to close this PR.
@jeffolsi @Ziliboba Is that OK?

@kuromt
Copy link
Author

kuromt commented Aug 2, 2020

OK, I close this PR.

@mik-laj Thanks for your support.

@kuromt kuromt closed this Aug 2, 2020
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.

7 participants