Add --probe-liveness and --probe-readiness flags #1697

dsimansk · 2022-06-30T12:00:08Z

Description

Quick format example:

// =======================================================================================
// Probes

// resolveProbe parses probes as a string
// It's split into two functions:
//   - resolveProbeOptions() -> common probe opts
//   - resolveProbeHandler() -> probe handler [HTTPGet, Exec, TCPSocket]
// Format:
//	- [http,https]:host:port:path;<common_opts>
//	- exec:cmd,cmd,...;<common_opts>
//  - tcp:host:port;<common_opts>
// Common opts (comma separated, case insensitive):
//	- <probe_opts>;InitialDelaySeconds=<int_value>,FailureThreshold=<int_value>,
//  	SuccessThreshold=<int_value>,PeriodSeconds==<int_value>,TimeoutSeconds=<int_value>

kn service create hello --image docker.io/dsimansk/helloworld --probe-liveness "http:::/;timeoutSeconds=60"

Changes

Add flag probe-liveness
Add flag probe-readiness
TODO: examples, more tests

Reference

Fixes #1522

/cc @vyasgun @rhuss

Release Note

Add --probe-* flags to manage liveness and readiness probes

knative-prow · 2022-06-30T12:00:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dsimansk

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [dsimansk]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

codecov · 2022-06-30T12:04:45Z

Codecov Report

Merging #1697 (65de98a) into main (f2dd5fc) will increase coverage by 0.13%.
The diff coverage is 91.13%.

@@            Coverage Diff             @@
##             main    #1697      +/-   ##
==========================================
+ Coverage   79.73%   79.86%   +0.13%     
==========================================
  Files         173      173              
  Lines       13277    13435     +158     
==========================================
+ Hits        10586    10730     +144     
- Misses       1960     1970      +10     
- Partials      731      735       +4

