You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This corrupts the cache for every other reader (the scheduler and other controllers read HyperNodes from the same informer). The sibling code in hypernode_handler.go already knows this and deep copies first:
// Create a deep copy to avoid modifying cache objectshyperNodeCopy:=hyperNode.DeepCopy()
2. It uses a stale resourceVersion for UpdateStatus.
HyperNode has a status subresource (+kubebuilder:subresource:status), so both Update (spec and metadata) and UpdateStatus (status) are needed. Update returns the object with a bumped resourceVersion, but the code discards it and reuses the old current for UpdateStatus:
_, err=...Update(ctx, current, ...) // bumps the resourceVersion, return discarded_, err=...UpdateStatus(ctx, current, ...) // current still has the old resourceVersion
Because current still carries the pre-Update resourceVersion, UpdateStatus fails the optimistic concurrency check. The call is wrapped in RetryOnConflict, so it retries and eventually succeeds once the informer cache catches up, but every spec change now goes through a conflict, a retry, and one or more redundant Update writes.
This second one came in with #4498, which fixed the original "spec is never updated" report in #4497 by adding the Update call, but it threaded the resourceVersion wrong.
Watch the controller: the spec update goes through, but the status update logs a conflict and retries until the cache catches up.
Describe the results you received and expected
Received: the cache object is mutated in place, and the status update conflicts on a stale resourceVersion and retries.
Expected: the update works on a copy of the cache object, and the status write uses the resourceVersion returned by Update, so there is no conflict.
The fix is small: deep copy current before changing it, and carry the status onto the object returned by Update before calling UpdateStatus. Happy to send the PR.
/kind bug
Description
UpdateHyperNodeinpkg/controllers/hypernode/utils/utils.gohas two problems in how it handles the HyperNode object.1. It mutates the informer cache.
lister.Get()returns a pointer to the shared object in the informer cache, so it must not be modified.UpdateHyperNodewrites straight into it:This corrupts the cache for every other reader (the scheduler and other controllers read HyperNodes from the same informer). The sibling code in
hypernode_handler.goalready knows this and deep copies first:2. It uses a stale resourceVersion for UpdateStatus.
HyperNode has a status subresource (
+kubebuilder:subresource:status), so bothUpdate(spec and metadata) andUpdateStatus(status) are needed.Updatereturns the object with a bumped resourceVersion, but the code discards it and reuses the oldcurrentforUpdateStatus:Because
currentstill carries the pre-Update resourceVersion,UpdateStatusfails the optimistic concurrency check. The call is wrapped inRetryOnConflict, so it retries and eventually succeeds once the informer cache catches up, but every spec change now goes through a conflict, a retry, and one or more redundantUpdatewrites.This second one came in with #4498, which fixed the original "spec is never updated" report in #4497 by adding the
Updatecall, but it threaded the resourceVersion wrong.Steps to reproduce the issue
Describe the results you received and expected
Received: the cache object is mutated in place, and the status update conflicts on a stale resourceVersion and retries.
Expected: the update works on a copy of the cache object, and the status write uses the resourceVersion returned by
Update, so there is no conflict.The fix is small: deep copy
currentbefore changing it, and carry the status onto the object returned byUpdatebefore callingUpdateStatus. Happy to send the PR.What version of Volcano are you using?
master