Remove incorrect/vague explanation of probe config #48536

qiyueyao · 2024-10-24T20:28:35Z

Description

Remove the incorrect/vague explanation of initialDelaySeconds with periodSeconds relation in configure probes. It misleads understanding ofinitialDelaySeconds and periodSeconds behavior.
Page: Configure Liveness, Readiness, Startup Probes.

Issue

Closes: #48519

Remove the incorrect explanation of initialDelaySeconds with periodSeconds relation in configure probes. Page: Configure Liveness, Readiness, Startup Probes. Signed-off-by: Qiyue Yao <[email protected]>

linux-foundation-easycla · 2024-10-24T20:28:38Z

The committers listed above are authorized under a signed CLA.

✅ login: qiyueyao / name: Qiyue Yao (d875e2e, 78c4a52, 14ab137, 5ad942d)

k8s-ci-robot · 2024-10-24T20:28:43Z

Welcome @qiyueyao!

It looks like this is your first PR to kubernetes/website 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/website has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2024-10-24T20:28:46Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign salaxander for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

content/en/docs/OWNERS

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

netlify · 2024-10-24T20:38:00Z

✅ Pull request preview available for checking

Built without sensitive environment variables

Name	Link
🔨 Latest commit	`14ab137`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/672bfd424d8e190008c614f1
😎 Deploy Preview	https://deploy-preview-48536--kubernetes-io-main-staging.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

tengqm · 2024-10-25T00:32:48Z

This line was introduced in #43289, and the author then said that the behavior was tested. So ... this needs a tech review.

sftim · 2024-10-25T16:24:50Z

/sig node

sftim · 2024-10-27T22:52:03Z

@kubernetes/sig-node-pr-reviews I've not tested this, but I do agree: it will need a tech review from SIG Node before we can merge the change.

qiyueyao · 2024-10-28T22:23:32Z

Thanks for your attention. Using this example to test on K8s v1.27 several times, the same behaviors show that if initialDelaySeconds: 0, first probe is not periodSeconds but immediately after start. From the code, we can also reach this conclusion. (initialDelaySeconds is neither ignored, nor shall assume it will be added to periodSeconds for fist probe.) So current explanation on the website mentioned is misleading, therefore opening this PR. Thanks for taking a look.

tengqm · 2024-10-29T00:11:00Z

Thanks for your attention. Using this example to test on K8s v1.27 several times, the same behaviors show that if initialDelaySeconds: 0, first probe is not periodSeconds but immediately after start. From the code, we can also reach this conclusion. (initialDelaySeconds is neither ignored, nor shall assume it will be added to periodSeconds for fist probe.) So current explanation on the website mentioned is misleading, therefore opening this PR. Thanks for taking a look.

Thanks for the confirmation. Please make sure the revised text clearly and precisely describe the system behavior then. Setting initialDelaySeconds: 0 is different from omitting it.

tengqm · 2024-10-31T02:35:10Z

content/en/docs/tasks/configure-pod-container/configure-liveness-readiness-startup-probes.md

-  readiness probe delays do not begin until the startup probe has succeeded. Defaults to 0
-  seconds. Minimum value is 0.
+  readiness probe delays do not begin until the startup probe has succeeded.
+  `initialDelaySeconds` will block probes, but `periodSeconds` remains active. The probe may


by "active", do you mean "effective"?

Better be explicit when you say "the probe". Which probe are you referring to in this context?

The original wording is much more clear for me. I can't think of a suggestion for rewording this, but I don't think the current phrasing is an improvement.

Maybe something like:

An `initialDelaySeconds` value greater than 0 will delay when the first probe starts. If `periodSeconds` is less than the `initialDelaySeconds` regular periodic probes will begin. For example, if `initialDelaySeconds` is set to 20, but `periodSeconds` is set to 10, periodic probes will still begin after an initial 10 seconds.

Thanks for the suggestions. This is probably still a bit confusing, because I was trying to say if initialDelaySeconds is set to 20, periodSeconds is set to 10, the initial probe is around 30; if initialDelaySeconds is set to 10, periodSeconds is set to 20, the initial probe is around 20.

I have combined your suggestions into a modified explanation, does that sound clearer? Thanks.

tengqm · 2024-11-05T00:01:20Z

content/en/docs/tasks/configure-pod-container/configure-liveness-readiness-startup-probes.md

