-
Notifications
You must be signed in to change notification settings - Fork 165
[1LP][RFR] Testing appliance restart scenario #9814
[1LP][RFR] Testing appliance restart scenario #9814
Conversation
f7307c4
to
bf94d75
Compare
2e12257
to
613f5cd
Compare
@john-dupuy all required changes are done, please have a look once |
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.
Test case looks good. Since you are restarting the appliance, I am wondering if we should use a temp appliance. What do you think?
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.
See individual comments, should be small changes.
613f5cd
to
1e9ae72
Compare
pass | ||
appliance = temp_appliance_preconfig_funcscope | ||
old_uptime = appliance.ssh_client.uptime() | ||
restart_appliance_cmd = ("ap", RETURN, "18", "1", "Y") |
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 would recommend to start using SSHExpect
to validate the messages from appliance_conosole. Whithout the validation you do not know what command you are actually ordering as the menu changes and also you may have hard time figuring out which command was the incorrect one.
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.
run_commands
function returns one list so we can check in which order commands executed and what output we got from commands.
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.
@dgaikwad the issue that @JaryN is raising is a bit different, its about a level of assertion that 18
, 1
, Y
inputs are being sent against an expected prompt. SSHExpect handles failing early if an expected prompt is not displayed - so it doesn't just continue sending inputs against an unknown state.
See here for an example of its use:
def configure_primary_replication_node(self, pwd): |
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.
Tried to use SSHExpect but TC was failing:
Below is the code which I tried:
# Restarting the appliance
with SSHExpect(appliance) as interaction:
interaction.send('ap')
interaction.answer('Press any key to continue.', '', timeout=30)
interaction.answer('Choose the advanced setting: ', app_con_menu["restart_app"], timeout=30)
interaction.answer('Choose the restart option: ', '1', timeout=30)
interaction.answer('Are you sure you want to restart the appliance now? (Y/N):, 'Y', timeout=30)
I was getting below error:
> + textwrap.indent(current_output, '> '))
E cfme.exceptions.SSHExpectTimeoutError: Timeouted when waiting for 'Are you sure you want to restart the appliance now? (Y/N):'. Current output:
E > 1
E > Are you sure you want to restart the appliance now? (Y/N):
cfme/utils/ssh_expect.py:40: SSHExpectTimeoutError
After discussion with @JaryN , we decided to keep the previous code due to SSHExpect issue. . Somehow SSHExpect was not able to match expected string.
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.
There is some misunderstanding. I think when we were discussing, we reached conclusion that you managed to make the SSHExpect match the string after using re.escape
.
The problem was in the waiting for shutdown (quoting otputs from our the discussion):
interaction.answer('Choose the restart option: ', '1', timeout=30)
interaction.answer(re.escape('Are you sure you want to restart the appliance now? (Y/N): '), 'Y', timeout=30)
wait_for(
net_check, num_sec=180, func_args=["22", appliance.hostname, True, 3],
message='waiting to shutdown machine',
> fail_condition=True
)
E wait_for.TimedOutError: Could not do 'waiting to shutdown machine' at ... cfme/utils/net.py:61 in time
I don't want to make a pressure on Devidas to use the SSHExpect. I would like him to consider starting to use 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.
@JaryN you are right, I have not got any error while executing the commands but the machine was not going down,
# Restarting the appliance
with SSHExpect(appliance) as interaction:
interaction.send('ap')
interaction.answer('Press any key to continue.', '', timeout=30)
interaction.answer('Choose the advanced setting: ', app_con_menu["restart_app"], timeout=30)
interaction.answer('Choose the restart option: ', '1', timeout=30)
interaction.answer(re.escape('Are you sure you want to restart the appliance now? (Y/N): '), 'Y', timeout=30)
wait_for(
net_check, num_sec=180, func_args=["22", appliance.hostname, True, 3],
message='waiting to shutdown machine',
> fail_condition=True
)
E wait_for.TimedOutError: Could not do 'waiting to shutdown machine' at /home/dgaikwad/redhat/integration_tests/cfme/utils/net.py:61 in time
cfme/tests/cli/test_appliance_console.py:805: TimedOutError
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.
Change to SSHExpect
for these tests is not required in this PR, though I would love to see and enhancement to all the existing console test coverage to use it.
Please flatten the test method though, there's no need to embed a function within the test when waiting for the machine to come back up.
Also, consider using net_check
again to wait for SSH to be available after reboot, then check uptime()
once, outside of the wait loop.
1e9ae72
to
d8043cd
Compare
d8043cd
to
591d520
Compare
restart_appliance_cmd = ("ap", RETURN, app_con_menu["restart_app"], "1", TimedCommand("Y", 0)) | ||
appliance.appliance_console.run_commands(restart_appliance_cmd, timeout=30) | ||
|
||
wait_for( |
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.
Just as a note:
There is a mgmt
property on the Appliance instance, that gives you a wrapanapi instance directly from the provider hosting the appliance itself. This can be used to detect the power state of the appliance - but its not available on IPAppliance instances, which are not connected to the provider hosting the appliance.
Its a better way to detect whether or not the reboot occurred, but its limited in scope that it supports. Because this is using the temp_appliance fixture, you can be fairly certain that the appliance instance is an Appliance class, but if someone were to create a temp_appliance backing method that used regular IPAppliance, the test case wouldn't work.
The test could look for the mgmt
property, and use the actual power state if possible, falling back to this net_check method if necessary.
Purpose or Intent
Adding tests to test the appliance restart scenario via appliance console
PRT Run
{{pytest: ./cfme/tests/cli/test_appliance_console.py::test_appliance_console_restart -v}}