Default timeoutSecond and failureThreshold values when the user opts into K8s probing #5741

dineshba · 2019-10-02T13:22:18Z

Reproduced this issue #5732 using unit tests.

Planning to set default value for timeoutSecond and failureThreshold in this method https://github.com/knative/serving/blob/master/pkg/apis/serving/k8s_validation.go#L443. Correct me if I am wrong.

knative-prow-robot · 2019-10-02T13:22:33Z

Hi @dineshba. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

vagababov · 2019-10-02T15:47:56Z

/ok-to-test

taragu

Thanks for the PR 🙂 I think the default values for timeoutSecond and failureThreshold should be set in pkg/apis/serving/v1/revision_defaults.go

savitaashture · 2019-10-02T16:58:47Z

Thank you @taragu @dineshba
Yes it has to be handled as part of pkg/apis/serving/v1/revision_defaults.go #5732
@dineshba please let me know if you are interested to work on this issue #5732

dineshba · 2019-10-02T17:47:43Z

@savitaashture I am interested to work on this. Shall I add commits to this PR ? OR raise a new PR?

dineshba · 2019-10-02T17:48:22Z

@taragu Thanks for your inputs. Will look into this file pkg/apis/serving/v1/revision_defaults.go

dineshba · 2019-10-02T18:37:19Z

Hi @savitaashture and @taragu Updated this PR with fix. Please have a look at it and provide your inputs. Thanks

savitaashture · 2019-10-02T18:45:25Z

pkg/apis/serving/v1/revision_defaults.go

+		probe.TimeoutSeconds = 10
+	}
+}
+


I would have added changes to existing SetDefaults function same as SuccessThreshold

@savitaashture Didn't notice that function. Will try to make changes as per your suggestion and amend to the PR. Thanks

savitaashture · 2019-10-02T18:51:35Z

pkg/apis/serving/k8s_validation.go

