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

Add timeout for script_run "test -e $inventory" #20004

Merged
merged 1 commit into from
Sep 6, 2024

Conversation

badboywj
Copy link
Contributor

Avoid sles4sap_publiccloud_basetest post_fail_hook to fail in case of timeout at script_run("test -e $inventory")

Copy link

Great PR! Please pay attention to the following items before merging:

Files matching lib/**.pm:

  • Consider adding or extending unit tests in t/

This is an automatically generated QA checklist based on modified files.

Copy link
Contributor

@lpalovsky lpalovsky left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@alvarocarvajald alvarocarvajald left a comment

Choose a reason for hiding this comment

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

In general LGTM, but I do have a suggestion regarding the changes below, and also, while checking the failure reported in the ticket, I believe a 90s timeout could be enough in some times and not enough in another, as root cause seems to be that the shell session is still busy with the previous command.

lib/qesapdeployment.pm Outdated Show resolved Hide resolved
@badboywj badboywj force-pushed the TEAM-9586 branch 2 times, most recently from e4f489f to 90c771a Compare August 21, 2024 23:27
Copy link
Contributor

@alvarocarvajald alvarocarvajald left a comment

Choose a reason for hiding this comment

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

LGTM

@badboywj badboywj force-pushed the TEAM-9586 branch 6 times, most recently from 22084d9 to 781ec3c Compare August 23, 2024 23:45
@mpagot
Copy link
Contributor

mpagot commented Aug 27, 2024

What is previous command running that needs CTRL+C? Is it safe to terminate it like this? Generic approach of this particular qesap method is to be as less impacting as possible: get and upload as many logs as possible without perturbing test execution.

Copy link
Contributor

@mpagot mpagot left a comment

Choose a reason for hiding this comment

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

LGTM

@alvarocarvajald
Copy link
Contributor

alvarocarvajald commented Aug 27, 2024

What is previous command running that needs CTRL+C? Is it safe to terminate it like this? Generic approach of this particular qesap method is to be as less impacting as possible: get and upload as many logs as possible without perturbing test execution.

A ssh command to connect to a VM in the cloud that has not timed out in the shell session, but that has timed out in script_output killing the test.

It would be prudent to search every possible place this function is called on and what are the possible previous commands that are run in the shell session. In general a CTRL-C would not do anything if the shell is running and no command is blocking it, but if we want this test -e inventory.yaml to work, we need a shell session that is not blocked by anything else.

Copy link
Contributor

@mpagot mpagot left a comment

Choose a reason for hiding this comment

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

LGTM

@alvarocarvajald alvarocarvajald merged commit 3ddac30 into os-autoinst:master Sep 6, 2024
10 checks passed
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.

4 participants