You are viewing a plain text version of this content. The canonical link for it is here.

Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/02/18 22:32:02 UTC

[GitHub] [airflow] kaxil opened a new issue #14301: Helm Chart: Review & Improve Current Test Coverage

kaxil opened a new issue #14301:
URL: https://github.com/apache/airflow/issues/14301


   We should add tests with different configurations for Helm Chart:
   
   - With different executors (Celery, Kubernetes and CeleryKubernetes)
   - With executor_config & different pod_template_file (and different type of Dags -- including KubernetesPodOperator)
   - TBD


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] ephraimbuddy commented on issue #14301: Helm Chart: Add Integration tests & expand unit tests coverage

Posted by GitBox <gi...@apache.org>.

ephraimbuddy commented on issue #14301:
URL: https://github.com/apache/airflow/issues/14301#issuecomment-839620895


   > Once we have this, we should be able to run
   
   > ./breeze kind-cluster start (once to start the cluster)
   ./breeze kind-cluster status (to check the status of the cluster)
   ./breeze kind-cluster deploy --kubernetes-mode CeleryKubernetes
   
   The above steps can already be done with `--executor CeleryKubernetes`. Maybe we should remove `--kubernetes-mode` or remove `--executor` and implement the `--kubernetes-mode` the way the `--executor` is implemented?


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] kaxil edited a comment on issue #14301: Helm Chart: Add Integration tests & expand unit tests coverage

Posted by GitBox <gi...@apache.org>.

kaxil edited a comment on issue #14301:
URL: https://github.com/apache/airflow/issues/14301#issuecomment-839296836


   We can close this issue once https://github.com/apache/airflow/pull/15695#issuecomment-837577849 is addressed:
   
   >@ephraimbuddy We should also now integrate this in CI to run Helm Chart integration test with Celery & Local Executor too
   
   cc @ephraimbuddy 


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] kaxil edited a comment on issue #14301: Helm Chart: Review & Improve Current Test Coverage

Posted by GitBox <gi...@apache.org>.

kaxil edited a comment on issue #14301:
URL: https://github.com/apache/airflow/issues/14301#issuecomment-782302244


   We should do both @sryabkov  :
   
   - Unit: https://github.com/apache/airflow/tree/0d366c1f297ec25b5f8e01c6d72a312e2ee64d4d/chart/tests
   - Integration: https://github.com/apache/airflow/tree/master/0d366c1f297ec25b5f8e01c6d72a312e2ee64d4d


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] ephraimbuddy commented on issue #14301: Helm Chart: Add Integration tests & expand unit tests coverage

Posted by GitBox <gi...@apache.org>.

ephraimbuddy commented on issue #14301:
URL: https://github.com/apache/airflow/issues/14301#issuecomment-840320598


   > Now using a single job and it seems it'll succeed this time with fewer logs. Running `./ci_setup_cluster_and_deploy_airflow_to_kubernetes.sh` locally runs the executor tests in other.
   
   This has failed :)


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] kaxil closed issue #14301: Helm Chart: Add Integration tests & expand unit tests coverage

Posted by GitBox <gi...@apache.org>.

kaxil closed issue #14301:
URL: https://github.com/apache/airflow/issues/14301


   


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] ephraimbuddy commented on issue #14301: Helm Chart: Add Integration tests & expand unit tests coverage

Posted by GitBox <gi...@apache.org>.

ephraimbuddy commented on issue #14301:
URL: https://github.com/apache/airflow/issues/14301#issuecomment-840062901


   I think we should use separate jobs for each executor that we want to test in k8s cluster because the logs when all is run in one job are just too much and hangs when you want to view it. See https://github.com/apache/airflow/pull/15791/checks?check_run_id=2568726142


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] kaxil commented on issue #14301: Helm Chart: Add Integration tests & expand unit tests coverage

Posted by GitBox <gi...@apache.org>.

