Details

Reviewers

Group Reviewers

Commits

rDSTOfd29fcbecf5f: storage*: Hex encode content hashes in HashCollision exception

Summary

This encodes the collision's referenced colliding hashes into hex hashes.
This does the encoding at HashCollision instanciation time.

This provides a content_colliding_hashes method to retrieve the hashes as a dict of bytes.
This is mainly for client which captures the exception and wants to do something with it.
For example the replayer does capture the colliding hashes, log them and put them aside to reapply the transaction without those.

Note that it also moves the HashCollision exception to the swh.storage.exc module.

Related to T2332#42793

Impact:

swh.journal: retry behavior which puts aside colliding hashes D2874

Test Plan

tox

Diff Detail

Repository

rDSTO Storage manager

Branch

master

Lint

No Linters Available

Unit

No Unit Test Coverage

Build Status

Buildable 11314
Build 17109: tox-on-jenkins	Jenkins
Build 17108: arc lint + arc unit

Event Timeline

ardumont created this revision.Mar 24 2020, 1:34 PM

Herald added a reviewer: Reviewers. · View Herald TranscriptMar 24 2020, 1:34 PM

I understand this is to work around the sentry issue, but I think it's a bad idea. We don't want parts of the API to use hex encoding while everything else uses bytes.

What about using log.error instead (which is caught by sentry_sdk just like exceptions)?

In D2872#69249, @vlorentz wrote:

I understand this is to work around the sentry issue, but I think it's a bad idea. We don't want parts of the API to use hex encoding while everything else uses bytes.

What about using log.error instead (which is caught by sentry_sdk just like exceptions)?

I think there's general value in keeping exception arguments inambiguous and human-readable.

Having to fish out and copy/paste a repr'd bytes object is really annoying when you want to quickly check whether something is in the database or not, for instance. Also, I don't trust our exception handling utilities (be that logging, sentry, serialization in the RPC protocol, ...) to handle arbitrary bytes unharmed.

As for your proposal, we'd need to call log.error and explicitly decode the arguments in all places which can raise a HashCollision; that doesn't sound very appealing

Instead of changing all calls to HashCollision to convert the arguments to hex, we could:

convert the hashes on initialization of the exception
add a method to pull the "bytes" version of the hashes if we need it later (e.g. for processing, in the journal)

ardumont mentioned this in T2332: Analyze hash collisions.Mar 24 2020, 3:04 PM

ardumont edited the summary of this revision. (Show Details)Mar 24 2020, 3:11 PM

In D2872#69252, @olasd wrote:

Instead of changing all calls to HashCollision to convert the arguments to hex, we could:

convert the hashes on initialization of the exception

add a method to pull the "bytes" version of the hashes if we need it later (e.g. for processing, in the journal)

Something like:

class HashCollision(Exception):
    """Exception raised when a content collides in a storage backend

    """
    def __init__(self, algo, hash_id, colliding_contents, *args, **kwargs):
        super().__init__(*args, **kwargs)
        self.algo = algo
        self.hash_id = hash_to_hex(hash_id)
        self.colliding_contents = [content_hex_hashes(c)
                                   for c in colliding_contents]

    def colliding_contents(self) -> Dict[str, bytes]:
        return [content_bytes_hashes(c) for c in self.colliding_contents]

In D2872#69255, @ardumont wrote:
In D2872#69252, @olasd wrote:

Instead of changing all calls to HashCollision to convert the arguments to hex, we could:

convert the hashes on initialization of the exception

add a method to pull the "bytes" version of the hashes if we need it later (e.g. for processing, in the journal)

Something like:
class HashCollision(Exception):
    """Exception raised when a content collides in a storage backend

    """
    def __init__(self, algo, hash_id, colliding_contents, *args, **kwargs):
        super().__init__(*args, **kwargs)
        self.algo = algo
        self.hash_id = hash_to_hex(hash_id)
        self.colliding_contents = [content_hex_hashes(c)
                                   for c in colliding_contents]

    def colliding_contents(self) -> Dict[str, bytes]:
        return [content_bytes_hashes(c) for c in self.colliding_contents]
?

Yeah, without the attribute name clash :)

Build was aborted

Link to build: https://jenkins.softwareheritage.org/job/DSTO/job/tox/1075/
See console output for more information: https://jenkins.softwareheritage.org/job/DSTO/job/tox/1075/console

Harbormaster failed remote builds in B11313: Diff 10236!Mar 24 2020, 3:45 PM

convert the hashes on initialization of the exception
add a method to pull the "bytes" version of the hashes for clients (e.g. journal)

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DSTO/job/tox/1076/
See console output for more information: https://jenkins.softwareheritage.org/job/DSTO/job/tox/1076/console

Harbormaster failed remote builds in B11314: Diff 10237!Mar 24 2020, 4:02 PM

ardumont edited the summary of this revision. (Show Details)Mar 24 2020, 4:07 PM

Fix missing adaptations on test_retry module

ardumont edited the summary of this revision. (Show Details)Mar 24 2020, 4:12 PM

ardumont edited the test plan for this revision. (Show Details)

Add missing revert in test_utils

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DSTO/job/tox/1077/
See console output for more information: https://jenkins.softwareheritage.org/job/DSTO/job/tox/1077/console

Harbormaster failed remote builds in B11317: Diff 10240!Mar 24 2020, 4:13 PM

Build is green
See https://jenkins.softwareheritage.org/job/DSTO/job/tox/1078/ for more details.

Harbormaster completed remote builds in B11318: Diff 10241.Mar 24 2020, 4:19 PM

In D2872#69251, @olasd wrote:

I think there's general value in keeping exception arguments inambiguous and human-readable.

Having to fish out and copy/paste a repr'd bytes object is really annoying when you want to quickly check whether something is in the database or not, for instance. Also, I don't trust our exception handling utilities (be that logging, sentry, serialization in the RPC protocol, ...) to handle arbitrary bytes unharmed.

sold!

ardumont edited the summary of this revision. (Show Details)Mar 24 2020, 6:11 PM

ardumont mentioned this in D2874: journal.replay: Migrate to latest HashCollision change.Mar 24 2020, 6:20 PM

I'm not sure about putting content_hex_hashes/content_bytes_hashes in utils, but I don't really have a better suggestion.

I've suggested one change inline.

swh/storage/exc.py
46–47	That's compatibility with the `BaseException` class, rather than backwards-compatibility. I guess you could just call `super().__init__(algo, hash_id, colliding_contents)` instead. I guess this would make the `swh.storage.api.client` de-serialization work with no change (except the import path).

This revision is now accepted and ready to land.Mar 24 2020, 6:27 PM

ardumont added inline comments.Mar 24 2020, 6:38 PM

swh/storage/exc.py
46–47	Ok, it sounded reasonable but computer says no. except if i change the constructor to: self.algo = algo self.hash_id = hash_to_hex(hash_id) self.colliding_contents = [content_hex_hashes(c) for c in colliding_contents] super().__init__(self.algo, self.hash_id, self.colliding_contents) I guess this would make the swh.storage.api.client de-serialization work with no change (except the import path). Yes. I found the api_client code change quite clearer though.