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

Sync endpoints before starting HTTP server #759

Merged
merged 1 commit into from
Feb 5, 2021

Conversation

alexellis
Copy link
Member

@alexellis alexellis commented Feb 4, 2021

Signed-off-by: Alex Ellis (OpenFaaS Ltd) [email protected]

Description

Sync endpoints before starting HTTP server

Motivation and Context

The HTTP server which is used for CRUD and invocations should
not be started until the cached informers for endpoints is
ready. This may be related to issue #749

The cache sync duration is now being logged and measured for
users to provide logs and additional information.

How Has This Been Tested?

With several thousand pods and varying levels of replicas I was unable to reproduce a 404 error with the operator, using hey to generate traffic, and Prometheus to monitor status codes.

The following new logs are present in addition to the wait upon start-up:

I0204 10:20:01.609846       1 main.go:168] Waiting for cache sync in main
I0204 10:20:01.710044       1 main.go:175] Cache sync done in: 0.100190s
I0204 10:20:01.710101       1 main.go:176] Endpoints synced? true

When the wait statement was before the informers were started, the output read "Endpoints synced? true" and the sync returned immediately, so this seems to be making progress.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

The HTTP server which is used for CRUD and invocations should
not be started until the cached informers for endpoints is
ready. This may be related to issue #749

The cache sync duration is now being logged and measured for
users to provide logs and additional information.

Signed-off-by: Alex Ellis (OpenFaaS Ltd) <[email protected]>
@alexellis
Copy link
Member Author

@LucasRoesler I can't see the harm in adding this and cutting a release.

@stefanprodan and @grampelberg helped with the suggestions based - there is more that can be done to optimise going forward, but that's already written up on the roadmap.

Copy link
Contributor

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@alexellis alexellis merged commit 7b95916 into master Feb 5, 2021
@alexellis alexellis deleted the openfaasltd/endpoint-sync branch February 5, 2021 09:37
@alexellis
Copy link
Member Author

I've cut a release so that the two users on the thread can try it out and give feedback. https://github.com/openfaas/faas-netes/releases/tag/0.12.16

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.

2 participants