kaxil commented on issue #14301:
URL: https://github.com/apache/airflow/issues/14301#issuecomment-839296836


   From https://github.com/apache/airflow/pull/15695#issuecomment-837577849:
   
   >@ephraimbuddy We should also now integrate this in CI to run Helm Chart integration test with Celery & Local Executor too
   
   cc @ephraimbuddy 


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] ephraimbuddy commented on issue #14301: Helm Chart: Add Integration tests & expand unit tests coverage

Posted by GitBox <gi...@apache.org>.

ephraimbuddy commented on issue #14301:
URL: https://github.com/apache/airflow/issues/14301#issuecomment-839369096


   Yes. Looking into it now


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] ephraimbuddy commented on issue #14301: Helm Chart: Add Integration tests & expand unit tests coverage

Posted by GitBox <gi...@apache.org>.

ephraimbuddy commented on issue #14301:
URL: https://github.com/apache/airflow/issues/14301#issuecomment-839399856


   I'm having a hard time on this. @potiuk can you help?


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] sryabkov commented on issue #14301: Helm Chart: Review & Improve Current Test Coverage

Posted by GitBox <gi...@apache.org>.

sryabkov commented on issue #14301:
URL: https://github.com/apache/airflow/issues/14301#issuecomment-782173442


   Is the scope for this just adding unit tests to [`airflow/chart/tests`](https://github.com/apache/airflow/tree/0d366c1f297ec25b5f8e01c6d72a312e2ee64d4d/chart/tests) or is there more (e.g. integration testing)?


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] potiuk commented on issue #14301: Helm Chart: Add Integration tests & expand unit tests coverage

Posted by GitBox <gi...@apache.org>.

potiuk commented on issue #14301:
URL: https://github.com/apache/airflow/issues/14301#issuecomment-839653598


   I did not even know we had --executor swtich already :)


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] potiuk commented on issue #14301: Helm Chart: Add Integration tests & expand unit tests coverage

Posted by GitBox <gi...@apache.org>.

potiuk commented on issue #14301:
URL: https://github.com/apache/airflow/issues/14301#issuecomment-839503085

Sure. We will need to make several configurations of tests for K8S. In short we need several different " modes" of running k8s - for now we only had one "KUBERNETES_MODE" : image . It was "legacy" and never really used beside some 1.5 years ago where we also had "git-sync" mode but I do not think we awnat to test that much granularity for integration tests so we can use this mode to become "deployment".

We need several things here (and we can do it gradually and share it so that it's not only me who knows how to do it :)/

1) we can change KUBERNETES_MODE to determine type of deployment: "Calery", "Kubernetes", "CeleryKubernetes". The current "image" mode is actually nothing more than "Celery". Just look for KUBERNETES_MODE and kubernetes_mode everywhere (mostly scripts/ci, breeze , breeze-complete) and you should be able to replace the "image" with "Celery" for CI sp that we can commmit the change.

2) In the `_kind.sh` in `kind::deploy_airflow_with_helm` we should deploy the right configuration of Aiflow based on the mode - basically overrid the values determining which K8S deployment we deply

That will be a good start. At this stage we should be able to deploy Airflow via Helm Chart in different modes using Breeze - folowing https://github.com/apache/airflow/blob/master/TESTING.rst#running-tests-with-kubernetes

You can first follow https://github.com/apache/airflow/blob/master/TESTING.rst#typical-testing-pattern-for-kubernetes-tests to see how it is normally done.

Once we have this, we should be able to run

* `./breeze kind-cluster start` (once to start the cluster)
* `./breeze kind-cluster status` (once to start the cluster)
* `./breeze kind-cluster deploy --kubernetes-mode CeleryKubernetes`

and you should be able to connect locally to Airflow running with CeleryKubernetes executor

Once we get there, we need to somehow decide and control which tests we run for which deployment. We can do it with custom pytest markers for example or separating them via sub-folders in kubernetes_tests.

I think we can discuss this part when we get there.

Does it sound reasonable? Happy to help/review/answer qustions on slack and get someone else to understand how it woks :).

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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] potiuk edited a comment on issue #14301: Helm Chart: Add Integration tests & expand unit tests coverage

