-
Notifications
You must be signed in to change notification settings - Fork 180
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
Write test log for deployment failures #3596
base: main
Are you sure you want to change the base?
Conversation
a4635e1
to
a5dada8
Compare
a5dada8
to
c2accaf
Compare
lisa/platform_.py
Outdated
@@ -177,7 +177,7 @@ def prepare_environment(self, environment: Environment) -> Environment: | |||
def deploy_environment(self, environment: Environment) -> None: | |||
platform_runbook = cast(schema.Platform, self.runbook) | |||
|
|||
log = get_logger(f"deploy[{environment.name}]", parent=self._log) | |||
log = get_logger("", parent=environment.log) |
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.
@squirrelsc is this fine?
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.
It doesn't need to change the logger prefix. The original prefix can help find log from different sinks.
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.
With my change, the log is
lisa.env[generated_0]. creating deployment
Without removing "deploy..."
lisa.env[generated_0].deploy[generated_0] creating deployment
With the original platform logger (self._log)
lisa.[azure].deploy[generated_0] creating deployment
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.
lisa.env[generated_0].deploy
is better.
c2accaf
to
e69a729
Compare
lisa/runners/lisa_runner.py
Outdated
@@ -318,6 +321,7 @@ def _deploy_environment_task( | |||
result=test_results[0], | |||
exception=identifier, | |||
) | |||
remove_handler(log_file_handler, environment.log) |
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.
It should be in final, not except.
lisa/testsuite.py
Outdated
@@ -91,6 +93,10 @@ def __post_init__(self, *args: Any, **kwargs: Any) -> None: | |||
self._timer: Timer | |||
|
|||
self._environment_information: Dict[str, Any] = {} | |||
parent_log = get_logger("suite", self.runtime_data.metadata.suite.name) | |||
self.log = get_logger("case", self.name, parent=parent_log) | |||
self.case_log_path = self.__create_case_log_path() |
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.
The log path shouldn't be created at this memont, because the timestamp of the path indicates when the case started.
e69a729
to
f4f962f
Compare
@@ -616,12 +620,15 @@ def _attach_failed_environment_to_result( | |||
# so deployment failure can be tracked. | |||
environment.platform = self.platform | |||
result.environment = environment | |||
|
|||
result.subscribe_log(self._log) |
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.
Does it need to subscribe runner here? The below line result.handle_exception
should have enough log. The runner is called in multi threads, and may bring logs from other cases.
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.
That seems to be sufficient for failed deployment.
In case of successful deployment, the deployment logs wouldn't show up in test log. That should be fine, right?
lisa/testsuite.py
Outdated
self._log_file_handler.close() | ||
self._log_file_handler = None | ||
|
||
def save_nodes_serial_console_logs(self) -> None: |
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.
capture_serial_console_log
f4f962f
to
a05d963
Compare
At present, test logs are not generated for deployment failures. This change creates case log for deployment failure with the failure reason.
a05d963
to
221f2f0
Compare
At present, test logs are not generated for deployment failures. This change creates case log for deployment failure with the failure reason.