+  the value of `periodSeconds` is greater than `initialDelaySeconds` then `periodSeconds`
+  intervals remain effective. For example, if `initialDelaySeconds` is set to 20, but
+  `periodSeconds` is set to 10, periodic probes will still begin after an initial 20 seconds.
+  Defaults to 0 seconds. Minimum value is 0.


The logic is much more confusing in the revised text.

If the value of periodSeconds is greater than initialDelaySeconds then periodSeconds intervals remain effective.

This talks about periodSeconds > initialDelaySeconds.

For example, if initialDelaySeconds is set to 20 ...

This talks about initialDelaySeconds > periodSeconds. It is not an example of the
sentence above.

Please clear your mind first and then try articulate what you are trying to clarify here.

Sorry that was a typo, I stick to the discussions above, and have modified the text.

Alright. The revised text makes sense now.
The next question then is what are the value of adding this example.
The existing text already indicated that the delay threshold is ignored when the period is longer than delay, right?

The intention of the PR was to remove the incorrect explanation "If the value of periodSeconds is greater than initialDelaySeconds then the initialDelaySeconds will be ignored", and then advised to precisely describe the actual system behavior, but it doesn't seems easy to come up with a good explanation. If no one knows how to explain it in a better way, should we just remove the wrong explanation, like the first version of the PR does: 78c4a52

When periodSeconds>initialDelaySeconds, the initialDelaySeconds is ignored. Isn't that what you are trying to clarify here? The existing text is clear on that, right?

The point of the PR is that's wrong. I thought this has been clarified in #48536 (comment) and the issue #48519?

qiyueyao · 2024-11-06T23:14:05Z

Trying to rephrase the problem: I intend to fix the explanation that when initialDelaySeconds<periodSeconds, the original text "then the initialDelaySeconds will be ignored" is misleading, it is different from "removing initialDelaySeconds or setting it to 0".
Because if initialDelaySeconds is omitted (defaults to 0), or set to 0, first probe happens immediately. But if 0<initialDelaySeconds<periodSeconds, initialDelaySeconds will be "contained"/"merged" into periodSeconds, first probe happens after interval.

Thanks for the discussions, and sorry for bringing confusion, but I intend to avoid future misunderstanding.
What's confusing: Doc says that initialDelaySeconds is time before probes are initiated, generally one would assume periodSeconds will start after initialDelaySeconds, therefore contradicts the behavior when initialDelaySeconds<periodSeconds, they don't add up.
However, the fact is periodSeconds starts on itself, initialDelaySeconds blocks any probe happening during this time since start, probes appear disabled; after that time probes go through and are effective. This means consistent behavior when initialDelaySeconds < = > periodSeconds.

Thanks @tengqm @stmcginnis @tnqn @Dyanngg

tengqm · 2024-11-07T02:20:35Z

content/en/docs/tasks/configure-pod-container/configure-liveness-readiness-startup-probes.md

+  liveness or readiness probes are effective. If a startup probe is defined, liveness and
+  readiness probe delays do not begin until the startup probe has succeeded. If the value of
+  `periodSeconds` is greater than `initialDelaySeconds` then the `initialDelaySeconds` will
+  be included in the first period interval. Defaults to 0 seconds. Minimum value is 0.


If the value of periodSeconds is greater than initialDelaySeconds then the initialDelaySeconds will be ignored. The delayed probes will start after the end
of the first periodSeconds. However, when initialDelaySeconds is omitted
(defaulted to 0), the liveness, readiness probes are not delayed and they will start
immediately after the container has started.

Is this what you were trying to clarify?

If the value of periodSeconds is greater than initialDelaySeconds then the initialDelaySeconds will be ignored.

I think we are on the same page in terms of the actual behavior here but I would not say it will be "ignored". My understanding is that if 0 < initialDelaySeconds < periodSeconds, then the "initial" probe immediately after container start will be skipped due to initialDelaySeconds. It is not "ignored"; it has a clear effect compared to the field not being set / default to 0. I think what the author was trying to express was, as long as 0 < initialDelaySeconds < periodSeconds, it does not matter what exact value you specify for initialDelaySeconds, the first probe will always happen at the end of first periodSeconds. However it is quite different from "it is ignored"

Yes, I intend to change the wording of "are initiated" and "will be ignored", because they are misleading and do not match the actual behavior, especially for the case initialDelaySeconds=0.
The issue is: based on the Doc, I tried to remove initialDelaySeconds for value less than periodSeconds, because Doc says it will be ignored anyway. Then the probe happened immediately, not after periodSeconds, which is not expected from the Doc.

