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

verifies support for temporarily unhealthy instances #782

Merged
merged 5 commits into from Jun 12, 2019

Conversation

Projects
None yet
2 participants
@shamsimam
Copy link
Contributor

commented Jun 7, 2019

Changes proposed in this PR

  • verifies support for temporarily unhealthy instances via integration test
  • allows temporarily setting the default response status for all kitchen requests

Why are we making these changes?

We would like to confirm, via an integration test, support for temporarily unhealthy service instances.

@shamsimam shamsimam requested a review from DaoWen Jun 7, 2019

@shamsimam shamsimam self-assigned this Jun 7, 2019


# Temporarily set status on responses
default_status = self.headers.get('x-kitchen-default-status-value')
default_timeout = self.headers.get('x-kitchen-default-status-timeout')

This comment has been minimized.

Copy link
@DaoWen

DaoWen Jun 7, 2019

Contributor

As discussed offline, let's make processing these headers optional, only enabled when a command-line flag is set. Maybe --enable-status-change?

This comment has been minimized.

Copy link
@shamsimam

shamsimam Jun 10, 2019

Author Contributor

Done.

@shamsimam shamsimam force-pushed the kitchen-temp-unhealthy branch from de288f7 to 22163ab Jun 7, 2019

@shamsimam

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2019

@DaoWen ready for another round of review

@shamsimam shamsimam force-pushed the kitchen-temp-unhealthy branch from 22163ab to 5f4aa19 Jun 11, 2019

@@ -478,7 +491,7 @@ class Kitchen(HTTPWebSocketsHandler):

def __handle_http_request(self):
"""Core logic for a Kitchen HTTP request (all verbs delegate to this handler)."""
global _pending_http_requests, _total_http_requests
global _default_response_status, _pending_http_requests, _total_http_requests

This comment has been minimized.

Copy link
@DaoWen

DaoWen Jun 12, 2019

Contributor

The change here isn't necessary: global is only for variables that are mutated.

This comment has been minimized.

Copy link
@shamsimam

shamsimam Jun 12, 2019

Author Contributor

Removed.

@@ -502,7 +515,7 @@ class Kitchen(HTTPWebSocketsHandler):
self.__response_body_callback = None
self.__response_bytes = None
self.__response_length = max_response_size
self.__status = 200
self.__status = _default_response_status

This comment has been minimized.

Copy link
@DaoWen

DaoWen Jun 12, 2019

Contributor

If we move the default response logic into the send_response method, then the request will return the status that's set at that time. This seems like more intuitive behavior in combination with the /sleep endpoint.

We could set __status = None here, and then use the default in send_response if the argument is None.

This comment has been minimized.

Copy link
@shamsimam

shamsimam Jun 12, 2019

Author Contributor

Done.

if _allow_response_status_change:
self.logger().info('Changing default response status to {}'.format(default_status_value))
_default_response_status = int(default_status_value)
self.__status = _default_response_status

This comment has been minimized.

Copy link
@DaoWen

DaoWen Jun 12, 2019

Contributor

Since __process_headers comes after __process_path, this line would clobber the /bad-status endpoint's status. If the default status logic is moved to send_response as described above, I think the interaction with /bad-status would be more intuitive.

This comment has been minimized.

Copy link
@shamsimam

shamsimam Jun 12, 2019

Author Contributor

Moved default status processing and setting of __status to the end of __process_headers

check-filtered-instances (fn [target-url healthy-filter-fn]
(let [instance-ids (->> (active-instances target-url service-id :cookies cookies)
(healthy-filter-fn :healthy?)

This comment has been minimized.

Copy link
@DaoWen

DaoWen Jun 12, 2019

Contributor

The indentation looks off here.

This comment has been minimized.

Copy link
@shamsimam

shamsimam Jun 12, 2019

Author Contributor

Done.


# Set the response status if it is not set
if self.__status is None:
self.__status = _default_response_status

This comment has been minimized.

Copy link
@DaoWen

DaoWen Jun 12, 2019

Contributor

I really liked the idea of moving this into send_response, but this works too.

@DaoWen

DaoWen approved these changes Jun 12, 2019

@DaoWen DaoWen merged commit d3367d2 into master Jun 12, 2019

2 checks passed

Mergeable Mergeable Run has been Completed!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@dposada dposada deleted the kitchen-temp-unhealthy branch Jun 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.