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.
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.
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.
Subtest 5-2-3 starts the controller with both the claim and the volume
already present and with the volume annotated as controller-bound. That
setup depends on watch/list ordering during controller startup: if the
initial objects are observed in a different order, the test can time out
even though the controller later converges to the expected state.
Model the scenario more explicitly. Start with the unbound volume only,
then inject the claim through AddClaimEvent after the controller is
running. Also drop the pre-set controller-bound annotation from the
initial volume since the binding is what the test wants the controller to
establish.
Tested:
go test -race ./pkg/controller/volume/persistentvolume -run TestControllerSync -count=50
Raname this because facing lint error, counter metrics should have
"_total" suffix. Add the test `volume_operation_errors_total`
Marked `volume_operation_total_errors` as deprecated
Signed-off-by: ChengHao Yang <17496418+tico88612@users.noreply.github.com>
Currently, we get each released PV every 15s, and in parallel. If there are a lot of released PV and we cannot finish all the get in 15s, it will starve other request by making the queue waiting for client-side throttling very long.
Even in a normal cluster, these requests are taking majority of all get requests from KCM (58% or 470 qps) in our stress test.
The "// import <path>" comment has been superseded by Go modules.
We don't have to remove them, but doing so has some advantages:
- They are used inconsistently, which is confusing.
- We can then also remove the (currently broken) hack/update-vanity-imports.sh.
- Last but not least, it would be a first step towards avoiding the k8s.io domain.
This commit was generated with
sed -i -e 's;^package \(.*\) // import.*;package \1;' $(git grep -l '^package.*// import' | grep -v 'vendor/')
Everything was included, except for
package labels // import k8s.io/kubernetes/pkg/util/labels
because that package is marked as "read-only".
kubernetes-prow[bot]