The tests start the reconciler with "go reconciler.Run(ctx)" and only
cancel its context on return, so a reconciler can finish one more loop
iteration after its test is done. When that iteration force detaches a
volume, the global force detach metric changes under the next test and
its "unchanged" check fails.
Run the reconciler through a helper that cancels it and waits for Run to
return in t.Cleanup. The metric is recorded on the reconciler goroutine,
so nothing can change it once Run has returned.
The reconciler records the force detach metric after DetachVolume returns,
but the detach itself runs in a goroutine started by the operation
executor. The tests wait for the fake detacher to be called and then read
the counter once, so on a slow or loaded machine they can observe the
detach before the increment.
Poll the counter the same way the other assertions in this file do.
Since commit "ktesting: cancel the TContext as soon as the
test is done", a TContext created by ktesting.Init is automatically
canceled as soon as the test binary considers the test done, before
any t.Cleanup callback runs. Previously, many tests had to explicitly
call tCtx.Cancel(...) at the end of the test (typically via defer) to
stop tCtx-driven background goroutines (controllers, informers,
schedulers) because automatic cancellation only happened during
Cleanup, i.e. too late. That workaround is now redundant and is
removed throughout the codebase.
Three different patterns had to be handled, depending on what the
removed Cancel call was ordering:
- Bare "defer tCtx.Cancel(...)" with nothing else depending on it:
simply deleted. This wasn't necessary in the first place.
- "tCtx.Cancel(...)" followed by dependent code in the same
defer/closure (e.g. wg.Wait(), informerFactory.Shutdown(),
runtime.WaitForShutdown()): kept the dependent code but moved it
(without the Cancel call) into tCtx.Cleanup(...), since Cleanup
callbacks now reliably run only after the automatic cancellation.
- "tCtx.Cancel(...)" was ordering a *separate* deferred teardown
function (e.g. "defer closeFn()" or "defer server.TearDownFn()")
registered *earlier* in the same function. Because Go defers run
LIFO, the old code relied on Cancel (registered later) running
before that teardown (registered earlier), guaranteeing
tCtx-driven goroutines stopped before the apiserver they talk to
was torn down. Simply deleting the Cancel call would break this
ordering: the deferred teardown would now run immediately when the
test function returns, while the goroutine keeps running
unconstrained until the (now later) automatic cancellation during
Cleanup - risking connection errors or a "Log in goroutine after
Test has completed" panic. These sites were fixed by converting the
deferred teardown into a t.Cleanup/tCtx.Cleanup registration.
In some cases the teardown can be removed entirely by directly
registering the corresponding operation inside the setup function.
Left untouched: Cancel calls used for legitimate mid-test, explicit
timing (e.g. servicecidr's migration test cancels the first
controller before switching to a second apiserver), and self-contained
helpers that synchronously call Cancel as their own first action
regardless of ktesting's automatic timing (e.g. resourceclaim's
stopInformers, invoked both via defer and explicitly mid-test).
This is for consistency with other controller constructors which truly need a
context already during construction because they spawn goroutines. Constructors
which don't do that (yet) can trivially retrieve the logger with FromContext.
checkVolumeSatisfyClaim compared PV capacity and PVC request through
Quantity.Value(), which projects onto int64 and overflows for storage
quantities past that range, so a large capacity or request was misordered.
FindMatchingVolume already compares the same two quantities with Cmp, so
use Cmp here too and keep the explicit-binding path in agreement.
Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
Kubernetes 1.37 finished the introduction of context-aware client-go APIs.
Components like kube-controller-manager which are meant to support contextual
logging should use those new APIs.
This is not enforced yet because existing code must be updated first. Once
that is done, enforcement of everything touched here can be enabled tree-wide.
The PV controller kept its own PV/PVC caches (ctrl.volumes, ctrl.claims)
separate from the shared informers, reconciled via storeObjectUpdate().
That split brain caused:
- an in-flight controller update could re-add an object the informer had
already observed as deleted, resurrecting a phantom PV/PVC, and
- concurrent read-compare-write of the separate store could overwrite a
newer version with a stale one.
Replace both caches and storeObjectUpdate() with a single
assumecache.AssumeCache[T] layered on each shared informer. The informer
indexer is the source of truth; the controller's own writes are recorded
with AssumeWritten() (comparable ResourceVersions, KEP-5504) so reads
observe them until the informer catches up, and never resurrect a deleted
object.
Details:
- The accessmodes index moves onto the PV shared informer; the ordered
index and matching helpers now read through the assume cache.
- Delete follow-up (schedule the bound partner's sync, clean up the
operation-timestamp cache and metrics) moves into the informer delete
handlers, which still hold the deleted object.
- syncVolumeByKey/syncClaimByKey read from the assume cache via
GetAssumed() and skip the sync when the object is still being served
from the controller's own not-yet-observed write. This matches
the old storeObjectUpdate() "skip older ResourceVersion" guard.
The 15s resync is also preserved.
- initializeCaches() is removed; the informer indexer is already
populated once the caches have synced.
All reads go through assumption-aware methods so they observe the
controller's own writes: AssumeCache grows List() (used by the metrics
collector) and ListIndexValues() (used to enumerate access-mode index
keys) alongside Get()/GetAssumed()/ByIndex(). AssumeCache.Indexer()
remains only as a low-level seam for tests to seed informer state; it is
never used for production reads.
The RV-ordering logic previously unit-tested via storeObjectUpdate() is
now covered by the assumecache package tests.
removeDeletionProtectionFinalizer() and rescheduleProvisioning() throw
away the object returned by Update() and put the copy they sent into the
controller cache instead. That copy still carries the ResourceVersion the
object had before the write, so the cache ends up holding content from
version N+1 labelled as version N. All the other writers in this
controller store the API server's response; do the same here, and name
the variables like they do.
This works today only because storeObjectUpdate() lets an equal
ResourceVersion pass, which happens to overwrite the entry. A cache that
decides what to keep by comparing ResourceVersions cannot make that call
correctly, so fix the input first.
Test_MarkDesireToDetach_Positive_MarkedAddVolumeNodeReset described
behaviour that changed in 2016: MarkDesireToDetach became
RemoveVolumeFromReportAsAttached, and the AddVolumeNode call in the name
was replaced by an explicit ResetDetachRequestTime when AddVolumeNode
stopped clearing the detach request time. What is left of the test
covers ResetDetachRequestTime, so name it after that.
That edit also left three markDesireToDetachErr variables behind, where
it renamed the others to removeVolumeDetachErr; use the latter here too.
Two failure messages still named the dead function as well, one of them
in Test_RemoveVolumeFromReportAsAttached_Positive_Marked, which would
send anyone debugging a failure looking for a function that no longer
exists.
AddVolumeNode seeded inUseVolumes with the volume on first attach,
defaulting MountedByNode to true until node status proved otherwise.
This inference dates to before commit 3a71fe57f7 ("ad controller: lift
nodeAttachedTo.mountedByNode"). Back then mountedByNode lived on the
per-volume nodeAttachedTo, and processVolumesInUse could only record it
for volumes already in the ASW (SetVolumeMountedByNode required the
node/volume to exist). So while node.Status.VolumesInUse was always
carried on every node update, the mounted state was lost for a volume
not yet in the ASW, and AddVolumeNode had to assume mounted. Since that
refactor the node-keyed inUseVolumes map holds the whole VolumesInUse
set regardless of ASW membership, so the assumption is no longer needed
-- and it is both unreliable and harmful:
- Unreliable: any unrelated node update calls SetVolumesMountedByNode,
which overwrites the whole set from node status, so the optimistic bit
survives only until the next node update.
- Harmful: it delays detach until the next node update the controller
observes (up to ~5min via kubelet status). For a failed attach marked
uncertain, or a volume kubelet never adds to VolumesInUse (pod deleted
before kubelet reports it), a normally-detachable volume needlessly
waits.
The safety net that matters is independent of this bit: the reconciler's
detach path re-reads Status.VolumesInUse with a live GET
(verifyVolumeIsSafeToDetach), and kubelet must publish the volume in
that same field before it may mount. MountedByNode now comes solely from
node status via SetVolumesMountedByNode.
The "assume mounted" default traces back to #52036 (2017), whose
discussion concluded that the race it guards against does not exist,
since a mount cannot proceed until the volume is written to
Node.Status.VolumesInUse. The follow-up #52037 proposed removing the
MountedByNode check outright and was rejected on data-safety grounds, so
this change is narrower: the check stays and detach is still blocked
whenever node status reports the volume mounted. Only the assumption
made before that status arrives goes away.
Tests updated to reflect that MountedByNode is no longer set on attach:
cache tests assert the false default, and the reconciler tests that
exercise the mounted-blocks-detach path now mark the volume mounted
explicitly, as a real node status update would. Mutation testing keeps
them honest -- neutering the reconciler's MountedByNode check must still
fail every one of them -- and it surfaced two that had gone vacuous: the
out-of-service taint test now covers the taint override of a
genuinely-mounted volume, and the no-taint test gives the reconciler
loops to confirm the mounted volume stays attached, instead of asserting
zero detaches instantly. Conversely, the tests that expect a detach no
longer call SetVolumesMountedByNode at all: unmounted is now the
default, so those calls were no-ops, and the comments claiming a volume
"is set to mounted by default" no longer described the code.
The SetVolumesMountedByNode cache tests are rebalanced around what the
node-keyed map can actually get wrong. Three cases only made sense while
the mounted bit lived on nodeAttachedTo next to detachRequestedTime and
was written back as a whole struct:
UnsetWithInitialSetAddVolumeNodeNotReset,
UnsetWithInitialSetVerifyDetachRequestedTimePerserved and its
RemoveVolumeFromReportAsAttached counterpart. SetVolumesMountedByNode
now assigns a whole set into inUseVolumes and nothing else touches that
map, so those invariants cannot break; re-introducing the seed is caught
by the AddVolumeNode tests instead. Two cases take their place:
SetOneOfTwoVolumes pins that mounted state is per volume rather than per
node and that each call replaces the previously reported set instead of
adding to it, and SetBeforeVolumeAdded pins the property this removal
rests on, which no test asserted -- a volume reported in use before it
is added to the cache keeps that state and has it applied on
AddVolumeNode. Test_SetVolumesMountedByNode_Positive_Set now asserts
that MountedByNode flips from false to true across the call rather than
only checking the end state. One assertion moves rather than disappears:
the exact DetachRequestedTime comparison now sits in
Test_AddVolumeNode_Positive_ExistingVolumeExistingNode, where nothing
covered AddVolumeNode keeping a pending detach request -- a failed
detach re-adds the volume there through MarkVolumeAsUncertain, and the
force detach timeout is measured from the preserved value, so clearing
it would restart that clock on every failed attempt. No covered block of
the cache package is lost by the deletions.
Test_Run_Negative_OneDesiredVolumeAttachThenDetachWithUnmountedVolumeUpdateStatusFail
asserted no detach immediately after deleting the pod, before the
reconciler could have acted, so it passed even when a failed node status
update no longer blocked the detach; it now gives the reconciler loops
first, like the mounted-volume tests. These reconciler tests remain
wall-clock based; making them deterministic wants testing/synctest, a
separate change.
Commit 9e29f95618 ("Refactor controller-manager volume plugins")
replaced ProbeControllerVolumePlugins with
ProbeProvisionableRecyclableVolumePlugins for the PV binder controller.
The new function filters plugins to those implementing
Provisionable/Deletable/Recyclable interfaces; the CSI plugin implements
none of these (it delegates to external drivers), so it was excluded
from volumePluginMgr.
metrics.Register was still passing &ctrl.volumePluginMgr, so
getPVPluginName() -> FindPluginBySpec() failed for any CSI PV,
falling back to pluginNameNotAvailable ("N/A") instead of the expected
"kubernetes.io/csi:<driver>" label value.
Fix by initializing a dedicated metricsPluginMgr from
ProbePersistentVolumePlugins (all plugins, no filter) and passing it
to metrics.Register.
The PVC protection controller's Unused condition write races with the
PV binder's bindClaimToVolume on the same PVC during initial binding,
causing 409 conflicts. Skip the evaluation until the PVC is Bound,
at which point the PV binder is done writing it.
AddEventHandlerWithOptions returns an error that the protection
controller constructors handled with utilruntime.Must, which panics
the controller-manager if handler registration ever fails. Return the
wrapped error instead so the failure propagates to the caller.
NewPVProtectionController previously returned only *Controller; give it
an error return to match the other protection controllers and update
its callers accordingly.
AddEventHandlerWithOptions returns an error, but several controller
constructors passed it to runtime.Must, which panics the whole
controller-manager. Since these constructors already return an error,
return the wrapped error instead, matching the existing pattern in the
selinux warning controller.
The attachdetach, expand, ephemeral, selinuxwarning and persistentvolume
controllers registered their informer event handlers via AddEventHandler,
which logs through the global klog.Background() logger. Switch them to
AddEventHandlerWithOptions and pass the controller's contextual logger so
event-handler logging is contextual.
For the controllers that previously discarded the registration result, the
returned error is now surfaced via runtime.Must instead of being ignored.
This is a follow-up to the protection controllers for the remaining volume
controllers.
The PV, PVC and VAC protection controllers registered their informer
event handlers via AddEventHandler, which logs through the global
klog.Background() logger. Switch them to AddEventHandlerWithOptions and
pass the controller's logger so event-handler logging is contextual.
This also handles the registration errors that were previously
discarded, surfacing them via utilruntime.Must.
When a node is deleted, its volumesInUse is preserved in the AD
controller cache as MountedByNode, preventing detach until the
6-minute force-detach timeout expires.
Clear MountedByNode immediately on node deletion instead. Node
deletion typically means the underlying node is gone, so waiting
6 minutes provides no benefit. If the node is deleted by accident,
this is still acceptable because detach is gated by pod deletion
(PodGC deletes pods on deleted nodes after ~40s quarantine), and
once pods are deleted, kubelet cannot clean up anyway (CNI/CSI
pods are force-deleted together with the node).
Without this change, volumes on deleted nodes are stuck permanently
when --disable-force-detach-on-timeout is enabled, as out-of-service
taint cannot be applied to a deleted node and MountedByNode blocks
reconciliation.
This also fixes a slow memory leak: deleted nodes' volumesInUse
entries remain in cache indefinitely if nodes are constantly recycled.
* Fix goroutine leaks in ephemeral volume controller test
Use context.WithCancel and properly shut down the informer factory
and workqueue in TestSyncHandler to prevent goroutine leaks.
Previously, the test used context.Background() which never cancels,
leaving informer and workqueue goroutines running after test completion.
Now that context support has been added to tools/cache (#126387),
the informers can be cleanly shut down via context cancellation.
Also add goleak.VerifyTestMain to detect goroutine leak regressions.
* Remove year from copyright header in main_test.go
* Drop main_test.go per review feedback
kubernetes-prow[bot]