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 xcom_sidecar stuck problem #24993

Merged
merged 4 commits into from
Jul 16, 2022
Merged

Conversation

MaksYermak
Copy link
Contributor

@MaksYermak MaksYermak commented Jul 12, 2022

This PR fixes the problem which is described here #22318
closes: #22318

Co-authored-by: Wojciech Januszek januszek@google.com
Co-authored-by: Lukasz Wyszomirski wyszomirski@google.com
Co-authored-by: Maksim Yermakou maksimy@google.com


^ 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 Jul 12, 2022
@@ -404,8 +404,12 @@ def await_pod_start(self, pod: k8s.V1Pod):
def extract_xcom(self, pod: k8s.V1Pod):
"""Retrieves xcom value and kills xcom sidecar container"""
result = self.pod_manager.extract_xcom(pod)
self.log.info("xcom result: \n%s", result)
return json.loads(result)
if isinstance(result, str) and result.replace('\n', '') == 'False':
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if isinstance(result, str) and result.replace('\n', '') == 'False':
if isinstance(result, str) and result.rstrip() == 'False':

It may also be worthwile to use a more obscure string (say __airflow_xcom_result_empty__?) to make the intent easier to identify.

Copy link
Member

Choose a reason for hiding this comment

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

+1 to using rstrip

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@uranusjr @kaxil I have changed replace() to rstrip()

Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

Agree with @uranusjr "else echo False; fi" is a bad idea, because "False" is quite likely return value from a Pod. Please change it to some more "unique" sentinel.

@MaksYermak
Copy link
Contributor Author

Agree with @uranusjr "else echo False; fi" is a bad idea, because "False" is quite likely return value from a Pod. Please change it to some more "unique" sentinel.

@potiuk I have updated the string name

@potiuk potiuk merged commit f05a065 into apache:main Jul 16, 2022
@ephraimbuddy ephraimbuddy added this to the Airflow 2.3.4 milestone Aug 9, 2022
@ephraimbuddy ephraimbuddy added the type:bug-fix Changelog: Bug Fixes label Aug 9, 2022
@ephraimbuddy ephraimbuddy removed this from the Airflow 2.3.4 milestone Aug 9, 2022
@ephraimbuddy ephraimbuddy removed the type:bug-fix Changelog: Bug Fixes label Aug 9, 2022
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.

KubernetesPodOperator xcom sidecar stuck in running
5 participants