Skip to content

Conversation

@perry2of5
Copy link
Contributor

This commit adds the capability to export log messages into XCOM on an optional basis. The existing behavior (no logs) is retained as the default making this an opt-in feature. Users of the operator can retrieve either all logs or only the last log message. In either case, the log messsages are presented as a list

The bash and docker operators, among others, can export logs into XCOM to provide some feedback about the outcome of the command in the results. The AzureContainerInstancesOperator just detects if the container exits cleanly and exits with either 0 (success) or 1 (failure). This commit adds the ability to put either the last log message or all log messages into XCOM under the key 'logs' so it can be used in future operators to decide if it the operation within the container failed or succeeded.

@boring-cyborg
Copy link

boring-cyborg bot commented Jul 30, 2024

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 Contributors' Guide (https://github.com/apache/airflow/blob/main/contributing-docs/README.rst)
Here are some useful points:

  • Pay attention to the quality of your code (ruff, mypy 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.
  • Always keep your Pull Requests rebased, otherwise your build might fail due to changes not related to your commits.
    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

@perry2of5 perry2of5 changed the title Add capability to export log messages into XCOM Export Azure Container Instance log messages to XCOM Jul 30, 2024
@potiuk
Copy link
Member

potiuk commented Aug 14, 2024

static checks failing

@perry2of5
Copy link
Contributor Author

perry2of5 commented Aug 14, 2024 via email

The bash and docker operators, among others, can export logs into XCOM
to provide some feedback about the outcome of the command in the
results. The AzureContainerInstancesOperator just detects if the
container exits cleanly. This commit adds the ability to put either
the last log message or all log messages into XCOM under the key
'logs' so it can be used in future operators to decide if it failed or
succeded.
@perry2of5
Copy link
Contributor Author

I believe I have fixed the issues from my code (currently re-running breeze static-checks), but when I run "breeze static-checks --all-files" it complains about a file I didn't change:
airflow/providers/fab/auth_manager/cli_commands/user_command.py

Should I ignore this?

@potiuk potiuk merged commit d2c9e8c into apache:main Aug 14, 2024
@boring-cyborg
Copy link

boring-cyborg bot commented Aug 14, 2024

Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions.

@potiuk
Copy link
Member

potiuk commented Aug 14, 2024

I believe I have fixed the issues from my code (currently re-running breeze static-checks), but when I run "breeze static-checks --all-files" it complains about a file I didn't change:
airflow/providers/fab/auth_manager/cli_commands/user_command.py

It was likely due to not rebased/rebuilt image with latest changes. Main succeeded, so all looks good.

@perry2of5 perry2of5 deleted the aci-export-log-messages-to-xcom branch August 14, 2024 16:03
@perry2of5
Copy link
Contributor Author

Thank you, @potiuk!

@perry2of5
Copy link
Contributor Author

perry2of5 commented Aug 14, 2024 via email

@potiuk
Copy link
Member

potiuk commented Aug 14, 2024

no idea. Generally the system is designed to be consistent when you a) rebase, b) build the image when asked . This is what CI check does a) checks out the latest version b) builds the latest image c) uses the latest image to run the tests. If any of the steps are missing or delayed, the end results might differ. So if you are in doubt - ABR - Always Be Rebased.

  1. rebased
  2. breeze ci-image build
  3. run your tests

That should yield same results as CI build - but of course there are race conditions - someone could have pushed another change between the steps manually run. So again - if in doubt

  1. rebase
  2. build image
  3. run your tests

Airflow has high traffic, things are chaning on hourly base ... so rebase, rebuild test is a recommended way.

Artuz37 pushed a commit to Artuz37/airflow that referenced this pull request Aug 19, 2024
* Add ability to export log messages into XCOM

The bash and docker operators, among others, can export logs into XCOM
to provide some feedback about the outcome of the command in the
results. The AzureContainerInstancesOperator just detects if the
container exits cleanly. This commit adds the ability to put either
the last log message or all log messages into XCOM under the key
'logs' so it can be used in future operators to decide if it failed or
succeded.

* Fix issues found by static checks

* Update format to satisfy ruff-format
romsharon98 pushed a commit to romsharon98/airflow that referenced this pull request Aug 20, 2024
* Add ability to export log messages into XCOM

The bash and docker operators, among others, can export logs into XCOM
to provide some feedback about the outcome of the command in the
results. The AzureContainerInstancesOperator just detects if the
container exits cleanly. This commit adds the ability to put either
the last log message or all log messages into XCOM under the key
'logs' so it can be used in future operators to decide if it failed or
succeded.

* Fix issues found by static checks

* Update format to satisfy ruff-format
@roman-doubrava
Copy link

@perry2of5 & @potiuk would it be possible to additionally enhace the AzureContainerInstancesOperator with the :param do_xcom_push: If True, the content of the file /airflow/xcom/return.json in the container will also be pushed to an XCom when the container completes, which would be similar behavior to the KubernetesPODOperator ?

@perry2of5
Copy link
Contributor Author

perry2of5 commented Oct 9, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants