-
Notifications
You must be signed in to change notification settings - Fork 267
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
Fix console/sshd problem on s390x or svirt backend #6360
Conversation
# Exit properly and check we're root again | ||
script_run("exit", 0); | ||
assert_script_run "whoami | grep root"; | ||
if (is_serial_terminal()) { |
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.
I consider this huge if/else too complex and not easy to read. The tests should be written so that they are easy to read from a testers point of view and not unfold into two completely different branches of test flow. Can we please try to find a nicer way?
@OleksandrOrlov this looks related to what you would like to prevent as well, doesn't it?
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.
Hello @okurz, I don't really see any other option over here, because of when the VirtOS serial terminal is available we wanna run this interactive part and when it isn't we wanna run the other.
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.
What is so complex on this if/else ? Do you know what complex even mean ?
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.
@okurz tbh the if/else looks fine (from a condition perspective) however while the instructions are long, I think they are more explicit... which helps.
# Exit properly and check we're root again | ||
script_run("exit", 0); | ||
assert_script_run "whoami | grep root"; | ||
if (is_serial_terminal()) { |
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.
@okurz tbh the if/else looks fine (from a condition perspective) however while the instructions are long, I think they are more explicit... which helps.
if (is_serial_terminal()) { | ||
# Make interactive SSH connection as the new user | ||
type_string "ssh -v -l $ssh_testman localhost -t\n"; | ||
wait_serial('Are you sure you want to continue connecting (yes/no)?', undef, 0, no_regex => 1); |
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.
Ahoj Pavel!
I think the wait_serial is waiting for the ssh response as you wrote it, but most likely there is a unwanted whitespace after the question mark as over here. Generally do we need host checking ?
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.
Ahoj @mloviska!
imo it's not about the whitespace, the serial terminal is simply not supported. On your example the result is empty, but on supported it contains the whole SSH output. That's why I added the is_serial_terminal()
condition.
I started to work on this test because of the SSH was recently broken and it was exactly in this part. That's why I'd like this test to be as close as possible to the real scenario.
Seems to be fixed on intel and s390x. Problem still appear on ppc64le. |
@pdostal, np. Just reporting seen issues. :) |
@pdostal I'm sorry, I'm not able to pass |
@pdostal, I'm sorry, is there any progress regarding the issue on ppc64le? Could you please point me to the PR if any, as I want to mark the current fail in openQA. It is still failing on the last build 121.1: https://openqa.suse.de/tests/2324918#step/sshd/23 Thank you. |
@OleksandrOrlov there's currently no progress. The VirtIO console should work on ppc64le so I don't want to disable it but to be fair I don't know how to fix it either. Anyways feel free to point to the progress issue mentioned in the description. |
svirt
implemented onselect_serial_terminal()
if (is_serial_terminal())
used inconsole/sshd
VirtIO
is missingThis is follow up of #6351 and #6295 reported by @OleksandrOrlov