Retry the setup of an ingestion reader when it fails - #2806
Conversation
Hello benzekrimaha,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 3 files with indirect coverage changes
@@ Coverage Diff @@
## development/9.5 #2806 +/- ##
===================================================
- Coverage 75.77% 75.64% -0.14%
===================================================
Files 200 200
Lines 13922 13925 +3
===================================================
- Hits 10549 10533 -16
- Misses 3363 3382 +19
Partials 10 10
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 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 |
5ad48bb to
bd6999f
Compare
| error: err, | ||
| }); | ||
| // the source may have been removed from the configuration | ||
| // while its setup was in progress |
There was a problem hiding this comment.
this comment is misleading: it is not that it "may have been removed", it is certain that is was removed c.f. the beginning of the function:
const newReaders = this.logReadersUpdate;
this.logReadersUpdate = [];
→ here you want to explain why you re-add it
| // while its setup was in progress | |
| // Setup failures are usually transient (source unreachable, | |
| // invalid credentials...): queue the reader again so that it | |
| // is retried on the next cycle |
There was a problem hiding this comment.
Reworded to say why the reader is queued again.
One precision : that comment was about _ingestionSources, the registry of configured sources, not about logReadersUpdate which the function drains at the top. Those are two different structures, and the guard reads the former.
It was imprecise for another reason though. _closeLogState only deletes the _ingestionSources entry when it finds the reader in logReadersUpdate or in logReaders, and while its setup is in progress the reader is in neither, so a source removed right then still leaves its entry behind. What the guard actually catches is a reader that is no longer the one registered for its bucket, which is what the new comment says.
| // Setup failures are usually transient (source unreachable, | ||
| // invalid credentials...). Queue the reader again so that it | ||
| // is retried on the next cycle: `applyUpdates` only refreshes | ||
| // sources that are already registered, so a reader dropped | ||
| // here would never be set up again. |
There was a problem hiding this comment.
not relevant to the code: the old comment was -and still is- correct, we don't add this one to logReaders and continue setting up others...
| // Setup failures are usually transient (source unreachable, | |
| // invalid credentials...). Queue the reader again so that it | |
| // is retried on the next cycle: `applyUpdates` only refreshes | |
| // sources that are already registered, so a reader dropped | |
| // here would never be set up again. | |
| // if setup fails for a log reader, don't add it to `logReaders` | |
| // log the error and continue setting up others |
| const zenkoBucket = logReader.getTargetZenkoBucketName(); | ||
| this.log.error('error setting up log reader, retrying later', { | ||
| method: 'IngestionPopulator._setupUpdatedReaders', | ||
| zenkoBucket, |
There was a problem hiding this comment.
I think we should log the location instead of the bucket, seems more appropriate - or in addition to the bucket, for consistency with other logs in this file:
| const zenkoBucket = logReader.getTargetZenkoBucketName(); | |
| this.log.error('error setting up log reader, retrying later', { | |
| method: 'IngestionPopulator._setupUpdatedReaders', | |
| zenkoBucket, | |
| this.log.error('error setting up log reader, retrying later', { | |
| method: 'IngestionPopulator._setupUpdatedReaders',) | |
| zenkoBucket: logReader.getTargetZenkoBucketName(), | |
| location: logReader.getLocationConstraint(), |
There was a problem hiding this comment.
Done, location sits next to zenkoBucket now. I kept the zenkoBucket const since the guard right below reads it.
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
|
not the right target branch: no point fixing so far back in time: from 9.5 is enough |
Branches have divergedThis pull request's source branch To avoid any integration risks, please re-synchronize them using one of the
Note: If you choose to rebase, you may have to ask me to rebuild |
When a reader's setup fails (typically the getRaftId call to the source), it was dropped from the pending list and never set up again: the reader stays registered in _ingestionSources, so applyUpdates considers the bucket already handled and only refreshes it. The bucket was therefore left out of ingestion until the pod was restarted. Queue the reader again instead, so it is retried on the next cycle. This follows what _processLogReaderEntries already does with read failures, and keeps the other locations ingesting, which restarting the pod on a failed healthcheck would not. The reader is only queued again if it is still the one registered for its bucket, so a source removed or replaced in the meantime is not brought back. Issue: BB-846
bd6999f to
2f9c7f5
Compare
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
|
Retargeted to On the branch prefix: BB-846 is already typed Bug in Jira, only the branch name says otherwise. Renaming it means closing this PR and opening a new one, which also moves the base of #2821 stacked on top, so I left the branch name as is. |
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 |
|
What happens when the location is deleted? Please confirm there no risk of path or race condition where we would keep retrying to setup a location which was removed... |
A reader whose setup is in flight is in neither `logReaders` nor `logReadersUpdate`, so `_closeLogState` found nothing to remove and left its `_ingestionSources` entry behind. The reader then registered itself once its setup completed: on failure it was queued again, and on success it became active and could process a batch on a location that no longer exists. Unregister the source unconditionally, which makes `_ingestionSources` authoritative, and hoist the guard above the error branch so that it covers the success path as well. Issue: BB-846
|
Fixed here as suggested, in a second commit. |
delthas
left a comment
delthas
left a comment
There was a problem hiding this comment.
LGTM high level, but I'm not very familiar with that part of the code, so needs a proper second review 😛
| }); | ||
|
|
||
| it('should activate a log reader once its setup succeeds', done => { | ||
| const logReaderMock = createLogReaderMock(WORKING_BUCKET, null); |
There was a problem hiding this comment.
nit: calling createLogReaderMock() is not very readable, the usual pattern is to setup a logReader in the fixture (beforeEach),and let each test setup expectations (logReader.setup.yieldsAsync(setupError))
Of you stick with this form to reduce dedup, than should the next 2 lines as well into that helper
function createLogReaderMock(zenkoBucket, setupError) {
const logReader = sinon.createStubInstance(IngestionReader);
logReader.getTargetZenkoBucketName.returns(zenkoBucket);
logReader.setup.yieldsAsync(setupError);
ip._ingestionSources[zenkoBucket] = logReader;
ip.logReadersUpdate = [logReader];
return logReader;
}(mixing both approaches makes it a bit weird)
There was a problem hiding this comment.
or even just just use the helper to create/add the reader ; but keep the useful mock call (setup) in the test
function addLogReader(zenkoBucket) {
const logReader = sinon.createStubInstance(IngestionReader);
logReader.getTargetZenkoBucketName.returns(zenkoBucket);
ip._ingestionSources[zenkoBucket] = logReader;
ip.logReadersUpdate = [logReader];
return logReader;
}
it(....) {
addLogReader("bucket1").setup.yieldsAsync(errors.InternalError);
}In addition to making the mocks more explicit, it would also make the tests more readable, by removing extraneous list modifications - like the following, where the test needs to manually edit only to simulate a race condition (nothing needed for simple case):
// should not queue a log reader again when its setup fails and its source is no longer configured
createLogReaderMock("bucket1")
delete ip._ingestionSources["bucket1"]; // remove from the config| }); | ||
|
|
||
| it('should queue a log reader again when its setup fails', done => { | ||
| const logReaderMock = |
There was a problem hiding this comment.
line break not needed I guess, fits within max column in our prettier config?
(same in following tests)
| }); | ||
| }); | ||
|
|
||
| describe('removing a source while its setup is in flight', () => { |
There was a problem hiding this comment.
this describe section is weird: this is still testing _setupUpdatedReaders()
There was a problem hiding this comment.
it tests the interaction between _setupUpdatedReaders and _closeLogState, but is it not redundant?
e.g. if you validate on one side the way _setupUpdatedReaders behaves (vs the various contents of _ingestionSources / logReadersUpdate) and the way _closeLogState updates these : then there is nothing left.... if however you want a more "integrated" test, should use the other methods to setup the readers (like applyUpdates and/or addNewLogSource)
| // its source may have been removed or replaced in the meantime. It | ||
| // must then neither be activated nor retried. | ||
| if (this._ingestionSources[zenkoBucket] !== logReader) { | ||
| return cb(); |
There was a problem hiding this comment.
should add a log message (just info is enough), would help to troubleshoot if something ever happens in this corner case...
| const FAILING_BUCKET = 'failing-zenko-bucket'; | ||
| const WORKING_BUCKET = 'working-zenko-bucket'; |
There was a problem hiding this comment.
nit: nothing working or failing for these buckets. Only ingestion is failing, not the buckets
putting the name here (which may be needed because the mocks are hidden deep down c.f. https://github.com/scality/backbeat/pull/2806/changes#r3880675313) gives a false impression there is something, where in fact it is just the ingestion setup mock which fail...
|
should this land on |
4 participants
When a reader's setup fails (typically the getRaftId call to the source), it was dropped from the pending list and never set up again: the reader stays registered in _ingestionSources, so applyUpdates considers the bucket already handled and only refreshes it. The bucket was therefore left out of ingestion until the pod was restarted.
Queue the reader again instead, so it is retried on the next cycle. This follows what _processLogReaderEntries already does with read failures, and keeps the other locations ingesting, which restarting the pod on a failed healthcheck would not.
The reader is only queued again if its source is still configured, as it may have been removed while its setup was in progress.
Issue: BB-846