Skip to content
This repository was archived by the owner on Apr 7, 2022. It is now read-only.

[1LP][RFR] Fix SSH timeout issue in verify_revert_snapshot#9952

Merged
mshriver merged 3 commits intoManageIQ:masterfrom
prichard77:fixSSHrhv
May 22, 2020
Merged

[1LP][RFR] Fix SSH timeout issue in verify_revert_snapshot#9952
mshriver merged 3 commits intoManageIQ:masterfrom
prichard77:fixSSHrhv

Conversation

@prichard77
Copy link
Contributor

@prichard77 prichard77 commented Feb 26, 2020

Purpose or Intent

  • Fixing verify_revert_snapshot. We were initializing the variables needed for ssh before the VM had IPs. This was causing failures on SSH. Usage of connect_ssh was added to fix this issue.

PRT Run

{{ pytest: --use-provider rhv43 --use-provider vsphere67-nested --long-running cfme/tests/infrastructure/test_snapshot.py::test_verify_revert_snapshot }}

PR failures

The test sometimes failed for reasons unrelated to the changes (timeout when waiting for ap command prompt) (see PRT runs)

@dajoRH dajoRH added the lint-ok label Feb 26, 2020
@prichard77 prichard77 changed the title [WIP ]Add wait_pingable to verify_revert_snapshot [WIP]Add wait_pingable to verify_revert_snapshot Feb 26, 2020
@prichard77 prichard77 changed the title [WIP]Add wait_pingable to verify_revert_snapshot [WIP] Add wait_pingable to verify_revert_snapshot Feb 26, 2020
@dajoRH dajoRH added the WIP label Feb 26, 2020
@prichard77 prichard77 changed the title [WIP] Add wait_pingable to verify_revert_snapshot [WIPTEST] Add wait_pingable to verify_revert_snapshot Feb 26, 2020
@dajoRH dajoRH added WIP-testing and removed WIP labels Feb 26, 2020
@prichard77 prichard77 changed the title [WIPTEST] Add wait_pingable to verify_revert_snapshot [RFR] Add wait_pingable to verify_revert_snapshot Feb 26, 2020
@john-dupuy
Copy link
Contributor

PR looks good, but PRT is failing with

        )[0]
E       wait_for.TimedOutError: Could not do 'function is_reachable()' at /cfme/cfme_tests/cfme/utils/net.py:213 in time

cfme/utils/net.py:227: TimedOutError
TimedOutError
b"Could not do 'function is_reachable()' at /cfme/cfme_tests/cfme/utils/net.py:213 in time"

Copy link
Contributor

@jarovo jarovo left a comment

Choose a reason for hiding this comment

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

I don't think this is right way to solve the issue.

@prichard77
Copy link
Contributor Author

I believe the error in PRT we are seeing now may be an intermittent issue I have seen. It didn't show up while testing this locally today. I'll look into it more now.

@prichard77 prichard77 changed the title [RFR] Add wait_pingable to verify_revert_snapshot [WIPTEST] Add wait_pingable to verify_revert_snapshot Feb 26, 2020
@prichard77
Copy link
Contributor Author

After further thought, PRT might be using the wrong template. I am looking in logs to see if I can determine this. We may be caught between issues. I have PR#9942 which fixes the isue with using the wrong template. I have the template updated to the correct value in cfme_data.yaml on my local machine. I am not 100% positive this is what is happening, but it is likely. We'll need to push PR9942 to get this to work (at least until it hits the other issue with active VM).

@prichard77
Copy link
Contributor Author

I added vsphere provider to the PRT command to see what it does in PRT. It has been more stable locally. I am also testing the connect_ssh code that has been suggested, although I suspect it will also have issues..

Copy link
Contributor

@jarovo jarovo Feb 27, 2020

Choose a reason for hiding this comment

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

I think you should not yield here, but rather put the rest of the code until the line perhaps 226 under the first with statement.

Then we need to connect again, so another with statement and "connect_ssh()" needs to be done. This will need to go until the line 265 where the with will take care about the ssh disconnect

@prichard77
Copy link
Contributor Author

I was sure I updated the title of this PR when I updated the details, but I don;t see the change. I'll update it again.

@prichard77 prichard77 changed the title [WIPTEST] Add wait_pingable to verify_revert_snapshot [WIPTEST] Fix SSH timeout issue in verify_revert_snapshot Feb 28, 2020
@prichard77
Copy link
Contributor Author

