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.

Sign up for GitHub

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

Uninstall OpenSearch after testing on dashboards and modify the exception handle to skip irrelevant exception #3823

Merged
merged 4 commits into from
Sep 14, 2023
Merged

Uninstall OpenSearch after testing on dashboards and modify the exception handle to skip irrelevant exception #3823

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c5c3fb1
Narrow the exception handle cases to avoid unwanted print
zelinh Jul 27, 2023
a885946
Update fix for the test_cluster to save all local cluster logs
zelinh Jul 27, 2023
85d5664
Update tests
zelinh Jul 28, 2023
4fd5967
Move uninstall
zelinh Sep 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions src/system/process.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,9 +66,9 @@ def terminate(self) -> int:
try:
for item in proc.open_files():
Copy link
Member

Choose a reason for hiding this comment

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

I believe this happened because we are checking all the process that is running on the host. The host has a number of processes running that may or may not be related to testing. Can you narrow down what process and open files we are checking?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that's the properties of psutil to iterate all the running process though some of them are not related to our testing at all. I don't think there is a way to narrow down to specific process across platforms. Also some of the exception may be caused by race condition and we think we could pass on them since they are already outdated.

Copy link
Member

Choose a reason for hiding this comment

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

You can get the process id from line 48. See if the process is subprocess of it? Rather than iterating over all the existing processes?

Copy link
Member Author

Choose a reason for hiding this comment

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

@gaiksaya Basically in windows, according to my research, the python.exe was the process that was holding the file from removing which is not the OpenSearch/Dashboards process so that process id is not the one we want to check.

Copy link
Member

@peterzhuamazon peterzhuamazon Sep 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The pid thing needs another research and bug fix as it was initially only designed for tar, not any other dist that can be handled by external means.

As of now the pid is not accurate unless is tar.

Thanks.

if self.stdout.name == item.path:
raise Exception(f"stdout {item} is being used by process {proc}")
except Exception as err:
logging.error(f"{err.args}")
logging.error(f"stdout {item} is being used by process {proc}")
except Exception:
pass
Copy link
Member

Choose a reason for hiding this comment

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

Catching and exception and not doing anything is not making sense. Can you elaborate what you are trying to do here?

Copy link
Member Author

Choose a reason for hiding this comment

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

These exceptions caught are mainly some process that were not related to our testing or they were already closed during the iteration so we may not have permission to go through it. I have seen quite few examples of using this regarding of whether certain file is closed/not used by any process. e.g. https://stackoverflow.com/questions/64599502/how-can-i-check-if-a-file-is-actively-open-in-another-process-outside-of-my-pyth. I think it's safe to do this.

os.unlink(self.stdout.name)
self.stdout = None

Expand All @@ -81,9 +81,9 @@ def terminate(self) -> int:
try:
for item in proc.open_files():
if self.stderr.name == item.path:
raise Exception(f"stderr {item} is being used by process {proc}")
except Exception as err:
logging.error(f"{err.args}")
logging.error(f"stderr {item} is being used by process {proc}")
except Exception:
pass
os.unlink(self.stderr.name)
self.stderr = None

Expand Down
4 changes: 2 additions & 2 deletions src/test_workflow/test_cluster.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,16 +83,16 @@ def start(self) -> None:
def terminate(self) -> None:
if self.service:
self.termination_result = self.service.terminate()
self.__save_test_result_data(self.termination_result)

for service in self.dependencies:
termination_result = service.terminate()
self.__save_test_result_data(termination_result)
service.uninstall()
Copy link
Member

Choose a reason for hiding this comment

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

Could you make sure the uninstallation of dependencies wont affect __save_test_result_data?


if not self.termination_result:
raise ClusterServiceNotInitializedException()

self.__save_test_result_data(self.termination_result)

self.service.uninstall()
peterzhuamazon marked this conversation as resolved.
Show resolved Hide resolved

def __save_test_result_data(self, termination_result: ServiceTerminationResult) -> None:
Expand Down
36 changes: 34 additions & 2 deletions tests/tests_system/test_process.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
# this file be licensed under the Apache-2.0 license or a
# compatible open source license.

import logging
import tempfile
import unittest
from unittest.mock import MagicMock, call, patch
Expand Down Expand Up @@ -61,13 +62,44 @@ def test_terminate_unstarted_process(self) -> None:

