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/05/03 08:19:06 UTC
[GitHub] [airflow] BKronenbitter opened a new pull request #15634: Fix multiple issues in Microsoft AzureContainerInstancesOperator
BKronenbitter opened a new pull request #15634:
URL: https://github.com/apache/airflow/pull/15634
After having worked with with Airflow in Azure using the AzureContainerInstancesOperator I encountered some issues.
- The sleep command seems to be indented wrongly and not run periodically.
- When log messages are collected, a check for None is missing.
- One possible state for logging is missing.
- Container exits are logged as error, should be info.
--
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] uranusjr commented on a change in pull request #15634: Fix multiple issues in Microsoft AzureContainerInstancesOperator
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #15634:
URL: https://github.com/apache/airflow/pull/15634#discussion_r632405161
##########
File path: airflow/providers/microsoft/azure/operators/azure_container_instances.py
##########
@@ -339,8 +340,8 @@ def _monitor_logging(self, resource_group: str, name: str) -> int:
"Exception while getting logs from container instance, retrying..."
)
- if state == "Terminated":
- self.log.error("Container exited with detail_status %s", detail_status)
+ if state in ["Terminated"]:
Review comment:
Why?
--
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] github-actions[bot] commented on pull request #15634: Fix multiple issues in Microsoft AzureContainerInstancesOperator
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #15634:
URL: https://github.com/apache/airflow/pull/15634#issuecomment-841088579
[The Workflow run](https://github.com/apache/airflow/actions/runs/841382900) is cancelling this PR. Building image for the PR has been cancelled
--
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 pull request #15634: Fix multiple issues in Microsoft AzureContainerInstancesOperator
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #15634:
URL: https://github.com/apache/airflow/pull/15634#issuecomment-841125915
> * "Build and test provider packages wheel" seems to fail with `error An unexpected error occurred: "https://registry.yarnpkg.com/d3-tip/-/d3-tip-0.9.1.tgz: connect ECONNREFUSED 104.16.21.35:443".`
Apparently temporary error.
> * "Quarantined tests" apparently runs out of memory: `Error: Process completed with exit code 137.`
Yep. That's one of the reason why those tests are quarantined. They fail occassionally.
> * "Build docs" seems to fail with `/opt/airflow/docs/_inventory_cache/jinja2/objects.inv' not fetchable due to <class 'FileNotFoundError'>: [Errno 2] No such file or directory: '/opt/airflow/docs/_inventory_cache/jinja2/objects.inv`.
That was also apparently temporary failure.
> I am not sure, to what degree these errors are caused by the changes in the PR.
Likely None. I will close/reopen to trigger the build again.
--
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 pull request #15634: Fix multiple issues in Microsoft AzureContainerInstancesOperator
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #15634:
URL: https://github.com/apache/airflow/pull/15634#issuecomment-867060088
As surprising as it is... pylint started to detect some errors . I think @ashb you were quite right that pylint is not worth it.. @BKronenbitter - can you please add `#pylint: disable=no-member` in those two files that are flagged (apparently randomly)
--
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] github-actions[bot] commented on pull request #15634: Fix multiple issues in Microsoft AzureContainerInstancesOperator
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #15634:
URL: https://github.com/apache/airflow/pull/15634#issuecomment-867636537
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.
--
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 pull request #15634: Fix multiple issues in Microsoft AzureContainerInstancesOperator
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #15634:
URL: https://github.com/apache/airflow/pull/15634#issuecomment-863838736
--
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] BKronenbitter commented on pull request #15634: Fix multiple issues in Microsoft AzureContainerInstancesOperator
Posted by GitBox <gi...@apache.org>.
BKronenbitter commented on pull request #15634:
URL: https://github.com/apache/airflow/pull/15634#issuecomment-866871529
No problem, done.
--
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] github-actions[bot] commented on pull request #15634: Fix multiple issues in Microsoft AzureContainerInstancesOperator
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #15634:
URL: https://github.com/apache/airflow/pull/15634#issuecomment-841088225
[The Workflow run](https://github.com/apache/airflow/actions/runs/841525864) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.
--
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] boring-cyborg[bot] commented on pull request #15634: Fix multiple issues in Microsoft AzureContainerInstancesOperator
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #15634:
URL: https://github.com/apache/airflow/pull/15634#issuecomment-831103766
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]( https://github.com/apache/airflow/blob/master/STATIC_CODE_CHECKS.rst#prerequisites-for-pre-commit-hooks) 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](https://github.com/apache/airflow/blob/master/docs/apache-airflow/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it.
- Consider using [Breeze environment](https://github.com/apache/airflow/blob/master/BREEZE.rst) 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.
- Please follow [ASF Code of Conduct](https://www.apache.org/foundation/policies/conduct) for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
- Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#coding-style-and-best-practices).
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://s.apache.org/airflow-slack
--
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 pull request #15634: Fix multiple issues in Microsoft AzureContainerInstancesOperator
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #15634:
URL: https://github.com/apache/airflow/pull/15634#issuecomment-866780674
Hey @BKronenbitter - can you please rebase again (one last time I hope). we had some changes in main that require rebasing (busy time).
--
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] github-actions[bot] commented on pull request #15634: Fix multiple issues in Microsoft AzureContainerInstancesOperator
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #15634:
URL: https://github.com/apache/airflow/pull/15634#issuecomment-841217148
[The Workflow run](https://github.com/apache/airflow/actions/runs/841817773) is cancelling this PR. Building images for the PR has failed. Follow the workflow link to check the reason.
--
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] BKronenbitter commented on pull request #15634: Fix multiple issues in Microsoft AzureContainerInstancesOperator
Posted by GitBox <gi...@apache.org>.
BKronenbitter commented on pull request #15634:
URL: https://github.com/apache/airflow/pull/15634#issuecomment-867486908
Done, I added the exception to the whole files, since everything else ended up in a mess of individual exceptions.
--
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 pull request #15634: Fix multiple issues in Microsoft AzureContainerInstancesOperator
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #15634:
URL: https://github.com/apache/airflow/pull/15634#issuecomment-866953066
Running build :P
--
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 closed pull request #15634: Fix multiple issues in Microsoft AzureContainerInstancesOperator
Posted by GitBox <gi...@apache.org>.
potiuk closed pull request #15634:
URL: https://github.com/apache/airflow/pull/15634
--
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 pull request #15634: Fix multiple issues in Microsoft AzureContainerInstancesOperator
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #15634:
URL: https://github.com/apache/airflow/pull/15634#issuecomment-832059665
> Thanks for your quick reaction. Let me address the individual fixes separately:
>
Just to give you a bit of context, we rely on automated testing heavily. And our philosophy is that with every single change we want to add unit tests so that the coverage grows and probability of regression goes down. Adding any change without tests almost by definition goes in the other direction, so almost any change that has no unit tests (and touches the python code) will end up with a request to add them.
> * It am not aware of a practical way to test the sleep time in the main loop of the `_monitor_logging`. If you know a good way, of course I can do so.
We usually mock any calls like that - example here where sleep command have been mocked:
https://github.com/apache/airflow/blob/master/tests/cli/commands/test_webserver_command.py
In this particular case we would like to add a test where we have at least two loops where state changes and sleep is called in-between. All that can be mocked rather easily.
> * I for sure can test the addition of the state for logging.
Coool
> * Testing the different log level does not make much sense, I would say.
Agree.
> * Testing the check for missing events is also tricky, since the exception is only logged and not raised further. If there is a good way to test, that no exception is logged within the main loop, of course I can do that.
I think mocking here should help as well.
--
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] BKronenbitter commented on pull request #15634: Fix multiple issues in Microsoft AzureContainerInstancesOperator
Posted by GitBox <gi...@apache.org>.
BKronenbitter commented on pull request #15634:
URL: https://github.com/apache/airflow/pull/15634#issuecomment-863807520
--
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] BKronenbitter commented on pull request #15634: Fix multiple issues in Microsoft AzureContainerInstancesOperator
Posted by GitBox <gi...@apache.org>.
BKronenbitter commented on pull request #15634:
URL: https://github.com/apache/airflow/pull/15634#issuecomment-832565886
Thanks a lot for the explanation. I added/changed tests according to your comment. They all fail in the current master and pass with fixes.
--
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 pull request #15634: Fix multiple issues in Microsoft AzureContainerInstancesOperator
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #15634:
URL: https://github.com/apache/airflow/pull/15634#issuecomment-866780813
Apologies for the hassle.
--
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] BKronenbitter commented on pull request #15634: Fix multiple issues in Microsoft AzureContainerInstancesOperator
Posted by GitBox <gi...@apache.org>.
BKronenbitter commented on pull request #15634:
URL: https://github.com/apache/airflow/pull/15634#issuecomment-841060833
Sorry for the failing static check. I was not aware, that it was skipped locally. Should be fixed now and all static checks are succeeding locally.
--
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] BKronenbitter commented on pull request #15634: Fix multiple issues in Microsoft AzureContainerInstancesOperator
Posted by GitBox <gi...@apache.org>.
BKronenbitter commented on pull request #15634:
URL: https://github.com/apache/airflow/pull/15634#issuecomment-866614964
@potiuk I think, everything is as it should be right 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] boring-cyborg[bot] commented on pull request #15634: Fix multiple issues in Microsoft AzureContainerInstancesOperator
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #15634:
URL: https://github.com/apache/airflow/pull/15634#issuecomment-867848545
Awesome work, congrats on your first merged pull request!
--
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] BKronenbitter commented on pull request #15634: Fix multiple issues in Microsoft AzureContainerInstancesOperator
Posted by GitBox <gi...@apache.org>.
BKronenbitter commented on pull request #15634:
URL: https://github.com/apache/airflow/pull/15634#issuecomment-841111859
Hi, I am confused by the output of the failing checks:
- "Build and test provider packages wheel" seems to fail with `error An unexpected error occurred: "https://registry.yarnpkg.com/d3-tip/-/d3-tip-0.9.1.tgz: connect ECONNREFUSED 104.16.21.35:443".`
- "Quarantined tests" apparently runs out of memory: `Error: Process completed with exit code 137.`
- "Build docs" seems to fail with `/opt/airflow/docs/_inventory_cache/jinja2/objects.inv' not fetchable due to <class
'FileNotFoundError'>: [Errno 2] No such file or directory: '/opt/airflow/docs/_inventory_cache/jinja2/objects.inv`.
I am not sure, to what degree these errors are caused by the changes in the PR.
--
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 pull request #15634: Fix multiple issues in Microsoft AzureContainerInstancesOperator
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #15634:
URL: https://github.com/apache/airflow/pull/15634#issuecomment-831186252
Thanks @BKronenbitter. Would you mind adding/extending unit tests to cover the failure scenarios ?
--
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 closed pull request #15634: Fix multiple issues in Microsoft AzureContainerInstancesOperator
Posted by GitBox <gi...@apache.org>.
potiuk closed pull request #15634:
URL: https://github.com/apache/airflow/pull/15634
--
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 pull request #15634: Fix multiple issues in Microsoft AzureContainerInstancesOperator
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #15634:
URL: https://github.com/apache/airflow/pull/15634#issuecomment-841125915
> * "Build and test provider packages wheel" seems to fail with `error An unexpected error occurred: "https://registry.yarnpkg.com/d3-tip/-/d3-tip-0.9.1.tgz: connect ECONNREFUSED 104.16.21.35:443".`
Apparently temporary error.
> * "Quarantined tests" apparently runs out of memory: `Error: Process completed with exit code 137.`
Yep. That's one of the reason why those tests are quarantined. They fail occassionally.
> * "Build docs" seems to fail with `/opt/airflow/docs/_inventory_cache/jinja2/objects.inv' not fetchable due to <class 'FileNotFoundError'>: [Errno 2] No such file or directory: '/opt/airflow/docs/_inventory_cache/jinja2/objects.inv`.
That was also apparently temporary failure.
> I am not sure, to what degree these errors are caused by the changes in the PR.
Likely None. I will close/reopen to trigger the build again.
--
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] BKronenbitter commented on a change in pull request #15634: Fix multiple issues in Microsoft AzureContainerInstancesOperator
Posted by GitBox <gi...@apache.org>.
BKronenbitter commented on a change in pull request #15634:
URL: https://github.com/apache/airflow/pull/15634#discussion_r632417722
##########
File path: airflow/providers/microsoft/azure/operators/azure_container_instances.py
##########
@@ -339,8 +340,8 @@ def _monitor_logging(self, resource_group: str, name: str) -> int:
"Exception while getting logs from container instance, retrying..."
)
- if state == "Terminated":
- self.log.error("Container exited with detail_status %s", detail_status)
+ if state in ["Terminated"]:
Review comment:
This is probably just left over from testing. I can change that back of course.
--
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] BKronenbitter commented on pull request #15634: Fix multiple issues in Microsoft AzureContainerInstancesOperator
Posted by GitBox <gi...@apache.org>.
BKronenbitter commented on pull request #15634:
URL: https://github.com/apache/airflow/pull/15634#issuecomment-831851094
Thanks for your quick reaction. Let me address the individual fixes separately:
- It am not aware of a practical way to test the sleep time in the main loop of the `_monitor_logging`. If you know a good way, of course I can do so.
- I for sure can test the addition of the state for logging.
- Testing the different log level does not make much sense, I would say.
- Testing the check for missing events is also tricky, since the exception is only logged and not raised further. If there is a good way to test, that no exception is logged within the main loop, of course I can do that.
--
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] BKronenbitter commented on pull request #15634: Fix multiple issues in Microsoft AzureContainerInstancesOperator
Posted by GitBox <gi...@apache.org>.
BKronenbitter commented on pull request #15634:
URL: https://github.com/apache/airflow/pull/15634#issuecomment-845912812
> > I am not sure, to what degree these errors are caused by the changes in the PR.
>
> Likely None. I will close/reopen to trigger the build again.
How do we proceed now? Is there anything I can do from my side to resolve the issues.
--
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 pull request #15634: Fix multiple issues in Microsoft AzureContainerInstancesOperator
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #15634:
URL: https://github.com/apache/airflow/pull/15634#issuecomment-867113760
BTW. I just started a vote (after @ashb hinting/lobbying for it for quite a while despite my reservations) to remove pylint ... It brings us more harm than good...
--
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 merged pull request #15634: Fix multiple issues in Microsoft AzureContainerInstancesOperator
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #15634:
URL: https://github.com/apache/airflow/pull/15634
--
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 pull request #15634: Fix multiple issues in Microsoft AzureContainerInstancesOperator
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #15634:
URL: https://github.com/apache/airflow/pull/15634#issuecomment-867630885
> Done, I added the exception to the whole files, since everything else ended up in a mess of individual exceptions.
Yeah. I hope we will finally get rid of pylint. Been proponent of it for quite some time but I see how much pain it causes and stopped liking it.
--
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