Skip to content

fix: Reclaim stale taps whose host-only stamp names no owner - #1334

Merged
privateip merged 3 commits into
mainfrom
fix/issue-1330
Nov 3, 2026
Merged

privateip merged 3 commits into
mainfrom
fix/issue-1330

Conversation

@privateip

Copy link
Copy Markdown
Collaborator

Summary

Tenant taps stamped before host-only stamps recorded an owner still leak, because the stale tap reclaimer from #1331 only accepts a stamp that names an owner.

Those taps were kept by their own DEL for the same reason, so nothing removes them, and each one keeps a route and an unattributed traffic policy row.

The reclaimer now also accepts a host-only stamp with no owner. Every other guard stays in place, so a tap a VMM holds, or one a VPCAttachment or advertisement still names, is kept.

Test plan

  • A detached tap with an owner-less host-only stamp is removed on the second pass, with its route and eBPF rows, on a real kernel
  • The same tap is kept while a VMM holds it, or a VPCAttachment or advertisement names it
  • After deploy, the leftover taps on the affected staging node are reclaimed and the unattributed policy counter there goes flat

Fixes #1330

A host-only tap stamped before d6a7731 carries an alias that records its ports and no owner. Its own DEL kept such a tap, since the stamp names no owner to match, and the stale tap reclaimer added in #1331 skipped it for the same reason. So the taps left behind by that DEL were never removed.

The reclaimer now takes a host-only stamp with no owner as an ADD's stamp, and removes the tap as the empty owner, which tap.Delete already accepts for such an alias. Every other guard is unchanged: no attached or disabled queues, no BGPAdvertisement, no identified VPCAttachment, two confirming passes and the re-check under the VRF lock. An empty alias and a malformed host-only stamp are still kept.

Unit tests cover removal of an owner-less host-only tap on the second pass, and keeping one that a VMM holds, a VPCAttachment carries or an advertisement names. The root test adds such a tap and shows the kernel removes it alongside the owned stale tap.

Fixes #1330
@privateip privateip self-assigned this Nov 3, 2026
@privateip
privateip requested a review from a team as a code owner November 3, 2026 16:03
Review asked that the reclaimer's comments describe the stamps it accepts rather than how owner-less host-only stamps came about, which the previous commit message records. The test comments and the CNI architecture bullet now name the stamp shape, a host-only stamp with no owner.
The stampedByADD docstring now reads as a sentence, the root test comment no longer leaves an orphaned line, and the GC sequence doc states that a host-only stamp counts whether or not it names an owner, without the history the earlier wording carried.
@privateip
privateip enabled auto-merge November 3, 2026 16:27
@privateip

Copy link
Copy Markdown
Collaborator Author

Two independent reviews ran on this PR, plus one second pass after the review fixes. All returned merge.

  • Correctness review: found no defect. It checked that removal with the empty owner only reaches taps whose owner-less stamp is unchanged under the VRF lock, and that a malformed stamp, a held tap, a live VPCAttachment and an advertisement each still keep the tap. It ran the stale tap unit tests and vet. It could not run the privileged kernel test, which the Unit Tests (root) job has since passed. The same test also passed locally as root against a real kernel.
  • Conventions review: asked for comments to state the stamp contract instead of its history, plus wording nits. These are fixed in the two follow-up commits, and a second pass confirmed them.

CI is green on the head. Auto-merge is on with the merge method, behind the required code owner approval.

@ecv ecv left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approve.

Widening the candidate test to a well-formed host-only stamp with no owner leaves every keep guard intact, and none is skipped for the owner-less case.

  • Guards are shared: detachedTenantTap (no fd on any queue), the advertisement prefix check, the VPCAttachment identity match and the two-pass confirmation all run on the same path for owner-less taps. The new keep-cases (VMM-held, live VPCAttachment, advertisement-named, malformed stamp) cover each.
  • Mid-ADD: tap.Add runs inside vrf.EnsureAndEnslave, so the tap's brief unstamped moment is under the same lock, and an empty alias is never a candidate. The removal re-reads the link under the VRF lock and compares the full candidate (ifindex and alias); an adopting ADD rewrites the alias, so again != t keeps the tap.
  • Deleting with the empty owner is safe: tap.OwnedBy accepts it only when AliasOwner(alias) is also empty, so a tap a new owner has claimed is left alone.
  • API reads are live, not cached: the client is client.New (no informer cache). Any list error resets the pending set and removes nothing, and an empty set of identified attachments aborts the pass with errNoIdentifiedAttachments. Read failure keeps the tap.
  • Owner-less stamps come from older builds whose ports-only alias recorded no owner, including an older build's ADD after a rollback; production HostOnlyAlias always passes the container ID. A live VM's tap stays safe through the guards above (VMM fd, an identified VPCAttachment, an advertisement), not through anything in the stamp, so only an orphan with none of them is reclaimed.

Follow-ups:

  • A first-time deploy reclaims leftover owner-less taps from old builds on the second 5-minute tick. Check the galactic_cni_stale_taps_reclaimed_total jump in staging before the tag goes out.
  • identifiedAttachments trusts that the node's client sees the same VPCAttachment set as the whole cluster. The non-empty guard only covers the all-missing case, so a partial view would still reclaim. Worth a note in the docs.

@privateip
privateip merged commit e24462d into main Nov 3, 2026
27 checks passed
@privateip
privateip deleted the fix/issue-1330 branch November 3, 2026 16:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Tenant taps outlive their VPCAttachment once its BGPAdvertisement is gone

2 participants