Trigger localization of clean room objects - #2829
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❌ Patch coverage is
Additional details and impacted files
... and 3 files with indirect coverage changes
@@ Coverage Diff @@
## development/9.6 #2829 +/- ##
===================================================
+ Coverage 75.77% 75.91% +0.14%
===================================================
Files 200 201 +1
Lines 13922 14024 +102
===================================================
+ Hits 10549 10647 +98
- Misses 3363 3367 +4
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}
| * @param {Function} cb - callback: cb(err, accountId) | ||
| * @return {undefined} | ||
| */ | ||
| getAccountId(ownerId, log, cb) { |
There was a problem hiding this comment.
Async/await migration: getAccountId is a new callback-style function (cb parameter). Per the project's migrate-when-you-touch rule, new code should use async/await. Since vaultClientWrapper.getAccountId is callback-based, wrap it with util.promisify. The same applies to _resolveAccountId in LifecycleUpdateTransitionTask — once this method is async, the caller can await it (or keep backward compat via util.callbackify until the task's processActionEntry is also migrated).
| _accountIdLookupEnabled() { | ||
| const authConfig = this.getAuthConfig(this._lcConfig); | ||
| return authConfig.type === authTypeAssumeRole && | ||
| !!(this._processConfig.vaultAdmin || authConfig.vault); |
There was a problem hiding this comment.
authConfig.vault fallback means any processor whose auth has type: 'assumeRole' and a vault field will also enable vault initialization and block isReady() until credentials arrive — not just the transition processor. The expiration processor falls through to lcConfig.auth which could carry vault in an assumeRole deployment.
Consider restricting to this._processConfig.vaultAdmin only, since that field is only in the transitionProcessor schema:
| !!(this._processConfig.vaultAdmin || authConfig.vault); | |
| !!(this._processConfig.vaultAdmin); |
| * @param {Function} cb - callback: cb(err, accountId) | ||
| * @return {undefined} | ||
| */ | ||
| getAccountId(ownerId, log, cb) { |
There was a problem hiding this comment.
getAccountId and _resolveAccountId are new functions using the callback pattern. Per the migrate-when-you-touch policy, new code should use async/await. The callee (VaultClientWrapper.getAccountId) can be wrapped with util.promisify, and backward compatibility for the callback-based caller chain can be kept with util.callbackify. (Suggestion, not a blocker.)
| const { traceHeadersFromEntry } = require('arsenal/build/lib/tracing').kafka; | ||
|
|
||
| const TRANSITION_ATTEMPT_MD = 'x-amz-meta-scal-s3-transition-attempt'; | ||
| const { transitionTasksTopic } = config.extensions.lifecycle; |
There was a problem hiding this comment.
Module-level destructuring of config.extensions.lifecycle crashes the queue populator on import if lifecycle is not configured — even when localization is disabled. Since LogReader.js requires this module unconditionally, any replication-only deployment would fail to start.
Move this into _publishLocalizationAction (the only consumer) so the access is deferred to when localization is actually enabled:
| const { transitionTasksTopic } = config.extensions.lifecycle; | |
| const { transitionTasksTopic } = config.extensions?.lifecycle ?? {}; |
Alternatively, add a config validation in Config.js that localization requires lifecycle to be configured.
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 localization work needs to read as well. Issue: BB-814
copyLocation actions were all reported as transitions, which is about to stop being true: localization uses the same pipeline but has a trigger latency of milliseconds where a transition is minutes to hours, and the two would share histogram buckets that start at a minute. Derive the metrics type from the origin of the action instead, so trigger, start and completion keep pairing up. Issue: BB-814
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
In a clean room, 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. Localization 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
8cd19f1 to
aedd849
Compare
| // one needing a vault client to resolve them. | ||
| const authConfig = this.getAuthConfig(this._lcConfig); | ||
| if (authConfig.type === authTypeAssumeRole && | ||
| (vaultAdminConfig || authConfig.vault)) { |
There was a problem hiding this comment.
The authConfig.vault fallback means the transition processor initializes a vault client and blocks isReady() on credential arrival in any assumeRole deployment that has a vault field in its auth config — even when clean room localization is not in use and vaultAdminConfig was not explicitly set.
Consider gating solely on the explicit vaultAdminConfig parameter:
| (vaultAdminConfig || authConfig.vault)) { | |
| if (authConfig.type === authTypeAssumeRole && vaultAdminConfig) { |
(Note: a previous comment raised this same concern but was posted against LifecycleObjectProcessor.js, which was not modified by this PR and may not be visible in the diff view.)
2 participants
In a clean room, 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 that, since bootstrap, re-bootstrap and streamed updates all go through the same oplog, so a single code path covers all three. When an object lands on a location flagged
isCRR, we publish acopyLocationaction on the data mover topic and let the existing data mover + transition merge pipeline do the actual copy.The check sits before any replication condition, on purpose:
replicationInfodescribes replication of a local object to remote sites, and the clean room copy resets it since the copy has not been replicated anywhere. A clean room site may also legitimately have its own forward replication rules. Localization is not tracked there either, updatinglocation/dataStoreNameis enough.There is no activation switch: the
isCRRcondition cannot occur outside a clean room, so the flag on the location is the trigger, consistently with how the design gates the write-time master key handling.Where the data goes
The target location is read from the object's source location entry, as
targetLocation, next to thebucketandrolethe copy already needs: the source-side rewrite pipeline resolves it from the bucket when it synthesizes that entry. That keeps the populator, a single threaded oplog reader, from having to look the bucket up per object, and makes the destination a property of the bucket rather than of the deployment.If that location no longer exists (deleted between the metadata write and the oplog entry), or the object predates the pipeline naming one, we log an error and fall back to the default local location, the first one that is neither cold nor
isCRR: localizing elsewhere beats leaving the data on the source forever.Account id resolution
The populator only knows the object owner's canonical id, and a Vault lookup per entry would throttle the whole populator. So the action carries
target.owneronly, and the lookup is deferred to the transition processor, which resolves it once up front and stamps it back onto the action, the same way the garbage collector already does. Lifecycle-originated actions always carry an account id, so the lookup never runs for them, and the GC entry emitted after the merge gets the resolved id rather than resolving it again.Metrics
A localization is a lifecycle transition, only triggered from the oplog instead of a bucket scan, so it reports
onLifecycleTriggeredwith the same labels the transition merge already reportsonLifecycleCompletedwith. Localized bytes are already accounted byCopyLocationTaskthroughReplicationMetrics. The queued counters do not fire, since we publish straight to the data mover topic instead of going throughsendDataMoverAction, which needs a producer a populator extension does not have.Skipped entries
Master keys (clean room buckets are versioned, the master is repaired by the metadata layer), delete markers, entries without a
dataStoreName(partial oplog projections), and objects with no location, since a 0-byte object has nothing to localize.isCRRwith a non-zero content length and no location is inconsistent metadata and gets logged as an error.Duplicates are expected and harmless: the topic is keyed per object, the data mover dedupes in memory,
CopyLocationTaskskips already-transitioned versions, and the merge task skips duplicate location updates.Not in this PR
transitionInProgress_garbageCollectLocationpublishes adeleteDatafor the old location, which against anisCRRsource would delete source data.Note that the full unit suite could not be run locally (needs Kafka/Redis); lint and the replication, gc, queuePopulator, lib and lifecycle task specs are green.
Issue: BB-814