Skip to content

openfaas-function-dev: add custom readiness check guidance#2

Draft
welteki wants to merge 1 commit into
openfaas:masterfrom
welteki:add-readiness-check-guidance
Draft

openfaas-function-dev: add custom readiness check guidance#2
welteki wants to merge 1 commit into
openfaas:masterfrom
welteki:add-readiness-check-guidance

Conversation

@welteki
Copy link
Copy Markdown
Member

@welteki welteki commented May 28, 2026

Description

Adds guidance to the openfaas-function-dev skill on configuring a custom
HTTP readiness endpoint for functions whose handlers perform initialization
(model load, cache warm-up, opening DB / SDK clients), with per-language
handler patterns and the required com.openfaas.ready.http.* annotations.

Motivation and context

Without a custom readiness endpoint, the first request after a cold start
or scale-from-zero can hit a Pod that is up but not yet able to serve. The
skill had no guidance for this.

How has this been tested

Testing is in progress by scaffolding several functions using the updated
skill.

Handlers that initialize state (model load, cache warm-up, DB pools)
mark the Pod Ready before they can actually serve, causing the first
request after a cold start or scale-from-zero to fail. Document the
custom /ready endpoint pattern so functions can signal true readiness.

Signed-off-by: Han Verstraete (OpenFaaS Ltd) <han@openfaas.com>
@reviewfn
Copy link
Copy Markdown

reviewfn Bot commented May 28, 2026

AI Pull Request Overview

Disclaimer: This review was generated by automated AI and may contain errors. Do not trust its outputs without human verification.

Summary

  • Adds a new reference document (reference/health-readiness.md) covering custom readiness/liveness check configuration for OpenFaaS functions.
  • Extends SKILL.md with an inline "Readiness checks for initialization" section and cross-references to the new reference file.
  • Updates the troubleshooting entry for slow-start pods with actionable annotation-level guidance.
  • Adds checklist item 6 to the pre-deploy checklist covering readiness endpoint verification.
  • The Go handler example contains an inconsistent nil-body guard that leaves io.ReadAll unprotected.
  • None of the three language examples handle initialization failure; a failed init silently leaves the Pod NotReady forever with no logged error or remediation guidance.

Approval rating (1-10)

7 — Content is accurate and fills a real gap; two issues in the code examples reduce reliability for implementors copying the patterns directly.

Summary per file

Summary per file
File path Summary
skills/openfaas-function-dev/SKILL.md Adds readiness section, updates troubleshooting table row, adds checklist item and reference link.
skills/openfaas-function-dev/reference/health-readiness.md New reference file with annotations table, handler contract, and per-language examples.
skills/openfaas-function-dev/reference/troubleshooting.md Expands the slow-start section with annotation snippet and a link to the new reference.

Overall Assessment

The documentation additions are well-scoped and address a genuine operational gap. The annotation table is complete, the max_inflight + /_/ready + ready_path combinator is non-obvious and its explanation here is valuable. Two issues in the example code are worth correcting before the skill is used to scaffold real handlers: a nil-body dereference in the Go example and a silent-failure mode common to all three languages that will confuse operators when init throws.

Detailed Review

Detailed Review

reference/health-readiness.md — Go example: unguarded io.ReadAll after nil check

func Handle(w http.ResponseWriter, r *http.Request) {
    if r.Body != nil {
        defer r.Body.Close()   // nil guard here …
    }
    // … early return for /ready …
    body, _ := io.ReadAll(r.Body)   // … but not here

The nil check at the top only wraps the defer Close. For the non-/ready code path, io.ReadAll(r.Body) is called unconditionally. If r.Body is nil — which the nil check acknowledges as possible — this panics at runtime (nil does not implement io.Reader). In practice, the of-watchdog sets a body on every request, but the guard signals the author knows nil is possible, and an implementor copying this verbatim may encounter it in other contexts (e.g., direct net/http testing with a manually constructed http.Request).

Fix: either remove the nil check and document that the of-watchdog guarantees a non-nil body, or mirror the guard around the Read:

var body []byte
if r.Body != nil {
    defer r.Body.Close()
    body, _ = io.ReadAll(r.Body)
}

reference/health-readiness.md — All three language examples silently swallow init failure

In every example, the readiness flag (_ready, state.ready, isReady) is only set to true on the happy path. If the init work throws/panics/returns an error — network unavailable, model file missing, DB unreachable — the flag stays false permanently and the Pod never transitions to Ready. Kubernetes will eventually exhaust failureThreshold and evict the Pod, but operators seeing a CrashLoopBackOff equivalent with no log output from the handler will have no indication of what failed.

None of the examples log the error or set an error state that the /ready endpoint can surface. This is not a code nit — it is a debuggability gap that will affect users directly. The reference should show, at minimum, capturing and logging the init error and optionally surfacing the error message in the 500 response body so kubectl logs or a probe response makes the failure visible.


SKILL.md / reference/troubleshooting.md — annotation guidance is consistent and correct

No issues. The annotation key names match the upstream docs. The /_/ready routing through the watchdog is correctly described and the direction to raise initialDelaySeconds for slow warm-ups is sound.

AI agent details.

Agent processing time: 1m20.071s
Environment preparation time: 10.687s
Total time from webhook: 1m33.6s

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.

1 participant