Repository navigation
fix: keep the shadow of a pod recreated under the same name - #584
Merged
dciangot merged 11 commits intoOct 28, 2026
Merged
Conversation
✅ Deploy Preview for interlink-dev canceled.
|
Shadow resources are named after the pod. When a pod is deleted and recreated under the same name (a StatefulSet replica, a Job retry with a fixed name), the new pod's shadow can be created before the old pod's cleanup runs, and that cleanup deleted the new shadow by name: the new pod ran without its tunnel. Every shadow resource is now labelled interlink.eu/shadow-owner-uid with the UID of the pod it serves, and the cleanup leaves alone resources stamped with another UID. Deletes are conditional on the resource's own UID, so a replacement created between the read and the delete survives. Unstamped resources, from earlier versions, are deleted as before. Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Matteo Bunino <[email protected]>
matbun
force-pushed
the
fix/shadow-cleanup-same-name
branch
from
October 23, 2026 09:13
9363810 to
95a2626
Compare
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
In-place ownership updates and unstamped cert-manager TLS Secrets still allow stale cleanup to remove resources serving a recreated pod.
2 open issues
What changed in this PR
This PR adds pod ownership tracking to protect shadow networking resources when a pod is recreated under the same name.
Changes:
- Carries the pod UID through shadow identities and labels applied resources.
- Checks ownership before cleanup and adds resource UID delete preconditions.
- Adds tests for recreated pods, owner cleanup, legacy resources, and node ConfigMap labeling.
| File | Description |
|---|---|
pkg/virtualkubelet/virtualkubelet.go |
Labels shadow resources and adds ownership-aware cleanup. |
pkg/virtualkubelet/virtualkubelet_mesh_test.go |
Updates cleanup tests to pass shadow identities. |
pkg/virtualkubelet/shadow.go |
Propagates pod ownership into identities, node ConfigMaps, and cleanup. |
pkg/virtualkubelet/shadow_test.go |
Tests ownership protection and backward-compatible cleanup. |
Review effort: Balanced
Two cases where the old pod's cleanup could still remove what the recreated pod's shadow uses: - cert-manager creates the <name>-tls Secret without the owner label, so it counted as a legacy resource and was deleted even when the Ingress it serves belongs to the new pod. The Secret now follows its Ingress: it is kept when the Ingress is kept. - The applyOrUpdate helpers update a resource in place for a recreated pod, keeping its UID. A delete conditional on the UID alone still succeeded if the update landed between the read and the delete; the delete now also requires the resourceVersion that was read. Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Matteo Bunino <[email protected]>
With the resourceVersion precondition, any update between the read and the delete made the delete fail, including the Deployment status updates the controller keeps writing, so the old pod's shadow could be left behind. A conflict now re-reads the resource and checks its owner again: it is deleted if it is still the pod's own, and kept if it now serves the recreated pod. A conflict counts as kept for the TLS Secret too, which was deleted when the Ingress changed between the read and the delete. Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Matteo Bunino <[email protected]>
…same-name Signed-off-by: Matteo Bunino <[email protected]> # Conflicts: # pkg/virtualkubelet/virtualkubelet.go
Collaborator
Author
|
@dciangot merged main in and CI is green. The TLS secret case you flagged is fixed and tested on a cluster with cert-manager, details in the thread. Ready for another look |
The cleanup of a deleted pod could delete the old Ingress, let the pod recreated under the same name create its new one, and then delete the TLS Secret that new Ingress uses: the Secret carries no owner label and creating the Ingress does not change it. Applying a shadow's manifests and cleaning them up now hold a lock per shadow name, one of a fixed set of 64, so the two never interleave in the virtual kubelet. The wait for the shadow pod stays outside the lock. An error other than NotFound while reading or deleting the Ingress now counts as kept too, so the TLS Secret is not deleted when its Ingress's owner is unknown. Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Matteo Bunino <[email protected]>
Two paths still touched the successor's node ConfigMap by name: - resetShadowNodeConfigMap runs before the shadow's manifests are applied, outside the shadow's lock. When the old pod's cleanup deleted the ConfigMap between its Create (AlreadyExists) and its Update, the Update failed with NotFound and the new pod's creation failed, permanently for a restartPolicy Never pod. It now creates the ConfigMap again. - A deleted pod stays tracked while its remote deletion runs, after its published node is forgotten, so the status loop could write the old job's node and socket directory into the successor's ConfigMap and point its tunnel at the old job. publishShadowNode now reads the ConfigMap, skips one stamped with another pod's UID, and patches with the resourceVersion it read. Tests also cover that applying a shadow stamps every object with the pod's UID and relabels an object left by the previous pod. Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Matteo Bunino <[email protected]>
…same-name Signed-off-by: Matteo Bunino <[email protected]> # Conflicts: # pkg/virtualkubelet/virtualkubelet.go
…us pod's When a pod recreated under the same name takes the shadow Deployment over in place, the Deployment keeps its creation time, so the previous pod's shadow pod is not older than it. waitForDeploymentPod could return it, giving the new pod the old pod's IP, or a pod about to be replaced, and currentShadowPod could take the previous pod's completed kept init containers for the new pod's, submitting it before its own had run. Both now skip shadow pods labelled interlink.eu/shadow-of for another pod. A resource stamped with another pod's UID was kept for good, so the leftovers of a pod whose cleanup never ran (deleted while the virtual kubelet was down) survived its successor's cleanup too. It is now kept only while that UID is the pod that has the name; when the pod cannot be read it is kept. Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Matteo Bunino <[email protected]>
ownerIsCurrentPod looked the owner up under the name of the pod being cleaned up. Two live pods can share a shadow name, since sanitising maps web.0 and web-0 (or namespaces a--b and a-b) to the same one, so deleting one of them deleted the shadow the other had taken over. Shadow resources now also carry the owner pod's namespace/name in the annotation interlink.eu/shadow-owner-pod, and the owner is looked up by that name. The cleanup tests now give the pods names and create the successor pod, so they take the path production takes instead of the no-name shortcut, and a test covers a pod that no longer exists. isShadowPodOf no longer sits between indexShadowPods and its comment. Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Matteo Bunino <[email protected]>
…tamps The node ConfigMap test checks that interlink.eu/shadow-owner-pod is written. Two cleanup cases are new: the owner lives in another namespace that maps to the same shadow names, and a resource stamped before the annotation existed is looked up under the cleaning pod's name. Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Matteo Bunino <[email protected]>
A third "metadata" string literal in the package, as the reverse tunnel adds one, makes goconst fail; the patch is now a typed struct, with the same JSON. Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Matteo Bunino <[email protected]>
matbun
added a commit
to matbun/interLink
that referenced
this pull request
Oct 23, 2026
…terlink-hq#577 The branch carried early copies of interlink-hq#580, interlink-hq#583, interlink-hq#584 and interlink-hq#585 folded into one commit. It now holds main as of interlink-hq#571 (interlink-hq#580, interlink-hq#581, interlink-hq#582, interlink-hq#583, interlink-hq#585 as merged), interlink-hq#584 as reviewed and the rebased reverse tunnel, with the demo's own commits re-applied: TenantNamespace, RefuseForeignPods, kept init containers on confined nodes and their logs. Fixes found while porting: - interlink-hq#571 refreshed the pod IP through computeShadowResourceIdentity, which ignores TenantNamespace; on a confined node it looked for the shadow in <ns>-shadow and never moved the IP. It uses the provider's identity now. - The completed kept-init wait saves the init containers' logs before it takes the wait over, which cancels its context. - RefuseForeignPods handles the errors of SetWatchErrorHandler and AddEventHandler (errcheck). - The node ConfigMap patch is a struct, so interlink-hq#584 and interlink-hq#577 together do not trip goconst on "metadata". Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Matteo Bunino <[email protected]>
Member
|
@matbun is this ready for review/merge? |
Collaborator
Author
yes |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Shadow resources are named after the pod, so a pod deleted and recreated under the same name (a StatefulSet replica, a retried Job pod) gets a shadow with the same names. When the new shadow is created before the old pod's cleanup runs, that cleanup deletes it by name and the new pod runs without its tunnel.
Every shadow resource now carries the label
interlink.eu/shadow-owner-uidwith the UID of the pod it serves, andcleanupShadowResourcesskips resources stamped with another UID. Each delete is conditional on the resource's own UID, so a replacement created between the read and the delete survives. Unstamped resources, from earlier versions, are deleted as before.Unit tests cover the three cases (old pod's cleanup, owner's cleanup, unstamped resource) and the stamp on the node ConfigMap.
Related issue :