File storage directory nesting #1056

dmanjunath · 2020-11-12T15:28:16Z

Trello Card Link

https://trello.com/c/dyViGxGG/1654-track-upload-errors

Description

The /file_storage directory is susceptible to hash collisions in ext4 file systems using the half md5 hashing algorithm because we write all files to one directory. Moving to a nested directory model should alleviate these concerns.

Services

Does it touch a critical flow like Discovery indexing, Creator Node track upload, Creator Node gateway, or Creator Node file system?

Delete an option.

🚨 Yes, this touches how files are stored in the file system

How Has This Been Tested?

Created an account, updated my profile picture and cover photo, uploaded a track with the old file storage system. Fetch to metadata and track segment via the ipfs gateway. Fetch a dir image via the ipfs gateway
Applied the new file storage system and uploaded more tracks and updated my profile picture and cover photo again. Fetch to metadata and track segment via the ipfs gateway. Fetch a dir image via the ipfs gateway
Used the dapp to play the tracks, view the track cover art, my profile picture and cover photo
Make sure a sync was triggered and verify clock state for both nodes was the same in the db
Verify the number of files in both volumes were the same and the directory structures were identical by listing out all the files, sorting and doing running the diff command
Disable fetching through IPFS for sync, only other cnode gateways
Upload another track to verify that sync triggered and fetched all files via the gateways
Randomly sample and fetch metadata and track segments via the ipfs gateway
Randomly sample and fetch dir images via the ipfs gateway
Swap the primary between the two nodes, upload another track and hit the ipfs gateway to fetch the content again

creator-node/scripts/run-tests.sh

raymondjacobson

This seems pretty good to me. Maybe we can test together?

creator-node/src/diskManager.js

creator-node/src/fileManager.js

dmanjunath · 2020-11-14T03:57:21Z

@raymondjacobson @SidSethi @roneilr @hareeshnagaraj i think this is ready for a final review. I tested a bunch locally.

raymondjacobson

Let's get it on staging! I'm down to very thoroughly test after.

dmanjunath · 2020-11-15T03:08:18Z

Let's get it on staging! I'm down to very thoroughly test after.

Yeah I'm planning to do this tomorrow. We can push the hotfix tag onto staging and test with it.

creator-node/src/fileManager.js

hareeshnagaraj

Looks great, just some minor suggestions - can chat offline if easier

creator-node/src/fileManager.js

hareeshnagaraj · 2020-11-16T20:02:01Z

creator-node/src/fileManager.js

+    // regex match to check if a directory or just a regular file
+    // if directory will have both outer and inner properties in match.groups
+    // else will have just outer
+    const matchObj = DiskManager.extractCIDsFromFSPath(expectedStoragePath)


can this get named better? matchObj vs dirMatchObj or something more descriptive

I didn't call it dirMatchObj cause it can be a a non-dir file. i guess we could call it cidMatchObj?

non-blocking imo, if it works thats more impt

creator-node/src/diskManager.js

hareeshnagaraj · 2020-11-16T20:10:32Z

creator-node/src/diskManager.js

+   */
+  static computeBasePath (fsDest) {
+    if (!fsDest || fsDest.length < 4) throw new Error(`Please pass in a valid fsDest to computeBasePath. Passed in ${fsDest}`)
+    if (fsDest.includes('/')) throw new Error('Cannot pass in a directory path into this function, please pass in the leaf dir or file name')


if this is guaranteed to always be a CID, can we improve this validatio by checking that format explicitly? i think theres helpers somewhere

not a blocker but if easy to do might be worth it

it's not always a CID (eg the case of a the original track artifacts uploaded it's a folder with a UUID name). also i don't want to add extra validation in the event we want to generalize this function because that may impact backwards compatibility

makes sense agreed

oh i had a similar comment about this

so this doesn't accept uuid's anymore. but should we do CID validation, i'm honestly not convinced one way or the other @hareeshnagaraj @SidSethi

SidSethi

this is awesome. thanks for doing and testing man
had some comments/questions but nothing too major

creator-node/src/diskManager.js

SidSethi · 2020-11-16T20:50:57Z

creator-node/src/fileManager.js

+    const matchObj = DiskManager.extractCIDsFromFSPath(expectedStoragePath)
+
+    // if this is a directory, make it compatible with our dir cid gateway url
+    if (matchObj && matchObj.isDir && matchObj.outer && fileNameForImage) {


if isDir = true and fileNameForImage = null shouldn't we throw an error

it's confusing but we actually don't want to throw an error. there's a chance we could get the multihash via IPFS and store it in the expectedStoragePath. the only thing this if statement does is format the ipfs gateway urls in case we need to fall back to the gateways

maybe i'm missing something but this seems wrong? also unclear what this means

there's a chance we could get the multihash via IPFS and store it in the expectedStoragePath

i don't think the error case i mentioned is possible given how this fn is called so its fine, just want to make sure i am reading code correctly

creator-node/src/fileManager.js

creator-node/src/diskManager.test.js

SidSethi · 2020-11-16T21:03:23Z

creator-node/src/diskManager.test.js

+  it('Should return null if extractCIDsFromFSPath is passed in no valid CID', function () {
+    const path = '/file_storage/files/QMcidhere'
+    const matchObj = DiskManager.extractCIDsFromFSPath(path)
+    assert.deepStrictEqual(matchObj, null)


why did you choose to have it return null instead of throw error in this case? am concerned we might see some silent failures if we don't explicitly throw

that's just the api i thought of. if you feel strongly i can change but didn't seem necessary to have a try/catch around this when we can just have an if check

so we don't necessarily have to try-catch this in each caller since they're all wrapped in handleResponse anyway, my concern would be if the function occasionally returns null and we have no awareness. i guess why return null if there's no valid scenario where that should happen. throwing seems safer 🤷
but will leave up to you, not huge deal

lol def ignore for now

creator-node/src/fileManager.js

hareeshnagaraj

Leaving final approval to @SidSethi looks good from my end - added some questions but no blockers

SidSethi · 2020-11-16T22:59:15Z

creator-node/src/diskManager.js

+   */
+  static getTmpTrackUploadArtifactsPath () {
+    const dirPath = path.join(config.get('storagePath'), 'files', 'tmp_track_artifacts')
+    this.ensureDirPathExists(dirPath)


i can see the argument for creating the CID shard dirs on demand but in this case we should def just create the tmp dir at the beginning and not have to check if it exists every single time. seems like unnecessary overhead
i'm glad we are encapsulating all ops here, but if we make above change, we can just build this path at diskManager init and store it as a class var instead of re-concating every time

i think we have on the order of a few hundred checks for tmp_track_artifacts but we have like tens of thousand cp or move ops? i don't think this is an optimization we need now?

not really sure what this means but with this code every track upload will call fs.mkdirSync for no reason. we should just avoid? if you want i can make this change

thanks for making fix

creator-node/src/diskManager.js

creator-node/src/fileManager.js

…factsPath

SidSethi

🚢

SidSethi · 2020-11-17T00:15:53Z

creator-node/test/audiusUsers.test.js

@@ -49,15 +49,15 @@ describe('test AudiusUsers with mocked IPFS', function () {
  it('successfully creates Audius user (POST /audius_users/metadata)', async function () {
    const metadata = { test: 'field1' }
    ipfsMock.add.twice().withArgs(Buffer.from(JSON.stringify(metadata)))
-    ipfsMock.pin.add.once().withArgs('testCIDLink')
+    ipfsMock.pin.add.once().withArgs('QmYfSQCgCwhxwYcdEwCkFJHicDe6rzCAb7AtLz3GrHmuU6')


lmao we only call this in health check now 🤦 k whatever, will rip out of tests and health check separately

dmanjunath added 7 commits November 12, 2020 00:07

First pass, everything I know of minus saveFileForMultihash

6c69e14

more stuff

978d2d2

more comments and test improvement

138b38f

test fixes

3606644

i think everything works

095a8ed

first set of test fixes

73caf73

lint

0789a65

SidSethi reviewed Nov 12, 2020

View reviewed changes

creator-node/scripts/run-tests.sh Show resolved Hide resolved

raymondjacobson approved these changes Nov 13, 2020

View reviewed changes

creator-node/src/diskManager.js Outdated Show resolved Hide resolved

creator-node/src/diskManager.js Outdated Show resolved Hide resolved

creator-node/src/fileManager.js Outdated Show resolved Hide resolved

dmanjunath added 12 commits November 12, 2020 22:05

test fixes?

9945e03

more fixes

0a42c79

another one

735f460

resize image tests

537a7cc

tracks tests?

06153c4

undo only

483d826

one more tracks fix

30fb97f

lint

37e3dab

refactor and naming

f46dfcf

test fix

a3a3582

bug fix

5e33b35

remove manual path.join

86c8efb

dmanjunath requested review from hareeshnagaraj, roneilr, raymondjacobson and SidSethi November 14, 2020 03:26

raymondjacobson approved these changes Nov 14, 2020

View reviewed changes

dmanjunath added 2 commits November 15, 2020 17:22

Also add functionality to parse cids and dir cids from a path

cb3881e

cleanup

3df83a7

up the ipfs content verification timeout

7b6f400

dmanjunath commented Nov 16, 2020

View reviewed changes

creator-node/src/fileManager.js Show resolved Hide resolved

hareeshnagaraj suggested changes Nov 16, 2020

View reviewed changes

raymondjacobson approved these changes Nov 16, 2020

View reviewed changes

SidSethi reviewed Nov 16, 2020

View reviewed changes

dmanjunath added 4 commits November 16, 2020 16:45

naming and comments

ec6bb85

store all track upload artifacts in the same dir

8aed635

fix unit tests

ee8a2d3

remove set caching in disk manager

149e0b3

hareeshnagaraj reviewed Nov 16, 2020

View reviewed changes

creator-node/src/fileManager.js Show resolved Hide resolved

hareeshnagaraj approved these changes Nov 16, 2020

View reviewed changes

SidSethi reviewed Nov 16, 2020

View reviewed changes

dmanjunath added 3 commits November 16, 2020 18:14

CID validation

9eae9de

cache if ensureDirPathExists has been called in getTmpTrackUploadArti…

141f091

…factsPath

test updates

50d8f34

SidSethi approved these changes Nov 17, 2020

View reviewed changes

dmanjunath merged commit 3ce3ec1 into master Nov 17, 2020

dmanjunath deleted the dm-file-storage-nesting-hotfix branch November 17, 2020 03:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

File storage directory nesting #1056

File storage directory nesting #1056

dmanjunath commented Nov 12, 2020 •

edited

raymondjacobson left a comment

dmanjunath commented Nov 14, 2020

raymondjacobson left a comment

dmanjunath commented Nov 15, 2020

hareeshnagaraj left a comment

hareeshnagaraj Nov 16, 2020

dmanjunath Nov 16, 2020

hareeshnagaraj Nov 16, 2020

hareeshnagaraj Nov 16, 2020

hareeshnagaraj Nov 16, 2020

dmanjunath Nov 16, 2020

hareeshnagaraj Nov 16, 2020

SidSethi Nov 16, 2020

dmanjunath Nov 16, 2020 •

edited

SidSethi left a comment

SidSethi Nov 16, 2020

dmanjunath Nov 16, 2020

SidSethi Nov 16, 2020

SidSethi Nov 16, 2020

dmanjunath Nov 16, 2020

SidSethi Nov 16, 2020 •

edited

SidSethi Nov 17, 2020

hareeshnagaraj left a comment

SidSethi Nov 16, 2020

dmanjunath Nov 16, 2020

SidSethi Nov 16, 2020

SidSethi Nov 17, 2020

SidSethi left a comment

SidSethi Nov 17, 2020

File storage directory nesting #1056

File storage directory nesting #1056

Conversation

dmanjunath commented Nov 12, 2020 • edited

Trello Card Link

Description

Services

Does it touch a critical flow like Discovery indexing, Creator Node track upload, Creator Node gateway, or Creator Node file system?

How Has This Been Tested?

raymondjacobson left a comment

Choose a reason for hiding this comment

dmanjunath commented Nov 14, 2020

raymondjacobson left a comment

Choose a reason for hiding this comment

dmanjunath commented Nov 15, 2020

hareeshnagaraj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmanjunath Nov 16, 2020 • edited

Choose a reason for hiding this comment

SidSethi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SidSethi Nov 16, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hareeshnagaraj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SidSethi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmanjunath commented Nov 12, 2020 •

edited

dmanjunath Nov 16, 2020 •

edited

SidSethi Nov 16, 2020 •

edited