Impacted Files	Coverage Δ
pkg/kn/flags/podspec.go	`75.43% <58.82%> (-2.26%)`	⬇️
pkg/kn/flags/podspec_helper.go	`85.21% <100.00%> (+2.95%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f2dd5fc...65de98a. Read the comment docs.

vyasgun

Would it be a better idea to have separate flag(s) for probe options? The probe string seems a bit long and detailed to me and it also doesn't provide a way to update only the options

pkg/kn/flags/podspec.go

dsimansk · 2022-07-01T11:21:20Z

Would it be a better idea to have separate flag(s) for probe options? The probe string seems a bit long and detailed to me and it also doesn't provide a way to update only the options

That's a good idea, I like it. The other possible improvement could be opinionated probe defaults wrt timeouts, thresholds, etc.

@rhuss what would be your preference?

rhuss

Looks good for me in general, but I think we need to add to kn container, too and maybe overthink how to provide the parameters for HTTP or HTTPS checks.

rhuss · 2022-07-01T10:55:58Z

docs/cmd/kn_container_add.md

@@ -40,6 +40,8 @@ kn container add NAME
      --limit strings                The resource requirement limits for this Service. For example, 'cpu=100m,memory=256Mi'. You can use this flag multiple times. To unset a resource limit, append "-" to the resource name, e.g. '--limit memory-'.
      --mount stringArray            Mount a ConfigMap (prefix cm: or config-map:), a Secret (prefix secret: or sc:), an EmptyDir (prefix ed: or emptyDir:)or an existing Volume (without any prefix) on the specified directory. Example: --mount /mydir=cm:myconfigmap, --mount /mydir=secret:mysecret, --mount /mydir=emptyDir:myvol or --mount /mydir=myvolume. When a configmap or a secret is specified, a corresponding volume is automatically generated. You can specify a volume subpath by following the volume name with slash separated path. Example: --mount /mydir=cm:myconfigmap/subpath/to/be/mounted. You can use this flag multiple times. For unmounting a directory, append "-", e.g. --mount /mydir-, which also removes any auto-generated volume.
  -p, --port string                  The port where application listens on, in the format 'NAME:PORT', where 'NAME' is optional. Examples: '--port h2c:8080' , '--port 8080'.
+      --probe-liveness string        Add liveness probe to Service deployment. Supported probe types are HTTGet, Exec and TCPSocket. Format: [http,https]:host:port:path;<common_opts>, exec:cmd,cmd,...;<common_opts>, tcp:host:port;<common_opts>. Common opts (comma separated, case insensitive): InitialDelaySeconds=<int_value>, FailureThreshold=<int_value>, SuccessThreshold=<int_value>, PeriodSeconds==<int_value>, TimeoutSeconds=<int_value>


I wonder whether we could just use an http/https URL directly, which feels more natural. We could then parse the different segments (host, port, path) to put it into the probe.

@dsimansk @vyasgun what is your opinion on that ? One one hand, using separate parts reflect the structure of the prove itself (so might be more familiar for users with an k8s background), on the other hand, a URL is already a concise format for glueing together multiple parts. We could go even further and say for the exec and TCP probes to use "tcp://host:port" and "exec://cmd,cmd,cmd" kind of URLs.

At some time is probably a matter of taste.

Maybe start as you propose, but remove the optional part for now in favor for a dedicated cli option (see the other comment by @vyasgun ). wdyt ?

rhuss · 2022-07-01T14:17:13Z

docs/cmd/kn_container_add.md

@@ -40,6 +40,8 @@ kn container add NAME
      --limit strings                The resource requirement limits for this Service. For example, 'cpu=100m,memory=256Mi'. You can use this flag multiple times. To unset a resource limit, append "-" to the resource name, e.g. '--limit memory-'.
      --mount stringArray            Mount a ConfigMap (prefix cm: or config-map:), a Secret (prefix secret: or sc:), an EmptyDir (prefix ed: or emptyDir:)or an existing Volume (without any prefix) on the specified directory. Example: --mount /mydir=cm:myconfigmap, --mount /mydir=secret:mysecret, --mount /mydir=emptyDir:myvol or --mount /mydir=myvolume. When a configmap or a secret is specified, a corresponding volume is automatically generated. You can specify a volume subpath by following the volume name with slash separated path. Example: --mount /mydir=cm:myconfigmap/subpath/to/be/mounted. You can use this flag multiple times. For unmounting a directory, append "-", e.g. --mount /mydir-, which also removes any auto-generated volume.
  -p, --port string                  The port where application listens on, in the format 'NAME:PORT', where 'NAME' is optional. Examples: '--port h2c:8080' , '--port 8080'.
+      --probe-liveness string        Add liveness probe to Service deployment. Supported probe types are HTTGet, Exec and TCPSocket. Format: [http,https]:host:port:path;<common_opts>, exec:cmd,cmd,...;<common_opts>, tcp:host:port;<common_opts>. Common opts (comma separated, case insensitive): InitialDelaySeconds=<int_value>, FailureThreshold=<int_value>, SuccessThreshold=<int_value>, PeriodSeconds=<int_value>, TimeoutSeconds=<int_value>


I wonder whether we could just use an URL instead of host:port:path as seems to be more natural for the user to provide ? If we need the parts separately we still can parse them.

Yep, I was on the fence to have URL for HTTP probe, but I've opted for keeping the consistent format with other probe types. Not a strong preference though.

IMO we can allow both URL format and current : separated one. I.e. try to create URL from the string first and proceed accordingly.

rhuss · 2022-07-01T14:19:50Z

docs/cmd/kn_service_update.md

@@ -68,6 +68,8 @@ kn service update NAME
      --no-lock-to-digest                 Do not keep the running image for the service constant when not explicitly specifying the image. (--no-lock-to-digest pulls the image tag afresh with each new revision)
      --no-wait                           Do not wait for 'service update' operation to be completed.
  -p, --port string                       The port where application listens on, in the format 'NAME:PORT', where 'NAME' is optional. Examples: '--port h2c:8080' , '--port 8080'.
+      --probe-liveness string             Add liveness probe to Service deployment. Supported probe types are HTTGet, Exec and TCPSocket. Format: [http,https]:host:port:path;<common_opts>, exec:cmd,cmd,...;<common_opts>, tcp:host:port;<common_opts>. Common opts (comma separated, case insensitive): InitialDelaySeconds=<int_value>, FailureThreshold=<int_value>, SuccessThreshold=<int_value>, PeriodSeconds=<int_value>, TimeoutSeconds=<int_value>


one general concern: Liveness and readiness probes are part of the ContainerSpec, so in case you have multiple containers you can also have multiple probes. I think we should keep it like here for the main container but also add it to the kn container commands (that's also one reason why we cant use pipes for the probe generation).

That's auto-magically part of kn container add flags because logic+flags are shared from our PodSpec implementation.

However, it might be a bit tricky with sidecars.
https://github.com/knative/serving/blob/main/pkg/apis/serving/k8s_validation.go#L410-L411

Ah, I see. So kn container add should not allow those options. Are there any other container related options that might be applied only on the main-container ? If so, we might consider to restrict the possible options on container add in the future. For now, I'm fine that we still have it, I guess it will lead to an error on the server side if somebody would use a probe on an extra container.

rhuss · 2022-07-01T14:20:55Z

pkg/kn/flags/podspec.go

+	flagNames = append(flagNames, "probe-liveness")
+	flagset.StringVarP(&p.ReadinessProbe, "probe-readiness", "", "", "Add readiness probe to Service deployment. "+
+		commonProbeDescription)
+	flagNames = append(flagNames, "probe-readiness")


at some point we should also add a probe-startup for the startup probe (but not sure whether startup probes are supported by knative, we should check this)

That's not supported, as far as I've seen.

Per: https://github.com/knative/serving/blob/main/pkg/apis/serving/k8s_validation.go#L513-L520

https://github.com/knative/serving/blob/main/pkg/apis/serving/fieldmask.go#L274-L276

error: services.serving.knative.dev "hello" could not be patched: admission webhook "validation.webhook.serving.knative.dev" denied the request: validation failed: must not set the field(s): spec.template.spec.containers[0].startupProbe

The flag should be a quick addition once enabled on the Serving side.

rhuss · 2022-07-01T14:23:46Z

pkg/kn/flags/podspec_helper.go

+//	- <probe_opts>;InitialDelaySeconds=<int_value>,FailureThreshold=<int_value>,
+//  	SuccessThreshold=<int_value>,PeriodSeconds==<int_value>,TimeoutSeconds=<int_value>
+func resolveProbe(probeString string) (*corev1.Probe, error) {
+	parts := strings.Split(probeString, ";")


Do you think that a value can contain also a ; ? Like as part of an argument for an exec probe ? I think this is quite unlikely, but not sure whether we should allows for escaping characters here. Maybe just continue like here and wait until the first bug reports occurs :-)

There's a bit of trimming done later in the other two resolve* functions. That space wouldn't make it too far. :)

Ok, let's keep it for now like it is.

pkg/kn/flags/podspec_helper.go

rhuss · 2022-07-01T14:25:51Z

pkg/kn/flags/podspec_helper.go

+		handler := corev1.ProbeHandler{
+			Exec: &corev1.ExecAction{},
+		}
+		cmd := strings.Split(probeParts[1], ",")


Should we allow for escaping , ?

A corner case of "that should be fine..." statement? :)

rhuss · 2022-07-12T08:53:09Z

Would it be a better idea to have separate flag(s) for probe options? The probe string seems a bit long and detailed to me and it also doesn't provide a way to update only the options

That's a good idea, I like it. The other possible improvement could be opinionated probe defaults wrt timeouts, thresholds, etc.

@rhuss what would be your preference?

Yes, that's sounds good to me, too. Maybe in a first step we can just omit the options in this PR (just using defaults), and then deliver the options as separate argument in a patch (or regular) release ?

dsimansk · 2022-07-12T09:08:55Z

Would it be a better idea to have separate flag(s) for probe options? The probe string seems a bit long and detailed to me and it also doesn't provide a way to update only the options

That's a good idea, I like it. The other possible improvement could be opinionated probe defaults wrt timeouts, thresholds, etc.
@rhuss what would be your preference?

Yes, that's sounds good to me, too. Maybe in a first step we can just omit the options in this PR (just using defaults), and then deliver the options as separate argument in a patch (or regular) release ?

I'll add a new flag for the options. Since the backbone impl for parsing it is already in place, it should be fairly quick.

How do you like the name for it? --probe-*-opts or --probe*-options?

--probe-liveness-opts

--probe-readiness-options

Just a note we will have 4 probe related flags:

--probe-liveness
--probe-liveness-opts
--probe-readiness
--probe-readiness-opts

rhuss · 2022-07-12T09:40:18Z

--probe-liveness
--probe-liveness-opts
--probe-readiness
--probe-readiness-opts

Sounds good to me, let's keep it as short as possible (it's already quite a mouthful)

Co-authored-by: Gunjan Vyas <vyasgun20@gmail.com>

dsimansk · 2022-07-12T12:41:47Z

/retest

rhuss · 2022-07-12T12:51:41Z

docs/cmd/kn_container_add.md

+      --mount stringArray             Mount a ConfigMap (prefix cm: or config-map:), a Secret (prefix secret: or sc:), an EmptyDir (prefix ed: or emptyDir:), a PersistentVolumeClaim (prefix pvc: or persistentVolumeClaim) or an existing Volume (without any prefix) on the specified directory. Example: --mount /mydir=cm:myconfigmap, --mount /mydir=secret:mysecret, --mount /mydir=emptyDir:myvol or --mount /mydir=myvolume. When a configmap or a secret is specified, a corresponding volume is automatically generated. You can specify a volume subpath by following the volume name with slash separated path. Example: --mount /mydir=cm:myconfigmap/subpath/to/be/mounted. You can use this flag multiple times. For unmounting a directory, append "-", e.g. --mount /mydir-, which also removes any auto-generated volume.
+  -p, --port string                   The port where application listens on, in the format 'NAME:PORT', where 'NAME' is optional. Examples: '--port h2c:8080' , '--port 8080'.
+      --probe-liveness string         Add liveness probe to Service deployment. Supported probe types are HTTGet, Exec and TCPSocket. Format: [http,https]:host:port:path, exec:cmd[,cmd,...], tcp:host:port.
+      --probe-liveness-opts string    Add common options to liveness probe. Common opts (comma separated, case insensitive): InitialDelaySeconds=<int_value>, FailureThreshold=<int_value>, SuccessThreshold=<int_value>, PeriodSeconds=<int_value>, TimeoutSeconds=<int_value>


These are "common opts" or all "possible values" ?

Yes, they are.

dsimansk · 2022-07-12T13:37:07Z

/retest

dsimansk · 2022-07-12T14:15:31Z

/retest

vyasgun · 2022-07-12T17:12:05Z

Thanks :) Adding hold for other reviewers

/hold

vyasgun · 2022-07-12T17:13:52Z

/lgtm

dsimansk · 2022-07-12T17:27:07Z

/unhold

dsimansk · 2022-07-13T11:24:34Z

/kind feature

knative-prow bot requested review from rhuss and vyasgun June 30, 2022 12:00

knative-prow bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 30, 2022

knative-prow bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 30, 2022

dsimansk force-pushed the pr/probes branch from dab46cb to 87fdacc Compare June 30, 2022 14:09

vyasgun reviewed Jul 1, 2022

View reviewed changes

pkg/kn/flags/podspec.go Outdated Show resolved Hide resolved

rhuss reviewed Jul 1, 2022

View reviewed changes

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 12, 2022

dsimansk and others added 9 commits July 12, 2022 13:55

Add --probe-liveness and --probe-readiness flags

c597e4e

Add more unit test

60e2019

Add even more unit tests

2a16776

Fix spelling

f61ddbe

Add error coverage

acab712

Update pkg/kn/flags/podspec.go

b656721

Co-authored-by: Gunjan Vyas <vyasgun20@gmail.com>

Regen docs

15c59d1

Fix error message

f50f268

Split probe opts into standalone flag

cc3944b

dsimansk force-pushed the pr/probes branch from f418de3 to cc3944b Compare July 12, 2022 11:58

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 12, 2022

Fix docs

65de98a

rhuss reviewed Jul 12, 2022

View reviewed changes

knative-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 12, 2022

knative-prow bot assigned vyasgun Jul 12, 2022

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jul 12, 2022

knative-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 12, 2022

knative-prow bot merged commit 9abb876 into knative:main Jul 12, 2022

knative-prow bot added the kind/feature New feature or request label Jul 13, 2022

Add --probe-liveness and --probe-readiness flags #1697

Add --probe-liveness and --probe-readiness flags #1697

Conversation

dsimansk commented Jun 30, 2022 • edited Loading

Description

Changes

Reference

knative-prow bot commented Jun 30, 2022

codecov bot commented Jun 30, 2022 • edited Loading

Codecov Report

vyasgun left a comment

Choose a reason for hiding this comment

dsimansk commented Jul 1, 2022

rhuss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsimansk Jul 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsimansk Jul 1, 2022 • edited Loading

Choose a reason for hiding this comment

dsimansk Jul 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhuss commented Jul 12, 2022

dsimansk commented Jul 12, 2022

rhuss commented Jul 12, 2022

dsimansk commented Jul 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsimansk commented Jul 12, 2022

dsimansk commented Jul 12, 2022

vyasgun commented Jul 12, 2022 • edited Loading

vyasgun commented Jul 12, 2022

dsimansk commented Jul 12, 2022

dsimansk commented Jul 13, 2022

dsimansk commented Jun 30, 2022 •

edited

Loading

codecov bot commented Jun 30, 2022 •

edited

Loading

dsimansk Jul 1, 2022 •

edited

Loading

dsimansk Jul 1, 2022 •

edited

Loading

dsimansk Jul 1, 2022 •

edited

Loading

vyasgun commented Jul 12, 2022 •

edited

Loading