Posted by GitBox <gi...@apache.org>.

potiuk edited a comment on issue #14301:
URL: https://github.com/apache/airflow/issues/14301#issuecomment-839503085

We need several things here (and we can do it gradually and share it so that it's not only me who knows how to do it :)/

2) In the `_kind.sh` in `kind::deploy_airflow_with_helm` we should deploy the right configuration of Aiflow based on the mode - basically overrid the values determining which K8S deployment we deply

You can first follow https://github.com/apache/airflow/blob/master/TESTING.rst#typical-testing-pattern-for-kubernetes-tests to see how it is normally done.

Once we have this, we should be able to run

* `./breeze kind-cluster start` (once to start the cluster)
* `./breeze kind-cluster status` (to check the status of the cluster)
* `./breeze kind-cluster deploy --kubernetes-mode CeleryKubernetes`

and you should be able to connect locally to Airflow running with CeleryKubernetes executor

I think we can discuss this part when we get there.

Does it sound reasonable? Happy to help/review/answer qustions on slack and get someone else to understand how it woks :).

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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] potiuk commented on issue #14301: Helm Chart: Add Integration tests & expand unit tests coverage

Posted by GitBox <gi...@apache.org>.

potiuk commented on issue #14301:
URL: https://github.com/apache/airflow/issues/14301#issuecomment-839513825


   Perfect! Happy to help


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] potiuk commented on issue #14301: Helm Chart: Add Integration tests & expand unit tests coverage

Posted by GitBox <gi...@apache.org>.

potiuk commented on issue #14301:
URL: https://github.com/apache/airflow/issues/14301#issuecomment-839624150


   yep


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] ephraimbuddy commented on issue #14301: Helm Chart: Add Integration tests & expand unit tests coverage

Posted by GitBox <gi...@apache.org>.

ephraimbuddy commented on issue #14301:
URL: https://github.com/apache/airflow/issues/14301#issuecomment-840173847


   Now using a single job and it seems it'll succeed this time with fewer logs. Running `./ci_setup_cluster_and_deploy_airflow_to_kubernetes.sh` locally runs the executor tests in other.


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] kaxil commented on issue #14301: Helm Chart: Review & Improve Current Test Coverage

Posted by GitBox <gi...@apache.org>.

kaxil commented on issue #14301:
URL: https://github.com/apache/airflow/issues/14301#issuecomment-782302244


   We should do both:
   
   - Unit: https://github.com/apache/airflow/tree/0d366c1f297ec25b5f8e01c6d72a312e2ee64d4d/chart/tests
   - Integration: https://github.com/apache/airflow/tree/master/0d366c1f297ec25b5f8e01c6d72a312e2ee64d4d


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] kaxil commented on issue #14301: Helm Chart: Add Integration tests & expand unit tests coverage

Posted by GitBox <gi...@apache.org>.

kaxil commented on issue #14301:
URL: https://github.com/apache/airflow/issues/14301#issuecomment-840086931


   > I think we should use separate jobs for each executor that we want to test in k8s cluster because the logs when all is run in one job are just too much and hangs when you want to view it. See https://github.com/apache/airflow/pull/15791/checks?check_run_id=2568726142
   
   Yup, same as what we have now for different Python versions and different DBs
   
   ![image](https://user-images.githubusercontent.com/8811558/118042258-c735da80-b36b-11eb-9fd1-d3ab58ffa333.png)
   


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] ephraimbuddy commented on issue #14301: Helm Chart: Add Integration tests & expand unit tests coverage

Posted by GitBox <gi...@apache.org>.

ephraimbuddy commented on issue #14301:
URL: https://github.com/apache/airflow/issues/14301#issuecomment-839510887


   Ah, just seeing your message now. Will keep the PR I just made to draft first


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] kaxil commented on issue #14301: Helm Chart: Add Integration tests & expand unit tests coverage

Posted by GitBox <gi...@apache.org>.

kaxil commented on issue #14301:
URL: https://github.com/apache/airflow/issues/14301#issuecomment-839696322


   Thank you both :) 


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org