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

Fix condition for k8 container completion check #27502

Closed
wants to merge 3 commits into from

Conversation

windmark
Copy link

@windmark windmark commented Nov 4, 2022

As described in #26796, the condition for checkin whether the k8 container is completed is incorrectly reversed. This was initially discovered and fixed in #23883, but ultimately reverted due to failing tests.

This PR attempts to address the failing tests by extending the tests to cover this path.

closes: #26796
related: #23883, #24479


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an 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 a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@boring-cyborg boring-cyborg bot added provider:cncf-kubernetes Kubernetes provider related issues area:providers labels Nov 4, 2022
@boring-cyborg
Copy link

boring-cyborg bot commented Nov 4, 2022

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/main/CONTRIBUTING.rst)
Here are some useful points:

  • Pay attention to the quality of your code (flake8, 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.
    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

while not self.container_is_running(pod=pod, container_name=container_name):
while self.container_is_running(pod=pod, container_name=container_name):
Copy link
Contributor

Choose a reason for hiding this comment

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

same fix was suggest and merged in #23883 but It was reverted in #24479
so just trying the same thing again is probably not going to work?
cc @akakakakakaa as the original author

Copy link
Author

@windmark windmark Nov 4, 2022

Choose a reason for hiding this comment

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

Hi, thanks, yeah I'm aware. I opened this PR as a Draft (but wasn't aware that still pinged anyone) to see if I could run the CI tests.

The original PR was reverted due to failing tests, but unfortunately the CI logs and the slack conversations have both fallen victim to the retention. I'm still figuring out the testing flow locally but haven't found any failing tests yet, so must be missing something (as a new contributor).

@windmark
Copy link
Author

windmark commented Nov 4, 2022

The test failure in CI, https://github.com/apache/airflow/actions/runs/3393106966/jobs/5643930560, does not seem to be caused by this change. It's failing on await_pod_start which follows a different code path than the await_container_completion. Might it have been a flake perhaps?

@potiuk
Copy link
Member

potiuk commented Nov 4, 2022

restarted it. Likely flake

@potiuk
Copy link
Member

potiuk commented Nov 7, 2022

Yep. Passed. Is it ready for review? Can you mark it as such if it is please ?

@windmark
Copy link
Author

windmark commented Nov 7, 2022

Yep. Passed. Is it ready for review? Can you mark it as such if it is please ?

Since the original PR failed due to missing tests, I have been trying to identify which those failing tests could be. However, I haven't been able to fail any tests yet. All tests succeed.

I noticed that one code path wasn't properly tested, so I'm refactoring the tests to cover that. That doesn't answer why the tests failed in main for the previous PR though..

@potiuk Do you know if there is a way to trigger more tests in CI than what was run for this PR?

I successfully ran breeze k8s run-complete-tests --force-recreate-cluster --upgrade --rebuild-base-image locally

@@ -810,7 +816,11 @@ def test_pod_template_file_with_full_pod_spec(self):
assert k.pod.spec.containers[0].env == [k8s.V1EnvVar(name="env_name", value="value")]
assert result == {"hello": "world"}

def test_full_pod_spec(self):
@pytest.mark.parametrize(
Copy link
Author

Choose a reason for hiding this comment

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

Adding support for this required me to move from a unittest.TestCase to pytest. This required a couple of other fixes, but is worth it in my mind, since other test classes are using pytest.

"""
).strip()
assert any(line.startswith(expected_line) for line in cm.output)
expected_line = (
Copy link
Author

Choose a reason for hiding this comment

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

Due to the different log format of caplog, I decided to reformat this to a regex search instead of the any(... logic. This was easier to do with this string format instead of the multiline.

@windmark windmark marked this pull request as ready for review November 7, 2022 11:41
@windmark windmark requested review from eladkal and removed request for jedcunningham November 9, 2022 09:15
@windmark windmark closed this Nov 10, 2022
@windmark windmark deleted the fix-k8-container-completion branch November 10, 2022 10:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers provider:cncf-kubernetes Kubernetes provider related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect await_container_completion in KubernetesPodOperator
3 participants