kubelet: Make probe to be on for time drift #123898

pperiyasamy · 2024-03-12T14:32:46Z

The initialDelaySeconds holds readiness probes for specified seconds after container has started. To achieve this, kubelet relies on StartedAt time of container which never change during its lifecycle. But in case of time drift scenario (example: During maintenance, NVRAM is flashed, looks like it resets the BIOS settings and RTC is also affected), The StartedAt time may contain future time which makes probe to be disabled until system reaches StartedAt time. Hence this commit handles this scenario and makes probe to be still initiated for those cases.

What type of PR is this?

/kind bug

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2024-03-12T14:32:56Z

Hi @pperiyasamy. Thanks for your PR.

I'm waiting for a kubernetes 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.

k8s-ci-robot · 2024-03-12T14:33:26Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: pperiyasamy
Once this PR has been reviewed and has the lgtm label, please assign tallclair 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:

pkg/kubelet/prober/OWNERS

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

HirazawaUi · 2024-03-12T15:53:04Z

/release-note-none
/ok-to-test

pperiyasamy · 2024-03-12T17:24:32Z

/retest-required

jiahuif · 2024-03-12T20:08:22Z

/remove-sig api-machinery

k8s-ci-robot · 2024-03-12T20:08:24Z

@jiahuif: Those labels are not set on the issue: sig/api-machinery

In response to this:

/remove-sig api-machinery

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.

sftim · 2024-03-17T15:19:44Z

Does this PR need a changelog entry?

bart0sh · 2024-03-22T22:04:08Z

pkg/kubelet/prober/worker.go

+	// earlier than system current time. But probe must be on for time drift case,
+	// This may happen due to nvram flash which resets system BIOS settings and RTC.
+	startedAtInSecs := time.Since(c.State.Running.StartedAt.Time).Seconds()
+	if startedAtInSecs > 0 && int32(startedAtInSecs) < w.spec.InitialDelaySeconds {


Would it make sense to wait InitialDelaySeconds since doProbe called first time if startedAtInSecs <= 0.

@bart0sh good point, but would that happen (i.e. pod startup followed by immediate time drift event before InitialDelaySeconds) ? In our case, time drift just happened during node maintenance on a existing pod.

My point was that if startedAtInSecs <= 0 probe would run immediately, which is not desirable. It's better to wait for InitialDelaySeconds just to be on the safe side.

ok @bart0sh , added code to wait for InitialDelaySeconds when time drift happens.

bart0sh · 2024-03-22T22:05:09Z

/triage accepted
/priority important-longterm
/cc

haircommander · 2024-03-29T16:15:49Z

out of curiosity: is the the only bug you found in this scenerio? I feel like there would be a lot of little issues with this situation (and I'm not even sure if this is something that the kubelet should support...)

The initialDelaySeconds holds readiness probe for specified seconds after container has started. To achieve this, kubelet relies on StartedAt time of container which never change during its lifetime. But in case of time drift scenario (example: During node maintenance, NVRAM is flashed, looks like it resets BIOS settings and RTC is affected), The StartedAt time may contain future time which makes probe to be disabled until system reaches StartedAt time. Hence this commit handles this scenario and makes probe to be still initiated for those cases. Signed-off-by: Periyasamy Palanisamy <pepalani@redhat.com>

pperiyasamy · 2024-04-02T09:08:13Z

out of curiosity: is the the only bug you found in this scenerio? I feel like there would be a lot of little issues with this situation (and I'm not even sure if this is something that the kubelet should support...)

@haircommander The readiness probe failure happened on networking pod, so it prevented K8s cluster itself not to come up properly. But after recreation of networking pod, cluster came up just fine. @brunogomes011 might have more details about this.

/cc @kyrtapz @jcaamano

k8s-ci-robot · 2024-04-02T09:08:16Z

@pperiyasamy: GitHub didn't allow me to request PR reviews from the following users: jcaamano, kyrtapz.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

out of curiosity: is the the only bug you found in this scenerio? I feel like there would be a lot of little issues with this situation (and I'm not even sure if this is something that the kubelet should support...)

@haircommander The readiness probe failure happened on networking pod, so it prevented K8s cluster itself not to come up properly. But after recreation of networking pod, cluster came up just fine. @brunogomes011 might have more details about this.

/cc @kyrtapz @jcaamano

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.

k8s-ci-robot · 2024-04-02T09:35:10Z

@pperiyasamy: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubernetes-verify	`1604d6a`	link	true	`/test pull-kubernetes-verify`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

bart0sh · 2024-04-03T09:35:43Z

pkg/kubelet/prober/worker.go

+	// once container's elapsed start time reaches +ve value.
+	timeDrift bool
+	// timeDriftStartTimeInSec contains container's elapsed start time at
+	// the time of first occurance of time drift.


Suggested change

// the time of first occurance of time drift.

// the time of first occurrence of time drift.

bart0sh · 2024-04-03T09:43:59Z

@pperiyasamy I agree with @haircommander here. Keeping machine system clock synchronized is a basic administration task. I'm not sure Kubelet should work around incorrectly set system clock.

bart0sh · 2024-04-03T13:52:21Z

What we may want to do is to check this and report(log & send an event), so user would at least be able to see the reason of probe(or anything else) not working as expected.

sftim · 2024-04-03T13:54:41Z

check this

One way to check: make a request to the API server, look at the Date: HTTP header, see if it's within n seconds of the local UTC time.

k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Mar 12, 2024

k8s-ci-robot requested review from dchen1107 and dims March 12, 2024 14:33

k8s-ci-robot added area/kubelet sig/node Categorizes an issue or PR as relevant to SIG Node. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Mar 12, 2024

bart0sh added this to Triage in SIG Node PR Triage Mar 12, 2024

bart0sh reviewed Mar 22, 2024

View reviewed changes

k8s-ci-robot requested a review from bart0sh March 22, 2024 22:05

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. labels Mar 22, 2024

k8s-ci-robot removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Mar 22, 2024

bart0sh moved this from Triage to Needs Reviewer in SIG Node PR Triage Mar 22, 2024

haircommander moved this from Needs Reviewer to Waiting on Author in SIG Node PR Triage Mar 29, 2024

pperiyasamy force-pushed the time-drift-readiness branch from e862d3c to 1604d6a Compare April 2, 2024 08:50

bart0sh reviewed Apr 3, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubelet: Make probe to be on for time drift #123898

kubelet: Make probe to be on for time drift #123898

pperiyasamy commented Mar 12, 2024 •

edited

k8s-ci-robot commented Mar 12, 2024

k8s-ci-robot commented Mar 12, 2024

HirazawaUi commented Mar 12, 2024

pperiyasamy commented Mar 12, 2024

jiahuif commented Mar 12, 2024

k8s-ci-robot commented Mar 12, 2024

sftim commented Mar 17, 2024

bart0sh Mar 22, 2024

pperiyasamy Mar 25, 2024 •

edited

bart0sh Mar 31, 2024

pperiyasamy Apr 2, 2024

bart0sh commented Mar 22, 2024

haircommander commented Mar 29, 2024

pperiyasamy commented Apr 2, 2024

k8s-ci-robot commented Apr 2, 2024

k8s-ci-robot commented Apr 2, 2024

bart0sh Apr 3, 2024

bart0sh commented Apr 3, 2024 •

edited

bart0sh commented Apr 3, 2024

sftim commented Apr 3, 2024

	// the time of first occurance of time drift.
	// the time of first occurrence of time drift.

kubelet: Make probe to be on for time drift #123898

Are you sure you want to change the base?

kubelet: Make probe to be on for time drift #123898

Conversation

pperiyasamy commented Mar 12, 2024 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Mar 12, 2024

k8s-ci-robot commented Mar 12, 2024

HirazawaUi commented Mar 12, 2024

pperiyasamy commented Mar 12, 2024

jiahuif commented Mar 12, 2024

k8s-ci-robot commented Mar 12, 2024

sftim commented Mar 17, 2024

bart0sh Mar 22, 2024

Choose a reason for hiding this comment

pperiyasamy Mar 25, 2024 • edited

Choose a reason for hiding this comment

bart0sh Mar 31, 2024

Choose a reason for hiding this comment

pperiyasamy Apr 2, 2024

Choose a reason for hiding this comment

bart0sh commented Mar 22, 2024

haircommander commented Mar 29, 2024

pperiyasamy commented Apr 2, 2024

k8s-ci-robot commented Apr 2, 2024

k8s-ci-robot commented Apr 2, 2024

bart0sh Apr 3, 2024

Choose a reason for hiding this comment

bart0sh commented Apr 3, 2024 • edited

bart0sh commented Apr 3, 2024

sftim commented Apr 3, 2024

pperiyasamy commented Mar 12, 2024 •

edited

pperiyasamy Mar 25, 2024 •

edited

bart0sh commented Apr 3, 2024 •

edited