cli: add upload-server destination for debug zip upload#168310
cli: add upload-server destination for debug zip upload#168310visheshbardia wants to merge 1 commit intocockroachdb:masterfrom
Conversation
visheshbardia
requested review from
Abhinav1299 and
dhartunian
and removed request for
a team
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
|
This change is |
visheshbardia
force-pushed
the
upload-debugZ-to-blob
branch
2 times, most recently
from
ef7418e to
aa1db04
Compare
|
Detected infrastructure failure (matched: ). Automatically rerunning failed jobs. (run link) |
1 similar comment
|
Detected infrastructure failure (matched: ). Automatically rerunning failed jobs. (run link) |
visheshbardia
force-pushed
the
upload-debugZ-to-blob
branch
2 times, most recently
from
05725df to
244bb7c
Compare
|
Detected infrastructure failure (matched: ). Automatically rerunning failed jobs. (run link) |
visheshbardia
force-pushed
the
upload-debugZ-to-blob
branch
from
244bb7c to
5c38ce5
Compare
|
Detected infrastructure failure (matched: ). Automatically rerunning failed jobs. (run link) |
1 similar comment
|
Detected infrastructure failure (matched: ). Automatically rerunning failed jobs. (run link) |
visheshbardia
force-pushed
the
upload-debugZ-to-blob
branch
from
5c38ce5 to
b6dcb2d
Compare
| func newGCSBlobUploader( | ||
| ctx context.Context, bucket, pathPrefix, bearerToken string, | ||
| ) (*gcsBlobUploader, error) { | ||
| uri := fmt.Sprintf( |
There was a problem hiding this comment.
Not sure but this token can appear in any error message that prints the URI, is there any way to redact it?
There was a problem hiding this comment.
The URI doesn't appear in any error or log path in cloud pkg as of now. Additionally, BEARER_TOKEN is also already registered as a RedactedParam in the GCS provider. If we ever add logging or surface the URI in errors in the future, we can use cloud.SanitizeExternalStorageURI to strip it, as the established pattern across the codebase.
| fmt.Fprintf(os.Stderr, "Session %s completed\n", sessionID) | ||
| } | ||
|
|
||
| fmt.Fprintf(os.Stderr, "Successfully uploaded debug zip.\n") |
There was a problem hiding this comment.
We can add few useful analytics during and after the upload is happening:
- Total time elapsed to upload the debug zip successfully after completion.
- Maybe reporting throughput during the upload process (MB/sec)
There was a problem hiding this comment.
added! Now completion shows the stats as below:
| }() | ||
|
|
||
| retryOpts := base.DefaultRetryOptions() | ||
| retryOpts.MaxRetries = 5 |
There was a problem hiding this comment.
NIT: remove magic number here, define a const for this
|
|
||
| // validateUploadServerReadiness checks that the required flags are set | ||
| // for upload-server uploads and that the input file is valid. | ||
| func validateUploadServerReadiness(inputPath string) error { |
There was a problem hiding this comment.
Should we add a check here to verify if the file is actually a valid zip? Checking the magic literal file signature of zips here.
There was a problem hiding this comment.
added the magic signature check.
| fmt.Fprintf(os.Stderr, | ||
| "Warning: failed to complete upload session: %v\n", err, | ||
| ) | ||
| } else { | ||
| fmt.Fprintf(os.Stderr, "Session %s completed\n", sessionID) | ||
| } | ||
|
|
||
| fmt.Fprintf(os.Stderr, "Successfully uploaded debug zip.\n") |
There was a problem hiding this comment.
Slightly misleading message here, if the completeUploadSession returns an error, we print Warning: failed to complete upload session along with Successfully uploaded debug zip..
There was a problem hiding this comment.
modified it to show warning along with error message as below:
| // 2. POST /api/v1/sessions/{id}/upload-token → {access_token, bucket, prefix} | ||
| var createUploadSession = func( | ||
| ctx context.Context, serverURL, apiKey string, | ||
| ) (sessionID, uploadToken, bucket, prefix, gcsToken string, err error) { |
There was a problem hiding this comment.
This function is returning too many values, can we create a struct to improve readability here? Maybe:
type uploadSessionInfo struct {
SessionID string
UploadToken string
Bucket string
Prefix string
GCSToken string
}
There was a problem hiding this comment.
makes sense, added!
| tmpDir := t.TempDir() | ||
| zipPath := filepath.Join(tmpDir, "debug.zip") | ||
| zipContent := []byte("PK-fake-zip-content") | ||
| require.NoError(t, os.WriteFile(zipPath, zipContent, 0644)) | ||
|
|
||
| origOpts := debugZipUploadOpts | ||
| origCreate := createUploadSession | ||
| origComplete := completeUploadSession | ||
| origNewUploader := newBlobUploader |
There was a problem hiding this comment.
Duplicate code lines in every test function, can we extract it to a helper function?
visheshbardia
force-pushed
the
upload-debugZ-to-blob
branch
from
b6dcb2d to
154f54a
Compare
Add support for uploading debug zip files directly to blob storage via CRL's upload server. The upload flow creates a session with the server to obtain short-lived GCS credentials, uploads the zip file directly to GCS, and marks the session complete. The implementation is split across two files for clear separation of concerns: - zip_upload_blob.go: reusable blob storage abstraction (blobUploader interface, GCS implementation, progress writer) - zip_upload_server.go: upload server session lifecycle (create session, get upload token, complete session, validation) The existing Datadog upload path is preserved and remains the default. Users select the upload server via --destination=upload-server along with --upload-server-api-key and --upload-server-url flags. Part of: CRDB-61946 Epic: CRDB-60645 Release note: None
visheshbardia
force-pushed
the
upload-debugZ-to-blob
branch
from
154f54a to
deef92a
Compare
3 participants
Add support for uploading debug zip files directly to blob storage via CRL's upload server. The upload flow creates a session with the server to obtain short-lived GCS credentials, uploads the zip file directly to GCS, and marks the session complete.
The implementation is split across two files for clear separation of concerns:
The existing Datadog upload path is preserved and remains the default. Users select the upload server via --destination=upload-server along with --upload-server-api-key and --upload-server-url flags.
Part of: CRDB-61946
Epic: CRDB-60645
Release note: None