🐛 Enqueue location upon synctarget update #2624
Conversation
| @@ -215,7 +215,7 @@ func (c *controller) enqueueLocation(obj interface{}) { | |||
| placements = append(placements, placementsByPath...) | |||
| } | |||
|
|
|||
| logger := logging.WithObject(logging.WithReconciler(klog.Background(), ControllerName), location) | |||
| logging.WithObject(logger, location) | |||
There was a problem hiding this comment.
| logging.WithObject(logger, location) | |
| logger = logging.WithObject(logger, location) |
| return | ||
| } | ||
| placements = append(placements, placementsByPath...) | ||
| } | ||
|
|
||
| logger := logging.WithObject(logging.WithReconciler(klog.Background(), ControllerName), obj.(*workloadv1alpha1.SyncTarget)) |
There was a problem hiding this comment.
Seems to me that the SyncTarget value added here to the logger, will be, at least in some logger implementation, overwritten by the same operation applied in enqueueLocation()
| for _, placement := range placements { | ||
| c.enqueuePlacement(placement, logger, " because of SyncTarget") | ||
| for _, location := range locations { | ||
| c.enqueueLocation(location, logger, " because of SyncTarget") |
There was a problem hiding this comment.
Instead of managing the source (==reason) of the enqueue by a suffix, can't we simply add distinct values ?
Something like: logger.WithValues(logging.FromPrefix("locationReason")) and logger.WithValues(logging.FromPrefix("syncTargetReason")) ?
So you can accumulate the reason objects if needed, and would have always a fixed log message.
qiujian16
force-pushed
the
fix-synctarget-enqueue
branch
from
6bec1c2 to
50b0986
Compare
We cannot directly read the logicalcluster path from synctarget to get the related placement, since the value of that path is not unique. Instead, we need to get the location of the related synctarget at first, which always has the path annotation. Signed-off-by: Jian Qiu <jqiu@redhat.com>
qiujian16
force-pushed
the
fix-synctarget-enqueue
branch
from
50b0986 to
97b7b0c
Compare
| if err != nil { | ||
| runtime.HandleError(err) | ||
| return | ||
| func (c *controller) enqueueSyncTarget(obj interface{}, logger logr.Logger) { |
There was a problem hiding this comment.
this is very noisy. We should rethink whether we really need to enqueue half of the world.
- What is the exact reason we have to enqueue a sync target?
- What can we do to get the placements more directly?
There was a problem hiding this comment.
Goal: only sync the placement that actually schedules to that SyncTarget.
There was a problem hiding this comment.
ideally we should only enqueue location and scheduled synctarget here. The missing part is we do not know what APIs are compatible in the location. So if a synctarget is updated with "api compaible/non-comptaible", we have to re-select the clusters.
There was a problem hiding this comment.
So if a synctarget is updated with "api compaible/non-comptaible"
For that we could have a special case, couldn't we?
There was a problem hiding this comment.
Also, can we queue only those locations that are affected, e.g. that include the SyncTarget? If you remove a label from a SyncTarget, we could also see that and queue those locations where this SyncTarget was removed from.
There was a problem hiding this comment.
ideally we should only enqueue location and scheduled synctarget here. The missing part is we do not know what APIs are compatible in the location. So if a synctarget is updated with "api compaible/non-comptaible", we have to re-select the clusters.
Actually, I now think the fix is not correct: the old code did not look at the SyncTarget, but on the LogicalCluster. The LogicalCluster always has a path annotation. What is missing is that we don't label the logical cluster for replication. Compare pkg/reconciler/tenancy/replicatelogicalcluster.
There was a problem hiding this comment.
Nvmd:
On the other hand, the new change does not really cause more noise, but it will list all locations and from there get all placements that reference these locations. So there is hope that these are not all placements, vs. the old code that queued all placements.
davidfestal
added
the
area/transparent-multi-cluster
|
With https://github.com/kcp-dev/kcp/pull/2624/files#r1101774288 /lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sttts The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
We cannot directly read the logicalcluster path from synctarget to get the related placement, since the value of that path is not unique. Instead, we need to get the location of the related synctarget at first, which always has the path annotation.
Signed-off-by: Jian Qiu jqiu@redhat.com
Summary
Related issue(s)
Fixes #2579