Repository navigation
S3UTILS-250 crrExistingObjects: wrong SITE_NAME leaves objects PENDING forever - #414
Conversation
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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## development/1.17 #414 +/- ##
====================================================
+ Coverage 45.34% 46.71% +1.36%
====================================================
Files 88 90 +2
Lines 6492 6741 +249
Branches 1366 1443 +77
====================================================
+ Hits 2944 3149 +205
- Misses 3502 3540 +38
- Partials 46 52 +6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Request changes. Thanks for this! The wrong-site guard and the cleanup script address the CERSAI case. I tested them end to end on two real S3C stacks with Backbeat replication between two sites, comparing this PR against the code before it (4b660d9):
- S3C 9.5.2.3 (CERSAI's version): CloudServer 9.2.24, Backbeat 9.0.25, metadata 8.23.0, Vault 7.84.0.
- S3C 10.1 (pre-release
10.1.0.0_pw6): CloudServer 9.4.4, Backbeat 9.5.1, metadata 9.17.0, Vault 7.94.0.
What works on both versions:
- Ticket reproduced. Before the PR, a wrong
SITE_NAMEleaves an already-replicated object PENDING (dr:COMPLETED,foo:PENDING). Re-running with the right site doesn't fix it. - The PR blocks the wrong site. The object is untouched and logged, including with V2 rules (Filter/Priority) on 10.1.
removeReplicationSiterepairs the object.- Dry run writes nothing; the real run gives
COMPLETED, and nothing is requeued. - It also works for objects created before replication was enabled (reset, then replicated with the right site), null versions, and a remaining PENDING site (which then completes).
- On 10.1 it keeps the per-backend
destination/role.
- Dry run writes nothing; the real run gives
- Your functional tests pass on the 9.5.2.3 stack (8/8).
Target branch. Thanks for retargeting to development/1.17. It merges cleanly into 1.17 and 1.18, and the PR merged into 1.18 passes the CRR unit tests (59/59). Bert-E will merge it forward to 1.19 and 1:
- Fix versions: Bert-E expects 1.17.16 / 1.18.3 / 1.19.3 / 1.20.0. S3UTILS-250 has none set yet.
- Conflicts: the integration branches
w/1.19andw/1conflict inCRR/ReplicationStatusUpdater.jsand its unit test, because of the V1/V2 split. - The remover doesn't work on their Arsenal (8.5.0 on 1.19,
@scality/arsenal8.6 on 1): R1–R4. 7 of the 22 remover unit tests fail on both, so those builds would fail. - Delivery: CERSAI (S3C 9.5.2.3) pins s3utils 1.17.1 and S3C 10.1 pins 1.18.0. Both need a Federation bump to pick up 1.17.16 / 1.18.3.
Inline comments:
Blocking:
- R1:
ReplicationSiteRemover.js:3.require('arsenal')doesn't resolve ondevelopment/1(w/1). - R2:
ReplicationSiteRemover.js:196.setReplicationStorageClass/setReplicationStorageTypewere removed in Arsenal 8.5+ (w/1.19,w/1). - R3:
ReplicationSiteRemover.js:200.updateMicroVersionId()without arguments writes a malformed ID on Arsenal 8.5+ (w/1.19,w/1). - R4:
ReplicationSiteRemover.js:192. The reset dropsisReplicaon Arsenal 8.5+ (w/1.19,w/1). - R5:
ReplicationStatusUpdater.js:172. On 9.5.x, replicas make the check reject a legit DR failback.
Should fix:
- R6:
ReplicationStatusUpdater.js:214. With mixed rules, the correct default site is rejected. - R7:
tests/unit/CRR/ReplicationStatusUpdater.js:973. The unit test hides R6. - R8:
ReplicationSiteRemover.js:200. On S3C 10.1, which gets this change through 1.18.3, requeueing an object that already exists at the destination gets stuck, because of the microVersionId format (S3UTILS-234). This predates the PR but affects the recovery paths it documents.
Minor:
- R9:
ReplicationSiteRemover.js:28. The remover can turn a replica into a PENDING object. - R10:
ReplicationStatusUpdater.js:537. Exit 0 when objects are skipped. - R11:
ReplicationSiteRemover.js:85. No guard onSITE_TO_REMOVE. - R12:
README.md:267. README cleanup scope.
FYI, outside this PR:
- s3utils CI uses workbench v0.8.0, where Backbeat replication can't complete:
- Vault's S3
allowFromis0.0.0.0/8(WKBCH-16, fixed in later workbench versions). - The populator is assigned raft sessions
0,1,2, but bucket data is on1,2,3(still in the latest release). - So the functional tests only check metadata.
- Vault's S3
autoRetryFailedCRR.jsprobably can't work on S3C. It relies on Backbeat/_/crr/status, which returns 404RouteNotFoundwithout MongoDB (disableAdditionalRoutes, routes). Seen with Backbeat 9.0.25; I didn't run the script itself.crrExistingObjects.js:108:stopis passed unbound, so SIGINT/SIGTERM throws instead of logging the resume markers.- On 10.1, the README's
TARGET_REPLICATION_STATUS=REPLICAre-sync matches nothing, because replicas havestatus ''.
I can share the E2E harness and workbench configs if you want to reproduce any of this.
| .setReplicationStorageType(storageType) | ||
| .setReplicationStatus(newStatus); | ||
| } | ||
| objMD.updateMicroVersionId(); |
There was a problem hiding this comment.
R8 [Should fix: S3C 10.1] The microVersionId format. This predates the PR (S3UTILS-234, fixed only in 1.19+), but it matters for the recovery paths this PR documents.
This now ships to S3C 10.1 through 1.18.3. On 1.17/1.18, this line and crrExistingObjects write a random 16-hex microVersionId.
- The mechanism. When the object already exists at the destination:
- Backbeat 9.5.1 sends
x-scal-micro-version-id: encode(microVersionId). - CloudServer 9.4.4 decodes it and compares it with the body.
- Backbeat 9.5.1 sends
- The format can't survive this. A random 16-hex value never survives encode/decode: 0/1000 (half come back different, half fail to decode).
- E2E on 10.1:
crrExistingObjectswith the right site andTARGET_REPLICATION_STATUS=COMPLETEDon an already-replicated object (the PR description's "requeued to its original site"):- The destination rejects it with
x-scal-micro-version-id header does not match body microVersionId. - The entry goes to replay, and the object stays PENDING (watched for 4 min).
- It reproduced on a V2 two-destination object too.
- In another run, Backbeat skipped the metadata put and completed, so the outcome isn't deterministic.
- The destination rejects it with
- Not affected: the cleanup itself, since its COMPLETED result triggers no replication.
- Affected: a remover result of
PENDING, or any later requeue of objects already at the destination.
Suggestion: since this ships on 1.18 for 10.1, backport S3UTILS-234 (6ca6cca: timestamp-ordered microVersionId with a random replicationGroupId) to both scripts.
There was a problem hiding this comment.
Agreed, it hits the recovery paths on 10.1. Tracked in S3C-11591, for both scripts. Until then, the README warns that on 10.1 a requeue of an object already at the destination can stay PENDING.
| cb(err); | ||
| return; | ||
| } | ||
| if (this._nUnknownSite > 0) { |
There was a problem hiding this comment.
R10 [Minor] When every object is skipped as unknown, the run still exits 0 and logs completed task for all buckets (crrExistingObjects.js:101-106; verified on both versions). The S3C oneshot service and the Zenko job pod both report success.
Suggestion: exit non-zero when _nUnknownSite > 0, or at least make this line as visible as the "completed" one.
| export MAX_UPDATES=10000 | ||
| ``` | ||
|
|
||
| # Remove a replication site from object metadata |
There was a problem hiding this comment.
R12 [Minor: docs] Two things worth adding:
- On 9.5: Backbeat 9.0 routes on the top-level
storageClass.- So a wrong run also makes the real site's processor re-send the full object, and the replica it writes on the destination carries the wrong site (
dr:REPLICA,foo:PENDING). - Running the script on the destination buckets too cleans those up.
- This is also why the object stays stuck: the status update is skipped as "already COMPLETED".
- So a wrong run also makes the real site's processor re-send the full object, and the replica it writes on the destination carries the wrong site (
- On 10.1: requeueing objects that already exist at the destination can get stuck because of the microVersionId format (R8). Backbeat 9.5.1 routes on PENDING backends, so the re-replication described above doesn't happen there.
Refs:
There was a problem hiding this comment.
Added both to the README, with a procedure: dry run, source then destination buckets on 9.5, final dry run, then crrExistingObjects for reset objects.
I checked the 9.5 part with Backbeat 9.1.4, which routes like 9.0/9.3. After a wrong run, a 1 KB object's replica was rewritten with foo=PENDING (still REPLICA). A 110 MB object was skipped: "source object version already COMPLETED". So it only re-sends under sourceCheckIfSizeGreaterThanMB (100 MB by default). That's in the note too.
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
|
Thanks for retargeting to
|
c76456a to
0604d2a
Compare
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
4361a4d to
447dda7
Compare
|
ping |
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 |
anurag4DSB
left a comment
anurag4DSB
left a comment
There was a problem hiding this comment.
Some minor stuff(specially for human error) , but otherwise LGTM
| */ | ||
| async _processBucket(bucket) { | ||
| this._bucketInProgress = bucket; | ||
| await this._checkSiteNotInRules(bucket); |
There was a problem hiding this comment.
[minor]
Please tell me if I understand this incorrectly. When I give the script several buckets, it looks at the replication rule of a bucket only when it gets to that bucket. It doesn't look at the rules of all the buckets first. So it can already change bucket "a" before it finds out, on bucket "b", that SITE_TO_REMOVE is a real site.
The dry run covers most of this: with the same bucket list, it stops on "b" before anything is written. The gap is a real run without that dry run first. Example: the real site is "dr", and I type SITE_TO_REMOVE=dr by mistake with DRY_RUN=false on buckets a,b. Bucket "a" has no StorageClass in its rule, so the script removes "dr" from every object in "a". Only when it reaches "b", whose rule says StorageClass=dr, does it stop. The final summary line doesn't print either, so I don't know how many objects it changed.
It only costs one GetBucketReplication call per bucket. Could the script read the rules of all the buckets at the start, before it changes any object, and still print the summary when it stops?
There was a problem hiding this comment.
Good catch. The script now logs the stats when it stops on an error, so you see how many objects it changed before the stop.
I kept the rules check per bucket. The dry run is the default, and it stops on "b" before anything is written. A real run without a dry run first is an operator choice, and the summary now shows what it changed.
| 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: ' |
There was a problem hiding this comment.
Again correct me if I mis understood, the script updates several objects at the same time: WORKERS of them, 10 by default(assuming). When one object has an unknown site, the run stops, but the objects that were already in progress at that moment still finish, and they get written with the bad site.
Check this -> It would be worth a test with 10 workers(if thats our production state), the run might on one object, and 9 other objects would still written with the bad site after it.
The message says "Objects processed before this point may already have this SITE_NAME". If the stop is on the first object, I would think nothing was written and skip the cleanup.
Could the final message give the count instead? For example something like: "This run wrote SITE_NAME=foo into 9 objects before it stopped. If foo is wrong, run removeReplicationSite in dry run on these buckets to find them." The script already has this count in _nUpdated when it prints the final message.
There was a problem hiding this comment.
You're right, workers already in progress can still write after the stop, at most WORKERS-1 objects.
I didn't add the count. _nUpdated only counts this run. If an earlier run already wrote the wrong site, it can say 0 while there's still something to clean, and the operator skips the cleanup. The dry run of removeReplicationSite finds every version with the site, whatever run wrote it, the in-flight ones included. So the message keeps pointing to it. On a first-object stop the dry run reports 0, and the cost is one dry run.
| it, stops, logs the object and the sites it compared against, logs the | ||
| bucket list, `KEY_MARKER` and `VERSION_ID_MARKER` to resume from that | ||
| point, and exits with code 1. Check `SITE_NAME`, then re-run from the | ||
| resume point with the right `SITE_NAME`, or with `ALLOW_NEW_SITE=true`. |
There was a problem hiding this comment.
The README says: if SITE_NAME was wrong, run removeReplicationSite, then re-run crrExistingObjects from the resume point.
But I think this is what happens: the objects before the resume point that got only the wrong site are reset to "never replicated" by removeReplicationSite. The re-run starts at the resume point, so it never goes
back to them, and they stay unreplicated. Is that right?
If so, could the README say to re-run from the start of the bucket list when removeReplicationSite reset objects? With the default TARGET_REPLICATION_STATUS=NEW, that's safe.
There was a problem hiding this comment.
Confirmed. I reproduced it end to end on 447dda7, on S3C 9.5.2.3 and 10.1:
SITE_NAME=fooonX,Y. X's never-replicated objects getfoo; the run stops on Y with resume list"Y".- The remover resets X's objects.
- Re-run from the resume point with
SITE_NAME=dr: X stays unreplicated, exit 0. - Re-run from the start of the bucket list: X replicates.
@nicolas2bert the stop message in ReplicationStatusUpdater.js gives the same advice ("Then re-run from the resume point…"), so it needs the fix too.
There was a problem hiding this comment.
Confirmed. I reproduced it end to end on 447dda7, on S3C 9.5.2.3 and 10.1:
SITE_NAME=fooonX,Y. X's never-replicated objects getfoo; the run stops on Y with resume list"Y".- The remover resets X's objects.
- Re-run from the resume point with
SITE_NAME=dr: X stays unreplicated, exit 0. - Re-run from the start of the bucket list: X replicates.
@nicolas2bert the stop message in ReplicationStatusUpdater.js gives the same advice ("Then re-run from the resume point…"), so it needs the fix too.
There was a problem hiding this comment.
Right, good catch. Objects that had only the wrong site are reset to "never replicated", and they are before the resume point. A re-run from the resume point never goes back to them.
Fixed in the README and in the stop message. If the cleanup reset objects, re-run from the start of the stopped run, with the same bucket list and markers. With the default TARGET_REPLICATION_STATUS=NEW it's safe: objects that already have the right site are skipped.
leif-scality
left a comment
leif-scality
left a comment
There was a problem hiding this comment.
some of Anurag's comments about the docs to fix
447dda7 to
a578e43
Compare
|
/create_integration_branches |
ConflictA conflict has been raised during the creation of I have not created the integration branch. Here are the steps to resolve this conflict: git fetch
git checkout -B w/1.19/bugfix/S3UTILS-250/crrExistingObjects-site-name origin/development/1.19
git merge origin/w/1.18/bugfix/S3UTILS-250/crrExistingObjects-site-name
# <intense conflict resolution>
git commit
git push -u origin w/1.19/bugfix/S3UTILS-250/crrExistingObjects-site-nameThe following options are set: create_integration_branches |
Integration data createdI have created the integration data for the additional destination branches.
The following branches will NOT be impacted:
You can set option The following options are set: create_integration_branches |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
The following options are set: create_integration_branches |
On arsenal 8.5 (development/1.19) the empty replicationInfo has no storageClass, so the reset leaves the field out instead of ''. - removeReplicationSite functional test: check storageClass is empty, '' or absent
|
@bert-e approve |
|
I have successfully merged the changeset of this pull request
The following branches have NOT changed:
This pull request did not target the following hotfix branch(es) so they
Please check the status of the associated issue S3UTILS-250. Goodbye nicolas2bert. The following options are set: approve, create_integration_branches |
crrExistingObjects: SITE_NAME validation
problem
crrExistingObjects never validated SITE_NAME. A wrong value adds a replication site that no backbeat processor handles -> the object stays PENDING forever, even when the real site replicated it. Re-running with the right SITE_NAME doesn't fix it. Seen at CERSAI (S3C 9.5.2.3).
new behavior
The script checks SITE_NAME when it has something reliable to compare against:
Otherwise the object is accepted.
At the first object with an unknown site, nothing is written for it and the run stops. The script logs the object, the sites it compared against, and the resume point (bucket list, KEY_MARKER, VERSION_ID_MARKER), then exits 1. No scattered skips, no full re-listing to find them.
NOTE:
ALLOW_NEW_SITE=truedisables the check, when objects must get a site they don't know yet: a DR failback with replicas written by S3C 9.5, or a new DR site with a new name on S3C."names X": a rule has StorageClass=X.
Only the "run stopped" rows are new. Before, these objects got the wrong site and stayed PENDING forever. All other rows behave as before.
(1) The rule changed since the object was written. If lab-9512 was removed from the federation config, nobody processes it: not caught (2).
(2) Rule with StorageClass (always on ARTESCA, optional on S3C): the rule names the destination site, so the script checks SITE_NAME against it. Rule without StorageClass (S3C): the destination is the default replication site, set in the S3C federation config (env_replication_endpoints). The script can't read that config. So for an object never replicated under such a rule, or a site removed from that config, a wrong value is not caught.
removeReplicationSite: cleanup of the broken metadata
what it does
New s3utils script. It removes a wrong site from the replication info of every object version in the given buckets, delete markers included, then recomputes the replication status from the remaining sites:
It refuses to remove a site that a bucket rule names in StorageClass, since that's a real destination, and stops.
FORCE_REMOVE_CONFIGURED_SITE=trueoverrides it.