feat: allow proxying readiness checks to the function #145

LucasRoesler · 2022-10-09T19:14:28Z

Allow setting an endpoint path for the function readiness check via an ENV variable function_ready_endpoint

When this value is set, the requests to /_/ready will execute an empty GET request with/to the configured endpoint. This allows the function authors to implement custom readiness check logic.

This custom request is checked after the the standard liveliness checks and the ConcurrencyLimiter check. For a completely custom readiness check, the function should be deployed with max_inflight == 0 and function_ready_endpoint to the custom path.

Motivation and Context

I have raised an issue to propose this change (required)

Resolved #144

How Has This Been Tested?

Additional unit tests have been added
Manually tested with a custom python function

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)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I've read the CONTRIBUTION guide
I have signed-off my commits with git commit -s
I have added tests to cover my changes.
All new and existing tests passed.

Allow setting an endpoint path for the function readiness check via an ENV variable `function_ready_endpoint` When this value is set, the requests to `/_/ready` will execute an empty GET request with/to the configured endpoint. This allows the function authors to implement custom readiness check logic. This custom request is checked _after_ the the standard liviness checks and the ConcurrencyLimiter check. For a completely custom readiness check, the function should be deployed with `max_inflight == 0` and `function_ready_endpoint` to the custom path. Signed-off-by: Lucas Roesler <roesler.lucas@gmail.com>

kevin-lindsay-1 · 2022-10-10T16:53:29Z

I have tested this, and the functionality appears to work as expected.

I built a golang fn that had a readiness endpoint of /_/function/ready and simply serve it, with an init() function defined in my fn. Once the init fn completed, the readiness endpoint was made available, and began outputting 200s, and the function entered the ready state in k8s.

kevin-lindsay-1 · 2022-10-10T16:57:20Z

One note: the readiness endpoint seems to emit logs, which should probably be silent by default.

kevin-lindsay-1 · 2022-10-10T17:02:15Z

This is a priority to get merged for me, because if the gateway forwards traffic to a fn before it is initially ready, while the handler itself is not yet available in the fn to process the invocation, this would likely result in errors. This error case is similar to the Istio issue we previously solved for, where the gateway was sending traffic to the pod just a few milliseconds early.

LucasRoesler · 2022-10-11T15:04:56Z

@kevin-lindsay-1 do you mean the error log or the proxy log line? I would have to add a bit more code to make that log line disappear because I am reusing the function proxy implementation, it just looks like any other request at the moment?

The error log line should never happen, if you are seeing that, then i am very curious for more details.

LucasRoesler · 2022-10-11T15:07:57Z

On more thing, we already exclude the kube-probe from the logs

	// Exclude logging for health check probes from the kubelet which can spam
	// log collection systems.
	if !strings.HasPrefix(r.UserAgent(), "kube-probe") {
		log.Printf("%s %s - %s - ContentLength: %d", r.Method, r.RequestURI, res.Status, res.ContentLength)
	}

so ... it should be omitted if/when i pass the headers along

kevin-lindsay-1 · 2022-10-11T15:18:42Z

I'm pretty sure I was seeing the proxy log line. Looked like the one you in the code example above.

I'd be fine with a follow-up patch, if I can confirm the behavior. It didn't seem to do it after the initial start, so it might have been an "error" due to not passing initial readiness.

When proxying the readiness request to the upstream function, we should copy the original request headers. This allows the handler to use headers, if needed. It also allows the of-watchdog to suppress the noisy request logging for readiness checks. Signed-off-by: Lucas Roesler <roesler.lucas@gmail.com>

LucasRoesler · 2022-10-11T19:51:51Z

I'm pretty sure I was seeing the proxy log line. Looked like the one you in the code example above.

I'd be fine with a follow-up patch, if I can confirm the behavior. It didn't seem to do it after the initial start, so it might have been an "error" due to not passing initial readiness.

Pretty easy to pass the headers along, i just pushed a followup commit to copy them from the original request, this should now include the User-Agent and allow the proxy log to be suppressed (if the user agent contains "kube-probe")

README.md

alexellis · 2022-10-12T09:11:56Z

main.go

+			// otherwise it will just route to `/`, typically this shouldn't be used or set
+			readyReq.RequestURI = r.endpoint
+			readyReq.Header = req.Header.Clone()
+			r.functionHandler.ServeHTTP(w, readyReq)


Why wouldn't we http.DefaultClient.Do this?

That requires re-parsing all of the config options to build the correct URL etc. The existing function handler already exists and has all of the desired configuration and URL parsing/construction.

I think it also matches the semantics of the feature:

"during readiness, invoke the function in a certain way"

This code matches this semantic directly because we pass down the function invoker (i.e. handler) and then invoke it.

@alexellis the doc strings have been added and it also ensures the basic unwrapped handler is used in the readiness check.

User read_path instead of function_ready_endpoint. This should be clearer that it needs to be a path on the function and match prior art in other projects. Signed-off-by: Lucas Roesler <roesler.lucas@gmail.com>

Document why we use the base function handler to provide custom ready checks to all function modes. Additionally, make sure that the unwrapped function handler is passed to the readiness check. This ensures that readiness checks do not count toward the concurrency limit Signed-off-by: Lucas Roesler <roesler.lucas@gmail.com>

alexellis · 2022-10-14T08:52:51Z

main.go

@@ -65,7 +65,16 @@ func main() {
 		os.Exit(1)
 	}

-	requestHandler := buildRequestHandler(watchdogConfig, watchdogConfig.PrefixLogs)
+	if watchdogConfig.ReadyEndpoint != "" {


I think we can omit this because it will generate a lot of logs over the course of a year

alexellis · 2022-10-14T09:29:01Z

Hi @LucasRoesler I did some more testing on this and wanted to share the results:

go build && mode=serializing ready_path=/ready  upstream_url="http://127.0.0.1:8888" fprocess="env" ./of-watchdog 

curl localhost:8080/_/ready

Document why we use the base function handler to provide custom ready checks to all function modes. Additionally, make sure that the unwrapped function handler is passed to the readiness check. This ensures that readiness checks do not count toward the concurrency limit Review by AE - reverts change to faas-middleware, by introducing an interface. Closes: #145 Signed-off-by: Lucas Roesler <roesler.lucas@gmail.com> Signed-off-by: Alex Ellis (OpenFaaS Ltd) <alexellis2@gmail.com>

alexellis · 2022-10-14T12:17:27Z

I have a fix for this, it wasn't a new issue in a new PR along with your commits (for you to get credit), but was possibly triggered by the changes.

Since you may want this PR to count towards hacktoberfest, feel free to open a PR to the README file to update how this feature works and I'll merge that after.

Document why we use the base function handler to provide custom ready checks to all function modes. Additionally, make sure that the unwrapped function handler is passed to the readiness check. This ensures that readiness checks do not count toward the concurrency limit Review by AE - reverts change to faas-middleware, by introducing an interface. Closes: #145 Signed-off-by: Lucas Roesler <roesler.lucas@gmail.com> Signed-off-by: Alex Ellis (OpenFaaS Ltd) <alexellis2@gmail.com>

LucasRoesler force-pushed the feat-allow-proxying-ready-check-to-function branch from 83d5ecf to 318d12d Compare October 10, 2022 12:44

LucasRoesler force-pushed the feat-allow-proxying-ready-check-to-function branch from 318d12d to fd43548 Compare October 10, 2022 12:51

LucasRoesler marked this pull request as ready for review October 10, 2022 12:51

alexellis reviewed Oct 12, 2022

View reviewed changes

README.md Outdated Show resolved Hide resolved

alexellis reviewed Oct 12, 2022

View reviewed changes

chore: rename function ready path env variable

8e2d5dd

User read_path instead of function_ready_endpoint. This should be clearer that it needs to be a path on the function and match prior art in other projects. Signed-off-by: Lucas Roesler <roesler.lucas@gmail.com>

LucasRoesler requested a review from alexellis October 12, 2022 16:08

alexellis reviewed Oct 14, 2022

View reviewed changes

alexellis mentioned this pull request Oct 14, 2022

Proxy readiness and make logs messages consistent #146

Merged

3 tasks

alexellis closed this in #146 Oct 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: allow proxying readiness checks to the function #145

feat: allow proxying readiness checks to the function #145

LucasRoesler commented Oct 9, 2022 •

edited

Loading

kevin-lindsay-1 commented Oct 10, 2022 •

edited

Loading

kevin-lindsay-1 commented Oct 10, 2022 •

edited

Loading

kevin-lindsay-1 commented Oct 10, 2022 •

edited

Loading

LucasRoesler commented Oct 11, 2022

LucasRoesler commented Oct 11, 2022

kevin-lindsay-1 commented Oct 11, 2022 •

edited

Loading

LucasRoesler commented Oct 11, 2022

alexellis Oct 12, 2022

LucasRoesler Oct 12, 2022

LucasRoesler Oct 12, 2022

alexellis Oct 14, 2022

alexellis commented Oct 14, 2022

alexellis commented Oct 14, 2022

feat: allow proxying readiness checks to the function #145

feat: allow proxying readiness checks to the function #145

Conversation

LucasRoesler commented Oct 9, 2022 • edited Loading

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

kevin-lindsay-1 commented Oct 10, 2022 • edited Loading

kevin-lindsay-1 commented Oct 10, 2022 • edited Loading

kevin-lindsay-1 commented Oct 10, 2022 • edited Loading

LucasRoesler commented Oct 11, 2022

LucasRoesler commented Oct 11, 2022

kevin-lindsay-1 commented Oct 11, 2022 • edited Loading

LucasRoesler commented Oct 11, 2022

alexellis Oct 12, 2022

Choose a reason for hiding this comment

LucasRoesler Oct 12, 2022

Choose a reason for hiding this comment

LucasRoesler Oct 12, 2022

Choose a reason for hiding this comment

alexellis Oct 14, 2022

Choose a reason for hiding this comment

alexellis commented Oct 14, 2022

alexellis commented Oct 14, 2022

LucasRoesler commented Oct 9, 2022 •

edited

Loading

kevin-lindsay-1 commented Oct 10, 2022 •

edited

Loading

kevin-lindsay-1 commented Oct 10, 2022 •

edited

Loading

kevin-lindsay-1 commented Oct 10, 2022 •

edited

Loading

kevin-lindsay-1 commented Oct 11, 2022 •

edited

Loading