self.assertEqual(str(ctx.exception), "Process has not started")

@patch('os.unlink')
@patch('psutil.Process')
@patch('subprocess.Popen')
@patch('psutil.process_iter')
def test_terminate_file_not_closed(self, procs: MagicMock, subprocess: MagicMock, process: MagicMock) -> None:
def test_terminate_process_file_not_closed(self, mock_procs: MagicMock, mock_subprocess: MagicMock, mock_process: MagicMock, mock_os_unlink: MagicMock) -> None:
process_handler = Process()

mock_process1 = MagicMock()
mock_process2 = MagicMock()

mock_item1 = MagicMock()
mock_process1.open_files.return_value = [mock_item1]

mock_item2 = MagicMock()
mock_process2.open_files.return_value = [mock_item2]

mock_procs.return_value = [mock_process1, mock_process2]

process_handler.start("mock_command", "mock_cwd")

mock_item1.path = process_handler.stdout.name
mock_item2.path = process_handler.stderr.name

process_handler.terminate()

procs.assert_called
mock_procs.assert_called()

mock_process1.open_files.assert_called()
mock_process2.open_files.assert_called()

with self.assertLogs(level='ERROR') as log:
logging.error(f'stdout {mock_item1} is being used by process {mock_process1}')
self.assertEqual(len(log.output), 1)
self.assertEqual(len(log.records), 1)
self.assertIn(f'stdout {mock_item1} is being used by process {mock_process1}', log.output[0])

with self.assertLogs(level='ERROR') as log:
logging.error(f'stderr {mock_item2} is being used by process {mock_process2}')
self.assertEqual(len(log.output), 1)
self.assertEqual(len(log.records), 1)
self.assertIn(f'stderr {mock_item2} is being used by process {mock_process2}', log.output[0])
21 changes: 12 additions & 9 deletions ..._workflow/test_integ_workflow/integ_test/test_local_test_cluster_opensearch_dashboards.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,8 @@ def test_start(self, mock_service_opensearch_dashboards: Mock, mock_service_open
@patch("test_workflow.integ_test.local_test_cluster_opensearch_dashboards.ServiceOpenSearch")
@patch("test_workflow.integ_test.local_test_cluster_opensearch_dashboards.ServiceOpenSearchDashboards")
@patch("test_workflow.test_cluster.TestResultData")
def test_terminate(self, mock_test_result_data: Mock, mock_service_opensearch_dashboards: Mock, mock_service_opensearch: Mock) -> None:
def test_terminate(self, mock_test_result_data: Mock, mock_service_opensearch_dashboards: Mock,
mock_service_opensearch: Mock) -> None:
mock_test_recorder = MagicMock()
mock_local_cluster_logs = MagicMock()
mock_test_recorder.local_cluster_logs = mock_local_cluster_logs
Expand Down Expand Up @@ -139,27 +140,29 @@ def test_terminate(self, mock_test_result_data: Mock, mock_service_opensearch_da
mock_service_opensearch_dashboards_object.terminate.assert_called_once()

mock_test_result_data.assert_has_calls([
call(self.component_name,
self.component_test_config,
123,
"test stdout_data",
"test stderr_data",
mock_log_files
),
call(
self.component_name,
self.component_test_config,
123,
"test stdout_data",
"test stderr_data",
mock_log_files_opensearch
),
call(self.component_name,
self.component_test_config,
123,
"test stdout_data",
"test stderr_data",
mock_log_files)
)
])

self.assertEqual(mock_local_cluster_logs.save_test_result_data.call_count, 2)

@patch("test_workflow.integ_test.local_test_cluster_opensearch_dashboards.ServiceOpenSearch")
@patch("test_workflow.integ_test.local_test_cluster_opensearch_dashboards.ServiceOpenSearchDashboards")
def test_terminate_service_not_initialized(self, mock_service_opensearch_dashboards: Mock, mock_service_opensearch: Mock) -> None:
def test_terminate_service_not_initialized(self, mock_service_opensearch_dashboards: Mock,
mock_service_opensearch: Mock) -> None:
mock_test_recorder = MagicMock()
mock_local_cluster_logs = MagicMock()
mock_test_recorder.local_cluster_logs = mock_local_cluster_logs
Expand Down