I am still testing and not seeing things work well 100% of the time. I'll keep working on it and I need to look at the suggestions from Jaroslov. I am pushing my updates anyway so we can discuss a few things in while I continue to debug and test. It also seems like a waste of time to need to wait for the "wrong" IP to time out. Is thee another way besides just checking to see if we have the 10.x.x.x IP versus the 192.x.x.x IP? I am considering checking the IP with a variable like ip_class_octet so we can update if the network settings change. Possibly get this from yaml? suggestions?

@prichard77
Copy link
Contributor Author

prichard77 commented Feb 28, 2020

I'll fix the flake8 needed blank line in next commit. Ignore for now. there should be more changes.

@dajoRH dajoRH added needs-lint and removed lint-ok labels Feb 28, 2020
@prichard77
Copy link
Contributor Author

prichard77 commented Mar 2, 2020

I added a loop and wait, and also a check for the class octet of IPs found for the VM to connect_ssh. I believe this fixes the ssh issue with our test case, but now if seems like I have "hijacked' utility code that was intended to be reusable and have made it pretty specific to the needs of the test case. I am also curious about the AttrDict I added. Comments?

@dajoRH dajoRH removed the needs-lint label Mar 2, 2020
@dajoRH
Copy link
Contributor

dajoRH commented Apr 9, 2020

I detected some fixture changes in commit 2b086ffecb66cdc7617c0a1d2a3c69606b53c284

The global fixture utility_vm_nfs_ip is used in the following files:

  • cfme/tests/cli/test_appliance_console_db_restore.py

The global fixture utility_vm_samba_ip is used in the following files:

  • cfme/tests/cli/test_appliance_console_db_restore.py

The global fixture utility_vm_proxy_data is used in the following files:

  • cfme/tests/configure/test_proxy.py
    • prepare_proxy_specific
    • prepare_proxy_default

The global fixture utility_vm_ssh is used in the following files:

  • cfme/tests/configure/test_proxy.py
    • validate_proxy_logs
    • prepare_proxy_specific
    • prepare_proxy_default
    • test_proxy_valid
    • test_proxy_override

The local fixture nosleep is used in the following files:

  • cfme/utils/tests/test_connect_ssh.py

The local fixture vm_mock is used in the following files:

  • cfme/utils/tests/test_connect_ssh.py
    • test_connect_ssh

The local fixture vm_mock2 is used in the following files:

  • cfme/utils/tests/test_connect_ssh.py
    • test_connect_ssh

Please, consider creating a PRT run to make sure your fixture changes do not break existing usage 😃

@prichard77 prichard77 changed the title [1LP][WIP] Fix SSH timeout issue in verify_revert_snapshot [1LP][WIPTEST] Fix SSH timeout issue in verify_revert_snapshot Apr 13, 2020
@dajoRH dajoRH added WIP-testing and removed WIP labels Apr 13, 2020
@prichard77
Copy link
Contributor Author

The functionality in this PR has been split into 2 PRs so that additional work can be done on connect_ssh. If this update fixes a test case and doesn't break anything, why wouldn't we push what we have and fix connect_ssh in another PR that will not hold up this original PR (which was intended to fix a test)?

@jarovo jarovo mentioned this pull request May 6, 2020
@dajoRH dajoRH changed the title [1LP][WIPTEST] Fix SSH timeout issue in verify_revert_snapshot [1LP][WIP] Fix SSH timeout issue in verify_revert_snapshot May 21, 2020
@prichard77 prichard77 changed the title [1LP][WIP] Fix SSH timeout issue in verify_revert_snapshot [1LP][RFR] Fix SSH timeout issue in verify_revert_snapshot May 21, 2020
@dajoRH dajoRH removed the WIP label May 21, 2020
@prichard77 prichard77 changed the title [1LP][RFR] Fix SSH timeout issue in verify_revert_snapshot [1LP][WIPTEST] Fix SSH timeout issue in verify_revert_snapshot May 21, 2020
@prichard77
Copy link
Contributor Author

This test is passing but I cannot see the files created via touch on the vm when I check manually. I am not sure this is not a false positive.

@prichard77
Copy link
Contributor Author

I have tested this locally and can see the files created. The sudo command was causing them to get put in a different directory than I was expecting. looks good.

@prichard77 prichard77 changed the title [1LP][WIPTEST] Fix SSH timeout issue in verify_revert_snapshot [1LP][RFR] Fix SSH timeout issue in verify_revert_snapshot May 21, 2020
@prichard77 prichard77 requested a review from mshriver May 21, 2020 19:33
@mshriver mshriver merged commit c1937a9 into ManageIQ:master May 22, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants