Skip to content
This repository has been archived by the owner on Oct 15, 2020. It is now read-only.

Add connection timeout #341

Merged
merged 1 commit into from
Dec 19, 2017
Merged

Add connection timeout #341

merged 1 commit into from
Dec 19, 2017

Conversation

eamonnotoole
Copy link
Contributor

We've had situations where we've hit against a system time-out when
connecting to OneView. That timeout was over 2 minutes long. To avoid
this we've added a timeout field to the config json, and if present the
value is used as a timeout in http.client.HTTPSConnection.

Description

Adds a connection time-out

Issues Resolved

[List any issues this PR will resolve. e.g., Fixes #1]

Check List

  • New functionality includes testing.
    • All tests pass for Python 2.7+ & 3.4+($ tox).
  • New functionality has been documented in the README if applicable.
    • New functionality has been thoroughly documented in the examples (please include helpful comments).
    • New endpoints supported are updated in the endpoints-support.md file.
  • Changes are documented in the CHANGELOG.

@eamonnotoole eamonnotoole force-pushed the add-connection-timeout branch 8 times, most recently from 3c85cf1 to c97dc75 Compare December 11, 2017 08:53
fgbulsoni
fgbulsoni suggested changes Dec 18, 2017
View reviewed changes
Copy link
Contributor

@fgbulsoni fgbulsoni left a comment

Choose a reason for hiding this comment

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

The changes suggested do seem to add value, and allowing for a custom timeout is something that does make sense.

I've made a few requests for changes, but overall the PR looks good. :octocat:

tests/unit/test_connection.py Outdated
@@ -858,6 +872,18 @@ def test_do_http_with_bad_status_line(self, mock_get_connection):

mock_conn.close.assert_has_calls([call(), call()])

@patch.object(connection, 'get_connection')
def test_do_http_with_io_error(self, mock_get_connection):
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest swapping out the more generic term io in this test title for the more specific timeout, which is being targeted in this test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

tests/unit/test_connection.py Outdated
@@ -604,6 +604,20 @@ def test_download_to_stream_when_error_status_with_empty_body(self, mock_get_con
else:
self.fail()

@patch.object(connection, 'get_connection')
def test_download_to_stream_with_io_error(self, mock_get_connection):
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest swapping out the more generic term io in this test title for the more specific timeout, which is being targeted in this test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

hpOneView/oneview_client.py Outdated

config = dict(ip=ip,
image_streamer_ip=image_streamer_ip,
api_version=api_version,
ssl_certificate=ssl_certificate,
credentials=dict(userName=username, authLoginDomain=auth_login_domain, password=password, sessionID=sessionID),
proxy=proxy)
proxy=proxy,
timeout=timeout)
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be added in line 242 after proxy=proxy, , no need to add a new line only for this field.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

hpOneView/oneview_client.py Outdated
@@ -116,7 +116,8 @@ class OneViewClient(object):
DEFAULT_API_VERSION = 300

def __init__(self, config):
self.__connection = connection(config["ip"], config.get('api_version', self.DEFAULT_API_VERSION), config.get('ssl_certificate', False))
self.__connection = connection(config["ip"], config.get('api_version', self.DEFAULT_API_VERSION),
config.get('ssl_certificate', False), config.get('timeout'))
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason to move config.get('ssl_certificate', False),? Otherwise it should remain in line 119.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

hpOneView/connection.py Outdated
@@ -148,6 +149,9 @@ def do_http(self, method, path, body, custom_headers=None):
conn.close()
time.sleep(1)
continue
except http.client.HTTPException as e:
raise HPOneViewException(str(e))
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps its worth using traceback.format_exc() here instead of str(e), in a similar way to this line, to make sure we get the same behavior in py2/3 and a full stack trace in case of hitting an issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

Choose a reason for hiding this comment

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

This line appears to still be using str(e) though, perhaps it was missed in the commit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Missed that one, fixed

CHANGELOG.md Outdated
@@ -1,3 +1,8 @@
# 4.5.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps 4.4.1 instead of 4.5.0 for the proposed version number?
Since no new resources are being added, timeout seems like a small enhancement.
I'd also suggest adding (Unreleased) 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.

Done.

@eamonnotoole eamonnotoole force-pushed the add-connection-timeout branch from c97dc75 to 8a91a71 Compare December 18, 2017 14:23
eamonnotoole
eamonnotoole commented Dec 18, 2017
View reviewed changes
Copy link
Contributor Author

@eamonnotoole eamonnotoole left a comment

Choose a reason for hiding this comment

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

See next patch-set for updates

@eamonnotoole eamonnotoole force-pushed the add-connection-timeout branch 3 times, most recently from 620f5dd to adf82aa Compare December 18, 2017 16:02
@eamonnotoole
Add connection timeout
0341436 
We've had situations where we've hit against a system time-out when
connecting to OneView.  That timeout was over 2 minutes long.  To avoid
this we've added a timeout field to the config json, and if present the
value is used as a timeout in http.client.HTTPSConnection.
@eamonnotoole eamonnotoole force-pushed the add-connection-timeout branch from adf82aa to 0341436 Compare December 18, 2017 17:03
eamonnotoole
eamonnotoole commented Dec 18, 2017
View reviewed changes
Copy link
Contributor Author

@eamonnotoole eamonnotoole left a comment

Choose a reason for hiding this comment

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

Fixed that line.

fgbulsoni
fgbulsoni approved these changes Dec 18, 2017
View reviewed changes
Copy link
Contributor

@fgbulsoni fgbulsoni left a comment

Choose a reason for hiding this comment

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

LGTM, is the PR ready to be merged?

@eamonnotoole
Copy link
Contributor Author

Yes, it can be merged.

@fgbulsoni fgbulsoni merged commit 81273f2 into HewlettPackard:master Dec 19, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@fgbulsoni fgbulsoni fgbulsoni approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Backup - Error in settings.py
2 participants
@eamonnotoole @fgbulsoni