Set k8s normal default when assuming non aggressive probe is used by nak3 · Pull Request #4864 · knative/serving

nak3 · 2019-07-22T01:54:16Z

Currently we have to set all periodSeconds, timeoutSeconds and
failureThreshold greater than 0, when we would like to use k8s style
readiness probe.

However, it could be assume that the user would like to use non
aggressive readiness probe when setting one of these values.
Hence, this patch sets k8s's normal default values for such case.

/lint

Ref #4780 (comment)

Proposed Changes

Set k8s normal default when assuming non aggressive probe is used

Release Note

NONE

Currently we have to set all periodSeconds, timeoutSeconds and failureThreshold greater than 0, when we would like to use k8s style readiness probe. However, it could be assume that the user would like to use non aggressive readiness probe when setting one of these values. Hence, this patch sets k8s's normal default values for such case.

knative-prow-robot

@nak3: 0 warnings.

Details

In response to this:

Currently we have to set all periodSeconds, timeoutSeconds and
failureThreshold greater than 0, when we would like to use k8s style
readiness probe.

However, it could be assume that the user would like to use non
aggressive readiness probe when setting one of these values.
Hence, this patch sets k8s's normal default values for such case.

/lint

Ref #4780 (comment)

Proposed Changes

Set k8s normal default when assuming non aggressive probe is used

Release Note
NONE

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.

knative-prow-robot · 2019-07-22T01:54:31Z

Hi @nak3. 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.

Details

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.

markusthoemmes

Minor nits.

markusthoemmes · 2019-07-22T07:19:20Z

 			},
 		},
+	}, {
+		name: "defaluting non aggressive probe",


Suggested change

name: "defaluting non aggressive probe",

name: "defaulting non aggressive probe",

(Don't use the commit button, please apply the change manually)

markusthoemmes · 2019-07-22T07:19:56Z

 				rs.PodSpec.Containers[idx].Resources.Limits[corev1.ResourceMemory] = *rsrc
 			}
 		}
+


Maybe fetch a pointer to the probe once to make this a bit more readable.

readinessProbe := &rs.PodSpec.Containers[idx].ReadinessProbe

I think that works, feel free to ignore if it doesn't.

Yes, it worked and it makes more readable! Thank you for the suggestion. Updated.

Ugh, but now you need to derefence on every call. May I instead suggest you obtain a pointer to the container at the start of the loop?

ctr := &rs.PodSpec.Containers[idx]

You can then replace a lot of boilerplate in the whole function and simplify all of the below to ctr.ReadinessProbe... which is simple enough I think. No dereferences should be needed then.

Thank you! Updated. (Sorry, I should have noticed it by myself.)

markusthoemmes

/approve

I was about to propose to vendor https://github.com/kubernetes/kubernetes/blob/1cb3b5807ec37490b4582f22d991c043cc468195/pkg/apis/core/v1/defaults.go#L192-L205 and use that but... oh well, vendoring kubernetes/kubernetes is probably not a good idea for the few LoC we need here.

/assign @dgerd

Leaving the LGTM for Dan.

knative-prow-robot · 2019-07-22T10:23:52Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

Details

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

markusthoemmes · 2019-07-22T10:27:53Z

/ok-to-test

dgerd · 2019-07-23T05:08:29Z

/hold

I don't have any problems with the code, but I would like to have a discussion on this in the API WG meeting on the behavior before merging this in.

I do have some reservation with the general behavior of changing a value causing the default behavior of another value to change. To make matters worse the defaults only change some of the time (i.e. successThreshold does not change the probe behavior, but failureThreshold does.) This can be very difficult to understand for new users, and can have a cascading effect if we were to use this pattern elsewhere.

vagababov

/lgtm

markusthoemmes · 2019-09-23T11:07:34Z

@dgerd Pingeroo!

dgerd · 2019-09-23T18:16:10Z

This has taken a different direction with detailed errors rather than assuming user intent. #5385 adds the error behavior that we want.

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

knative-prow-robot requested review from dgerd and evankanderson July 22, 2019 01:54

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

knative-prow-robot reviewed Jul 22, 2019

View reviewed changes

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 Jul 22, 2019

knative-prow-robot added the area/API API objects and controllers label Jul 22, 2019

nak3 mentioned this pull request Jul 22, 2019

Fix invalid helloworld example #4780

Merged

markusthoemmes reviewed Jul 22, 2019

View reviewed changes

nak3 added 3 commits July 22, 2019 17:13

Fix typo

6520af2

Make test more readable

c07c184

Make loop more readable

d2d4f55

markusthoemmes approved these changes Jul 22, 2019

View reviewed changes

knative-prow-robot assigned dgerd Jul 22, 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 Jul 22, 2019

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

vagababov reviewed Jul 23, 2019

View reviewed changes

knative-prow-robot assigned vagababov Jul 23, 2019

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 23, 2019

dgerd closed this Sep 23, 2019

nak3 deleted the default-non-aggressive-readiness branch October 20, 2019 11:45

	name: "defaluting non aggressive probe",
	name: "defaulting non aggressive probe",

Conversation

nak3 commented Jul 22, 2019

Proposed Changes

Uh oh!

knative-prow-robot left a comment

Choose a reason for hiding this comment

Proposed Changes

Uh oh!

knative-prow-robot commented Jul 22, 2019

Uh oh!

markusthoemmes left a comment

Choose a reason for hiding this comment

Uh oh!

markusthoemmes Jul 22, 2019

Choose a reason for hiding this comment

Uh oh!

markusthoemmes Jul 22, 2019

Choose a reason for hiding this comment

Uh oh!

nak3 Jul 22, 2019

Choose a reason for hiding this comment

Uh oh!

markusthoemmes Jul 22, 2019

Choose a reason for hiding this comment

Uh oh!

nak3 Jul 22, 2019

Choose a reason for hiding this comment

Uh oh!

markusthoemmes left a comment

Choose a reason for hiding this comment

Uh oh!

knative-prow-robot commented Jul 22, 2019

Uh oh!

markusthoemmes commented Jul 22, 2019

Uh oh!

dgerd commented Jul 23, 2019

Uh oh!

vagababov left a comment

Choose a reason for hiding this comment

Uh oh!

markusthoemmes commented Sep 23, 2019

Uh oh!

dgerd commented Sep 23, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants