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

Optionally not follow logs in KPO pod_manager #22412

Merged
merged 4 commits into from
Mar 23, 2022

Conversation

dstandish
Copy link
Contributor

When writing an async KPO, you want to be able to read logs up to the current moment and exit (i.e. and not follow the logs). Additionally you want to be able to resume from a particular moment in time. That's what this PR enables.

When writing an async KPO, you want to be able to read logs up to the current moment and exit (i.e. and not follow the logs).  Additionally you want to be able to resume from a particular moment in time.  That's what this PR enables.
@dstandish dstandish requested a review from kaxil March 21, 2022 22:36
@boring-cyborg boring-cyborg bot added provider:cncf-kubernetes Kubernetes provider related issues area:providers labels Mar 21, 2022
@jedcunningham
Copy link
Member

Overall looks good, no test coverage though?

@dstandish
Copy link
Contributor Author

Overall looks good, no test coverage though?

added some coverage there.

@github-actions github-actions bot added the okay to merge It's ok to merge this PR as it does not require more tests label Mar 22, 2022
@github-actions
Copy link

The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.

@kaxil
Copy link
Member

kaxil commented Mar 23, 2022

Tests are failing 😞

@kaxil
Copy link
Member

kaxil commented Mar 23, 2022

=================================== FAILURES ===================================
  ________________ TestPodManager.test_fetch_container_since_time ________________
  
  self = <tests.providers.cncf.kubernetes.utils.test_pod_manager.TestPodManager object at 0x7f2aeb2d9f10>
  container_running = <MagicMock name='container_is_running' id='139822292685072'>
  mock_now = <MagicMock name='now' id='139822237497808'>
  
      @mock.patch('pendulum.now')
      @mock.patch('airflow.providers.cncf.kubernetes.utils.pod_manager.container_is_running')
      def test_fetch_container_since_time(self, container_running, mock_now):
          """If given since_time, should be used."""
          mock_pod = MagicMock()
          mock_now.return_value = DateTime(2020, 1, 1, 0, 0, 5, tzinfo=Timezone('UTC'))
          container_running.return_value = False
          self.mock_kube_client.read_namespaced_pod_log.return_value = [b'2021-01-01 hi']
          since_time = DateTime(2020, 1, 1, tzinfo=Timezone('UTC'))
          self.pod_manager.fetch_container_logs(pod=mock_pod, container_name='base', since_time=since_time)
          call_kwargs = self.mock_kube_client.read_namespaced_pod_log.call_args_list[0].kwargs
  >       assert call_kwargs['since_seconds'] == 5
  E       TypeError: tuple indices must be integers or slices, not str
  
  tests/providers/cncf/kubernetes/utils/test_pod_manager.py:309: TypeError
  ----------------------------- Captured stderr call -----------------------------
  INFO  [airflow.providers.cncf.kubernetes.utils.pod_manager.PodManager] hi
  ------------------------------ Captured log call -------------------------------
  INFO     airflow.providers.cncf.kubernetes.utils.pod_manager.PodManager:pod_manager.py:216 hi

@dstandish
Copy link
Contributor Author

thanks, will fix

@dstandish
Copy link
Contributor Author

call_kwargs = self.mock_kube_client.read_namespaced_pod_log.call_args_list[0].kwargs

  assert call_kwargs['since_seconds'] == 5

E TypeError: tuple indices must be integers or slices, not str

@kaxil
yeah locally i use 3.8 so sometimes i get bitten by changes to mock call arg handling between 3.7 and 3.8 ... and that's what happened here.

@dstandish dstandish merged commit 0a99be7 into apache:main Mar 23, 2022
@dstandish dstandish deleted the kpo-async-waking-airflow branch March 23, 2022 06:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers okay to merge It's ok to merge this PR as it does not require more tests provider:cncf-kubernetes Kubernetes provider related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants