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

Refactoring the interceptor #105

Merged
merged 41 commits into from
Apr 12, 2021
Merged

Conversation

arschles
Copy link
Collaborator

@arschles arschles commented Mar 17, 2021

This PR fixes a bug in the interceptor in which it would not wait for a connection to be established if there was no server already waiting to accept conns, by adding a retry mechanism for the DialContext function in the HTTP proxy.

It does a few more things:

  • Adding more testing to the proxy
  • Refactoring the proxy to use standard net/http (gives it more control over the response lifecycle)
  • Using the standard library test framework, because stretchr/testify/suite was not adding much (stretchr/testify/require is still in use though)
  • Add a check in the proxy for the number of replicas in the deployment. If there are 0 replicas, it holds the request without trying to do any network operations until the replica count goes up

Checklist

  • Commits are signed with Developer Certificate of Origin (DCO)
  • Have the interceptor watch the backing deployment and only send requests when they're at 1 or more replicas (when we get to Use the external push gRPC protocol for external scaler => interceptor communication #97, we can push to the scaler when we see a request come in and 0 replicas on the deployment)
  • Fix tests
  • Add more tests for the proxy handler
  • Make the operator start up the interceptor with the proper new env vars
  • Figure out the connection timeout tests (the ones that are failing right now)
  • Make the interceptor time out on the waitFunc() (see newForwardingHandler)

Fixes #61

- Adding more testing to the proxy
- Refactoring the proxy to use standard net/http (gives it more control over the response lifecycle)
- Using the standard library test framework, because stretchr/testify/suite was not adding much (stretchr/testify/require is still in use though)

Signed-off-by: Aaron Schlesinger <[email protected]>
Signed-off-by: Aaron Schlesinger <[email protected]>
@arschles arschles added this to the Alpha 2 milestone Mar 17, 2021
arschles and others added 17 commits March 23, 2021 11:29
Signed-off-by: Aaron Schlesinger <[email protected]>
Signed-off-by: Aaron Schlesinger <[email protected]>
Signed-off-by: Aaron Schlesinger <[email protected]>
Signed-off-by: Aaron Schlesinger <[email protected]>
Signed-off-by: Aaron Schlesinger <[email protected]>
Signed-off-by: Aaron Schlesinger <[email protected]>
Signed-off-by: Aaron Schlesinger <[email protected]>
Signed-off-by: Aaron Schlesinger <[email protected]>
Signed-off-by: Aaron Schlesinger <[email protected]>
arschles and others added 13 commits March 25, 2021 13:46
Signed-off-by: Aaron Schlesinger <[email protected]>
Signed-off-by: Aaron Schlesinger <[email protected]>
Signed-off-by: Aaron Schlesinger <[email protected]>
Signed-off-by: Aaron Schlesinger <[email protected]>
Signed-off-by: Aaron Schlesinger <[email protected]>
Signed-off-by: Aaron Schlesinger <[email protected]>
Turns out, don't run a handler in a goroutine because (net/http/httptest).ResponseRecorder
is not concurrency-safe. Thanks to @asw101 and @khaosdoctor for help on this!

Signed-off-by: Aaron Schlesinger <[email protected]>
@arschles arschles marked this pull request as ready for review March 31, 2021 21:16
@arschles
Copy link
Collaborator Author

@tomkerkhove @khaosdoctor @zroubalik this was a bigger change and one of the big blockers for releasing alpha 2. Can you PTAL?

@khaosdoctor
Copy link
Contributor

I think that's fine, there's a test failing though.

@tomkerkhove tomkerkhove removed their assignment Apr 8, 2021
Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

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

LGTM, there's failing test though

Signed-off-by: Aaron Schlesinger <[email protected]>
@arschles
Copy link
Collaborator Author

Ok, tests fixed. Thanks for the reviews @khaosdoctor @zroubalik !

@arschles arschles merged commit 6b8d4ab into kedacore:main Apr 12, 2021
@arschles arschles deleted the interceptor-hold branch April 12, 2021 19:48
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.

Interceptor doesn't always "hold" request when there are no replicas available
4 participants