Support deployments/workloads probe overrides #1247

skonto · 2022-10-12T13:27:49Z

Proposed Changes

Customers are facing slow environments and need to adjust timeouts and other settings in probes.
Some settings can be adjusted on the fly via changing the deployments eg. a deployment uses the probe defaults and given that the operator will not touch those (manifest uses apply) it does the trick. However this is a hack and we need a proper way to change probes and cover cases where reconciliation will touch the settings.

Release Note

Support is added for readiness, liveness probe overrides.

knative-prow

@skonto: 0 warnings.

In response to this:

Proposed Changes

Customers are facing slow environments and need to adjust timeouts and other settings in probes.

Some settings can be adjusted on the fly via changing the deployments eg. a deployment uses the probe defaults and given that the operator will not touch those (manifest uses apply) it does the trick. However this is a hack and we need a proper way to change probes and cover cases where reconciliation will touch the settings.

Release Note
Support is added for readiness, liveness probe overrides.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

codecov · 2022-10-12T13:32:11Z

Codecov Report

Base: 79.40% // Head: 79.61% // Increases project coverage by +0.21% 🎉

Coverage data is based on head (cee945f) compared to base (59e506e).
Patch coverage: 86.27% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1247      +/-   ##
==========================================
+ Coverage   79.40%   79.61%   +0.21%     
==========================================
  Files          35       36       +1     
  Lines        1602     1653      +51     
==========================================
+ Hits         1272     1316      +44     
- Misses        239      244       +5     
- Partials       91       93       +2

Impacted Files	Coverage Δ
pkg/reconciler/common/probe_override.go	`76.47% <76.47%> (ø)`
pkg/reconciler/common/workload_override.go	`85.82% <91.17%> (+1.95%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

skonto · 2022-10-13T10:31:59Z

gentle ping @houshengbo @nak3

matzew · 2022-10-13T14:21:07Z

pkg/apis/operator/base/common.go

+	// +optional
+	ReadinessProbes []ProbesRequirementsOverride `json:"readinessProbes,omitempty"`
+
+	// LivenessProbes overrides liveness probes for the containers.
+	// +optional
+	LivenessProbes []ProbesRequirementsOverride `json:"livenessProbes,omitempty"`


very valid to have these as overrides too! Good catch!

matzew · 2022-10-13T14:23:39Z

Let's have a knative/docs issue as well, so we describe this valuable feature!

matzew · 2022-10-13T14:23:46Z

LGTM

skonto · 2022-10-14T09:19:27Z

I will create a doc PR for this too once it is approved. @houshengbo gentle ping.

pierDipi · 2022-10-14T09:32:20Z

pkg/apis/operator/base/common.go

+// ProbesRequirementsOverride enables the user to override any container's env vars.
+type ProbesRequirementsOverride struct {
+	// The container name
+	Container string `json:"container"`
+	// The desired ProbeRequirements
+	Probe corev1.Probe `json:"probe,omitempty"`
+}


Just a thought unrelated to this particular change because it's affecting existing fields (env, etc).

I'm wondering if for v1 we should consider something simpler to work with:

Currently, overrides will endup being

deployments: resources: - container: webhook requests: # ... readinessProbes: - container: webhook probe: # ...

which seems repetitive and error prone.

For v1, a more podspec-like structure (with a subset of fields) would be easier to work with.

For v1, a more podspec-like structure (with a subset of fields) would be easier to work with.

yeah, currently is hard...

Yes I would prefer a pod spec per workload to apply as an override.

pierDipi · 2022-10-14T09:34:55Z

pkg/apis/operator/base/common.go

+	// The container name
+	Container string `json:"container"`
+	// The desired ProbeRequirements
+	Probe corev1.Probe `json:"probe,omitempty"`


Should we prevent setting the ProbeHandler field and allow only these fields https://github.com/skonto/operator/blob/23892c03b5a5caeb2e41691a4bff41248903dd73/vendor/k8s.io/api/core/v1/types.go#L2194-L2226.

I didnt want to limit people on how they can use this (they could override the handler with their own custom thing) but we can keep it limited for now.

Let's not assume the options for users. If we plan to add some, what is the use case? Otherwise, limit to what is necessary. Drop ProbeHandler at least.

houshengbo · 2022-10-17T14:21:27Z

Do we really need to all fields under livenessProbes???
If it is just a few fileds, like TimeoutSeconds, InitialDelaySeconds, etc, we do not need to add all the other fields. Just pick the ones related to the *** Seconds, that are necessary.

skonto · 2022-10-17T15:30:29Z

@houshengbo

If it is just a few fileds, like TimeoutSeconds, InitialDelaySeconds, etc, we do not need to add all the other fields. Just pick the ones related to the *** Seconds, that are necessary.

Anything that is timeout, threshold related are useful as a starting point in the field. I would just exclude ProbeHandler as @pierDipi mentioned above. Wdyth?

houshengbo · 2022-10-18T14:38:24Z

Just keep

InitialDelaySeconds int32 `json:"initialDelaySeconds,omitempty" protobuf:"varint,2,opt,name=initialDelaySeconds"`
	
	TimeoutSeconds int32 `json:"timeoutSeconds,omitempty" protobuf:"varint,3,opt,name=timeoutSeconds"`
	
	PeriodSeconds int32 `json:"periodSeconds,omitempty" protobuf:"varint,4,opt,name=periodSeconds"`
	
	SuccessThreshold int32 `json:"successThreshold,omitempty" protobuf:"varint,5,opt,name=successThreshold"`
	
	FailureThreshold int32 `json:"failureThreshold,omitempty" protobuf:"varint,6,opt,name=failureThreshold"`
	
	TerminationGracePeriodSeconds

in the CRDs. Remove the others. @skonto

skonto · 2022-10-18T15:49:26Z

Missed your comment I will add the rest, I used an even smaller subset.

skonto · 2022-10-18T16:04:48Z

@houshengbo kept those six fields. Adapted the PR.

houshengbo · 2022-10-18T20:00:22Z

@skonto Plz do another rebase. I will merge this pr.

skonto · 2022-10-18T22:17:34Z

@houshengbo sorry missed it I was off. Let's see if tests pass.

skonto · 2022-10-18T22:20:39Z

/hold let me check crds first

skonto · 2022-10-18T22:34:50Z

/unhold

matzew

/lgtm
/approve

/hold
feel free to unhold

knative-prow · 2022-10-19T05:47:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: matzew, skonto

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~~ [matzew]

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

skonto · 2022-10-19T07:39:11Z

/unhold

knative-prow bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Oct 12, 2022

knative-prow bot reviewed Oct 12, 2022

View reviewed changes

skonto force-pushed the add_readiness_probe_configuration branch 2 times, most recently from a81bd84 to 23892c0 Compare October 13, 2022 10:09

matzew reviewed Oct 13, 2022

View reviewed changes

matzew mentioned this pull request Oct 13, 2022

DeploymentOverrides allow more overrides than documented knative/docs#5276

Closed

pierDipi reviewed Oct 14, 2022

View reviewed changes

knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 18, 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 Oct 18, 2022

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

skonto added 4 commits October 19, 2022 00:55

support deployment probe overrides

93c02db

limit fields in probe overrides

22b2f1d

adjust fields in probe override

249479b

fix deepcopy

f929955

skonto force-pushed the add_readiness_probe_configuration branch from df48d31 to f929955 Compare October 18, 2022 22:12

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

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

add workload probes to crd

cee945f

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

skonto changed the title ~~Support deployment probe overrides~~ Support deployment/workloads probe overrides Oct 18, 2022

skonto changed the title ~~Support deployment/workloads probe overrides~~ Support deployments/workloads probe overrides Oct 18, 2022

matzew approved these changes Oct 19, 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 Oct 19, 2022

knative-prow bot assigned matzew Oct 19, 2022

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

knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 19, 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 Oct 19, 2022

knative-prow bot merged commit 28c5b30 into knative:main Oct 19, 2022

This was referenced Oct 31, 2022

[SRVCOM-2061] Add deployment probe overrides support openshift-knative/serverless-operator#1780

Closed

[BACKPORT] workloads and probes overrides openshift-knative/serverless-operator#1782

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support deployments/workloads probe overrides #1247

Support deployments/workloads probe overrides #1247

skonto commented Oct 12, 2022

knative-prow bot left a comment

codecov bot commented Oct 12, 2022 •

edited

Loading

skonto commented Oct 13, 2022 •

edited

Loading

matzew Oct 13, 2022

matzew commented Oct 13, 2022

matzew commented Oct 13, 2022

skonto commented Oct 14, 2022

pierDipi Oct 14, 2022

matzew Oct 14, 2022

skonto Oct 17, 2022

pierDipi Oct 14, 2022

skonto Oct 17, 2022 •

edited

Loading

houshengbo Oct 18, 2022

houshengbo Oct 18, 2022

houshengbo commented Oct 17, 2022

skonto commented Oct 17, 2022 •

edited

Loading

houshengbo commented Oct 18, 2022

skonto commented Oct 18, 2022 •

edited

Loading

skonto commented Oct 18, 2022

houshengbo commented Oct 18, 2022

skonto commented Oct 18, 2022

skonto commented Oct 18, 2022

skonto commented Oct 18, 2022

matzew left a comment

knative-prow bot commented Oct 19, 2022

skonto commented Oct 19, 2022

Support deployments/workloads probe overrides #1247

Support deployments/workloads probe overrides #1247

Conversation

skonto commented Oct 12, 2022

Proposed Changes

knative-prow bot left a comment

Choose a reason for hiding this comment

Proposed Changes

codecov bot commented Oct 12, 2022 • edited Loading

Codecov Report

skonto commented Oct 13, 2022 • edited Loading

matzew Oct 13, 2022

Choose a reason for hiding this comment

matzew commented Oct 13, 2022

matzew commented Oct 13, 2022

skonto commented Oct 14, 2022

pierDipi Oct 14, 2022

Choose a reason for hiding this comment

matzew Oct 14, 2022

Choose a reason for hiding this comment

skonto Oct 17, 2022

Choose a reason for hiding this comment

pierDipi Oct 14, 2022

Choose a reason for hiding this comment

skonto Oct 17, 2022 • edited Loading

Choose a reason for hiding this comment

houshengbo Oct 18, 2022

Choose a reason for hiding this comment

houshengbo Oct 18, 2022

Choose a reason for hiding this comment

houshengbo commented Oct 17, 2022

skonto commented Oct 17, 2022 • edited Loading

houshengbo commented Oct 18, 2022

skonto commented Oct 18, 2022 • edited Loading

skonto commented Oct 18, 2022

houshengbo commented Oct 18, 2022

skonto commented Oct 18, 2022

skonto commented Oct 18, 2022

skonto commented Oct 18, 2022

matzew left a comment

Choose a reason for hiding this comment

knative-prow bot commented Oct 19, 2022

skonto commented Oct 19, 2022

codecov bot commented Oct 12, 2022 •

edited

Loading

skonto commented Oct 13, 2022 •

edited

Loading

skonto Oct 17, 2022 •

edited

Loading

skonto commented Oct 17, 2022 •

edited

Loading

skonto commented Oct 18, 2022 •

edited

Loading