-
Notifications
You must be signed in to change notification settings - Fork 3.5k
[health-report CI] Print Logstash logs when pipeline faces an issue. #17991
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
Changes from all commits
1763cb4
dba2a0c
f07e625
ef1dc75
acc787b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -52,6 +52,7 @@ def __resolve_latest_stack_version_for(self, major_version: str) -> str: | |
return release_version | ||
|
||
def install_plugin(self, plugin_path: str) -> None: | ||
print("Installing logstash-integration-failure_injector plugin") | ||
util.run_or_raise_error( | ||
["bin/logstash-plugin", "install", plugin_path], | ||
f"Failed to install {plugin_path}") | ||
|
@@ -71,27 +72,23 @@ def run_logstash(self, full_start_required: bool) -> subprocess.Popen: | |
# --config.reload.automatic is to make instance active | ||
# it is helpful when testing crash pipeline cases | ||
config_path = os.getcwd() + "/.buildkite/scripts/health-report-tests/config" | ||
process = subprocess.Popen(["bin/logstash", "--config.reload.automatic", "--path.settings", config_path, | ||
"-w 1"], stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True, shell=False) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. review note: in pipeline.yaml config files (e.g here), we set worker count, this is not necessary. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed, this flag is not necessary and arguably was not appropriate for a generalized |
||
process = subprocess.Popen(["bin/logstash", "--config.reload.automatic", "--path.settings", config_path], | ||
stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True, shell=False) | ||
if process.poll() is not None: | ||
print(f"Logstash failed to run, check the the config and logs, then rerun.") | ||
return None | ||
|
||
print(f"Logstash started running with PID: {process.pid}.") | ||
|
||
# Read stdout and stderr in real-time | ||
logs = [] | ||
for stdout_line in iter(process.stdout.readline, ""): | ||
logs.append(stdout_line.strip()) | ||
print(stdout_line.strip()) | ||
# we don't wait for Logstash fully start as we also test slow pipeline start scenarios | ||
if full_start_required is False and "Starting pipeline" in stdout_line: | ||
break | ||
if full_start_required is True and "Pipeline started" in stdout_line: | ||
if "Pipeline started" in stdout_line: | ||
break | ||
if "Logstash shut down" in stdout_line or "Logstash stopped" in stdout_line: | ||
print(f"Logstash couldn't spin up.") | ||
print(logs) | ||
return None | ||
|
||
print(f"Logstash is running with PID: {process.pid}.") | ||
return process | ||
|
||
def stop_logstash(self, process: subprocess.Popen): | ||
|
@@ -102,7 +99,7 @@ def stop_logstash(self, process: subprocess.Popen): | |
if "Logstash shut down" in stdout_line or "Logstash stopped" in stdout_line: | ||
print(f"Logstash stopped.") | ||
return None | ||
# shudown watcher keep running, we should be good with considering time spent | ||
# shutdown watcher keep running, we should be good with considering time spent | ||
if time.time() - start_time > 60: | ||
print(f"Logstash didn't stop in 1min, sending SIGTERM signal.") | ||
process.kill() | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,11 +26,12 @@ def git_check_out_branch(branch_name: str) -> None: | |
|
||
def run_or_raise_error(commands: list, error_message): | ||
f""" | ||
Executes the {list} commands and raises an {Exception} if opration fails. | ||
Executes the {list} commands and raises an {Exception} if operation fails. | ||
""" | ||
result = subprocess.run(commands, env=os.environ.copy(), universal_newlines=True, stdout=subprocess.PIPE) | ||
if result.returncode != 0: | ||
full_error_message = (error_message + ", output: " + result.stdout.decode('utf-8')) \ | ||
full_error_message = (error_message + ", output: " + result.stdout) \ | ||
if result.stdout else error_message | ||
raise Exception(f"{full_error_message}") | ||
print(result.stdout) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. review note: regardless of failure status, prints log for better visibility. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice. Looks like the majority of the logging that is beneficial will happen at https://github.com/elastic/logstash/pull/17991/files/acc787b0aeac14acc37704857dccddd75661990f#diff-cabfb7b7a18cf3ea856fc69cec74a74345761db2625d41b10ceb7ec13c447252R85 but this is good too i think. |
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a generalized method? Does this print statement belong here specific to one plugin?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup! The
logstash-integration-failure_injector
isn't embedded in the core by default and we install while health-report integration tests to simulate various scenarios (failure, backpressure, etc...). This log statement is very useful because a) plugin-manager (logstash-plugin {operation}) silently processes its operations. If any failure we get while installing the plugin w/o this log statement that b) might be confusing if it came from LS build.