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.

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

Only delete disk on secondaries #4158

Merged
merged 6 commits into from
Oct 24, 2022
Merged

Conversation

theoilie
Copy link
Contributor

Description

Prevents deleting data from disk from nodes that were once a user's primary. This is to prevent any damage from the trackBlockchainId bug from being made permanent.

Tests

  1. Use A seed to create a user and upload a track
  2. Reconfig the user off of their primary (ex: http://cn3_creator-node_1:4002/manually_update_replica_set?userId=2&newPrimarySpId=1&newSecondary1SpId=4&newSecondary2SpId=2)
  3. Wait for orphaned data recovery to clean up the user's data, and make sure it fails the forceWipe sync with the reason 'abort_node_used_to_be_primary'
  4. Perform the same reconfig for a non-primary, and it should successfully wipe the orphaned node

Note that we don't set self SP ID (spID env var), so the above doesn't technically work out of the box locally. I tested with a debugger to verify each step in both scenarios, though (primary and secondary).

Monitoring - How will this change be monitored? Are there sufficient logs / alerts?

Monitor for syncs failing with abort_node_used_to_be_primary. Some should fail with this message, but other forceWipe=true syncs should pass (spot check some failures to make sure the orphaned node was a primary at some point according to the /users/history route).

Copy link
Contributor

@dmanjunath dmanjunath left a comment

Choose a reason for hiding this comment

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

some comments regarding the limitations on the discovery query side. currently there's a 500 record limit so we can probably lift that requirement for this query and make it return only certain columns cause we don't need the entire user record. but for now i just left some comments to make it work with our existing discovery query

@theoilie
Copy link
Contributor Author

some comments regarding the limitations on the discovery query side. currently there's a 500 record limit so we can probably lift that requirement for this query and make it return only certain columns cause we don't need the entire user record. but for now i just left some comments to make it work with our existing discovery query

thanks for the suggestions. I made some changes to bias toward not deleting when the history is too large. since DN changes will take longer to roll out, I'm guessing this is what you'd prefer for now? I agree it would be nice to have a route that just returns select columns by wallet though

@dmanjunath
Copy link
Contributor

yeah lets roll with this approach for now @theoilie. i actually checked the response size for a user with a lot of playlists and 500 records was like 6 mb 😬 . how would you feel about keeping it at 100 for now and we can either modify the route or make a new one as a follow up?

@theoilie
Copy link
Contributor Author

yeah lets roll with this approach for now @theoilie. i actually checked the response size for a user with a lot of playlists and 500 records was like 6 mb 😬 . how would you feel about keeping it at 100 for now and we can either modify the route or make a new one as a follow up?

ah yeah I was worried about size. so like just skip deleting for any user >100 for now? that might actually hold us over until the root bug is resolved and then maybe we won't need this check at all

@dmanjunath
Copy link
Contributor

yes that sounds great

@theoilie theoilie merged commit 772c5a9 into master Oct 24, 2022
@theoilie theoilie deleted the theo-gate-deletion-on-primary branch October 24, 2022 22:26
dmanjunath pushed a commit that referenced this pull request Oct 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants