Fix multiple issues in Microsoft AzureContainerInstancesOperator #15634

BKronenbitter · 2021-05-03T08:18:52Z

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.

boring-cyborg · 2021-05-03T08:18:54Z

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.
Please follow ASF Code of 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.
Apache Airflow is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: [email protected]
Slack: https://s.apache.org/airflow-slack

potiuk · 2021-05-03T10:58:53Z

Thanks @BKronenbitter. Would you mind adding/extending unit tests to cover the failure scenarios ?

BKronenbitter · 2021-05-04T10:50:41Z

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.

potiuk · 2021-05-04T16:06:08Z

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.

BKronenbitter · 2021-05-05T10:00:08Z

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.

BKronenbitter · 2021-05-14T07:10:55Z

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.

github-actions · 2021-05-14T08:09:15Z

The Workflow run 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*.

github-actions · 2021-05-14T08:09:55Z

The Workflow run is cancelling this PR. Building image for the PR has been cancelled

BKronenbitter · 2021-05-14T08:56:05Z

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.

potiuk · 2021-05-14T09:22:39Z

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

uranusjr · 2021-05-14T09:30:01Z

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"]:


This is probably just left over from testing. I can change that back of course.

github-actions · 2021-05-14T12:38:28Z

The Workflow run is cancelling this PR. Building images for the PR has failed. Follow the workflow link to check the reason.

BKronenbitter · 2021-05-21T12:22:19Z

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.

BKronenbitter · 2021-06-18T07:01:33Z

@potiuk Please feel gently poked. I think, I addressed all the comments and am basically waiting on your feedback.

potiuk · 2021-06-18T07:55:04Z

There are some failures resulting from your changes being behind main (409 commits). can you please rebase to latest main ? (and ping me please then)

BKronenbitter · 2021-06-18T10:46:29Z

@potiuk Thanks for the quick answer. The rebase is done. The build of some of the containers is now failing. This issue seems to be present also in the main branch.

potiuk · 2021-06-18T11:28:54Z

Should be fixed now

BKronenbitter · 2021-06-23T07:52:42Z

@potiuk I think, everything is as it should be right now.

potiuk · 2021-06-23T12:08:59Z

Hey @BKronenbitter - can you please rebase again (one last time I hope). we had some changes in main that require rebasing (busy time).

potiuk · 2021-06-23T12:09:13Z

Apologies for the hassle.

BKronenbitter · 2021-06-23T14:12:07Z

No problem, done.

potiuk · 2021-06-23T15:48:20Z

Running build :P

potiuk · 2021-06-23T18:21:37Z

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)

potiuk · 2021-06-23T19:51:06Z

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

…t importantly run sleep during main loop while executing.

BKronenbitter · 2021-06-24T09:28:16Z

Done, I added the exception to the whole files, since everything else ended up in a mess of individual exceptions.

potiuk · 2021-06-24T13:19:45Z

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.

github-actions · 2021-06-24T13:27:41Z

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.

boring-cyborg · 2021-06-24T18:07:43Z

Awesome work, congrats on your first merged pull request!

boring-cyborg bot added area:providers provider:microsoft-azure Azure-related issues labels May 3, 2021

potiuk closed this May 14, 2021

potiuk reopened this May 14, 2021

uranusjr reviewed May 14, 2021

View reviewed changes

BKronenbitter force-pushed the FixesInAzureContainerInstancesOperator branch 2 times, most recently from f852f40 to a7088d6 Compare June 18, 2021 09:45

potiuk closed this Jun 18, 2021

potiuk reopened this Jun 18, 2021

BKronenbitter force-pushed the FixesInAzureContainerInstancesOperator branch from a7088d6 to 284b2a5 Compare June 23, 2021 14:11

Bastian Kronenbitter added 5 commits June 24, 2021 11:26

Fix multiple issues in Microsoft AzureContainerInstancesOperator, mos…

3d43bd7

…t importantly run sleep during main loop while executing.

add tests to cover changes

24b3e60

fix static tests

df8115e

use equal instead of in

a2ea027

add pylint exception

7d9e66b

BKronenbitter force-pushed the FixesInAzureContainerInstancesOperator branch from c749ec7 to 7d9e66b Compare June 24, 2021 09:27

potiuk approved these changes Jun 24, 2021

View reviewed changes

github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Jun 24, 2021

potiuk merged commit ffb1fca into apache:main Jun 24, 2021

potiuk mentioned this pull request Jul 15, 2021

Status of testing Providers that were prepared on July 15, 2021 #17037

Closed

26 tasks

potiuk mentioned this pull request Jul 27, 2021

Status of testing Providers that were prepared on July 27, 2021 (rc2) #17268

Closed

66 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix multiple issues in Microsoft AzureContainerInstancesOperator #15634

Fix multiple issues in Microsoft AzureContainerInstancesOperator #15634

BKronenbitter commented May 3, 2021

boring-cyborg bot commented May 3, 2021

potiuk commented May 3, 2021

BKronenbitter commented May 4, 2021

potiuk commented May 4, 2021

BKronenbitter commented May 5, 2021

BKronenbitter commented May 14, 2021

github-actions bot commented May 14, 2021

github-actions bot commented May 14, 2021

BKronenbitter commented May 14, 2021

potiuk commented May 14, 2021 •

edited

Loading

uranusjr May 14, 2021

BKronenbitter May 14, 2021

github-actions bot commented May 14, 2021

BKronenbitter commented May 21, 2021

BKronenbitter commented Jun 18, 2021

potiuk commented Jun 18, 2021

BKronenbitter commented Jun 18, 2021

potiuk commented Jun 18, 2021

BKronenbitter commented Jun 23, 2021

potiuk commented Jun 23, 2021

potiuk commented Jun 23, 2021

BKronenbitter commented Jun 23, 2021

potiuk commented Jun 23, 2021

potiuk commented Jun 23, 2021

potiuk commented Jun 23, 2021

BKronenbitter commented Jun 24, 2021

potiuk commented Jun 24, 2021

github-actions bot commented Jun 24, 2021

boring-cyborg bot commented Jun 24, 2021

Fix multiple issues in Microsoft AzureContainerInstancesOperator #15634

Fix multiple issues in Microsoft AzureContainerInstancesOperator #15634

Conversation

BKronenbitter commented May 3, 2021

boring-cyborg bot commented May 3, 2021

potiuk commented May 3, 2021

BKronenbitter commented May 4, 2021

potiuk commented May 4, 2021

BKronenbitter commented May 5, 2021

BKronenbitter commented May 14, 2021

github-actions bot commented May 14, 2021

github-actions bot commented May 14, 2021

BKronenbitter commented May 14, 2021

potiuk commented May 14, 2021 • edited Loading

uranusjr May 14, 2021

Choose a reason for hiding this comment

BKronenbitter May 14, 2021

Choose a reason for hiding this comment

github-actions bot commented May 14, 2021

BKronenbitter commented May 21, 2021

BKronenbitter commented Jun 18, 2021

potiuk commented Jun 18, 2021

BKronenbitter commented Jun 18, 2021

potiuk commented Jun 18, 2021

BKronenbitter commented Jun 23, 2021

potiuk commented Jun 23, 2021

potiuk commented Jun 23, 2021

BKronenbitter commented Jun 23, 2021

potiuk commented Jun 23, 2021

potiuk commented Jun 23, 2021

potiuk commented Jun 23, 2021

BKronenbitter commented Jun 24, 2021

potiuk commented Jun 24, 2021

github-actions bot commented Jun 24, 2021

boring-cyborg bot commented Jun 24, 2021

potiuk commented May 14, 2021 •

edited

Loading