ozone icon indicating copy to clipboard operation
ozone copied to clipboard

HDDS-11290. Container scanner should keep scanning after non-fatal errors

Open errose28 opened this issue 5 months ago • 2 comments

What changes were proposed in this pull request?

Motivation

In order to build the merkle tree of all data in the container, the scanner should not exit after the first issue it encounters like it does currently. The scanner should track and return all errors that it sees, and only stop the scan on fatal errors that prevent further scanning of the container, like DB access errors.

This PR is a pre-requisite to HDDS-10374. It does not actually generate a merkle tree during the scan and is also not testing this functionality. It sets up HDDS-10374 to be an easy drop in to the scanner which will allow the focus of that change to be testing of merkle tree generation.

Primary Changes

Previously ScanResult was an object that encapsulated a singe error. This was the first error the scanner saw which would abort the scan. This change decouples the ScanResult from the errors, which are now represented by a list ofContainerScanErrors in the ScanResult.

  • ScanResult is a general interface that can represent a data or metadata scan. It can be logged by entities like the ContainerLogger which do not care where the unhealthy result came from.
  • MetadataScanResult is a ScanResult implementation produced from a container metadata scan.
    • This scan will not produce a merkle tree since it does not check data.
  • DataScanResult extends MetadataScanResult by adding a merkle tree representing the data that was scanned.
    • All data scans begin with a metadata scan, and then proceed to scan the data only if the metadata scan succeeds.
  • The datanode application log will only log the first error encountered unless debug logging is enabled, then all errors will be logged.
    • The container log does not have this option to keep it from rolling off.

Secondary Changes

  • General cleanup of KeyValueContainerCheck internals were done since this PR already required invasive changes in this area.
  • Fixed a bug from #5485/HDDS-9005 where the DB was being used to check for container deletion during a scan instead of the container state in memory.
    • This would fail to detect a schema v2 container which is completely lost but not actually deleted. The container would remain in the datanode's memory as healthy.
    • Since the memory state is updated first and our container object will still be valid even after being removed from the ContainerSet, we can use the DELETED state as the deletion check which is both safer and simpler.
  • When a container is deleted during a scan, this is indicated with a different field in the ScanResult. It is no longer considered an error and will not cause the ScanResult to be unhealthy.
  • Container log checks are more stringent now making sure that the error logged is actually for the expected container ID.
    • Since the tests re-use the same cluster, one could have passed if a previous test logged the failure type this test was expecting.

Notes to Reviewers

It is probably best to review the scan flow end-to-end instead in addition to just viewing the diff.

What is the link to the Apache JIRA

HDDS-11290

How was this patch tested?

The scanner has a lot of existing tests that should all pass ensuring no regressions:

  • TestKeyValueContainerCheck
  • TestKeyValueHandlerWithUnhealthyContainer
  • Test{Background,OnDemand}Container{Data,Metadata}Scanner
  • Test{Background,OnDemand}Container{Data,Metadata}ScannerIntegration

New tests added for detecting multiple errors will be added in HDDS-11471 since this already a large PR.

errose28 avatar Aug 27 '24 23:08 errose28