@qiyueyao I take it as, for people writing pod specs and wonder if they can/should set initialDelaySeconds to be a value less than periodSeconds, we don't want them to read the doc as is currently and think they can just remove the initialDelaySeconds (which will be ignored anyways). As a fact, setting it to a non-zero value will cause the "0th" probe to delay for periodSeconds

Remove incorrect explanation of probe config

78c4a52

Remove the incorrect explanation of initialDelaySeconds with periodSeconds relation in configure probes. Page: Configure Liveness, Readiness, Startup Probes. Signed-off-by: Qiyue Yao <[email protected]>

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Oct 24, 2024

k8s-ci-robot requested review from sftim and windsonsea October 24, 2024 20:28

qiyueyao mentioned this pull request Oct 24, 2024

[zh-cn]Sync configure liveness, readiness, startup probes #48537

Open

k8s-ci-robot added the sig/node Categorizes an issue or PR as relevant to SIG Node. label Oct 25, 2024

T-Lakshmi mentioned this pull request Oct 28, 2024

Wrong explanation of initialDelaySeconds in configuring Probes #48519

Open

Add system behavior desc address comments

5ad942d

tengqm reviewed Oct 31, 2024

View reviewed changes

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 4, 2024

tengqm reviewed Nov 5, 2024

View reviewed changes

Address comments

d875e2e

qiyueyao force-pushed the fix-probe-desc branch from e4e4597 to d875e2e Compare November 5, 2024 21:40

qiyueyao changed the title ~~Remove incorrect explanation of probe config~~ Remove incorrect/vague explanation of probe config Nov 6, 2024

Rephrase to avoid misunderstanding

14ab137

k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 6, 2024

tengqm reviewed Nov 7, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove incorrect/vague explanation of probe config #48536

Remove incorrect/vague explanation of probe config #48536

qiyueyao commented Oct 24, 2024 •

edited

Loading

linux-foundation-easycla bot commented Oct 24, 2024 •

edited

Loading

k8s-ci-robot commented Oct 24, 2024

k8s-ci-robot commented Oct 24, 2024

netlify bot commented Oct 24, 2024 •

edited

Loading

tengqm commented Oct 25, 2024

sftim commented Oct 25, 2024

sftim commented Oct 27, 2024

qiyueyao commented Oct 28, 2024 •

edited

Loading

tengqm commented Oct 29, 2024

tengqm Oct 31, 2024

tengqm Oct 31, 2024

stmcginnis Oct 31, 2024

qiyueyao Nov 1, 2024 •

edited

Loading

qiyueyao Nov 4, 2024

tengqm Nov 5, 2024

qiyueyao Nov 5, 2024

tengqm Nov 6, 2024

tnqn Nov 6, 2024

tengqm Nov 6, 2024

tnqn Nov 6, 2024

qiyueyao commented Nov 6, 2024 •

edited

Loading

tengqm Nov 7, 2024

Dyanngg Nov 7, 2024

qiyueyao Nov 7, 2024

Dyanngg Nov 7, 2024 •

edited

Loading

Remove incorrect/vague explanation of probe config #48536

Are you sure you want to change the base?

Remove incorrect/vague explanation of probe config #48536

Conversation

qiyueyao commented Oct 24, 2024 • edited Loading

Description

Issue

linux-foundation-easycla bot commented Oct 24, 2024 • edited Loading

k8s-ci-robot commented Oct 24, 2024

k8s-ci-robot commented Oct 24, 2024

netlify bot commented Oct 24, 2024 • edited Loading

✅ Pull request preview available for checking

tengqm commented Oct 25, 2024

sftim commented Oct 25, 2024

sftim commented Oct 27, 2024

qiyueyao commented Oct 28, 2024 • edited Loading

tengqm commented Oct 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiyueyao Nov 1, 2024 • 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

Choose a reason for hiding this comment

qiyueyao commented Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dyanngg Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

qiyueyao commented Oct 24, 2024 •

edited

Loading

linux-foundation-easycla bot commented Oct 24, 2024 •

edited

Loading

netlify bot commented Oct 24, 2024 •

edited

Loading

qiyueyao commented Oct 28, 2024 •

edited

Loading

qiyueyao Nov 1, 2024 •

edited

Loading

qiyueyao commented Nov 6, 2024 •

edited

Loading

Dyanngg Nov 7, 2024 •

edited

Loading