Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Second attempt: Plumb context to Kubelet CRI calls #113591

Merged
merged 3 commits into from Nov 5, 2022

Conversation

dashpole
Copy link
Contributor

@dashpole dashpole commented Nov 3, 2022

Related to #113566.

Second attempt at #113408, which was reverted in #113548

What type of PR is this?

/kind feature

What this PR does / why we need it:

Plumb context to kubelet CRI calls.

This includes two additional commits on-top-of the original PR.

The "clean up extra timeouts" commit fixes a confusing place where we set the same timeout on a context twice, identified here: #113408 (comment).

The "try fixing incorrectly canceled context" commit ignores the incoming context from the pod worker. I believe this will fix the test failure.

Which issue(s) this PR fixes:

Part of #113414

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

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

- [KEP]: [Kubelet tracing KEP](https://github.com/kubernetes/enhancements/issues/2831)

cc @liggitt @dims @bobbypage @dchen1107 @aojea

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. 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 Nov 3, 2022
@dashpole
Copy link
Contributor Author

dashpole commented Nov 3, 2022

/sig node

@k8s-ci-robot k8s-ci-robot added sig/node Categorizes an issue or PR as relevant to SIG Node. area/kubelet area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Nov 3, 2022
@dims
Copy link
Member

dims commented Nov 3, 2022

/approve
/lgtm

Let's try again! :)

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 3, 2022
@dashpole
Copy link
Contributor Author

dashpole commented Nov 3, 2022

cc @sallyom

pkg/kubelet/kubelet.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 3, 2022
@smarterclayton
Copy link
Contributor

Let's have a chat about this - #107829 dealt with the correct behavior that getting context into the Kubelet so that we could reduce propagation latency of "pod termination requests" (which leads to measurable end user outcomes). There are probably fixes in 107829 that address the issues involved, but I would like to review this before we put it in again.

@smarterclayton
Copy link
Contributor

smarterclayton commented Nov 3, 2022

It looks like this attempt doesn't try to fix all of the context propagation in the kubelet, is that correct? I took a maximalist approach in my change, but it didn't have to be that way.

@dashpole
Copy link
Contributor Author

dashpole commented Nov 3, 2022

It looks like this attempt doesn't try to fix all of the context propagation in the kubelet, is that correct? I took a maximalist approach in my change, but it didn't have to be that way.

My primary goal was to propagate context from SyncPod -> CRI calls for tracing purposes. See #113414. There are definitely plenty of other places in the kubelet where context would be helpful (Device plugins, CNI, probably others). During the process of plumbing SyncPod -> CRI, I had to add context to functions that were used elsewhere, and used existing contexts where possible.

@smarterclayton
Copy link
Contributor

After our discussion we agreed that for sync*Pods having a context.TODO() with a go comment pointing to #113606 will unblock us.

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 3, 2022
@dashpole
Copy link
Contributor Author

dashpole commented Nov 3, 2022

Cluster didn't come up

Master not detected. Is the cluster up?

/retest

@dims
Copy link
Member

dims commented Nov 4, 2022

@dashpole where are you seeing this?

@dashpole
Copy link
Contributor Author

dashpole commented Nov 4, 2022

@aojea
Copy link
Member

aojea commented Nov 4, 2022

/label tide/merge-method-squash

@k8s-ci-robot k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Nov 4, 2022
@dims
Copy link
Member

dims commented Nov 4, 2022

looks like most recent run of pull-kubernetes-e2e-gce-ubuntu-containerd is 💚

@dims
Copy link
Member

dims commented Nov 4, 2022

/triage accepted
/priority important-soon
/milestone v1.26
/assign @liggitt

@k8s-ci-robot k8s-ci-robot added the triage/accepted Indicates an issue or PR is ready to be actively worked on. label Nov 4, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.26 milestone Nov 4, 2022
@k8s-ci-robot k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and 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 Nov 4, 2022
@dashpole
Copy link
Contributor Author

dashpole commented Nov 4, 2022

/assign @mrunalp

@mrunalp
Copy link
Contributor

mrunalp commented Nov 4, 2022

/approve

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 4, 2022
@klueska
Copy link
Contributor

klueska commented Nov 4, 2022

/approve
for changes in staging/src/k8s.io/cri-api/

@dims
Copy link
Member

dims commented Nov 5, 2022

/approve
/lgtm

let's try this again.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 5, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dashpole, dims, klueska, mrunalp

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:

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

@k8s-ci-robot k8s-ci-robot merged commit 64af1ad into kubernetes:master Nov 5, 2022
@sallyom
Copy link
Contributor

sallyom commented Nov 7, 2022

late to the party but yay! 🥳

@dashpole dashpole deleted the second_kube_context branch November 7, 2022 16:58
jaehnri pushed a commit to jaehnri/kubernetes that referenced this pull request Jan 3, 2023
* plumb context from CRI calls through kubelet

* clean up extra timeouts

* try fixing incorrectly cancelled context
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubelet area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note-none Denotes a PR that doesn't merit a release note. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

9 participants