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

Custom socket connect timeout in Session() #210

Merged

Conversation

akhil-jha
Copy link
Contributor

@akhil-jha akhil-jha commented Apr 30, 2023

What?
Let the user give a timeout for socket.connect(). Default is still 60.

Why?
In cloud VMs, we get Connection Refused as the machine is not ready to take ssh connection just yet. Increasing the timeouts(reties) solves the issue.

Usage:

with Session(username="ec2-user",  hostname=hostname,  key_filename=private_key_path, socket_connect_timeout=500) as host:
      assert "hostname" in host.run("rpm -q hostname").stdout

@akhil-jha
Copy link
Contributor Author

Test:

ipdb> with BrokerSession(username="ec2-user", hostname=ip, key_filename=pubkey_data["private_key_path"], socket_connect_timeout=500,) as host:
                assert "hostname" in host.run("rpm -q hostname").stdout
                host.run("hostname").stdout
 

[WARNING 230502 13:07:53] Tried cmd=<built-in method connect of socket object at 0x7f0ecc1353a0> with cmd_args=[('54.197.154.60', 22)], cmd_kwargs={} but received err=ConnectionRefusedError(111, 'Connection refused')
    Trying again in 1 seconds.
[WARNING 230502 13:07:54] Tried cmd=<built-in method connect of socket object at 0x7f0ecc1353a0> with cmd_args=[('54.197.154.60', 22)], cmd_kwargs={} but received err=ConnectionRefusedError(111, 'Connection refused')
    Trying again in 2 seconds.
[WARNING 230502 13:07:56] Tried cmd=<built-in method connect of socket object at 0x7f0ecc1353a0> with cmd_args=[('54.197.154.60', 22)], cmd_kwargs={} but received err=ConnectionRefusedError(111, 'Connection refused')
    Trying again in 4 seconds.
[WARNING 230502 13:08:00] Tried cmd=<built-in method connect of socket object at 0x7f0ecc1353a0> with cmd_args=[('54.197.154.60', 22)], cmd_kwargs={} but received err=ConnectionRefusedError(111, 'Connection refused')
    Trying again in 8 seconds.
[WARNING 230502 13:08:09] Tried cmd=<built-in method connect of socket object at 0x7f0ecc1353a0> with cmd_args=[('54.197.154.60', 22)], cmd_kwargs={} but received err=ConnectionRefusedError(111, 'Connection refused')
    Trying again in 16 seconds.
[WARNING 230502 13:08:25] Tried cmd=<built-in method connect of socket object at 0x7f0ecc1353a0> with cmd_args=[('54.197.154.60', 22)], cmd_kwargs={} but received err=ConnectionRefusedError(111, 'Connection refused')
    Trying again in 32 seconds.

ipdb>

'ip-172-31-93-71.ec2.internal\n'

Copy link
Member

@ogajduse ogajduse left a comment

Choose a reason for hiding this comment

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

ACK
@JacobCallahan would it make sense to propagate this attribute to broker_settings.yaml?

@akhil-jha
Copy link
Contributor Author

ACK @JacobCallahan would it make sense to propagate this attribute to broker_settings.yaml?

I don't think so. When you're checking out the VM, AT is turning it on anyway(if I recall correctly). This setting will mostly be used in cloud VMs.

@JacobCallahan
Copy link
Member

@akhil-jha are you using Broker sessions directly instead of through a host object?

@akhil-jha
Copy link
Contributor Author

@akhil-jha are you using Broker sessions directly instead of through a host object?

Yea, directly .

I did checkout(didn't practically tried) init() of Broker(), I am probably gonna hit No data load from file exception. Since I don't really need any provider setup and I don't think I need to have a broker_settings.yaml for any reason at all. That would be just another thing to maintain.

@akhil-jha
Copy link
Contributor Author

akhil-jha commented May 2, 2023

Oops, I didn't checkout Host() 🤦‍♂️

EDIT: I saw that, I think using directly would be simpler. no? To accommodate socket_connect_timeout arg in Host() would require more changes.

@JacobCallahan
Copy link
Member

It would indeed, which is why I asked. Most users make connections from the Host objects themselves instead of directly with a Session object. Perhaps, if you make it a configurable option (with a sane default), then that would help them. Otherwise, it would need to be passed down during host object creation.

@akhil-jha akhil-jha force-pushed the add_socket_connect_timeout branch from aa6296e to aedf3b3 Compare May 4, 2023 10:04
@akhil-jha akhil-jha marked this pull request as draft May 4, 2023 10:23
@akhil-jha akhil-jha marked this pull request as ready for review May 4, 2023 10:52
@akhil-jha
Copy link
Contributor Author

from broker.hosts import Host
        for ip in host_ips:
            host = Host(
                username="ec2-user",
                hostname=ip,
                key_filename=pubkey_data["private_key_path"],
                socket_connect_timeout=500,
            )
            host.execute("rpm -q hostname").stdout
$ ENV_FOR_DYNACONF=stage_proxy iqe tests plugin provisioning -k test_aws_reservations_e2e_launch_template -k test_aws_reservations_e2e_launch_template
...
======================================================== 1 passed, 22 deselected in 137.50s (0:02:17) ========================================================

@akhil-jha akhil-jha marked this pull request as draft May 4, 2023 12:15
@JacobCallahan
Copy link
Member

@akhil-jha is this ready for merge, or did you want to do some more testing on it?

Copy link
Member

@JacobCallahan JacobCallahan left a comment

Choose a reason for hiding this comment

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

Let's switch from this implementation to simply passing this timeout
https://github.com/SatelliteQE/broker/blob/master/broker/hosts.py#L57
to the Sessions created here
https://github.com/SatelliteQE/broker/blob/master/broker/hosts.py#L65

I don't know why I removed it in #151

@akhil-jha akhil-jha force-pushed the add_socket_connect_timeout branch 2 times, most recently from 35f829e to 695a80c Compare May 7, 2023 16:06
@@ -68,6 +68,7 @@ def connect(
password=password,
port=_port,
key_filename=key_filename,
timeout=timeout
Copy link
Contributor Author

@akhil-jha akhil-jha May 7, 2023

Choose a reason for hiding this comment

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

Dependent on settings.HOST_CONNECTION_TIMEOUT. If not , it'll be none. That means no retry.

Copy link
Member

Choose a reason for hiding this comment

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

If None, simple_retry should still get your default of 60 so I think we're good here!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah right.okay yes.

@akhil-jha akhil-jha force-pushed the add_socket_connect_timeout branch from 695a80c to e8738c8 Compare May 7, 2023 17:43
@akhil-jha akhil-jha marked this pull request as ready for review May 8, 2023 08:56
Copy link
Member

@JacobCallahan JacobCallahan left a comment

Choose a reason for hiding this comment

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

ACK, thanks for the revision!

@@ -68,6 +68,7 @@ def connect(
password=password,
port=_port,
key_filename=key_filename,
timeout=timeout
Copy link
Member

Choose a reason for hiding this comment

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

If None, simple_retry should still get your default of 60 so I think we're good here!

@@ -70,7 +70,7 @@ def init_settings(settings_path, interactive=False):
validators = [
Validator("HOST_USERNAME", default="root"),
Validator("HOST_PASSWORD", default="toor"),
Validator("HOST_CONNECTION_TIMEOUT", default=None),
Validator("HOST_CONNECTION_TIMEOUT", default=60),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So you want to keep it? or should I remove it?

Copy link
Member

Choose a reason for hiding this comment

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

let's try to keep this version, but we'll need to pay attention to automation after this release is live.

@JacobCallahan JacobCallahan merged commit e34979b into SatelliteQE:master May 11, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants