feat: Add dif assemble endpoint #7141

HazAT · 2018-02-01T16:00:55Z

This adds POST /api/0/projects/sentry/internal/files/difs/assemble/ API endpoint to Sentry.

tl;dr
This is part 2 #7095
This enables us the upload dif (Debug Information Files) of arbitrary size.
95%+ of lines is the added crash.sym fixture for tests

This endpoint pieces together chunks that were uploaded before in the mentioned PR.
It adds a new model called FileBlobOwner which makes sure that someone who uploads a blob has access rights to it (Organization).

The request is JSON scheme validated.

Request
POST /api/0/projects/sentry/internal/files/difs/assemble/
Body:

{
	"38fbd8b2cbe56884115e324dd5f2a10c8201450c": {
		"type": "dif",
		"name": "test.dsym",
		"params": {
			"project": "java",
		},
		"chunks": [
			"38fbd8b2cbe56884115e324dd5f2a10c8201450c"
		]
	}
}

This actually tries to assemble a file if all chunks were uploaded before.
If chunks are missing (or ownership is missing) the response will be:

{
    "38fbd8b2cbe56884115e324dd5f2a10c8201450c": {
        "state": "not_found",
        "missingChunks": ["38fbd8b2cbe56884115e324dd5f2a10c8201450c"]
    }
}

If all chunks are already uploaded and the file did not exsist before response will be:

{
    "38fbd8b2cbe56884115e324dd5f2a10c8201450c": {
        "state": "created",
        "missingChunks": []
    }
}

State can be:

ChunkFileState = enum(
    OK='ok', # File in database
    NOT_FOUND='not_found', # File not found in database
    CREATED='created', # File was created in the request and send to the worker for assembling
    ASSEMBLING='assembling', # File still being processed by worker
    ERROR='error' # Error happened during assembling
)

This will trigger the task sentry.tasks.assemble.assemble_chunks to do the actual assembling.

The assemble task supports difs (Debug information files) like dsyms and so on.
It does not currently support proguard and source map files.

Legacy

**Request** `POST` `/api/0/chunk-assemble/` Body: ```json { "38fbd8b2cbe56884115e324dd5f2a10c8201450c": true } ```

This will check the File model if a file with this checksum already exists in the database.

Response:

{
    "38fbd8b2cbe56884115e324dd5f2a10c8201450c": {
        "state": "ok",
        "missingChunks": []
    }
}

ghost · 2018-02-01T16:04:00Z

	2 Warnings
⚠️	You should update CHANGES due to the size of this PR
⚠️	PR includes migrations

Migration Checklist

new columns need to be nullable (unless table is new)
migration with any new index needs to be done concurrently
data migrations should not be done inside a transaction
before merging, check to make sure there aren't conflicting migration ids

Generated by 🚫 danger

HazAT · 2018-02-05T20:24:05Z

src/sentry/api/endpoints/chunk.py

