Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

satellite/audit: do not fail or contain nodes for audited segments that are not piece-hash-verified #3161

Merged
merged 5 commits into from
Oct 7, 2019
Merged

satellite/audit: do not fail or contain nodes for audited segments that are not piece-hash-verified #3161

merged 5 commits into from
Oct 7, 2019

Conversation

mobyvb
Copy link
Member

@mobyvb mobyvb commented Oct 2, 2019 •
edited by ethanadams

What:
When a node is audited during verify or reverify, if the segment it is being audited for has not been piece hash verified (i.e. it was uploaded before #2690), we want to behave differently on an audit failure. We still want to remove the node from the segment, but we do not want to penalize it. We also do not want to add it into containment mode.

Why:
https://storjlabs.atlassian.net/browse/V3-2706

Please describe the tests:

  • Test 1:
  • Test 2:

Please describe the performance impact:

Code Review Checklist (to be filled out by reviewer)

  • Does the PR describe what changes are being made?
  • Does the PR describe why the changes are being made?
  • Does the code follow our style guide?
  • Does the code follow our testing guide?
  • Is the PR appropriately sized? (If it could be broken into smaller PRs it should be)
  • Does the new code have enough tests? (every PR should have tests or justification otherwise. Bug-fix PRs especially)
  • Does the new code have enough documentation that answers "how do I use it?" and "what does it do?"? (both source documentation and higher level, diagrams?)
  • Does any documentation need updating?
  • Do the database access patterns make sense?

@mobyvb mobyvb added the Request Code Review Code review requested label Oct 2, 2019
@mobyvb mobyvb requested a review from a team October 2, 2019 18:46
@cla-bot cla-bot bot added the cla-signed label Oct 2, 2019
@ghost ghost requested review from navillasa and thepaul and removed request for a team October 2, 2019 18:46
@VinozzZ VinozzZ requested a review from a team October 4, 2019 14:30
@ghost ghost removed their request for review October 4, 2019 14:30
ethanadams
ethanadams approved these changes Oct 4, 2019
View reviewed changes
Copy link
Collaborator

@ethanadams ethanadams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. The defers seem a little awkward, but I still think it is easier to read than the alternatives.

@mobyvb mobyvb added the Reviewer Can Merge If all checks have passed, non-owner can merge PR label Oct 7, 2019
navillasa
navillasa approved these changes Oct 7, 2019
View reviewed changes
Copy link
Contributor

@navillasa navillasa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks right to me

@mobyvb mobyvb merged commit e1b7d01 into master Oct 7, 2019
@mobyvb mobyvb deleted the green/audit-fail-validation branch October 7, 2019 20:06
bryanchriswhite added a commit that referenced this pull request Oct 9, 2019
@bryanchriswhite
Merge remote-tracking branch 'storj/master' into team-literally-anyth…
fa4547a 
…ing/track-monkit-func-names-used

* storj/master: (62 commits)
  Fix typo in libuplink docs. (#3222)
  satellite/audit: fix audit panic (#3217)
  installer/windows: checkbox for opening SNOboard after installation (#3212)
  satellite/console: Removed 'user not found' message on password reset request (#3184)
  satellite/audit: add mutex to pieceHashesVerified map (#3214)
  satellite/overlay: do not include gracefully exiting nodes in node selection (#3211)
  cmd/storagenode: enable migration of configs of different types (#3189)
  satellite/gracefulexit: Add protobuf definitions for communication between storage node and satellite (#3201)
  satellite/metainfo: remove Iterate from service (#3196)
  satellite/overlay: remove overlay.IsVetted (#3203)
  Fix jenkins builds of storagenode console. (#3208)
  installer/windows: add to private Jenkins build (#3206)
  storagenode/signature: Reject uploads with a timestamp too far in the future (#3194)
  satellite/audit: Add piece hash verified to log messages (#3204)
  storagenode: embed the console into the binary and makefile (#3164)
  Update links to documentation (#3198)
  Storage Node Dashboard shortcut opens the SNOboard (#3171)
  satellite/accounting: implement tally as an observer (#2992)
  satellite/gracefulexit: add transfer queue for pieces (#3174)
  satellite/audit: do not fail or contain nodes for audited segments that are not piece-hash-verified (#3161)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ethanadams ethanadams ethanadams approved these changes

@navillasa navillasa navillasa approved these changes

@thepaul thepaul Awaiting requested review from thepaul

@littleskunk littleskunk Awaiting requested review from littleskunk

Assignees
No one assigned
Labels
cla-signed Request Code Review Code review requested Reviewer Can Merge If all checks have passed, non-owner can merge PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@mobyvb @ethanadams @navillasa