Skip to content

host-local: implement CNI v1.1 GC - #1289

Open
ravi-arnan wants to merge 1 commit into
containernetworking:mainfrom
ravi-arnan:feat/host-local-gc
Open

host-local: implement CNI v1.1 GC#1289
ravi-arnan wants to merge 1 commit into
containernetworking:mainfrom
ravi-arnan:feat/host-local-gc

Conversation

@ravi-arnan

Copy link
Copy Markdown

Fixes #1047.

Implements the GC verb for host-local, replacing the /* FIXME GC */ placeholder in main.go. The runtime supplies the complete set of attachments it still considers live via cni.dev/valid-attachments, and every reservation outside that set is released. That reclaims an allocation leaked by a DEL which never arrived, which is the case the issue calls out.

As far as I can tell this is the first GC implementation in this repo, so I have tried to keep the semantics conservative and to write down the reasoning where a later plugin will face the same question.

Three things the implementation has to get right

Each has a test, because each is a way to silently destroy live allocations rather than a way to fail loudly.

The data dir is not only reservations. lock and the per-range last_reserved_ip.<rangeID> markers live alongside them, so a GC that just walked the directory would delete them. isReservation skips both, and lockFileName is now a shared constant so the skip cannot drift from the name NewFileLock actually uses.

Reservations predating ifname hold only a container ID. ReleaseByID already has a fallback for those, and GC has to agree with it. Without that, the first GC after an upgrade would release every pre-upgrade allocation on the host.

An empty valid-attachments list is meaningful. It means every reservation for this network is stale, not that there is nothing to do. Treating absent as no-op would make GC quietly useless in exactly the situation it exists for.

One deliberate non-behaviour

GC does not rewind last_reserved_ip. A reclaimed address therefore comes back into service on wrap-around rather than being the next one handed out. Rewinding would change allocation order for everyone in order to make reclamation look more immediate, which seemed like the wrong trade for a garbage collector. The address is genuinely free either way, and the disk-level test asserts that directly by reserving it again.

I have called this out because it is the one thing where a reasonable reviewer might want the opposite, and it is a one-line change if you do.

Scope

  • GC is a method on *disk.Store rather than an addition to the backend.Store interface, matching how cmdCheck already reaches for the concrete store. Happy to move it onto the interface if you would rather every backend be required to implement it; that just means touching the fake store too. I asked on the issue and went with the smaller change in the meantime.
  • STATUS is left as a separate change, so this one stays reviewable.

Tests

plugins/ipam/host-local/backend/disk/gc_test.go, 7 specs at the store level: valid set honoured, ifname distinguishes two attachments of the same container, empty set releases everything, lock and last_reserved_ip survive, legacy container-ID-only reservations are both kept when live and released when stale, and a reclaimed address is reservable again.

plugins/ipam/host-local/host_local_test.go, 3 specs at the plugin level driving cmdGC through a real config: a leaked attachment is released while a live one is kept, an empty list empties the store, and the store still serves allocations afterwards.

go test ./plugins/ipam/host-local/...
ok  	github.com/containernetworking/plugins/plugins/ipam/host-local
ok  	github.com/containernetworking/plugins/plugins/ipam/host-local/backend/allocator
ok  	github.com/containernetworking/plugins/plugins/ipam/host-local/backend/disk

go build ./... is clean and go vet ./plugins/ipam/host-local/... is clean.

Unrelated pre-existing failure, confirmed by stashing this branch and re-running on a clean main: pkg/utils/sysctl fails without root because it writes under /proc/sys. Not touched by this change.

Adds the GC verb to host-local, replacing the FIXME placeholder in main.
The runtime hands GC the complete set of attachments it still considers
live, and every reservation outside that set is released. That is how an
allocation leaked by a DEL which never arrived gets reclaimed, which is
the case the issue calls out.

Three details the implementation has to get right, each covered by a test:

The lock and the per-range last_reserved_ip markers live in the same
directory as the reservations, so a GC that simply walked the data dir
would delete them.

Reservations written by older versions hold only a container ID with no
ifname. ReleaseByID already falls back to matching those, and GC has to
agree, otherwise the first GC after an upgrade would release every
pre-upgrade allocation.

An empty valid-attachments list means every reservation for this network
is stale. It is not the same as having nothing to do.

GC deliberately does not rewind last_reserved_ip, so a reclaimed address
returns to service on wrap-around rather than being handed out next. That
keeps allocation order unchanged for everyone else.

Status is left as a separate change.

Signed-off-by: Ravi Arnan <raviarnankeren@gmail.com>
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.

Implement CNI v1.1 for the host-local plugin

1 participant