Repository navigation
S3UTILS-256 Write the S3C 10.1 replication format in crrExistingObjects - #419
nicolas2bert wants to merge 4 commits into
Conversation
crrExistingObjects picked its write path from the bucket rule format. Rules without Filter (V1) got the old single-destination format: a new backend had no bucket and no role. Since S3C 10.1, cloudserver writes the multi-destination format for every bucket, whatever the rule format, so backbeat failed with BadRole and the object stayed PENDING (S3C-11599). The rule format only changes where the prefix is. s3utils 1.19 runs on S3C 10.1 and later only. - one write path for every bucket: bucket and role per backend, from the bucket rules (arsenal resolveBackends, like cloudserver on a PUT) - top-level status PENDING: the backbeat queue populator only queues PENDING objects. PROCESSING left objects of V2 buckets never replicated (S3C-11596) - compute content when it is empty (object put before the rule) - rule without StorageClass: SITE_NAME is the S3C default site. Fails with "missing SITE_NAME" when it's not set - SITE_NAME unset: every site of the rules, instead of the site of the first rule - keys outside every rule prefix are skipped: backbeat refuses them anyway - match object backends on site, bucket and role: one site can have several buckets since S3C 10.1. Backends written by S3C 9.5 match on the site - don't add a rule site that TARGET_REPLICATION_STATUS doesn't pick, keep the backends this run doesn't pick as they are - top-level role: source role from the bucket config. Keep the object role and destination while a backend written by S3C 9.5 is kept: backbeat needs them for it - FORCE_USING_CONFIGURATION is ignored: the bucket config is always used (S3UTILS-208) - warn when SITE_NAME is not a site of the bucket rules - keep the unknown-site check (S3UTILS-250), the SITE_NAME requeue of a site the rules don't name anymore, and isReplica - STORAGE_TYPE is ignored
Hello nicolas2bert,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| if (process.env.FORCE_USING_CONFIGURATION) { | ||
| log.warn('FORCE_USING_CONFIGURATION is ignored: the script always uses the bucket ' | ||
| + 'replication configuration'); | ||
| } |
There was a problem hiding this comment.
STORAGE_TYPE is now ignored too (README says so, and ReplicationStatusUpdater never reads this.storageType anymore), but unlike FORCE_USING_CONFIGURATION it gets no warning. Operators who set it for public cloud sites won't know it has no effect. Warn the same way, and consider dropping the dead storageType param from the constructor.
| } | |
| } | |
| if (process.env.STORAGE_TYPE) { | |
| log.warn('STORAGE_TYPE is ignored: backbeat reads the location type from the location ' | |
| + 'config of the site'); | |
| } |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## development/1.19 #419 +/- ##
====================================================
+ Coverage 47.14% 47.18% +0.04%
====================================================
Files 90 90
Lines 6809 6792 -17
Branches 1460 1454 -6
====================================================
- Hits 3210 3205 -5
+ Misses 3543 3532 -11
+ Partials 56 55 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Request integration branchesWaiting for integration branch creation to be requested by the user. To request integration branches, please comment on this pull request with the following command: Alternatively, the |
| // If the default site is SITE_NAME too, we get it wrong: r1 is left out without a | ||
| // word. Workaround: set StorageClass sf on r1 for the run, then put the bucket | ||
| // replication config back. | ||
| const isRuleSite = this._getConfiguredSites(Rules).includes(this.siteName); |
There was a problem hiding this comment.
isRuleSite counts disabled rules, while hasRuleWithoutSite in _markPending only counts enabled ones. If an old disabled rule has StorageClass = SITE_NAME, the enabled rules without StorageClass get no site, so every object is skipped. The "SITE_NAME is not a site of the bucket rules" warning doesn't fire either, because ruleSites also includes the disabled rule. Only use enabled rules here and in the warning check.
| const isRuleSite = this._getConfiguredSites(Rules).includes(this.siteName); | |
| const isRuleSite = this._getConfiguredSites(Rules.filter(r => r.Status === 'Enabled')) | |
| .includes(this.siteName); |
|
|
||
| #### Notes on S3C versions | ||
|
|
||
| * s3utils 1.19 and later run on S3C 10.1 and later only. They write the |
There was a problem hiding this comment.
This change ships as 1.19.3 (package.json), and 1.19.0–1.19.2 still write the S3C 9.5 format. "1.19 and later" tells S3C 9.5 users that no 1.19.x works for them, and doesn't flag that a patch upgrade drops 9.5 support. Name the exact version. Also consider a minor bump for this breaking change.
| * s3utils 1.19 and later run on S3C 10.1 and later only. They write the | |
| * s3utils 1.19.3 and later run on S3C 10.1 and later only. They write the |
44d86c8 to
49053fe
Compare
| this.log.error('unknown replication site, run stopped. SITE_NAME is not a site ' | ||
| + 'of this object or of the bucket rules. Expected for the S3C default site ' | ||
| + '(rules without StorageClass), e.g. DR failback or renamed DR site. ' | ||
| + 'Objects processed before this point may already have this SITE_NAME: ' | ||
| + 'if SITE_NAME is wrong, run removeReplicationSite (dry run) with ' | ||
| + `SITE_TO_REMOVE=${this.siteName} on these buckets to find and remove it. ` | ||
| + 'If it reset objects, re-run with the right SITE_NAME from the start of ' | ||
| + 'this run (same bucket list and markers): the reset objects are before ' | ||
| + 'the resume point. Otherwise re-run from the resume point with the right ' | ||
| + 'SITE_NAME, or with ALLOW_NEW_SITE=true', { | ||
| bucket, | ||
| key, | ||
| versionId, | ||
| site: this.siteName, | ||
| ruleSites: this._getConfiguredSites(rules), | ||
| objectSites: this._getObjectSites(objMD), | ||
| }); |
There was a problem hiding this comment.
Can we make this message shorter and add the values? Today it does not show which SITE_NAME was used, or which sites we expected. Also, "Expected for the S3C default site" makes it sound normal, but the run stops. Something like this?
| this.log.error('unknown replication site, run stopped. SITE_NAME is not a site ' | |
| + 'of this object or of the bucket rules. Expected for the S3C default site ' | |
| + '(rules without StorageClass), e.g. DR failback or renamed DR site. ' | |
| + 'Objects processed before this point may already have this SITE_NAME: ' | |
| + 'if SITE_NAME is wrong, run removeReplicationSite (dry run) with ' | |
| + `SITE_TO_REMOVE=${this.siteName} on these buckets to find and remove it. ` | |
| + 'If it reset objects, re-run with the right SITE_NAME from the start of ' | |
| + 'this run (same bucket list and markers): the reset objects are before ' | |
| + 'the resume point. Otherwise re-run from the resume point with the right ' | |
| + 'SITE_NAME, or with ALLOW_NEW_SITE=true', { | |
| bucket, | |
| key, | |
| versionId, | |
| site: this.siteName, | |
| ruleSites: this._getConfiguredSites(rules), | |
| objectSites: this._getObjectSites(objMD), | |
| }); | |
| const ruleSites = this._getConfiguredSites(rules); | |
| const objectSites = this._getObjectSites(objMD); | |
| this.log.error(`run stopped: SITE_NAME=${this.siteName} is not a site of the ` | |
| + `bucket rules (${ruleSites.join(',') || 'none'}) or of this object ` | |
| + `(${objectSites.join(',')}). If SITE_NAME has a typo, fix it. If it is the ` | |
| + 'S3C default site, re-run with ALLOW_NEW_SITE=true. See the README ' | |
| + '(ALLOW_NEW_SITE) to find objects already updated and to resume.', { | |
| bucket, | |
| key, | |
| versionId, | |
| site: this.siteName, | |
| ruleSites, | |
| objectSites, | |
| }); |
There was a problem hiding this comment.
Agreed on the wording: "Expected for the S3C default site" sounds normal, but the run stops. The values are already in the log fields: site, ruleSites and objectSites. This message comes from S3UTILS-250 and this PR doesn't touch it, so I'll take your version in a follow-up ticket.
- remove the "every rule names its site" check in _isKnownSite: we only write a site that a rule names or that the object already has - warn when STORAGE_TYPE is set, and drop the unused storageType parameter - warn about FORCE_USING_CONFIGURATION only for true or 1, like before - README: s3utils 1.19.3+ runs on S3C 10.1+ and Zenko 2.16+, remove the S3C 9.5.2 / 9.5.4 note
| _isSameBackend(existing, candidate, replicationInfo) { | ||
| // With the SITE_NAME fallback (step 3 of _markObjectPending), the candidate is the | ||
| // object backend itself. So it is the same, even with no bucket to compare (S3C 9.5). | ||
| if (existing === candidate) { |
There was a problem hiding this comment.
Wondering if this test is always false in practice, unless existing and candidate can be strings?
There was a problem hiding this comment.
It's true in one case: when no rule sends the key to SITE_NAME anymore, step 3 takes the candidates from the object backends themselves, so the candidate is the object backend.
We need it for a backend written by S3C 9.5: it has no bucket, so the other checks compare the top-level destination with nothing, and fail.
Without this, "should accept a site known by the object but not in the bucket rules" fails.
I tried to make the comment above it (I hope) clearer, with an example.
thanks.
There was a problem hiding this comment.
Thanks for the explanation, it makes sense regarding the logic but it looks confusing to me, as it's a helper for _markObjectPending, could it be clearer if the special case there (https://github.com/scality/s3utils/pull/419/changes#diff-7b318b892978054ccebeb486aa1483139261da7f0efc90a1d2f96a7045330940R512) shunted the filtering with isSameBackend and instead, build its objectTargets separately from the other cases?
Feel free to ignore, I am not sure if this suggestion makes sense, I'm just trying to see if we can simplify a pretty complicated logic that has to deal with backward compat ⚙️
| * Marks an object as pending for replication using the V2 multi-destination format. | ||
| * Writes per-backend destination and role; strips V1-only top-level storageClass/storageType/destination. | ||
| * Stops the run when we don't know SITE_NAME (see _isKnownSite), unless | ||
| * ALLOW_NEW_SITE is set. We stop the whole run, not only this object: if SITE_NAME is |
There was a problem hiding this comment.
I don't understand how the "stop the whole run" part is enforced: the function may still return false on an object with a valid site after it found an object with an unknown site, since this._unknownSite is only checked after we call _isKnownSite?
There was a problem hiding this comment.
This function only decides for one object: it skips it and sets _unknownSite. The callers stop the run: _markPending doesn't start new objects, and the listing stops at this page and keeps its markers. Objects already in flight still finish: we write the ones with a known site, which is fine, and skip the ones with an unknown site. I updated the doc block to say that.
- README: name TARGET_REPLICATION_STATUS with its values, rephrase the single ALLOW_NEW_SITE check without a bullet - explain when the candidate is the object backend itself in _isSameBackend, and check the bucket first - say where the run stops in _stopOnUnknownSite - remove the role note from step 4, it is in _isSameBackend - rename pairs to objectTargets
| // bucket and role. | ||
| // Example: the rules now only name dr3. With SITE_NAME=sf, we take the | ||
| // object backend for site sf. | ||
| candidateBackends = existingBackends.filter(b => b.site === this.siteName); |
There was a problem hiding this comment.
No unit test covers this fallback. The only SITE_NAME-not-in-rules test (should warn once per bucket...) uses an object with no backend for that site, so nothing reaches this line.
It also applies when a rule still names SITE_NAME but its prefix doesn't match the key. Example from the README: r2 docs/ → dr2, SITE_NAME=dr2, FAILED. A key a.pdf with an old dr2 backend gets requeued, but the README says "The other keys are skipped". If backbeat checks the rule for each site, that backend stays PENDING forever. Add tests for both cases, and either limit the fallback to sites no rule names, or document it.
problem
On S3C 10.1, crrExistingObjects marks objects for replication, but backbeat never replicates them. The objects stay PENDING.
The script chose how to write the object metadata from the format of the bucket replication rules. Both ways were wrong on S3C 10.1:
Since S3C 10.1, cloudserver writes the same format for every bucket, whatever the rule format: one backend per destination, each with its own bucket and role. The rule format only changes where the prefix is (Prefix or Filter.Prefix).
Example used below:
fix
We now write the object metadata like cloudserver does on a PUT (arsenal resolveBackends), whatever the rule format:
Example: 'docs/a.pdf' matches r1 and r2, so we get site sf, bucket backup-sf, and site dr2, bucket backup-dr2.
Example: site sf, bucket backup-sf is COMPLETED. Site dr2, bucket backup-dr2 has no backend, so NEW picks it.
Example: we add site dr2, bucket backup-dr2, role crr-dst, PENDING. The site sf backend stays COMPLETED.
We also fix two cases:
Example: say r2 is a V2 bucket replication rule with no StorageClass, and we run with SITE_NAME=dr2 and NEW. 'docs/b.pdf' now gets site dr2, bucket backup-dr2. Before, we skipped it.
Example: say r2 sends to site sf, bucket backup-sf-2. 'docs/a.pdf' has site sf, bucket backup-sf, COMPLETED, and site sf, bucket backup-sf-2, FAILED. A FAILED run now retries site sf, bucket backup-sf-2 only. Before, we matched on the site only, so we saw one status for site sf, and could retry the wrong bucket, or none.
behavior changes
They are all in the README, with examples.
Before: the format we wrote depended on the rule format. With V1 bucket replication rules, we wrote the S3C 9.5 format.
Why: since S3C 10.1, backbeat expects each backend to carry its own bucket and role, and S3C 9.5 doesn't understand that. Keeping both formats means keeping the write path that broke (S3C-11599).
Before: with V1 bucket replication rules, we only requeued the site of the first rule, with a warning. With V2 bucket replication rules, nothing changes: we already requeued every site.
Example: we run with NEW. 'docs/b.pdf' matches r1 and r2, so we now add site sf, bucket backup-sf, and site dr2, bucket backup-dr2. Before, with V1 bucket replication rules, we only added site sf.
Why: the rule format only changes where the prefix is, so it must not change which sites we requeue. The old V1 code took one site per bucket as input, so without SITE_NAME it used the site of the first rule. Now one function handles both formats with resolveBackends, like cloudserver on a PUT.
Before: with V1 bucket replication rules, we fell back to the site of the first rule, and only failed when the first rule had no StorageClass. With V2 bucket replication rules, we didn't fail: we ignored the rule.
Example: say r2 has no StorageClass, and SITE_NAME is not set. The run now fails. Before, with V1 bucket replication rules, we used site sf from r1 for every key, so 'docs/b.pdf' never went to the S3C default site.
Why: a rule without StorageClass goes to the S3C default site. That site is in the federation config, which the script can't read, so only SITE_NAME gives it. Falling back to the site of the first rule sent objects to the wrong site: we fail instead of guessing.
Before: with V1 bucket replication rules, the run stopped at the first object with no backend for SITE_NAME. With V2 bucket replication rules, we skipped the objects without a warning.
Example: we run with SITE_NAME=dr3 and NEW. We skip 'docs/a.pdf' and 'docs/b.pdf', and log "SITE_NAME is not a site of the bucket rules". Before, with V1 bucket replication rules, the run stopped at 'docs/a.pdf'.
Why: no rule gives a backend for SITE_NAME, so we have nothing to write. The stop protects objects we are about to write a wrong site to (S3UTILS-250): here there are none. It also stopped runs for nothing: a FAILED run stopped on objects with nothing to retry. The warning says why few or no objects are updated.
Before: with V1 bucket replication rules, we marked them PENDING, and backbeat refused them ("replication disabled for object"). With V2 bucket replication rules, we already skipped them.
Example: say r1 has Prefix 'photos/' instead of ''. 'notes.txt' matches no rule, so we skip it. Before, with V1 bucket replication rules, we marked it PENDING on site sf, and it stayed PENDING.
Why: bug. Backbeat refuses these objects, so they stayed PENDING forever. Cloudserver and the old V2 code already skipped them.
Before: we kept the role the object was sent with, unless FORCE_USING_CONFIGURATION was set.
Example: say the bucket Role is now '...:role/crr-src,...:role/crr-dst-new'. We run with COMPLETED. 'docs/a.pdf' gets site sf, bucket backup-sf, role crr-dst-new, PENDING. Before, with V1 bucket replication rules and without FORCE_USING_CONFIGURATION, backbeat sent it again with the old role crr-dst.
Why: keeping the old bucket and role made backbeat fail with BadRole after a config change (S3UTILS-208). Each backend now gets its bucket and role from the current rule, so the flag has nothing left to do.
Before: we matched backends on the site only. NEW skipped the object, because the site already had a backend. COMPLETED with FORCE_USING_CONFIGURATION=true sent it to the new bucket, but replaced the backend for the old bucket.
Example: say r1 now uses site sf, bucket backup-sf-new. We run with NEW. 'docs/a.pdf' gets site sf, bucket backup-sf-new, PENDING, and keeps site sf, bucket backup-sf, COMPLETED. Before, NEW skipped it.
Why: since S3C 10.1, one site can have several buckets, so we match backends on site, bucket and role, not on the site only. A new bucket is a new destination, so NEW picks it. Before, overwriting the old backend lost the record of the copy in the old bucket.
Before: with V1 bucket replication rules, we wrote it to the top-level storageType of the object.
Why: the S3C 10.1 format has no top-level storageType: there is nowhere to write it.
Issue: S3C-11599, S3C-11596