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

In patch_sle we should wait a while to get the password. #6934

Merged
merged 2 commits into from Mar 4, 2019

Conversation

hjluo
Copy link
Contributor

@hjluo hjluo commented Mar 1, 2019

in patch_sle we need to wait for a while to get the password, the following check for is-active network expect 'active' but it got the root password from the preceding step. the issue is change user and gpasswd cost too much time.

@@ -483,6 +483,7 @@ sub wait_boot {
type_line_svirt '', expect => $login_ready, timeout => $ready_time + 100, fail_message => 'Could not find login prompt';
type_line_svirt "root", expect => 'Password';
type_line_svirt "$testapi::password";
sleep 10;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add comments here to describe why need add 10s sleep.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added, thanks.

Copy link
Member

Choose a reason for hiding this comment

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

After looking at the above again I wonder how that should help. Why call sleep 10 after typing the password?

@hjluo hjluo changed the title patach_sle should wait a while to get the password. In patach_sle we should wait a while to get the password. Mar 4, 2019
@lemon-suse lemon-suse changed the title In patach_sle we should wait a while to get the password. In patch_sle we should wait a while to get the password. Mar 4, 2019
@lemon-suse lemon-suse merged commit 219aada into os-autoinst:master Mar 4, 2019
@okurz
Copy link
Member

okurz commented Mar 4, 2019

Hi, I think you were on a good track with "waiting a bit" before typing into the password prompt but sorry I reverted your PR but I did so for the following reasons:

  • Just putting in a sleep with an arbitrary amount of time is evil. I have already seen the svirt s390x login to fail even with a sleep time of 10 seconds. In general sleep in coding is a bad sign, also see for example https://stackoverflow.com/questions/1096794/is-sleep-evil . When we are waiting for a specific event to happen then we should not just silently sleep but wait exactly for that: The specific event
  • The "sleep 10" is a considerable waste of time for all the svirt based test runs and in general we should try to optimize our code base to invest as little time as necessary. Sorry but this change is not helping.
  • Your PR contains two commits, both with the same subject line, both with the same misspelling. After all: You are changing a generic function that we use in multiple places. Just mentioning "patch_sle" is misleading
  • The change about the serial permissions is unmotivated and not commented at all
  • There was no review feedback from other teams yet and also did not ask for it even though you were changing generic methods which are used in nearly all tests.
  • Your ticket as well as your PR does not hint to any statistics even though this is a sporadic issue. You simply can not sufficiently adress a sporadic issue with a single verification run, right?
  • The ticket is far from the only one describing the issue and I highly suggest you look for better ticket relations to better understand the original issue and also so that we are able to provide a better fix which helps us all.

Every person with merge rights should keep the above in mind for any following pull requests and keep the quality of the overall code base in mind.

@hjluo
Copy link
Contributor Author

hjluo commented Mar 4, 2019

Hi okurz,
Thanks for the comments.
This issue was not reproducible on our migration tests, we just want to put this on to test the sporadic issue.
so I'd suggest if we can file a ticket for OpenQA tools while close this ticket. or can you provide us with a suggesting fix so we could avoid using the sleep in that step by means of "waiting for a specific event to happen"?

Huajian.Luo

@okurz
Copy link
Member

okurz commented Mar 4, 2019

Sure. I have linked the according – already existing – ticket in https://progress.opensuse.org/issues/45515 . It's https://progress.opensuse.org/issues/46394

@hjluo
Copy link
Contributor Author

hjluo commented Mar 5, 2019

OK, thanks for the info.

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