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

Return with error code 408 when basic commands timeout on the cluster #24

Merged
merged 2 commits into from
Mar 5, 2025

Conversation

ekouts
Copy link
Collaborator

@ekouts ekouts commented Mar 5, 2025

Fixes RESTAPI-1462.

@ekouts ekouts requested review from theely and jpdorsch March 5, 2025 08:33
@ekouts ekouts self-assigned this Mar 5, 2025
Copy link
Collaborator

@jpdorsch jpdorsch left a comment

Choose a reason for hiding this comment

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

LGTM

@ekouts ekouts merged commit 4f1e420 into master Mar 5, 2025
2 checks passed
@ekouts ekouts deleted the bugfix/timeout_error_code branch March 5, 2025 09:58
@@ -34,6 +34,11 @@ def error_handling(self, stderr: str, exit_status: int):
if len(stderr) > 0:
error_mess += f" and error message:{stderr.strip()}"

if exit_status == 124:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This assumption exit_status:124 = Timeout is only true if the the command was executed with timeout in front.
The base_command_error_handling has no way to know if that was the case.

I believe the proper solution would be to implement a new class BaseCommandWithTimeout and BaseCommandWithTimeoutErrorHandling that extends BaseCommand and ensures all commands are executed with a timeout. Meaning the assumption above would always be true.

Alternatively we should state the assumption in a comment in the above code.

Copy link
Collaborator

Choose a reason for hiding this comment

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

p.s. my preference is to simply add a comment and state the assumption.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, sorry, I have already merged but I can fix it in another PR! :)
If I remember right, FirecREST runs every command with a timeout, right? I will check the code and see if we use it for a different case too. I can add the comment anyway

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants