Support startupProbe in webhook

[ahmetb]

In what area(s)?

/area API /kind spec

What version of Knative?

v0.17.2-gke.3

Expected Behavior

startupProbe should be supported.

Actual Behavior

It seems startupProbe usage is actively prevented in serving webhook.

Error from server (BadRequest): error when creating "STDIN": admission webhook "validation.webhook.serving.knative.dev" denied the request: validation failed: must not set the field(s): spec.template.spec.containers[0].startupProbe

Steps to Reproduce the Problem

apiVersion: serving.knative.dev/v1alpha1
kind: Service
metadata:
  name: hello
spec:
  template:
    spec:
      containers:
      - image: gcr.io/google-samples/hello-app:1.0
        startupProbe:
          exec:
            command: ["/usr/bin/test", "-f", "/start"]
          periodSeconds: 2
          failureThreshold: 100

[markusthoemmes]

I believe we want to look at consuming this field from Knative itself rather than relying on readinessProbe as much as we do today. Once we have a minimum K8s version that allows us to consume startupProbe, I assume we'll implement that and allow the same subset to be defined as we do today for readinessProbes (at least roughly).

If we allow this today, we might run into compatibility issues when we want to do that.

[evankanderson]

/triage accepted

[github-actions[bot]]

This issue is stale because it has been open for 90 days with no activity. It will automatically close after 30 more days of inactivity. Reopen the issue with /reopen. Mark the issue as fresh by adding the comment /remove-lifecycle stale.

[julz]

/remove-lifecycle stale

[julz]

I think now we've landed the changes to use StartupProbe properly for our own startup-readiness-probe optimisations we should be in a good place to look at allowing user-provided StartupProbes now.

/assign

[joke]

Knative release 1.2 requires a minimum K8s version of 1.21.

[psschwei]

@julz if you're okay with it, I can take over this one (I've been working on substituting in a startupProbe for the default readinessProbe, which addresses this)

[julz]

/unassign /assign @psschwei

[psschwei]

I've pulled together a list of various probing scenarios that @dprotaso mentioned in a Google doc:

https://docs.google.com/document/d/1ODpaq-7ChhF7UeN7OSgekPlDrZXhT-3DZUVyl0SGRGQ/edit

Figured it's easier to discuss in this issue and/or the doc rather than in the PR...

[ankit-db]

Is there any update on support for startupProbe here?

[psschwei]

Nope, haven't had any cycles to devote to this. /unassign

[jwcesign]

/assign

[jwcesign]

I read the docs and code, so the implement way shoud be: implement a startup prob in qp like readiness prob and once it success, stop it and start readinessProb.

I understand right?

But it looks quite few benifit because it does not support exec prob(I mean from qp side).

cc @psschwei

[psschwei]

We had some discussion in #12565 (in particular, @dprotaso 's comment here). I also pulled together this doc that covers some of the different scenarios Dave was alluding to.