+
+        return Response(
+            {
+                'url': '{}{}'.format(endpoint, reverse('sentry-api-0-chunk-upload')),


This is the only thing that changed here.
We want to return the full url instead of just the "domain"

mattrobenolt · 2018-02-05T20:24:40Z

src/sentry/conf/server.py

 CELERY_QUEUES = [
    Queue('alerts', routing_key='alerts'),
    Queue('auth', routing_key='auth'),
+    Queue('assemble', routing_key='assemble'),


cc @JTCunning I'm not sure if we need to do anything special anymore from ops to handle a new queue.

No, we're good. If the task takes up a significant amount of resources, we'll isolate it with another pool of workers.

mattrobenolt

Just blocking this until we add verification to checksums like we discussed offline.

JTCunning · 2018-02-05T20:53:36Z

src/sentry/tasks/assemble.py

+    The type is a File.ChunkAssembleType
+    '''
+    if len(file_blob_ids) == 0:
+        logger.warning('sentry.tasks.assemble.assemble_chunks', extra={


You can remove all of your log statements that are prepended with sentry.tasks.assemble since that will be in the logger name and is unnecessary.

JTCunning · 2018-02-05T20:58:26Z

src/sentry/tasks/assemble.py

+    file.assemble_from_file_blob_ids(file_blob_ids, checksum)
+    if file.headers.get('state', '') == ChunkFileState.ERROR:
+        logger.error(
+            'sentry.tasks.assemble.assemble_chunks',


Since you have multiple assemble_chunks error statements throughout your code, you should append them with the logical reason they're erroring, so assemble_chunks.state_error or something.

mitsuhiko · 2018-02-05T22:12:31Z

I think we should kill mode 1 and always require the chunks and metadata to be sent (as a file with the same checksum might exist with other parameters). Additionally we will need to org scope the chunk upload for security reasons as discussed on slack.

I'm fine storing a chunk-verified bit in cache for 12 hours which should also be our "chunk not stable" time. David also says we can keep a huge table. Either works I think.

mitsuhiko · 2018-02-05T22:38:27Z

What we discussed on slack:

store org chunk ownership in a table (TODO: consider if we want to do this for internal writes too or only external chunk upload)
later introduce shared dsym files again (separate model, shared_dsymfile) to avoid users having to upload common files multiple times. Also consider symbolserver here again
move the endpoint to org level

Notes unrelated to above convo:

remove sha1 only file assemble, require meta info.
make assemble fully dsym specific (eg: move it into a project url)

jan-auer · 2018-02-05T22:42:47Z

Would be great if we could return the error description in /chunk-assemble/, if any. Right now, there's no convenient way to retrieve it via the new endpoint.

HazAT · 2018-02-06T17:31:48Z

I've added a new model called FileBlobOwner which checks if someone from the same org already uploaded the chunk.
It wasn't that trivial after all the handle all possible scenarios but the tests should cover it and we tested it in conjunction with what @jan-auer did already with sentry-cli 🎉
(see gif in the first comment)

HazAT · 2018-02-06T22:34:18Z

@mattrobenolt any new feedback?

mitsuhiko

Left some specific comments.

Generally though I think we should really make this a DIF specific endpoint for now (eg: on a dif specific url instead of generic chunk-assemble). Reason being that it seems cleaner from an access management point of view and fits better into how current code functions.

mitsuhiko · 2018-02-06T22:37:36Z

src/sentry/api/endpoints/chunk.py

+                    )
+            except IntegrityError:
+                pass
+            if blob.checksum not in checksum_list:


Can we change this loop to be a for checksum, chunk in izip(checksums, files) and then check the blob.checksum against the checksum directly?

mitsuhiko · 2018-02-06T22:38:12Z

src/sentry/api/endpoints/chunk.py

+        for owned_blob in all_owned_blobs:
+            owned_blobs.append((owned_blob.blob.id, owned_blob.blob.checksum))
+
+        # If the request does not cotain any chunks for a file


typo "cotain"

mitsuhiko · 2018-02-06T22:39:37Z

src/sentry/api/endpoints/chunk.py

+        elif len(owned_blobs) != len(chunks):
+            # Create a missing chunks array which we return as response
+            # so the client knows which chunks to reupload
+            missing_chunks = list(chunks)


make this into missing_chunks = set(chunks) and then remove items with missing_chunks.discard(blob[1]). Faster and easier (O(1) vs O(n something))

mattrobenolt · 2018-02-06T23:12:46Z

src/sentry/south_migrations/0388_auto__add_index_file_checksum.py

+
+    def forwards(self, orm):
+        # Adding index on 'File', fields ['checksum']
+        db.create_index('sentry_file', ['checksum'])


We can't run this in production.

This needs to be done with CREATE INDEX CONCURRENTLY. grep the repo for examples of other migrations adding indexes.

mattrobenolt · 2018-02-06T23:13:30Z

src/sentry/south_migrations/0388_auto__add_index_file_checksum.py

+
+    # Flag to indicate if this migration is too risky
+    # to run online and needs to be coordinated for offline
+    is_dangerous = False


Because creating this index will take a while, especially with CONCURRENTLY, this needs to be flipped to True so we don't block deploy for however long it takes to create the index.

mitsuhiko

As far as I can tell this is good to go from my side. Annoyingly we can't push this to staging because of the migration.

HazAT · 2018-02-07T13:32:16Z

Also need @mattrobenolt seal of approval.

mattrobenolt

Blocking for the prefetch_related comment and make sure the migration doesn’t need to be rebased.

mattrobenolt · 2018-02-08T17:59:24Z

src/sentry/api/bases/chunk.py

+    def _check_file_blobs(self, organization, checksum, chunks):
+        files = File.objects.filter(
+            checksum=checksum
+        ).select_related('blobs').all()


I think you’re looking for prefetch_related here. Otherwise, below you’re doing an O(n) for each file to fetch the blobs.

mattrobenolt · 2018-02-08T18:00:52Z

src/sentry/api/bases/chunk.py

+            name=name,
+            checksum=checksum,
+            type='chunked',
+            headers={'state': ChunkFileState.CREATED}


I’m not a fan of using a header here to manage the state. Is this file truly temporary?

Might I suggest a name like, __state to better signal that it’s not real?

mattrobenolt · 2018-02-08T18:28:06Z

Migration needs debased over #7191

feat: Add assemble endpoint

c2da3ae

HazAT self-assigned this Feb 1, 2018

HazAT added 8 commits February 2, 2018 14:40

feat: Fix assembling, Add enum states

60f8875

feat: Assemble dsym

0a8460b

feat: Add comments

3f77fda

feat: Add comments, Add full url to chunk upload GET

1310dda

feat: Add tests for assemble endpoint

89f6915

feat: Add test for assemble file

b711fe2

fix: Tests import and async call

74b82d3

feat: Add index to file checksum

171b6c5

HazAT force-pushed the feature/dsym-assemble branch from 93549ad to 171b6c5 Compare February 5, 2018 15:30

HazAT added 6 commits February 5, 2018 16:34

Merge branch 'master' into feature/dsym-assemble

c6d068f

fix: Test async call

5bed7ed

fix: dsym file detection

df4aa81

fix: Order of assert_called_once_with params

5b08411

fix: Order of blob ids

fd786be

feat: Add tests for assemble task

323a03c

HazAT requested review from mattrobenolt and mitsuhiko February 5, 2018 20:18

HazAT commented Feb 5, 2018

View reviewed changes

mattrobenolt reviewed Feb 5, 2018

View reviewed changes

mattrobenolt suggested changes Feb 5, 2018

View reviewed changes

JTCunning reviewed Feb 5, 2018

View reviewed changes

mitsuhiko closed this Feb 5, 2018

mitsuhiko reopened this Feb 5, 2018

feat: Add remove max chunks limit

c4bc323

HazAT added 2 commits February 6, 2018 19:08

fix: Chunk upload tests

f9fc9b1

feat: Readd max chunks and new max request size

06b05c0

mitsuhiko suggested changes Feb 6, 2018

View reviewed changes

mattrobenolt suggested changes Feb 6, 2018

View reviewed changes

mattrobenolt added the Impact: Migration label Feb 6, 2018

HazAT added 2 commits February 7, 2018 08:52

fix: Migration

c3630dd

fix: Add more tests, refactoring

f094fc5

mitsuhiko mentioned this pull request Feb 7, 2018

error: http error: generic error (504) on dSYM upload getsentry/sentry-cli#241

Closed

HazAT added 6 commits February 7, 2018 11:04

feat: Remove generic assemble endpoint and move to dsyms

f0ddad5

feat: Remove public field in blobowner

315bdad

Merge branch 'master' into feature/dsym-assemble

47d6a39

feat: Create new migration

aa61357

fix: Add missing is_postgres to migration

a8d5cf0

ref: Rename dsym assemble to dif assemble

13772ed

HazAT changed the title ~~feat: Add assemble endpoint~~ feat: Add dif assemble endpoint Feb 7, 2018

mitsuhiko approved these changes Feb 7, 2018

View reviewed changes

mattrobenolt suggested changes Feb 8, 2018

View reviewed changes

ref: Use prefetch_related, Rename state

c9f13bd

HazAT added 2 commits February 8, 2018 19:31

Merge branch 'master' into feature/dsym-assemble

fec2fb1

meta: Bump migration

38f6631

mattrobenolt approved these changes Feb 8, 2018

View reviewed changes

HazAT merged commit 6088008 into master Feb 8, 2018

HazAT deleted the feature/dsym-assemble branch February 8, 2018 19:09

jan-auer mentioned this pull request Feb 12, 2018

ref(upload): Add support for the chunked DIF upload getsentry/sentry-cli#245

Merged

13 tasks

github-actions bot locked and limited conversation to collaborators Dec 22, 2020

Uh oh!

feat: Add dif assemble endpoint #7141

feat: Add dif assemble endpoint #7141

Uh oh!

Conversation

HazAT commented Feb 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Feb 1, 2018 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Migration Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattrobenolt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mitsuhiko commented Feb 5, 2018

Uh oh!

mitsuhiko commented Feb 5, 2018

Uh oh!

jan-auer commented Feb 5, 2018

Uh oh!

HazAT commented Feb 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HazAT commented Feb 6, 2018

Uh oh!

mitsuhiko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mitsuhiko left a comment

Choose a reason for hiding this comment

Uh oh!

HazAT commented Feb 7, 2018

Uh oh!

mattrobenolt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattrobenolt commented Feb 8, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

HazAT commented Feb 1, 2018 •

edited

Loading

ghost commented Feb 1, 2018 •

edited by ghost

Loading

HazAT commented Feb 6, 2018 •

edited

Loading