Pull replication of objects still on the source location - #2829
Pull replication of objects still on the source location#2829francoisferrand wants to merge 7 commits into
Conversation
Hello francoisferrand,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
... and 6 files with indirect coverage changes
@@ Coverage Diff @@
## development/9.6 #2829 +/- ##
===================================================
+ Coverage 76.13% 76.14% +0.01%
===================================================
Files 204 205 +1
Lines 14056 14150 +94
===================================================
+ Hits 10701 10774 +73
- Misses 3345 3366 +21
Partials 10 10
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
8cd19f1 to
aedd849
Compare
aedd849 to
d2d9f37
Compare
The x-amz-meta-scal-s3-transition-attempt key was open-coded in five places across lifecycle and gc, each with a slightly different way of reading or clearing it - one of which would throw on user metadata that does not parse. Move it behind a small helper, which the pull replication work needs to read as well. Issue: BB-814
copyLocation actions were all reported as transitions, which is about to stop being true: pull replication reuses the same pipeline but times a different thing, the delay from an object's metadata landing locally to its data being copied, over a different population of objects. Folding both under type=transition would give quantiles that describe neither, and would dilute the LifecycleLatency alert, which groups by type. Derive the metrics type from the origin of the action instead, so trigger, start and completion keep pairing up. Issue: BB-814
3743d3f to
0ac827a
Compare
francoisferrand
changed the title
0ac827a to
5ee333b
Compare
The lifecycle conductor knows the account id of the bucket it is scanning and stamps it on the actions it publishes. The queue populator does not: it works off the oplog, where an object only carries its owner's canonical id, and resolving an account per entry would throttle the whole populator. So let the transition processor do the lookup, once per action and only when needed, the same way the garbage collector already does, and pass the result on to the garbage collection entry it emits. Issue: BB-814
5ee333b to
e0f5d0f
Compare
Objects are created locally but their metadata still points at the source cluster's location: the data itself has not been copied over yet. Something has to notice those objects and ask for the data to be pulled in. The queue populator is the natural place for it, since bootstrap, re-bootstrap and streamed updates all go through the same oplog. When an object lands on a location flagged isCRR, publish a copyLocation action on the data mover topic and let the existing data mover + transition merge pipeline do the actual copy. The destination comes from the object metadata, which the source-side rewrite stamps as it prepares the entry; if it names a location we do not know, fall back to the first local one and log about it. This is unrelated to replicationInfo, which describes replication of a *local* object to remote sites, so the check sits before any replication condition. Pulling data in is neither lifecycle nor CRR replication, so it gets its own action origin, and the legacy CRR byte metrics - which only make sense for replication to a remote site - skip it like they already skip lifecycle. Issue: BB-814
The queue populator counted the objects it queued for replication, and the data mover reported the bytes it copied once they arrived, but nothing counted the objects queued for pull replication: a growing backlog was indistinguishable from an idle cluster. Count those too, and tell the two flows apart with the direction the data is moving. 'origin' cannot do it: it names the extensions loaded in the populator, and is the same either way. Their partition label is left empty, as the populator batches its publishes and only learns the partition long after queuing; an empty label and a missing one are the same series to prometheus. Issue: BB-814
BACKBEAT_QUEUEPOPULATOR_EXTENSIONS selects the extensions a queue populator runs, but it was applied by removing the others from the configuration altogether. Anything reading another extension's settings -a topic they share, for instance- then got nothing, in the populator processes only. Select them where they are loaded instead, and leave the configuration alone. Issue: BB-814
e0f5d0f to
8b96b6f
Compare
| // Data still on the source location has to be fetched first. This is | ||
| // unrelated to replicationInfo, which tracks replication of a *local* | ||
| // object to remote sites, hence the check before any of its conditions. | ||
| if (locationConfig.isCRR && this.transitionTasksTopic) { |
There was a problem hiding this comment.
this always returns, including when _publishPullReplicationAction published nothing.
For empty objects CopyLocationTask already puts 0-byte without a GET, skipping them leaves dataStoreName on the source forever, and push CRR never runs either... Same for delete markers as those usually must be replicated, and they have no data to pull. We could either publish a copyLocation for the 0-byte case (so metadata moves to the local location), or only return after an actual publish I guess ?. Delete markers should fall through to the existing CRR path...
| return; | ||
| } | ||
| const locations = queueEntry.getLocation(); | ||
| if (!locations || locations.length === 0) { |
There was a problem hiding this comment.
“nothing to fetch” is true for the bytes, not for the metadata. the object still advertises the source location...
| }); | ||
| return undefined; | ||
| } | ||
| this.log.error('invalid target location in object metadata', { |
There was a problem hiding this comment.
This path then succeeds with a fallback , better gowarn?
| */ | ||
| function getTransitionAttempt(objMD) { | ||
| const attempt = Number.parseInt(objMD.getValue()[TRANSITION_ATTEMPT_MD], 10); | ||
| return Number.isInteger(attempt) ? attempt : undefined; |
There was a problem hiding this comment.
Should we return 0 or 1 ? Not sure we should as the caller code will be much complex
There was a problem hiding this comment.
No, we need keep undefined : this is used to add the user-metadata and show the retry to the user. If not set (or invalid), we don't want to show anything. So the contract is really "give me the transition attempt if any".
(this field is added in the metadata by backbeat, using a value computed by SorbetCtl when requeuing from DLQ: sorbetctl builds the new attempt counter, send the "reset/retry" message to backbeat, which writes the counter in MD)
|
|
||
| publish() { | ||
| // Lifecycle metrics not yet implemented. | ||
| if (this._isLifecycleAction()) { |
There was a problem hiding this comment.
maybe a good idea to keep the function and rename it with "canPublish"
There was a problem hiding this comment.
canPublish() would be misleading: as publish can do nothing more multiple other reasons (no producer, send error...)
In this context, it is also not a good abstraction: we end up just wondering "why wouldn't we", any reader will just go look at the code of the function, and it would actually reduce readability.
I initially tried that, but could not find any fitting name to describe the condition (isLifeycleAction, skipMetrics, isTransitionAction, isCrrReplication, hasOwnMetrics) -but they were all either incorrect or did not provide an abstraction- thus I ended keeping the code simple, inline.
The information the reader actually needs is why those are excluded, but only the comment can say it (the metrics API/routes only implement CRR), the name can't.
| // Every publisher sets one or the other, so this is a malformed | ||
| // entry: log it, and let the task fail on its own further down | ||
| // rather than retrying something that cannot be fixed. |
There was a problem hiding this comment.
| // Every publisher sets one or the other, so this is a malformed | |
| // entry: log it, and let the task fail on its own further down | |
| // rather than retrying something that cannot be fixed. | |
| // Every publisher sets one or the other, so this is a malformed | |
| // entry |
There was a problem hiding this comment.
To avoid the comment I would do:
if (!accountId && !owner) {
log.error('cannot resolve account id: entry has no account id nor owner');
return process.nextTick(cb);
}
if (accountId)
...
this.getAccountId
The needed value of accountId and owner is explicit instead of implicit and the comment not needed
| if (this._isLifecycleAction()) { | ||
| // Metrics API/routes only support CRR | ||
| const { origin } = this._entry.getContext(); | ||
| if (['lifecycle', 'pullReplication'].includes(origin)) { |
| if (queueEntry.getIsDeleteMarker()) { | ||
| return; | ||
| } | ||
| const locations = queueEntry.getLocation(); |
There was a problem hiding this comment.
| const locations = queueEntry.getLocation(); | |
| const locations = queueEntry.getLocations(); |
Hard to refacto ?
There was a problem hiding this comment.
yes, its' coming from ObjectMD in arsenal... (and looking at the code, it supports older ObjMD model versions which did not support multiple parts/locations)
| * @return {String|undefined} target location, undefined if there is none | ||
| */ | ||
| _getPullReplicationTarget(queueEntry, locations) { | ||
| const { targetLocation } = locations[0]; |
There was a problem hiding this comment.
If I remember well we'll keep in location only the one that is usuable before reaching this step (on the source oplog right?)
|
|
||
| // Where the data is fetched to when the object metadata does not name a usable | ||
| // target: the first location that can actually hold a local copy. | ||
| this.defaultLocalLocation = Object.keys(locationsConfig).find( |
There was a problem hiding this comment.
Why do we want a fallback ? In which case ? I read the description but didn't find the use case (neither in JIRA).
There was a problem hiding this comment.
it is not a use case, it is the corner case where the location is not usable...
- mongo-processor reads the message from Kafka, it writes the ObjMD with the "pointer" location -
including the location where the data should be written to. - while the message is in kafka, that location is removed (or renamed)
- queue processor eventually sees the insertion, and triggers replication
--> the queue processor cannot store in that location (it has been removed, or possibly renamed)
What we would like is to use the bucket's default location -i.e. not passing any location in
putObject-, however we need a destination location to the message can be picked'up by a worker; and
we don't have that information in here (nor want to bear the cost of the extra I/O)
The actual target location is not really important (most likely there is only a single "local"
location). We compute it early because we have that information already (in mongo processor), to
avoid a hard coded value; and this also leaves room for implementing custom mapping (if needed at
some point in the future).
But in the end, the most critical part is that the data is replicated, so we use such a fallback in
the corner case above, instead of failing.
The only case where no copyLocation action is queued for an object still on the source was tested by stubbing the target resolution itself, which asserted nothing about the resolution logic. Drive the real path instead, with a deployment where no configured location can hold a local copy, so the populator is pinned to skip the object rather than copy it back to the source or to a location that does not exist. Issue: BB-814 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
The 2 uncovered patch lines were Worth noting the guard is log-only: without it the fallback returns the same undefined, so the new test has teeth on the skip behaviour (verified: it fails if the guard returns the unusable target instead) but not on which of the two error messages is emitted. |
4 participants
Objects are created locally but their metadata still points at the source cluster: the data itself has not been copied over yet. Something has to notice them and ask for it to be pulled in.
The queue populator is the natural place for that, since bootstrap, re-bootstrap and streamed updates all go through the same oplog. When an object lands on a location flagged
isCRR, publish acopyLocationaction and let the existing data mover/transition workflow do the actual copy. No activation switch: the flag on the location is the trigger. It is skipped without lifecycle -S3C runs replication without it- as there would be nothing to merge the copy into the object metadata.This is unrelated to
replicationInfo, which tracks replication of a local object to remote sites, hence the check before any of its conditions.The destination comes from the object metadata, where the source-side rewrite stamps it next to the bucket and role the copy already needs. If it names a location we do not know, we fall back to the first local one and print a : copying the data elsewhere beats leaving it on the source forever.
The populator only knows the object owner's canonical id, and a Vault lookup per entry would throttle it, so the account id is resolved by the transition processor instead, once per action and only when missing.
Pull replication reuses the transition pipeline, but remains a separate workflow to report on and to troubleshoot, so it gets its own metrics type. It is also counted as queued on the replication metrics: without it a growing backlog would be indistinguishable from an idle cluster.
Along the way
BACKBEAT_QUEUEPOPULATOR_EXTENSIONSwas applied by dropping the other extensions from the configuration, so anything reading their settings got nothing in the populator processesIssue: BB-814