Repository navigation
tests for the same socket in Device Plugin - #139338
kubernetes-prow[bot] merged 1 commit into
Conversation
|
@SergeyKanzhelev: GitHub didn't allow me to request PR reviews from the following users: zxqlxy. Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs. DetailsIn response to this:
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-sigs/prow repository. |
|
This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The DetailsInstructions 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-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: SergeyKanzhelev The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
951f820 to
f5961b8
Compare
|
/test pull-kubernetes-node-kubelet-serial-containerd |
5ed67f3 to
2bb72ee
Compare
|
@SergeyKanzhelev: The following test failed, say
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. DetailsInstructions 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-sigs/prow repository. I understand the commands that are listed here. |
|
/retest-required |
|
Thanks @SergeyKanzhelev for the detailed tests and comments. I have run the tests locally and confirmed it works. The comments are very detailed and noted that the same socket registration is the unsupported case here. We could make it more obvious somewhere if possible. |
|
@ffromani are you OK to merge these code comments? Can you please review/lgtm? |
2bb72ee to
b6b122e
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: SergeyKanzhelev The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
0a4d135 to
84aa688
Compare
84aa688 to
9b01da6
Compare
|
/retest |
|
@SergeyKanzhelev ping on the |
|
/unhold |
Signed-off-by: Sergey Kanzhelev <[email protected]>
9b01da6 to
d0da39c
Compare
|
/retest |
|
@ffromani could you take another look? The author has moved since your last review. |
|
/lgtm |
|
LGTM label has been added. DetailsGit tree hash: f6474cb5eae9f4876802c271dcdd27edec94f754 |
…ubstrate#3486) Follow-up to agent-substrate#3323, as @SergeyKanzhelev suggested there. kubelet expects a device plugin to switch to a new socket when it re-registers. Since 1.37 it rejects a Register for a socket it still has connected, and two overlapping Registers for one socket can wedge the resource as "already connected" until kubelet restarts: kubernetes/kubernetes#139338 - the device plugin listens on a new `ate.dev-kvm-<random>.sock` for each registration, and removes sockets left over from earlier atelets - the device plugin stays up until atelet exits again. agent-substrate#3323 stopped it on SIGTERM to avoid overlapping with the replacement pod, but the DaemonSet only starts the replacement once the old pod is gone, so that just left `ate.dev/kvm` unhealthy for the whole drain <!-- e.g. "AI-assisted, I reviewed and tested it." No AI co-author trailers. --> AI use: I used Claude to draft this and reviewed it. <!-- Breaking change? Describe what users must do on upgrade and ask for the `breaking-change` label. --> - [x] Tests pass - [x] Docs updated --------- Signed-off-by: Benjamin Elder <[email protected]> (cherry picked from commit 38e9ef6) Signed-off-by: Eitan Yarmush <[email protected]>
…ubstrate#3486) Follow-up to agent-substrate#3323, as @SergeyKanzhelev suggested there. kubelet expects a device plugin to switch to a new socket when it re-registers. Since 1.37 it rejects a Register for a socket it still has connected, and two overlapping Registers for one socket can wedge the resource as "already connected" until kubelet restarts: kubernetes/kubernetes#139338 - the device plugin listens on a new `ate.dev-kvm-<random>.sock` for each registration, and removes sockets left over from earlier atelets - the device plugin stays up until atelet exits again. agent-substrate#3323 stopped it on SIGTERM to avoid overlapping with the replacement pod, but the DaemonSet only starts the replacement once the old pod is gone, so that just left `ate.dev/kvm` unhealthy for the whole drain <!-- e.g. "AI-assisted, I reviewed and tested it." No AI co-author trailers. --> AI use: I used Claude to draft this and reviewed it. <!-- Breaking change? Describe what users must do on upgrade and ask for the `breaking-change` label. --> - [x] Tests pass - [x] Docs updated --------- Signed-off-by: Benjamin Elder <[email protected]>
Follow-up to #3323, as @SergeyKanzhelev suggested there. kubelet expects a device plugin to switch to a new socket when it re-registers. Since 1.37 it rejects a Register for a socket it still has connected, and two overlapping Registers for one socket can wedge the resource as "already connected" until kubelet restarts: kubernetes/kubernetes#139338 - the device plugin listens on a new `ate.dev-kvm-<random>.sock` for each registration, and removes sockets left over from earlier atelets - the device plugin stays up until atelet exits again. #3323 stopped it on SIGTERM to avoid overlapping with the replacement pod, but the DaemonSet only starts the replacement once the old pod is gone, so that just left `ate.dev/kvm` unhealthy for the whole drain <!-- e.g. "AI-assisted, I reviewed and tested it." No AI co-author trailers. --> AI use: I used Claude to draft this and reviewed it. <!-- Breaking change? Describe what users must do on upgrade and ask for the `breaking-change` label. --> - [x] Tests pass - [x] Docs updated --------- Signed-off-by: Benjamin Elder <[email protected]> (cherry picked from commit 38e9ef6) Signed-off-by: Eitan Yarmush <[email protected]>
What type of PR is this?
/kind cleanup
/area test
/sig node
/cc @ffromani
/cc @zxqlxy
What this PR does / why we need it:
This is another follow up from #135437 that ensures we are not breaking behavior for same socket take over scenarios. It highlights the race conditions that are possible when socket is being reused. But neither are a new problems.
Special notes for your reviewer:
Some AI was used.
Does this PR introduce a user-facing change?