@@ -471,6 +472,7 @@ func validateReadinessProbe(p *corev1.Probe) *apis.FieldError {
 			})
 		}
 	} else {
+		v1.SetDefaults(p)


If you add changes in pkg/apis/serving/v1/revision_defaults.go setting default values will be taken care and you no need to handle it here again

… probe

pkg/apis/serving/v1/revision_defaults.go

pkg/apis/serving/k8s_validation_test.go

This reverts commit f5d7800.

savitaashture · 2019-10-04T04:59:10Z

@dineshba
Please add few more test case scenario

PeriodSeconds = 0 and set some values to FailureThreshold , TimeoutSeconds validate output
Don't set any values for any of the attribute and validate output

dineshba · 2019-10-04T15:49:37Z

@savitaashture Updated the PR with missing tests

dineshba · 2019-10-15T14:00:32Z

@savitaashture @taragu Could you please review this PR?

vagababov

I think @dgerd comented on the issue that is like this by purpose.

vagababov · 2019-10-15T17:08:30Z

pkg/apis/serving/v1/revision_defaults.go

@@ -93,7 +93,15 @@ func (rs *RevisionSpec) SetDefaults(ctx context.Context) {
 		if rs.PodSpec.Containers[idx].ReadinessProbe.SuccessThreshold == 0 {
 			rs.PodSpec.Containers[idx].ReadinessProbe.SuccessThreshold = 1
 		}
+		if rs.PodSpec.Containers[idx].ReadinessProbe.PeriodSeconds != 0 {
+			if rs.PodSpec.Containers[idx].ReadinessProbe.FailureThreshold == 0 {
+				rs.PodSpec.Containers[idx].ReadinessProbe.FailureThreshold = 3


Where are those defaults coming from?

@vagababov https://kubernetes.io/docs/tasks/configure-pod-container/configure-liveness-readiness-startup-probes/#configure-probes

timeoutSeconds: Number of seconds after which the probe times out. Defaults to 1 second. Minimum value is 1.
failureThreshold: When a Pod starts and the probe fails, Kubernetes will try failureThreshold times before giving up. Giving up in case of liveness probe means restarting the container. In case of readiness probe the Pod will be marked Unready. Defaults to 3. Minimum value is 1.

Should they be named constant with a comment where you got them from?

dgerd · 2019-10-15T17:22:40Z

I added a comment on the original issue. We deviated from K8s defaults on purpose here to enable faster probes and avoid setting default based on values of other fields within the same struct.

markusthoemmes · 2019-10-28T09:03:38Z

/hold

To solve the discussion in the issue first.

markusthoemmes · 2019-10-28T09:04:04Z

/assign @dgerd

markusthoemmes · 2019-11-28T08:51:45Z

Hanging for a month, any new status on this?

markusthoemmes · 2020-01-14T07:08:32Z

@dineshba any news?

dineshba · 2020-01-23T13:27:11Z

Its on hold. We have to finalize on this comment

ahmetb · 2020-04-15T19:42:24Z

It would be great to unblock this, as #5798 was closed in favor of this.

vagababov · 2020-05-06T19:06:48Z

/assign @dprotaso @mattmoor
@dgerd is not longer with the project, so Dan and Matt, can yo chime in here so we can finish this?
Thanks.

dprotaso · 2020-05-12T14:48:43Z

Sorry for the delay - this PR looks good to me since we're applying the k8s defaults only when periodSeconds == 0 - which is inline with option #2 here

@dineshba looks like you need to rebase

dprotaso · 2020-05-12T14:50:57Z

/ok-to-test

…obe_parameters

knative-prow-robot · 2020-05-17T13:41:05Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dineshba
To complete the pull request process, please assign dprotaso
You can assign the PR to them by writing /assign @dprotaso in a comment when ready.

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

Needs approval from an approver in each of these files:

pkg/apis/OWNERS

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

dineshba · 2020-05-17T13:42:35Z

@dprotaso Thanks for the follow up. Update the branch with latest master branch. Please review it now

knative-test-reporter-robot · 2020-05-17T13:47:29Z

The following jobs failed:

Test name	Triggers	Retries
pull-knative-serving-unit-tests		0/3

Failed non-flaky tests preventing automatic retry of pull-knative-serving-unit-tests:

cmd/activator.[build failed]
cmd/autoscaler.[build failed]
cmd/queue.[build failed]
pkg/activator/handler.[build failed]
pkg/activator/net.[build failed]
pkg/activator/util.[build failed]
pkg/apis/serving/v1.[build failed]
pkg/apis/serving/v1alpha1.[build failed]

and 30 more.

knative-prow-robot · 2020-05-17T13:58:05Z

@dineshba: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-knative-serving-unit-tests	`ab8679a`	link	`/test pull-knative-serving-unit-tests`
pull-knative-serving-integration-tests	`ab8679a`	link	`/test pull-knative-serving-integration-tests`
pull-knative-serving-build-tests	`ab8679a`	link	`/test pull-knative-serving-build-tests`
pull-knative-serving-autotls-tests	`ab8679a`	link	`/test pull-knative-serving-autotls-tests`
pull-knative-serving-upgrade-tests	`ab8679a`	link	`/test pull-knative-serving-upgrade-tests`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

dprotaso · 2020-05-17T17:47:26Z

pkg/apis/serving/v1/revision_defaults.go

+	if rs.PodSpec.Containers[idx].ReadinessProbe.SuccessThreshold == 0 {
+		rs.PodSpec.Containers[idx].ReadinessProbe.SuccessThreshold = 1
+	}
+	if rs.PodSpec.Containers[idx].ReadinessProbe.PeriodSeconds != 0 {
+		if rs.PodSpec.Containers[idx].ReadinessProbe.FailureThreshold == 0 {
+			rs.PodSpec.Containers[idx].ReadinessProbe.FailureThreshold = 3
+		}

+		if rs.PodSpec.Containers[idx].ReadinessProbe.TimeoutSeconds == 0 {
+			rs.PodSpec.Containers[idx].ReadinessProbe.TimeoutSeconds = 1
+		}
+	}


This block of code should move to the applyProbes method now

markusthoemmes · 2020-07-13T08:16:34Z

@dineshba any news on this one? Are you still pursuing this?

markusthoemmes · 2020-08-11T06:40:59Z

~30 days since my last inquiry. I'll close this for now. @dineshba please feel free to reopen if you're still pursuing this and want to complete this. Thanks!

Added tests for missing timeoutSecond and failureThreshold

f5d7800

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Oct 2, 2019

knative-prow-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 2, 2019

knative-prow-robot requested review from mattmoor and vaikas October 2, 2019 13:22

knative-prow-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 2, 2019

knative-prow-robot added the area/API API objects and controllers label Oct 2, 2019

knative-prow-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 2, 2019

taragu reviewed Oct 2, 2019

View reviewed changes

savitaashture reviewed Oct 2, 2019

View reviewed changes

Set default value for timeoutSecond and failureThreshold of readiness…

656383d

… probe

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 3, 2019

taragu reviewed Oct 3, 2019

View reviewed changes

pkg/apis/serving/v1/revision_defaults.go Outdated Show resolved Hide resolved

pkg/apis/serving/k8s_validation_test.go Outdated Show resolved Hide resolved

Revert "Added tests for missing timeoutSecond and failureThreshold"

dec30ec

This reverts commit f5d7800.

knative-prow-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 4, 2019

Add tests with PeriodSeconds equal to zero

55c2567

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 4, 2019

Set default value for TimeoutSeconds to be 1

3a43799

savitaashture mentioned this pull request Oct 12, 2019

set default readiness timeoutSeconds and failureThreshold #5798

Closed

vagababov reviewed Oct 15, 2019

View reviewed changes

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

knative-prow-robot assigned dgerd Oct 28, 2019

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

mattmoor assigned dprotaso and unassigned dgerd May 6, 2020

dprotaso changed the title ~~Added tests for missing timeoutSecond and failureThreshold~~ Default timeoutSecond and failureThreshold values when the user opts into K8s probing May 12, 2020

Merge branch 'master' of github.com:knative/serving into readiness_pr…

ab8679a

…obe_parameters

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

dprotaso reviewed May 17, 2020

View reviewed changes

markusthoemmes closed this Aug 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Default timeoutSecond and failureThreshold values when the user opts into K8s probing #5741

Default timeoutSecond and failureThreshold values when the user opts into K8s probing #5741

dineshba commented Oct 2, 2019

knative-prow-robot commented Oct 2, 2019

vagababov commented Oct 2, 2019

taragu left a comment

savitaashture commented Oct 2, 2019 •

edited

Loading

dineshba commented Oct 2, 2019

dineshba commented Oct 2, 2019

dineshba commented Oct 2, 2019

savitaashture Oct 2, 2019 •

edited

Loading

dineshba Oct 2, 2019

savitaashture Oct 2, 2019 •

edited

Loading

savitaashture commented Oct 4, 2019

dineshba commented Oct 4, 2019

dineshba commented Oct 15, 2019

vagababov left a comment

vagababov Oct 15, 2019

pacoxu Oct 17, 2019

vagababov May 12, 2020

dgerd commented Oct 15, 2019

markusthoemmes commented Oct 28, 2019

markusthoemmes commented Oct 28, 2019

markusthoemmes commented Nov 28, 2019

markusthoemmes commented Jan 14, 2020

dineshba commented Jan 23, 2020

ahmetb commented Apr 15, 2020

vagababov commented May 6, 2020

dprotaso commented May 12, 2020

dprotaso commented May 12, 2020

knative-prow-robot commented May 17, 2020

dineshba commented May 17, 2020

knative-test-reporter-robot commented May 17, 2020

knative-prow-robot commented May 17, 2020

dprotaso May 17, 2020 •

edited

Loading

markusthoemmes commented Jul 13, 2020

markusthoemmes commented Aug 11, 2020

Default timeoutSecond and failureThreshold values when the user opts into K8s probing #5741

Default timeoutSecond and failureThreshold values when the user opts into K8s probing #5741

Conversation

dineshba commented Oct 2, 2019

knative-prow-robot commented Oct 2, 2019

vagababov commented Oct 2, 2019

taragu left a comment

Choose a reason for hiding this comment

savitaashture commented Oct 2, 2019 • edited Loading

dineshba commented Oct 2, 2019

dineshba commented Oct 2, 2019

dineshba commented Oct 2, 2019

savitaashture Oct 2, 2019 • edited Loading

Choose a reason for hiding this comment

dineshba Oct 2, 2019

Choose a reason for hiding this comment

savitaashture Oct 2, 2019 • edited Loading

Choose a reason for hiding this comment

savitaashture commented Oct 4, 2019

dineshba commented Oct 4, 2019

dineshba commented Oct 15, 2019

vagababov left a comment

Choose a reason for hiding this comment

vagababov Oct 15, 2019

Choose a reason for hiding this comment

pacoxu Oct 17, 2019

Choose a reason for hiding this comment

vagababov May 12, 2020

Choose a reason for hiding this comment

dgerd commented Oct 15, 2019

markusthoemmes commented Oct 28, 2019

markusthoemmes commented Oct 28, 2019

markusthoemmes commented Nov 28, 2019

markusthoemmes commented Jan 14, 2020

dineshba commented Jan 23, 2020

ahmetb commented Apr 15, 2020

vagababov commented May 6, 2020

dprotaso commented May 12, 2020

dprotaso commented May 12, 2020

knative-prow-robot commented May 17, 2020

dineshba commented May 17, 2020

knative-test-reporter-robot commented May 17, 2020

knative-prow-robot commented May 17, 2020

dprotaso May 17, 2020 • edited Loading

Choose a reason for hiding this comment

markusthoemmes commented Jul 13, 2020

markusthoemmes commented Aug 11, 2020

savitaashture commented Oct 2, 2019 •

edited

Loading

savitaashture Oct 2, 2019 •

edited

Loading

savitaashture Oct 2, 2019 •

edited

Loading

dprotaso May 17, 2020 •

edited

Loading