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

[ASI-1013] Remove remaining IPFS from creator-node #2825

Merged
merged 36 commits into from
Apr 7, 2022
Merged

Conversation

theoilie
Copy link
Contributor

@theoilie theoilie commented Apr 2, 2022

Description

  • Add /content as a route alias for /ipfs so we can eventually remove the /ipfs route (note that this uses a regular expression, which slows down nginx, but that should be OK in the long run because it’s only 1 regexp and the /ipfs route will eventually be removed)
  • Remove IPFS container from creator node, along with its connection and config
  • Update to use renamed fileHasher functions and rename name some other variables that have IPFS
  • Change fileHasher in creator-node and mad-dog to use the new libs util instead of duplicating between the 2

Tests

Make sure you can manually access content from /content (I confirmed this locally). Automated tests are updated to pass as well.

How will this change be monitored? Are there sufficient logs?

Search logs for:

  • File contents don't their expected CID
  • [fileHasher - convertToBuffer()] Could not convert content into buffer

rickyrombo and others added 16 commits April 4, 2022 14:10
* Add linting flake8 plugin to ensure decorator order and route params in API declarations

* Remove __init__.py

* Revert "Remove __init__.py"

This reverts commit 2bd9b80.

* Modify visitor to make testing easier

* Disable plugin test, rely on visitor test instead

* Move to using local plugin

* Disable rule temporarily

* Fix bugs, add new rule, solve edge cases

* Enable linter

* add default case

* Add tests to vscode test config

* Made plugin more readable

* Move to subfolder

* Move to plugin.py

* Split to several files

* Update setup.cfg to point to plugin

* Make readme

* Fix order config

* Fix README typo

* Fix lint errors

* Use absolute imports

* Set root correctly
@theoilie
Copy link
Contributor Author

theoilie commented Apr 5, 2022

verified that test-creator-node is passing locally (it fails on Circle because Circle doesn't have libs linked). also verified upload flow and ran the full mad dog suite without any regressions so this is ready for review!

@theoilie theoilie marked this pull request as ready for review April 5, 2022 05:58
@theoilie
Copy link
Contributor Author

theoilie commented Apr 6, 2022

test-creator-node is passing on CI as well now that I got the libs PR through and consumed that version (mad dog is still passing locally), so this should be ready to go pending a review from any one of @SidSethi @dmanjunath @vicky-g

@theoilie theoilie changed the title Remove remaining IPFS from creator-node [ASI-1013] Remove remaining IPFS from creator-node Apr 6, 2022
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.

nice work! looks great to me, will let @SidSethi give the green light as well since he's primary reviewer

creator-node/scripts/run-tests.sh Show resolved Hide resolved
creator-node/test/dbManager.test.js Outdated Show resolved Hide resolved
creator-node/test/pollingTracks.test.js Outdated Show resolved Hide resolved
Copy link
Contributor

@SidSethi SidSethi left a comment

Choose a reason for hiding this comment

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

fantastic work once again! changes look great. only note is to make sure you run all maddog tests (not just the default one) and manually test against client locally. i'll send you a QA checklist to go against (we actually should have done this on the earlier IPFS commit since that included the majority of the changes)

thanks for adding the detail in PR description as well. one thing i think would be good to have is a documented release plan, listing out all the tests we will run on staging to ensure thorough coverage of every edge case. we want to cover read and write flows for metadata, images, track segments, and track 320kbps copy. i'm assuming you don't know what this means haha, any of dheeraj, vicky, or me can help out

but great job!

creator-node/package.json Show resolved Hide resolved
creator-node/package.json Show resolved Hide resolved
creator-node/src/routes/files.js Show resolved Hide resolved
mad-dog/src/tests/test_ipldBlacklist.js Show resolved Hide resolved
@gitguardian
Copy link

gitguardian bot commented Apr 7, 2022

⚠️ GitGuardian has uncovered 1 secret following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secret in your pull request
GitGuardian id Secret Commit Filename
3086103 Generic High Entropy Secret 032096e discovery-provider/integration_tests/tasks/test_anchor_program_indexer.py View secret
🛠 Guidelines to remediate hardcoded secrets
  1. Understand the implications of revoking this secret by investigating where it is used in your code.
  2. Replace and store your secret safely. Learn here the best practices.
  3. Revoke and rotate this secret.
  4. If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

Our GitHub checks need improvements? Share your feedbacks!

@theoilie
Copy link
Contributor Author

theoilie commented Apr 7, 2022

thanks @SidSethi! I addressed your and Dheeraj's comments and re-ran mad dog (results pasted below and follow up card here). there are a number of pre-existing mad dog issues, but should I merge anyway since this branch isn't causing any new issues?

Updated Testing Plan

  1. Checkout the branch and start the services (cd audius-protocol && git checkout theo-remove-ipfs3 && aup)
  2. Start the web client (cd audius-client/packages/web && scp -r remote-dev-instance:.audius ~/ && npm run start:dev:cloud)
  3. Sign up and verify changing your profile photo -- upload a new photo and look for the profile photo hash inside the payload of /metadata POST call in the Network tab. Then refresh the page and filter the Network tab for that hash. You should see valid requests for /ipfs/<your hash>/<dimensions> and 404s (for legacy purposes) for /ipfs/<your hash>
  4. Verify that you can also hit /content/<your hash>
  5. Verify uploading and playing track. Find the hash of the segments in the final /async_processing_status request and the hash of the cover artwork (the /tracks/metadata metadataMultihash and the/image_upload dirCID). Click "View track page" and do a hard refresh to make sure you see the right cover image hash loaded. Click Play to make sure you see the segment hashes loaded and hear the right audio

Before releasing to prod: push to staging and repeat steps 3-5 there

Mad Dog Results

Ran the following tests (in order) on master in one box and this branch on another box:

  • npm run start test-snapback
    • this branch: passed the second time, failed the first: Error: Failed to create new user for index 5 in 300000 at Timeout._onTimeout (/home/ubuntu/audius-protocol/mad-dog/src/helpers.js:236:29)
    • master: passed the second time, failed the first: Error: Failed to create new user for index 17 in 300000 at Timeout._onTimeout (/home/ubuntu/audius-protocol/mad-dog/src/helpers.js:236:29)
  • npm run start test-ursm 41
    • this branch: passed the first time
    • master: passed the first time
  • npm run start test-ursm-sat 46
    • this branch: passed the first time
    • master: passed the first time
  • npm run start test-blacklist 48 ❌ UPDATE 4/13 -- fixed! ✅
    • this branch: didn't try since master is failing, but it was getting past that error in an earlier run
    • master: errored the first time: TypeError: Cannot read property 'length' of undefined at LibsWrapper.getLibsUserInfo (/home/ubuntu/audius-protocol/service-commands/src/libs.js:413:16)
  • npm run start test-listencount 50 ✅ 👎
    • this branch: passed but took 33 minutes: Processed 1003 (solana: 802, solana anon: 138, base: 32, base anon: 31) in 1985954ms
    • master: I cancelled after >30 minutes of running slowly (not stalled, though)
  • npm run start test-ursm-nodes 52
    • this branch: didn't try since master is failing
    • master: errored the first time:
Building creator-node
Creating cn7_creator-node-db_1    ... done
Creating cn7_creator-node-redis_1 ... done
Creating cn7_creator-node_1       ...

ERROR: for cn7_creator-node_1  UnixHTTPConnectionPool(host='localhost', port=None): Read timed out. (read timeout=200)

ERROR: for creator-node  UnixHTTPConnectionPool(host='localhost', port=None): Read timed out. (read timeout=200)
An HTTP request took too long to complete. Retry with --verbose to obtain debug information.
If you encounter this issue regularly because of slow network conditions, consider setting COMPOSE_HTTP_TIMEOUT to a higher value (current value: 200).
Error: creator-node failed to start for the command: cd /home/ubuntu/audius-protocol; cd creator-node; mkdir -p compose/env/tmp/file-storage-7; . compose/env/tmp/shellEnv7.sh; docker-compose -f compose/docker-compose.yml up --build -d
    at ChildProcess.<anonymous> (/home/ubuntu/audius-protocol/service-commands/src/setup.js:85:11)
  • npm run start test 55
    • this branch: passed first time locally and on CI
    • master: passed first time

Error across multiple tests on both master and my branch (maybe this file was accidentally deleted at some point?): [_addUsers] Error fetching random image: ENOENT: no such file or directory, mkdir '/home/ubuntu/audius/audius-protocol/mad-dog/local-storage/tmp-imgs'

@SidSethi
Copy link
Contributor

SidSethi commented Apr 7, 2022

amazing, good to merge 👍

@theoilie theoilie merged commit 25c278e into master Apr 7, 2022
@theoilie theoilie deleted the theo-remove-ipfs3 branch April 7, 2022 15:47
@AudiusProject AudiusProject deleted a comment from linear bot Sep 11, 2023
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

6 participants