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

Make sure terraform command is canceled #11105

Merged
merged 1 commit into from Oct 6, 2020

Conversation

michaelgrifalconi
Copy link
Contributor

@michaelgrifalconi michaelgrifalconi commented Sep 29, 2020

Make sure terraform command is correctly canceled after timeout

Full work log:
Changed "lib/publiccloud/provider.pm" to make it easier to reproduce
From "use constant TERRAFORM_TIMEOUT => 17 * 60;"
To "use constant TERRAFORM_TIMEOUT => 2 * 60;"

Reproduced issue on run: https://openqa.suse.de/tests/4749425#step/ssh_interactive_init/51
Error on log: "No map for '�' at /usr/lib/os-autoinst/consoles/VNC.pm line 741."

Try to fix with change on "lib/publiccloud/provider.pm"

From "type_string(qq(\c\));"
To "type_string("\cC");"
Still same error: "No map for '�' at /usr/lib/os-autoinst/consoles/VNC.pm line 741."

Turns out, "ssh_interactive_init" is running on VNC and "type_string(\cC||\c\)" are meant for serial only.

Added a check for serial terminal on "provider.pm" not if fails as expected:
https://openqa.suse.de/tests/4751094

Changed back to "use constant TERRAFORM_TIMEOUT => 17 * 60;"

One last run without code to force reproducing the issue: https://openqa.suse.de/tests/4759779

@michaelgrifalconi michaelgrifalconi force-pushed the terraformAzure branch 4 times, most recently from 28b853f to faa8d71 Compare October 1, 2020 06:39
@michaelgrifalconi michaelgrifalconi changed the title [WIP] Make sure terraform command is canceled Make sure terraform command is canceled Oct 1, 2020
@michaelgrifalconi
Copy link
Contributor Author

Hey @pdostal can you have a look? Thank you!

Comment on lines +382 to +387
if (is_serial_terminal()) {
type_string(qq(\c\\)); # Send QUIT signal
}
else {
send_key('ctrl-\\'); # Send QUIT signal
}
Copy link
Member

Choose a reason for hiding this comment

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

As those lines are twice the same they can be a subroutine. But it's a minor priority suggestion 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

a grep shows:

os-autoinst-distri-opensuse> grn '\\c\\\\'
lib/publiccloud/azure.pm:226:            type_string(qq(\c\\));
lib/publiccloud/provider.pm:382:        type_string(qq(\c\\));        # Send QUIT signal
lib/publiccloud/provider.pm:440:        type_string(qq(\c\\));        # Send QUIT signal
tests/kernel/virtio_console_long_output.pm:65:            type_string(qq(\c\\));              # Send QUIT signal

The last one seems to be for virtio_console so no change needed. But the one in azure.pm might hit your case as well, WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shouldn't hurt to patch also azure.pm, you are right. I just updated my commit and started a new run https://openqa.suse.de/tests/4772971#details

when a test is using VNC instead of serial console
https://progress.opensuse.org/issues/69952
@michaelgrifalconi
Copy link
Contributor Author

@cfconrad and @pdostal if you are ok with the PR, can you merge it please? I have no permission to do it.

Thanks!

@pdostal pdostal merged commit 41aca92 into os-autoinst:master Oct 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants